fix ENG-24015#25222
Conversation
|
Updated 8:35 AM PT - Nov 29th, 2025
❌ @dylan-conway, your commit 11d1c4f has 3 failures in
🧪 To try this PR locally: bunx bun-pr 25222That installs a local version of the PR into your bun-25222 --bun |
WalkthroughThe PR adds support for HeapBigInt pointer types in FFI symbol resolution, enabling symbol_from_dynamic_library assignment from big integer heap values. Documentation and declaration ordering updates are made to JSValue bindings, and a test case for invalid pointer error handling is added. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/bun.js/api/ffi.zig (1)
1358-1369: Consider explicit validation for invalid ptr types.The HeapBigInt handling correctly prevents the crash described in the linked issue. However, when
ptris provided but is neither a number nor a HeapBigInt (e.g., a string), the code silently ignores it, leavingsymbol_from_dynamic_libraryas null. The downstream error at line 1190 reports the symbol is "missing" a ptr field, which could confuse users sinceptrwas provided—it's just the wrong type.Consider adding explicit type validation to provide a clearer error message:
if (try value.get(global, "ptr")) |ptr| { - if (ptr.isNumber()) { + if (!ptr.isNumber() and !ptr.isHeapBigInt()) { + abi_types.clearAndFree(allocator); + return global.toInvalidArguments("ptr must be a number or BigInt, got {s}", .{@tagName(ptr.jsTypeLoose())}); + } + if (ptr.isNumber()) { const num = ptr.asPtrAddress(); if (num > 0) function.symbol_from_dynamic_library = @as(*anyopaque, @ptrFromInt(num)); } else if (ptr.isHeapBigInt()) { const num = ptr.toUInt64NoTruncate(); if (num > 0) { function.symbol_from_dynamic_library = @as(*anyopaque, @ptrFromInt(num)); } } }This would fail immediately with a clear message like "ptr must be a number or BigInt, got string" rather than later reporting ptr is "missing".
📜 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 (3)
src/bun.js/api/ffi.zig(1 hunks)src/bun.js/bindings/JSValue.zig(1 hunks)test/js/bun/ffi/ffi-error-messages.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
test/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test,describe,expect) and import frombun:test
Usetest.eachand data-driven tests to reduce boilerplate when testing multiple similar cases
Files:
test/js/bun/ffi/ffi-error-messages.test.ts
test/js/bun/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
For Bun-specific API tests, use the
test/js/bun/directory (for http, crypto, ffi, shell, etc.)
Files:
test/js/bun/ffi/ffi-error-messages.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: For single-file tests, prefer using-eflag overtempDir
For multi-file tests, prefer usingtempDirfromharnessandBun.spawnover other temporary directory creation methods
Always useport: 0for network tests and do not hardcode ports or use custom random port number functions
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for no 'panic' or 'uncaught exception' or similar in the test output - that is NOT a valid test
UsetempDirfromharnessto create temporary directories, do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, check stdout/stderr expectations BEFORE checking exit code to get more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests, insteadawaitthe condition to be met as you are testing the CONDITION not the TIME PASSING
Verify your test fails withUSE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>- your test is NOT VALID if it passes withUSE_SYSTEM_BUN=1
Files:
test/js/bun/ffi/ffi-error-messages.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/js/bun/ffi/ffi-error-messages.test.ts
src/**/*.{cpp,zig}
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
src/**/*.{cpp,zig}: Usebun bdorbun run build:debugto build debug versions for C++ and Zig source files; creates debug build at./build/debug/bun-debug
Run tests usingbun bd test <test-file>with the debug build; never usebun testdirectly as it will not include your changes
Execute files usingbun bd <file> <...args>; never usebun <file>directly as it will not include your changes
Enable debug logs for specific scopes usingBUN_DEBUG_$(SCOPE)=1environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required
Files:
src/bun.js/api/ffi.zigsrc/bun.js/bindings/JSValue.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Use
bun.Output.scoped(.${SCOPE}, .hidden)for creating debug logs in Zig codeImplement core functionality in Zig, typically in its own directory in
src/
src/**/*.zig: Zig source code should be organized insrc/*.zigfor core runtime, JavaScript bindings, and package manager
In Zig code, be careful with allocators and use defer for cleanup to manage memory properly
UseBUN_DEBUG_QUIET_LOGS=1to disable debug logging, orBUN_DEBUG_<scopeName>=1to enable specific Output.scoped calls in debug builds
Runbun run zig:check-allto compile the Zig code on all platforms when making platform-specific changes
src/**/*.zig: Private fields in Zig are fully supported using the#prefix:struct { #foo: u32 };
Use decl literals in Zig for declaration initialization:const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer@importat the bottom of the file (auto formatter will move them automatically)
Files:
src/bun.js/api/ffi.zigsrc/bun.js/bindings/JSValue.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.zig: Expose generated bindings in Zig structs usingpub const js = JSC.Codegen.JS<ClassName>with trait conversion methods:toJS,fromJS, andfromJSDirect
Use consistent parameter nameglobalObjectinstead ofctxin Zig constructor and method implementations
Usebun.JSError!JSValuereturn type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup usingdeinit()method that releases resources, followed byfinalize()called by the GC that invokesdeinit()and frees the pointer
UseJSC.markBinding(@src())in finalize methods for debugging purposes before callingdeinit()
For methods returning cached properties in Zig, declare external C++ functions usingextern fnandcallconv(JSC.conv)calling convention
Implement getter functions with naming patternget<PropertyName>in Zig that acceptthisandglobalObjectparameters and returnJSC.JSValue
Access JavaScript CallFrame arguments usingcallFrame.argument(i), check argument count withcallFrame.argumentCount(), and getthiswithcallFrame.thisValue()
For reference-counted objects, use.deref()in finalize instead ofdestroy()to release references to other JS objects
Files:
src/bun.js/api/ffi.zigsrc/bun.js/bindings/JSValue.zig
src/**/*.{ts,zig,cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use absolute paths in file operations
Files:
src/bun.js/api/ffi.zigsrc/bun.js/bindings/JSValue.zig
src/**/*.{ts,zig}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in code - don't use
findorgrep; use Bun's Glob and built-in tools instead
Files:
src/bun.js/api/ffi.zigsrc/bun.js/bindings/JSValue.zig
🧠 Learnings (35)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:458-475
Timestamp: 2025-10-18T20:59:45.579Z
Learning: In src/bun.js/telemetry.zig, the RequestId (u64) to JavaScript number (f64) conversion in jsRequestId() is intentionally allowed to lose precision beyond 2^53-1. This is acceptable because: (1) at 1M requests/sec it takes ~285 years to overflow, (2) the counter resets per-process, and (3) these are observability IDs, not critical distributed IDs. Precision loss is an acceptable trade-off for this use case.
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/BunIDLConvert.h:29-42
Timestamp: 2025-10-01T21:49:27.862Z
Learning: In Bun's IDL bindings (src/bun.js/bindings/BunIDLConvert.h), IDLStrictNull intentionally treats both undefined and null as null (using isUndefinedOrNull()), matching WebKit's IDLNull & IDLNullable behavior. This is the correct implementation and should not be changed to only accept null.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Use tagged pointers for efficient V8 value representation where small integers are stored directly as Smis and objects use pointer tagging with map pointers
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.tssrc/bun.js/api/ffi.zig
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts : Builtin functions must include `this` parameter typing in TypeScript to enable direct method binding in C++
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Create test functions in test/v8/v8-module/main.cpp that take FunctionCallbackInfo<Value> parameter, use the test V8 API, print results for comparison with Node.js, and return Undefined
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to test/js/bun/**/*.test.{ts,tsx} : For Bun-specific API tests, use the `test/js/bun/` directory (for http, crypto, ffi, shell, etc.)
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to **/*.test.{ts,tsx} : Verify your test fails with `USE_SYSTEM_BUN=1 bun test <file>` and passes with `bun bd test <file>` - your test is NOT VALID if it passes with `USE_SYSTEM_BUN=1`
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/**/*.test.ts : Specify expected bundling errors via the `errors` option when a client is expected to encounter an error; by default, a client opening a page to an error fails the test
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `Buffer.alloc(count, fill).toString()` instead of `'A'.repeat(count)` to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-10-18T23:43:42.502Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23817
File: src/js/node/test.ts:282-282
Timestamp: 2025-10-18T23:43:42.502Z
Learning: In the Bun repository, the error code generation script (generate-node-errors.ts) always runs during the build process. When reviewing code that uses error code intrinsics like $ERR_TEST_FAILURE, $ERR_INVALID_ARG_TYPE, etc., do not ask to verify whether the generation script has been run or will run, as it is automatically executed.
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.dyn : Add symbol names with leading underscore and semicolons in braces to src/symbols.dyn for each new V8 API method
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.tssrc/bun.js/api/ffi.zig
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always check exit codes and test error scenarios in error tests
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to **/*.test.{ts,tsx} : Never write tests that check for no 'panic' or 'uncaught exception' or similar in the test output - that is NOT a valid test
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/js/bun/ffi/ffi-error-messages.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : For each new V8 C++ method, add both GCC/Clang and MSVC mangled symbol names to the V8API struct in src/napi/napi.zig using extern fn declarations
Applied to files:
src/bun.js/api/ffi.zigsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-11-24T18:35:39.205Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Applies to src/bun.js/api/BunObject.zig : Implement getter functions in `src/bun.js/api/BunObject.zig` that return your feature, and export them in the `exportAll()` function
Applied to files:
src/bun.js/api/ffi.zig
📚 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/api/ffi.zig
📚 Learning: 2025-11-24T18:36:33.069Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.069Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : When implementing JavaScript classes in C++, define properties using HashTableValue arrays and add iso subspaces for classes with C++ fields, caching structures in ZigGlobalObject
Applied to files:
src/bun.js/api/ffi.zigsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Include new class bindings in `src/bun.js/bindings/generated_classes_list.zig` to register them with the code generator
Applied to files:
src/bun.js/api/ffi.zigsrc/bun.js/bindings/JSValue.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/bun.js/api/ffi.zig
📚 Learning: 2025-11-24T18:35:25.883Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-11-24T18:35:25.883Z
Learning: Applies to *.zig : In Zig, declare external C++ functions and wrap them in public methods using the convention `extern fn Bun__ClassName__toJS(...)` and `pub fn toJS(...)`
Applied to files:
src/bun.js/api/ffi.zigsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-11-24T18:35:39.205Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Applies to **/js_*.zig : Implement proper memory management with reference counting using `ref()`/`deref()` in JavaScript bindings
Applied to files:
src/bun.js/api/ffi.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/bun.js/bindings/JSValue.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/bun.js/bindings/JSValue.zig
📚 Learning: 2025-11-24T18:35:39.205Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Applies to **/js_*.zig : Use `bun.JSError!JSValue` for proper error propagation in JavaScript bindings
Applied to files:
src/bun.js/bindings/JSValue.zig
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to **/*.zig : Use `bun.JSError!JSValue` return type for Zig methods and constructors to enable proper error handling and exception propagation
Applied to files:
src/bun.js/bindings/JSValue.zig
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to **/*.zig : Expose generated bindings in Zig structs using `pub const js = JSC.Codegen.JS<ClassName>` with trait conversion methods: `toJS`, `fromJS`, and `fromJSDirect`
Applied to files:
src/bun.js/bindings/JSValue.zig
📚 Learning: 2025-11-24T18:35:39.205Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Applies to **/*js_bindings.classes.ts : Use `JSC.Codegen` correctly to generate necessary binding code for JavaScript-Zig integration
Applied to files:
src/bun.js/bindings/JSValue.zig
📚 Learning: 2025-09-07T14:00:36.526Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22258
File: src/bun.js/test/jest.zig:63-86
Timestamp: 2025-09-07T14:00:36.526Z
Learning: In Bun's Zig codebase, the established pattern for HashMap hash functions is to truncate hash values to u32, even when the underlying hash computation produces u64. This is seen throughout the codebase, such as in src/css/rules/rules.zig where u64 hashes are stored but truncated to u32 in HashMap contexts. When implementing HashContext for HashMapUnmanaged, the hash function should return u32 to match this pattern.
Applied to files:
src/bun.js/bindings/JSValue.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/bindings/JSValue.zig
📚 Learning: 2025-10-18T20:59:45.579Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:458-475
Timestamp: 2025-10-18T20:59:45.579Z
Learning: In src/bun.js/telemetry.zig, the RequestId (u64) to JavaScript number (f64) conversion in jsRequestId() is intentionally allowed to lose precision beyond 2^53-1. This is acceptable because: (1) at 1M requests/sec it takes ~285 years to overflow, (2) the counter resets per-process, and (3) these are observability IDs, not critical distributed IDs. Precision loss is an acceptable trade-off for this use case.
Applied to files:
src/bun.js/bindings/JSValue.zig
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to **/*.zig : Implement getter functions with naming pattern `get<PropertyName>` in Zig that accept `this` and `globalObject` parameters and return `JSC.JSValue`
Applied to files:
src/bun.js/bindings/JSValue.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/bindings/JSValue.zig
📚 Learning: 2025-11-24T18:35:25.883Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-11-24T18:35:25.883Z
Learning: Applies to *.cpp : To create JavaScript objects from Zig, implement C++ functions following the `Bun__ClassName__toJS(Zig::GlobalObject*, NativeType*)` convention that construct and return the JavaScript object as an encoded JSValue
Applied to files:
src/bun.js/bindings/JSValue.zig
🔇 Additional comments (1)
src/bun.js/bindings/JSValue.zig (1)
1159-1163: LGTM! Documentation clarifies the precondition.The doc comment accurately describes the expected types for
toUInt64NoTruncate, matching the assertion in the linked issue. Reordering the extern declaration improves code organization.
| test("linkSymbols with non-number ptr does not crash", () => { | ||
| expect(() => { | ||
| linkSymbols({ | ||
| fn: { | ||
| // @ts-expect-error | ||
| ptr: "not a number", | ||
| }, | ||
| }); | ||
| }).toThrow('you must provide a "ptr" field with the memory address of the native function.'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the test file to see the actual implementation
cat -n test/js/bun/ffi/ffi-error-messages.test.ts | head -100Repository: oven-sh/bun
Length of output: 3146
🏁 Script executed:
#!/bin/bash
# Search for linkSymbols implementation
rg -n "linkSymbols" --type=ts --type=tsx -A 10 -B 2 | head -150Repository: oven-sh/bun
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Search for linkSymbols implementation
rg -n "linkSymbols" -A 10 -B 2 | head -200Repository: oven-sh/bun
Length of output: 12556
🏁 Script executed:
#!/bin/bash
# Check FFIFunction type definition
rg -n "type FFIFunction\|interface FFIFunction" -A 20 | head -100Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Look at the FFI type definitions
cat packages/bun-types/ffi.d.ts | head -300Repository: oven-sh/bun
Length of output: 5483
🏁 Script executed:
#!/bin/bash
# Search for FFIFunction interface/type
grep -n "FFIFunction\|interface.*Function\|type.*Function" packages/bun-types/ffi.d.ts | head -30Repository: oven-sh/bun
Length of output: 1239
🏁 Script executed:
#!/bin/bash
# Read FFIFunction interface definition
sed -n '453,540p' packages/bun-types/ffi.d.tsRepository: oven-sh/bun
Length of output: 2503
🏁 Script executed:
#!/bin/bash
# Search for Pointer type definition
rg -n "type Pointer\|interface Pointer" packages/bun-types/ffi.d.ts -A 5Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Search for Pointer definition
grep -n "Pointer" packages/bun-types/ffi.d.ts | head -20Repository: oven-sh/bun
Length of output: 1266
🏁 Script executed:
#!/bin/bash
# Search for ptr handling in Zig FFI code
rg -n "symbol_from_dynamic_library\|ptr" src/bun.js/api/ffi.zig -B 3 -A 3 | head -200Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Read the linkSymbols and generateSymbols functions
sed -n '1000,1250p' src/bun.js/api/ffi.zigRepository: oven-sh/bun
Length of output: 10775
🏁 Script executed:
#!/bin/bash
# Search for generateSymbols function
rg -n "fn generateSymbols" src/bun.js/api/ffi.zig -A 100 | head -200Repository: oven-sh/bun
Length of output: 4161
🏁 Script executed:
#!/bin/bash
# Search for generateSymbolForFunction
rg -n "fn generateSymbolForFunction" src/bun.js/api/ffi.zig -A 150 | head -250Repository: oven-sh/bun
Length of output: 6920
🏁 Script executed:
#!/bin/bash
# Verify the error message text matches between test and Zig code
echo "=== From test (line 74) ==="
sed -n '74p' test/js/bun/ffi/ffi-error-messages.test.ts
echo ""
echo "=== From Zig code (line 1190) ==="
sed -n '1190p' src/bun.js/api/ffi.zigRepository: oven-sh/bun
Length of output: 473
🏁 Script executed:
#!/bin/bash
# Check test file structure and imports
head -10 test/js/bun/ffi/ffi-error-messages.test.ts
echo ""
echo "=== Test count ==="
grep -c "test(" test/js/bun/ffi/ffi-error-messages.test.tsRepository: oven-sh/bun
Length of output: 431
🏁 Script executed:
#!/bin/bash
# Summary of findings
cat > /tmp/summary.txt << 'EOF'
VERIFICATION RESULTS:
1. TEST IMPLEMENTATION
- File: test/js/bun/ffi/ffi-error-messages.test.ts, lines 66-75
- Uses correct test pattern: expect(() => {...}).toThrow()
- Error message substring matches the full error from Zig code
2. FFI IMPLEMENTATION (src/bun.js/api/ffi.zig)
- Lines 1358-1369: ptr field extraction
- Accepts: number (line 1359) or HeapBigInt (line 1363)
- String values: Not explicitly rejected, but result in null symbol_from_dynamic_library
- LinkSymbols checks null and throws expected error (line 1190)
3. TYPE DEFINITIONS (packages/bun-types/ffi.d.ts)
- Line 521: FFIFunction.ptr type is "Pointer | bigint"
- Pointer = number with phantom type
- So ptr accepts: number or bigint
4. REVIEW COMMENT ANALYSIS
Original claims:
a) "Test correctly validates error handling for non-numeric ptr values" - TRUE
b) "Verify test fails with USE_SYSTEM_BUN=1 and passes with bun bd test" - RECOMMENDATION (valid)
c) "Consider adding test for valid HeapBigInt ptr values" - VALID SUGGESTION
(Types support bigint, implementation supports bigint, but no test validates it)
5. CODING GUIDELINE COMPLIANCE
- ✓ Uses bun:test correctly
- ✓ Appropriate test pattern for error validation
- ✓ No flaky patterns or resource issues
- ✓ In correct FFI test directory
EOF
cat /tmp/summary.txtRepository: oven-sh/bun
Length of output: 1386
Verify test follows coding guidelines for Bun-specific FFI tests and add validation for supported BigInt ptr values.
The test correctly validates that string ptr values throw the expected error. However, there are two improvements needed:
-
Verify test behavior pattern: Per coding guidelines, you must verify the test fails with
USE_SYSTEM_BUN=1 bun test test/js/bun/ffi/ffi-error-messages.test.tsand passes withbun bd test test/js/bun/ffi/ffi-error-messages.test.tsto ensure it's testing Bun's canary build behavior, not system Bun. -
Add test for valid BigInt ptr values: The FFI implementation (src/bun.js/api/ffi.zig, lines 1363-1367) and type definitions (packages/bun-types/ffi.d.ts, line 521) both explicitly support
bigintfor theptrfield. Currently, the test only validates rejection of invalid types but doesn't verify that valid BigInt pointers are accepted. Add:
test("linkSymbols accepts BigInt ptr", () => {
const lib = linkSymbols({
fn: {
ptr: 0x123456789ABCDEFn,
args: [],
returns: "void",
},
});
expect(lib.symbols.fn).toBeDefined();
});
3 participants
What does this PR do?
Ensures
ptris either a number or heap big int before converting to a number.also fixes ENG-24039
How did you verify your code works?
Added a test