Replace std.ArrayList with bun.collections.ArrayListDefault #24495
+42
−42
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replace 5 instances of `std.ArrayList(...).init(bun.default_allocator)` with `bun.collections.ArrayListDefault(...)` for better memory management. Changes: - src/string.zig: ArrayList(u8) for string formatting - src/install/lockfile.zig: ArrayList(u8) for lockfile serialization - src/io/PipeWriter.zig: ArrayList(u8) in StreamBuffer struct - src/s3/list_objects.zig: ArrayList for S3 contents and prefixes - src/bundler/bundle_v2.zig: ArrayList(OutputFile) return type ArrayListDefault uses the default allocator with zero overhead while providing consistent deinit semantics. Used deinitShallow() where element types don't have deinit methods (e.g., []const u8).
Collaborator
Author
Contributor
WalkthroughThis pull request migrates multiple files from Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/bake/production.zig(1 hunks)src/bundler/bundle_v2.zig(2 hunks)src/bundler/linker_context/generateChunksInParallel.zig(1 hunks)src/install/lockfile.zig(1 hunks)src/io/PipeWriter.zig(5 hunks)src/s3/list_objects.zig(5 hunks)src/string.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bake/production.zigsrc/install/lockfile.zigsrc/s3/list_objects.zigsrc/bundler/bundle_v2.zigsrc/bundler/linker_context/generateChunksInParallel.zigsrc/io/PipeWriter.zigsrc/string.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
Files:
src/bake/production.zigsrc/install/lockfile.zigsrc/s3/list_objects.zigsrc/bundler/bundle_v2.zigsrc/bundler/linker_context/generateChunksInParallel.zigsrc/io/PipeWriter.zigsrc/string.zig
🧠 Learnings (25)
📓 Common learnings
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bake/production.zigsrc/install/lockfile.zigsrc/s3/list_objects.zigsrc/bundler/bundle_v2.zigsrc/bundler/linker_context/generateChunksInParallel.zigsrc/io/PipeWriter.zigsrc/string.zig
📚 Learning: 2025-11-03T20:43:06.996Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/test/snapshot.zig:19-19
Timestamp: 2025-11-03T20:43:06.996Z
Learning: In Bun's Zig codebase, when storing JSValue objects in collections like ArrayList, use `jsc.Strong.Optional` (not raw JSValue). When adding values, wrap them with `jsc.Strong.Optional.create(value, globalThis)`. In cleanup code, iterate the collection calling `.deinit()` on each Strong.Optional item before calling `.deinit()` on the ArrayList itself. This pattern automatically handles GC protection. See examples in src/bun.js/test/ScopeFunctions.zig and src/bun.js/node/node_cluster_binding.zig.
Applied to files:
src/install/lockfile.zigsrc/s3/list_objects.zigsrc/bundler/bundle_v2.zigsrc/string.zig
📚 Learning: 2025-09-02T18:27:23.279Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/collections/multi_array_list.zig:24-24
Timestamp: 2025-09-02T18:27:23.279Z
Learning: The `#allocator` syntax in bun's custom Zig implementation is valid and does not require quoting with @"#allocator". Private fields using the `#` prefix work correctly throughout the codebase without special quoting syntax.
Applied to files:
src/install/lockfile.zigsrc/string.zig
📚 Learning: 2025-09-24T00:08:49.281Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22917
File: src/collections/array_list.zig:302-308
Timestamp: 2025-09-24T00:08:49.281Z
Learning: In Zig's ArrayListAlignedUnmanaged, the resize method when shrinking (new_len < old_len) does not free or move the underlying memory allocation - it only updates the items.len field to new_len. This means memory beyond the new length remains valid and accessible, making it safe to access self.items().ptr[new_len..old_len] after calling resize.
Applied to files:
src/install/lockfile.zigsrc/io/PipeWriter.zigsrc/string.zig
📚 Learning: 2025-10-15T22:03:50.832Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/envvars.zig:135-144
Timestamp: 2025-10-15T22:03:50.832Z
Learning: In src/envvars.zig, the boolean feature flag cache uses a single atomic enum and should remain monotonic. Only the string cache (which uses two atomics: ptr and len) requires acquire/release ordering to prevent torn reads.
Applied to files:
src/install/lockfile.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/s3/list_objects.zigsrc/string.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/s3/list_objects.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/s3/list_objects.zigsrc/bundler/bundle_v2.zigsrc/string.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/s3/list_objects.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Applied to files:
src/s3/list_objects.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/s3/list_objects.zigsrc/bundler/bundle_v2.zigsrc/bundler/linker_context/generateChunksInParallel.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/s3/list_objects.zig
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/s3/list_objects.zig
📚 Learning: 2025-10-11T15:19:30.301Z
Learnt from: mastermakrela
Repo: oven-sh/bun PR: 19167
File: src/bun.js/api.zig:31-31
Timestamp: 2025-10-11T15:19:30.301Z
Learning: The learnings about `pub const js = JSC.Codegen.JS<ClassName>` and re-exporting toJS/fromJS/fromJSDirect apply to class-based Zig bindings with constructors and prototype methods (e.g., those with `new` constructors). They do NOT apply to simple namespace-style API objects like TOMLObject, YAMLObject, and CSVObject which expose static functions via a `create()` method that returns a plain JS object.
Applied to files:
src/s3/list_objects.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Implement getters as get<PropertyName>(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Applied to files:
src/s3/list_objects.zig
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/bundler/bundle_v2.zigsrc/bundler/linker_context/generateChunksInParallel.zig
📚 Learning: 2025-09-02T18:32:32.309Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/bun.zig:668-675
Timestamp: 2025-09-02T18:32:32.309Z
Learning: The `std.meta.trait` module has been removed from the Zig standard library, which will cause compilation failures in functions that use it like `std.meta.trait.isSingleItemPtr()`. This affects functions like `isHeapMemory()` in `src/bun.zig`.
Applied to files:
src/string.zig
📚 Learning: 2025-09-02T17:09:17.391Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators.zig:778-782
Timestamp: 2025-09-02T17:09:17.391Z
Learning: In Zig's bun codebase, the `isDefault` function in `src/allocators.zig` only needs to compare `allocator.vtable == c_allocator.vtable` rather than also checking the `ptr` field, because: (1) the codebase never creates multiple allocators that use `c_allocator.vtable` but have different `ptr`s, and (2) the default allocator vtable ignores the `ptr` field anyway, so any allocators sharing the same vtable would function identically.
Applied to files:
src/string.zig
📚 Learning: 2025-10-21T20:57:01.892Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23889
File: src/string.zig:751-752
Timestamp: 2025-10-21T20:57:01.892Z
Learning: In Zig code handling bun.String: calling deref() on an empty bun.String (String.empty or after being set to .empty) is well-defined behavior and safely does nothing (no-op). While such calls may be redundant and could be removed for code clarity, their presence is not a correctness issue.
Applied to files:
src/string.zig
📚 Learning: 2025-09-04T02:04:43.094Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22278
File: src/ast/E.zig:980-1003
Timestamp: 2025-09-04T02:04:43.094Z
Learning: In Bun's Zig codebase, `as(i32, u8_or_u16_value)` is sufficient for casting u8/u16 to i32 in comparison operations. `intCast` is not required in this context, and the current casting approach compiles successfully.
Applied to files:
src/string.zig
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/string.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.cpp : Provide an extern "C" Bun__<Type>__toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Applied to files:
src/string.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/string.zig
🔇 Additional comments (4)
src/bake/production.zig (1)
325-325: LGTM! API accessor updated correctly.The change from
.itemsto.items()is consistent with thebun.collections.ArrayListDefaultAPI migration.src/bundler/linker_context/generateChunksInParallel.zig (1)
5-5: Return type updated to ArrayListDefault.The return type change is consistent with the migration to
bun.collections.ArrayListDefault. This should align with the implementation at line 548 whereoutput_files.take()is returned.src/string.zig (1)
860-863: LGTM! ArrayList migration completed correctly.The changes properly migrate from
std.ArrayListtobun.collections.ArrayListDefault:
- Initialization no longer requires an explicit allocator
- Item access updated to use
.items()method- Memory management remains correct with
defer builder.deinit()src/install/lockfile.zig (1)
1260-1270: LGTM! Proper ownership transfer implemented.The migration to
bun.collections.ArrayListDefaultis correct:
- Initialization no longer requires explicit allocator
- Item access updated to
.items()method- The addition of
.toOwnedSlice() catch bun.outOfMemory()explicitly transfers ownership to the caller, which is properly freed on line 1272This is an improvement over implicit ownership semantics.
Comment on lines
66
to
69
| if (this.contents) |contents| { | ||
| for (contents.items) |*item| item.deinit(); | ||
| for (contents.items()) |*item| item.deinit(); | ||
| contents.deinit(); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid double-deinit on contents elements
With bun.collections.ArrayListDefault, deinit() already walks the list and runs each element's deinit. Manually looping the elements before calling contents.deinit() now triggers S3ListObjectsContents.deinit() twice, so the etag OwnedIn (and any other owned fields) will be freed twice. Please drop the manual loop and rely on contents.deinit() (or use deinitShallow() only if you truly want to skip element finalizers).
🤖 Prompt for AI Agents
In src/s3/list_objects.zig around lines 66 to 69, remove the manual loop that
iterates over contents.items() and calls each item.deinit() because
bun.collections.ArrayListDefault::deinit() already finalizes each element;
leaving the loop causes double-deinit and double-free of owned fields like etag
— simply call contents.deinit() (or replace with contents.deinitShallow() only
if you intentionally want to skip element finalizers) and delete the explicit
for-loop.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Replace 5 instances of
std.ArrayList(...).init(bun.default_allocator)withbun.collections.ArrayListDefault(...)for better memory management and consistency.Changes
1. src/string.zig:860
ArrayList(u8).items→.items()2. src/install/lockfile.zig:1260
ArrayList(u8).items→.items().toOwnedSlice()to transfer ownership3. src/io/PipeWriter.zig:1100
ArrayList(u8)(struct field inStreamBuffer).expandToCapacity()to include requiredinit_valueparameter4. src/s3/list_objects.zig:199-200
ArrayList(S3ListObjectsContents)andArrayList([]const u8).deinitShallow()forcommon_prefixesbecause[]const u8has no deinit method.deinit()forcontentssinceS3ListObjectsContentshas a deinit method5. src/bundler/bundle_v2.zig:1599
ArrayList(options.OutputFile)(return type)generateFromBakeProductionCLIreturn typegenerateChunksInParallelinsrc/bundler/linker_context/generateChunksInParallel.zigsrc/bake/production.zigBenefits
ArrayListDefaulthas no runtime overhead compared tostd.ArrayListwith default allocatorOutputFileandS3ListObjectsContentsTesting
bun bdcc @Jarred-Sumner