zig: fix missing uses of bun.callmod_inline#24738
Merged
Merged
Conversation
Collaborator
|
Updated 11:33 PM PT - Nov 14th, 2025
❌ @nektro, your commit 9c8291a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 24738That installs a local version of the PR into your bun-24738 --bun |
This comment was marked as duplicate.
This comment was marked as duplicate.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/codegen/generate-classes.ts (2)
216-231: Fix duplicate "fn" in destructuring assignmentThe destructuring pattern in
propRow()containsfntwice (lines 219 and 221), which is a syntax error. Remove the duplicate:var { defaultValue, getter, setter, fn, accessor, - fn, length = 0,
2223-2229: Generator bug: duplicate parameter "thisValue" causes invalid Zig in getInternalProperties wrapperLines 2225–2229 in
src/codegen/generate-classes.tsgenerate invalid Zig with duplicate parameter names. The function declaresthisValuetwice: once asthisValue: *${typeName}and again asthisValue: jsc.JSValue. The call also incorrectly passes the first parameter twice instead of using the JSValue parameter.Fix:
- pub fn ${classSymbolName(typeName, "getInternalProperties")}(thisValue: *${typeName}, globalObject: *jsc.JSGlobalObject, thisValue: jsc.JSValue) callconv(jsc.conv) jsc.JSValue { + pub fn ${classSymbolName(typeName, "getInternalProperties")}(thisValue: *${typeName}, globalObject: *jsc.JSGlobalObject, js_this_value: jsc.JSValue) callconv(jsc.conv) jsc.JSValue { if (comptime Environment.enable_logs) log_zig_get_internal_properties("${typeName}"); - return @call(bun.callmod_inline, ${typeName}.getInternalProperties, .{thisValue, globalObject, thisValue}); + return @call(bun.callmod_inline, ${typeName}.getInternalProperties, .{thisValue, globalObject, js_this_value}); }src/deps/uws/Response.zig (1)
187-195: I need to examine the full code context to verify the claim about the extra argument.Now I need to examine the actual Response.zig file to verify the exact code context.
Based on my verification using web searches about Zig's
@callfunction, I can confirm the issue identified in the review comment.In Zig,
@callstrictly validates argument count: when a function expects N arguments, the tuple passed to@callmust have exactly N elements. Passing 3 elements where 2 are expected results in a compilation error ("expected 3 argument(s), found 4").In the code at lines 187-195:
- When
UserDataType == void, the handler has signaturefn(void, *Response) void— taking exactly 2 parameters- The current code passes
.{ {}, castRes(this), {} }— a 3-element tuple- This will cause a compilation error due to argument count mismatch
The proposed fix is correct: removing the trailing
{}creates a 2-element tuple matching the function signature.Fix the extra argument in the void branch
Remove the trailing
{}from the tuple passed to@callon the void branch:- @call(bun.callmod_inline, handler, .{ {}, castRes(this), {} }); + @call(bun.callmod_inline, handler, .{ {}, castRes(this) });
📜 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 (12)
src/bun.js/api/BunObject.zig(1 hunks)src/bun.js/api/ffi.zig(1 hunks)src/bun.js/event_loop/AnyTaskWithExtraContext.zig(1 hunks)src/bun.js/jsc/host_fn.zig(2 hunks)src/bun.js/webcore/BakeResponse.zig(1 hunks)src/codegen/generate-classes.ts(12 hunks)src/codegen/generate-js2native.ts(1 hunks)src/deps/uws/App.zig(2 hunks)src/deps/uws/Response.zig(2 hunks)src/js_printer.zig(1 hunks)src/router.zig(2 hunks)src/string/immutable/unicode.zig(1 hunks)
🧰 Additional context used
🧠 Learnings (30)
📓 Common learnings
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.
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:17:26.376Z
Learning: In Bun's Zig codebase, when handling error unions where the same cleanup operation (like `rawFree`) needs to be performed regardless of success or failure, prefer using boolean folding with `else |err| switch (err)` over duplicating the cleanup call in multiple switch branches. This approach avoids code duplication while maintaining compile-time error checking.
Learnt from: pfgithub
Repo: oven-sh/bun PR: 23117
File: src/bun.js/test/snapshot.zig:265-276
Timestamp: 2025-09-30T22:53:19.887Z
Learning: In Bun's snapshot testing (src/bun.js/test/snapshot.zig), multiple inline snapshots at the same line and column (same call position) must have identical values. However, multiple inline snapshots on the same line at different columns are allowed to have different values. The check is position-specific (line+col), not line-wide.
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators/allocation_scope.zig:284-314
Timestamp: 2025-09-02T18:25:27.976Z
Learning: In bun's custom Zig implementation, the `#` prefix for private fields is valid syntax and should not be flagged as invalid. The syntax `#fieldname` creates private fields that cannot be accessed from outside the defining struct, and usage like `self.#fieldname` is correct within the same struct. This applies to fields like `#parent`, `#state`, `#allocator`, `#trace`, etc. throughout the codebase.
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.
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators/allocation_scope.zig:79-85
Timestamp: 2025-09-02T18:09:21.647Z
Learning: In Bun's Zig codebase, prefer simple, direct error handling patterns. When an error type has only one variant (like FreeError with only NotAllocated), use a switch statement rather than generic catch blocks to leverage compile-time knowledge and avoid unnecessary complexity. Avoid duplicating function calls across error paths when a cleaner structure is possible.
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.
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.
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 24622
File: src/deps/uws/us_socket_t.zig:112-113
Timestamp: 2025-11-12T04:11:52.293Z
Learning: In Bun's Zig codebase, when passing u32 values to C FFI functions that expect c_uint parameters, no explicit intCast is needed because c_uint is equivalent to u32 on Bun's target platforms and Zig allows implicit coercion between equivalent types. This pattern is used consistently throughout src/deps/uws/us_socket_t.zig in functions like setTimeout, setLongTimeout, and setKeepalive.
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.
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:14:59.734Z
Learning: In Zig, the `inline` keyword is not a performance optimization hint. It forces inlining at every call site and has semantic implications on types and values involved in function calls. The Zig documentation advises that "it is generally better to let the compiler decide when to inline a function" rather than manually specifying `inline`. Only use `inline` when there are specific semantic requirements, not for performance optimization.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22231
File: src/bundler/bundle_v2.zig:48-48
Timestamp: 2025-08-30T09:09:18.384Z
Learning: In Zig, when a module exports a top-level struct, import("./Module.zig") directly returns that struct type and can be used as a type alias without needing to access a field within the module. This is a common pattern in the Bun codebase.
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`.
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23680
File: cmake/targets/BuildBun.cmake:822-822
Timestamp: 2025-10-15T20:19:37.256Z
Learning: In the Bun codebase, FFI (Foreign Function Interface) code is compiled separately using TinyCC (tcc), which barely supports C99. Headers like src/bun.js/api/FFI.h and src/bun.js/api/ffi-stdbool.h are only used for FFI compilation with tcc, not with the main compiler. Therefore, C standard changes to the main Bun target do not affect FFI code compilation.
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/safety/alloc.zig:93-95
Timestamp: 2025-09-02T17:14:46.924Z
Learning: In bun's Zig codebase, they use a custom extension of Zig that supports private field syntax with the `#` prefix (e.g., `#allocator`, `#trace`). This is not standard Zig syntax but is valid in their custom implementation. Fields prefixed with `#` are private fields that cannot be accessed from outside the defining struct.
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators.zig:873-879
Timestamp: 2025-09-02T17:07:29.377Z
Learning: In Zig's bun codebase, in functions like `unpackNullable` that return `?Allocator`, anonymous method calls like `.get()` are preferred over fully qualified calls like `Nullable(Allocator).get(allocator)` because the compiler can infer the correct type from the return type context. This follows Zig's anonymous literal syntax conventions.
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.
📚 Learning: 2025-10-15T20:19:37.256Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23680
File: cmake/targets/BuildBun.cmake:822-822
Timestamp: 2025-10-15T20:19:37.256Z
Learning: In the Bun codebase, FFI (Foreign Function Interface) code is compiled separately using TinyCC (tcc), which barely supports C99. Headers like src/bun.js/api/FFI.h and src/bun.js/api/ffi-stdbool.h are only used for FFI compilation with tcc, not with the main compiler. Therefore, C standard changes to the main Bun target do not affect FFI code compilation.
Applied to files:
src/bun.js/jsc/host_fn.zigsrc/bun.js/api/ffi.zigsrc/bun.js/api/BunObject.zig
📚 Learning: 2025-09-30T22:53:19.887Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 23117
File: src/bun.js/test/snapshot.zig:265-276
Timestamp: 2025-09-30T22:53:19.887Z
Learning: In Bun's snapshot testing (src/bun.js/test/snapshot.zig), multiple inline snapshots at the same line and column (same call position) must have identical values. However, multiple inline snapshots on the same line at different columns are allowed to have different values. The check is position-specific (line+col), not line-wide.
Applied to files:
src/bun.js/jsc/host_fn.zigsrc/bun.js/api/ffi.zig
📚 Learning: 2025-09-20T05:35:57.318Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 22534
File: src/bun.js/bindings/headers.h:729-731
Timestamp: 2025-09-20T05:35:57.318Z
Learning: symbols.txt in the Bun codebase is specifically for V8 API mangled symbols (without leading underscore), not for general Bun host functions declared with BUN_DECLARE_HOST_FUNCTION. Host functions are handled through different build mechanisms.
Applied to files:
src/bun.js/jsc/host_fn.zigsrc/codegen/generate-classes.tssrc/bun.js/api/ffi.zig
📚 Learning: 2025-11-14T16:07:01.064Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Applied to files:
src/bun.js/jsc/host_fn.zigsrc/bun.js/api/ffi.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/bun.js/jsc/host_fn.zigsrc/bun.js/event_loop/AnyTaskWithExtraContext.zigsrc/codegen/generate-js2native.tssrc/codegen/generate-classes.tssrc/bun.js/api/ffi.zigsrc/deps/uws/App.zig
📚 Learning: 2025-09-02T19:14:59.734Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-09-02T19:14:59.734Z
Learning: In Zig, the `inline` keyword is not a performance optimization hint. It forces inlining at every call site and has semantic implications on types and values involved in function calls. The Zig documentation advises that "it is generally better to let the compiler decide when to inline a function" rather than manually specifying `inline`. Only use `inline` when there are specific semantic requirements, not for performance optimization.
Applied to files:
src/bun.js/jsc/host_fn.zigsrc/codegen/generate-js2native.tssrc/js_printer.zigsrc/bun.js/api/ffi.zig
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/jsc/host_fn.zigsrc/deps/uws/Response.zig
📚 Learning: 2025-11-12T04:11:52.293Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 24622
File: src/deps/uws/us_socket_t.zig:112-113
Timestamp: 2025-11-12T04:11:52.293Z
Learning: In Bun's Zig codebase, when passing u32 values to C FFI functions that expect c_uint parameters, no explicit intCast is needed because c_uint is equivalent to u32 on Bun's target platforms and Zig allows implicit coercion between equivalent types. This pattern is used consistently throughout src/deps/uws/us_socket_t.zig in functions like setTimeout, setLongTimeout, and setKeepalive.
Applied to files:
src/bun.js/event_loop/AnyTaskWithExtraContext.zigsrc/string/immutable/unicode.zigsrc/bun.js/api/ffi.zigsrc/deps/uws/App.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/codegen/generate-js2native.tssrc/codegen/generate-classes.ts
📚 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/codegen/generate-js2native.tssrc/codegen/generate-classes.tssrc/bun.js/api/BunObject.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/codegen/generate-js2native.tssrc/codegen/generate-classes.tssrc/bun.js/api/ffi.zigsrc/bun.js/api/BunObject.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/codegen/generate-js2native.tssrc/codegen/generate-classes.tssrc/bun.js/api/BunObject.zig
📚 Learning: 2025-09-02T17:14:46.924Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/safety/alloc.zig:93-95
Timestamp: 2025-09-02T17:14:46.924Z
Learning: In bun's Zig codebase, they use a custom extension of Zig that supports private field syntax with the `#` prefix (e.g., `#allocator`, `#trace`). This is not standard Zig syntax but is valid in their custom implementation. Fields prefixed with `#` are private fields that cannot be accessed from outside the defining struct.
Applied to files:
src/codegen/generate-js2native.tssrc/bun.js/api/BunObject.zig
📚 Learning: 2025-10-01T21:27:44.943Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/api/server/SSLConfig.zig:202-210
Timestamp: 2025-10-01T21:27:44.943Z
Learning: In Zig, decl literals (anonymous syntax like `.fromJS(...)` or `.method(...)`) can be used for method calls when the target type is explicitly known from context, such as through a type annotation. For example, `var generated: Type = try .fromJS(args)` is equivalent to and preferred over `var generated: Type = try Type.fromJS(args)`. This is part of Zig's idiomatic anonymous literal syntax.
Applied to files:
src/codegen/generate-js2native.tssrc/codegen/generate-classes.ts
📚 Learning: 2025-08-30T09:09:18.384Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22231
File: src/bundler/bundle_v2.zig:48-48
Timestamp: 2025-08-30T09:09:18.384Z
Learning: In Zig, when a module exports a top-level struct, import("./Module.zig") directly returns that struct type and can be used as a type alias without needing to access a field within the module. This is a common pattern in the Bun codebase.
Applied to files:
src/codegen/generate-js2native.tssrc/codegen/generate-classes.tssrc/bun.js/api/BunObject.zig
📚 Learning: 2025-10-19T23:27:43.240Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23840
File: src/bun.js/node/node_fs_stat_watcher.zig:400-411
Timestamp: 2025-10-19T23:27:43.240Z
Learning: In Zig code (e.g., src/bun.js/node/node_fs_stat_watcher.zig), taking the address of a function return value (e.g., &this.getLastStat()) is valid when passed as a const pointer parameter (e.g., *const bun.sys.PosixStat) to another function in the same expression. The compiler materializes the temporary with appropriate lifetime for the call scope.
Applied to files:
src/codegen/generate-js2native.tssrc/codegen/generate-classes.ts
📚 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 codebase, `.js_undefined` is a valid way to represent JavaScript's undefined value when working with JSPromise.resolve() and similar JavaScript interop functions. This is the correct pattern to use rather than `jsc.JSValue.jsUndefined()`.
Applied to files:
src/codegen/generate-js2native.tssrc/bun.js/api/BunObject.zig
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/codegen/generate-classes.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
src/codegen/generate-classes.ts
📚 Learning: 2025-09-02T17:07:29.377Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators.zig:873-879
Timestamp: 2025-09-02T17:07:29.377Z
Learning: In Zig's bun codebase, in functions like `unpackNullable` that return `?Allocator`, anonymous method calls like `.get()` are preferred over fully qualified calls like `Nullable(Allocator).get(allocator)` because the compiler can infer the correct type from the return type context. This follows Zig's anonymous literal syntax conventions.
Applied to files:
src/codegen/generate-classes.ts
📚 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/immutable/unicode.zig
📚 Learning: 2025-10-15T20:19:38.580Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23680
File: cmake/targets/BuildBun.cmake:822-822
Timestamp: 2025-10-15T20:19:38.580Z
Learning: In the Bun codebase, FFI is compiled with tcc (TinyCC), which barely supports C99. The headers `src/bun.js/api/FFI.h` and `src/bun.js/api/ffi-stdbool.h` are only used for FFI compilation with tcc, not for the main Bun target. Therefore, C23 compatibility concerns (such as bool/true/false keyword conflicts) do not apply to these FFI headers.
Applied to files:
src/bun.js/api/ffi.zig
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
src/bun.js/api/ffi.zig
📚 Learning: 2025-10-14T04:04:17.132Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23636
File: src/shell/interpreter.zig:672-0
Timestamp: 2025-10-14T04:04:17.132Z
Learning: In Bun's shell interpreter (src/shell/interpreter.zig), the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization and not updated afterward, even though fields like `root_io`, `vm_args_utf8`, and `export_env` may be mutated later. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
Applied to files:
src/bun.js/api/BunObject.zig
📚 Learning: 2025-10-14T04:04:47.894Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23636
File: src/shell/ParsedShellScript.zig:15-0
Timestamp: 2025-10-14T04:04:47.894Z
Learning: In src/shell/ParsedShellScript.zig, the JavaScript garbage collector's memory reporting API only allows reporting extra memory allocation once at object creation time. Therefore, `estimated_size_for_gc` is intentionally set once during initialization (via `#computeEstimatedSizeForGC()`) and not updated afterward, even though fields like `args`, `export_env`, and `cwd` may be mutated by methods like `setCwd()`, `setEnv()`, and `take()`. The `estimatedSize()` method returns this cached initial value, while `memoryCost()` computes the size on-demand.
Applied to files:
src/bun.js/api/BunObject.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/bun.js/api/BunObject.zig
📚 Learning: 2025-10-16T02:17:35.237Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/analytics.zig:15-21
Timestamp: 2025-10-16T02:17:35.237Z
Learning: In src/analytics.zig and similar files using bun.EnvVar boolean environment variables: the new EnvVar API for boolean flags (e.g., bun.EnvVar.do_not_track.get(), bun.EnvVar.ci.get()) is designed to parse and return boolean values from environment variables, not just check for their presence. This is an intentional design change from the previous presence-based checks using bun.getenvZ().
Applied to files:
src/bun.js/api/BunObject.zig
📚 Learning: 2025-08-31T09:08:12.104Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22279
File: test/js/web/structured-clone-fastpath.test.ts:12-12
Timestamp: 2025-08-31T09:08:12.104Z
Learning: In Bun, process.memoryUsage.rss() is called as a direct function, not as process.memoryUsage().rss like in Node.js. This is the correct API shape for Bun.
Applied to files:
src/bun.js/api/BunObject.zig
📚 Learning: 2025-10-26T04:50:17.892Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24086
File: src/bun.js/api/server.zig:2534-2535
Timestamp: 2025-10-26T04:50:17.892Z
Learning: In src/bun.js/api/server.zig, u32 is acceptable for route indices and websocket_context_index fields, as the practical limit of 4 billion contexts will never be reached.
Applied to files:
src/deps/uws/App.zig
🔇 Additional comments (13)
src/bun.js/event_loop/AnyTaskWithExtraContext.zig (1)
56-64: Code change verified;bun.callmod_inlineis properly defined insrc/bun.zig:20.The replacement of the call modifier with
bun.callmod_inlineis correct. The bun module exports it aspub const callmod_inline: std.builtin.CallModifier = if (builtin.mode == .Debug) .auto else .always_inline;, which aligns with the PR objective of improving stack traces in debug mode by using.autoinstead of forced inlining.The change is syntactically and semantically valid. Please verify independently that the CI failures are unrelated to this refactoring.
src/js_printer.zig (1)
834-837: No issues found—verification confirms correct usage.The
bun.callmod_inlineconstant is properly defined, and the codebase uses it consistently. No lingering.always_inlinecall modifiers were detected. The changes in lines 834–837 and 841–844 are correct and align with the broader refactoring pattern across the codebase.src/string/immutable/unicode.zig (1)
1887-1889: Verify the change is correctly applied.Line 1888 is marked as changed, but the visible code shows a plain function call without an
@calldirective. Based on the PR objectives (replacing.always_inlinewithbun.callmod_inline), the expected change would be something like:-return @call(.always_inline, encodeWTF8RuneT, .{p, u32, @intCast(r)}); +return encodeWTF8RuneT(p, u32, @intCast(r));or
-return @call(.always_inline, encodeWTF8RuneT, .{p, u32, @intCast(r)}); +return @call(bun.callmod_inline, encodeWTF8RuneT, .{p, u32, @intCast(r)});If the change is removing an unnecessary inline directive and letting the compiler decide (first option), that aligns with Zig best practices. If it's replacing with
bun.callmod_inlinebut not visible in the diff, please verify the change was applied correctly.src/deps/uws/App.zig (1)
83-109: LGTM! Consistent inline directive update.The replacement of
.always_inlinewithbun.callmod_inlinein both branches (void and non-void UserDataType) is correct and consistent. This change will improve stack traces in debug mode while maintaining the necessary calling semantics for the HTTP route handler callbacks.src/router.zig (1)
592-630: LGTM! Route sorting inline directives updated correctly.The changes to use
bun.callmod_inlinefor thesortByNameStringcalls in both the static/dynamic and catch_all/optional_catch_all branches are consistent and correct. This improves debuggability of the route sorting logic during initialization.src/codegen/generate-js2native.ts (1)
222-233: LGTM! Code generation updated for new inline directive.The update to generate
@call(bun.callmod_inline, ...)instead of.always_inlineensures that all generated wrapper functions for Zig-to-JavaScript FFI calls will use the correct calling convention. This change will propagate to all generated binding code and provide better stack traces across the FFI boundary.src/bun.js/jsc/host_fn.zig (2)
543-702: LGTM! Instance method wrapper updated correctly.The change to use
bun.callmod_inlinefor the final method invocation inwrapInstanceMethodis correct. This ensures better stack traces when calling Zig instance methods from JavaScript.
704-844: LGTM! Static method wrapper updated correctly.The change to use
bun.callmod_inlinefor the final method invocation inwrapStaticMethodmirrors the instance method change and is correct. Both wrappers now consistently use the improved calling convention.src/deps/uws/Response.zig (2)
263-271: Cork wrapper now uses bun.callmod_inline — LGTMConsistent with the rest of the refactor; preserves tuple-arg forwarding of handler_fn.
Please confirm uws_res_cork invokes the callback synchronously (so &args_tuple lifetime is safe). If not, we need to heap‑allocate the tuple.
435-443: AnyResponse.onData uses bun.callmod_inline — LGTMMatches the standardized call mode; no behavioral change expected.
src/codegen/generate-classes.ts (1)
2171-2176: Bulk switch to bun.callmod_inline in generated Zig — verified completeVerification confirms no remaining
.always_inlinepatterns in the codebase. The migration tobun.callmod_inlineis complete and consistent across all generated bindings (methods, getters/setters, constructors, call(), structured clone hooks, etc.). This change should improve debug stack traces as intended.src/bun.js/api/ffi.zig (1)
34-38: Unifying call mode to bun.callmod_inline — LGTMSwitching to bun.callmod_inline here is consistent with the PR's goal and follows the established pattern across the codebase (js_printer.zig, url.zig, resolver/, http.zig, deps/uws/, etc.). Verification confirms no direct
.always_inlinemodifiers remain in @call statements—the abstraction has been adopted consistently throughout.src/bun.js/api/BunObject.zig (1)
758-761: LGTM: Clean refactoring to improve debug stack traces.The refactoring from a direct public function to an exported C-ABI function with a public alias is well-structured. The use of
callconv(.c)ensures proper stack traces in debug mode, aligning with the PR objectives.
dylan-conway
approved these changes
Nov 16, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
results in better strack traces in debug mode