bun:ffi: infer exact signatures from symbol definitions, fix cc argument/return conversions - #32075
bun:ffi: infer exact signatures from symbol definitions, fix cc argument/return conversions#32075robobun wants to merge 7 commits into
Conversation
|
Updated 5:07 PM PT - Jun 28th, 2026
❌ @robobun, your commit 58fd5a7 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32075That installs a local version of the PR into your bun-32075 --bun |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImproves FFI typing, coercion, and verification
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
CI status: the diff's own tests are green on all lanes. The three red lanes across the last two runs are unrelated flakes, each from a different area and none touching bun:ffi:
The one real CI failure this PR caused (ffi.test.js running on Windows ARM64 where bun:ffi is disabled) was fixed in 95a0ece and has stayed green since. Ready for review. |
|
@robobun why did any test files change (except bun-types fixture, of course). help me understand please |
|
The PR is not types-only:
The one commit after that ( Fail-before numbers for the whole set are in the PR description (ffi.test.js fails 3, cc.test.ts segfaults, fixture produces 6 diagnostics on old types). |
|
Can you break it into two prs so i can quickly merge the typescript types? Or is it wrong to do that if the types rely on the behavioural changes |
|
@robobun ^ |
132c867 to
3a717c4
Compare
|
Rebased onto main. The only conflict was the harness import line in test/js/bun/ffi/ffi.test.js (main gained imports from the NaN-purification and JSCallback-termination test additions, this branch added isMusl/isArm64/isWindows); resolved as the union. No semantic overlap with the upstream FFI changes: src/js/bun/ffi.ts on main only swapped $newZigFunction for $newRustFunction. Both test files pass locally after the rebase, including the new upstream tests. |
|
The bun-plugin-svelte check on this PR is failing on main too (same assertion on the __commonJS wrapper text: https://github.com/oven-sh/bun/actions/runs/28314465661/job/83884987730), so it's pre-existing and unrelated to this diff. |
|
Addressed the review comments in b9eb46c:
The CI failure on the previous run (build 66365) was a single unrelated flake (test/bake/deinitialization.test.ts on Windows aarch64). |
|
The "TypeScript types" check failure on the previous two runs was not a type error: every tsc lane passed, and the tsgo (TypeScript 7 preview) subtest failed to even start because @typescript/native-preview 7.0.0-dev.20260628.1 (published today) renamed its entry point from bin/tsgo.js to bin/tsgo, which test/integration/bun-types/bun-types.test.ts hardcoded. 18961a2 resolves the path from the package's bin field instead; the tsgo subtest passes locally again. This workflow only runs for PRs that touch packages/bun-types, so any bun-types PR would have hit it starting today. |
|
Addressed the latest review in e55322e: the runtime FFIType lookup object now has reverse entries for 18, 19, and 20, so declaring argument types with the FFIType.napi_env / FFIType.napi_value enum values works through FFIBuilder (previously "Unsupported type 18"). The cc napi passthrough test now uses the enum spellings to cover that path. Note that FFIType.buffer (numeric 20) is still rejected earlier by the native ABI parser on every entry point (pre-existing, covered by #31449); the JS-side entry is included so it starts working when that lands. Also trimmed the oversized comment in ffi.d.ts. |
|
Status: all FFI tests and type checks are green on every lane, three rounds of review feedback are addressed (FFIType string map alignment, numeric napi/buffer lookup entries, size_t spelling, sliced-CString pointer offset), and every review thread is resolved. The remaining red is unrelated to this diff and differs per run:
Nothing left to address on my side; ready for review. |
|
Addressed in d14ad60: the JS FFIType lookup now accepts "size_t" (the one native-accepted spelling it was missing), mapped to uint64_t like "usize", with a cc round-trip test and the FFITypeStringToType entry so it is representable in types. With that, every spelling the native ABI parser accepts resolves through FFIBuilder. |
Type changes (packages/bun-types/ffi.d.ts): - dlopen, cc, linkSymbols, CFunction, and JSCallback now use const type parameters, so argument arity is checked from the symbol definitions without requiring "as const". Previously add(1, 2, 3, 4) compiled fine for args: ["i32", "i32"]. - CFunction returns a typed call signature instead of CallableFunction. - JSCallback infers the callback's parameter and return types from the definition. cstring arguments are typed as raw Pointer | null because the native trampoline does not wrap them in CString. - ptr/cstring arguments accept ArrayBuffer and DataView, which the runtime converts. Runtime fixes (src/js/bun/ffi.ts): - cc() read symbol definitions from options[key] instead of options.symbols[key], so FFIBuilder was never applied: cstring returns came back as raw pointer numbers, large uint32 arguments were corrupted (take_u32(0xFFFFFFFF) returned 4292870144), and passing a JSCallback object as a function argument segfaulted. This was the cause of the "FIXME: bus error" skips in cc.test.ts. - Pointer arguments now accept CString (unwrapped to its ptr), matching what the published types have always claimed; previously this threw "Unable to convert hello to a pointer". - The ArrayBufferView check now includes DataView, which the compiled stubs already handle (JSType range includes DataView). - napi_env/napi_value arguments pass through unmodified instead of going through the default val|0 coercion, which corrupts JSValues now that cc applies wrappers. Tests: - test/js/bun/ffi/ffi.test.js: dlopen pointer-argument conversion tests (CString, ArrayBuffer, DataView, rejection), using libc strlen. - test/js/bun/ffi/cc.test.ts: un-skipped the ping/strlen cstring tests (fixing makeValidCase, which returned the library variable before beforeAll assigned it), plus new coverage for cstring returns, u32, ArrayBuffer, JSCallback function arguments, and napi_value passthrough. - test/integration/bun-types/fixture/ffi.ts: type-level assertions for inferred arity, CFunction, and JSCallback signatures.
The runtime and the native parser both map "function", "callback", and "fn" to FFIType.function, so string-spelled function arguments accept a JSCallback like the enum spelling does. The string map also gains the other spellings the runtime accepts (c_int, c_uint, isize, char*, void*, i64_fast, u64_fast). Also trims a comment in cc.test.ts to the repo's 3 line limit and drops a non-null assertion in the docs example that had no effect.
…ppers The FFIType lookup object only had reverse entries for 0 through 17, so declaring an argument with FFIType.napi_env, FFIType.napi_value, or FFIType.buffer (the numbers, not the string spellings) made FFIBuilder throw "Unsupported type". For dlopen and linkSymbols this was a pre-existing bug; for cc it would have become reachable once symbol definitions are read from options.symbols, so the napi passthrough test now declares its argument types with the enum values.
The native ABI parser maps "size_t" to uint64_t but the JS lookup object did not, so FFIBuilder threw "Unsupported type size_t" for dlopen and, once cc applies wrappers, for cc as well. Also exposes the spelling in FFITypeStringToType.
|
Rebased onto main again. One conflict: main independently landed the same tsgo entrypoint fix in test/integration/bun-types/bun-types.test.ts (e2a69b7), so this branch's version of that commit was dropped in favor of main's and the file no longer differs from main. No other FFI changes landed upstream; both FFI test files pass locally after the rebase. |
d14ad60 to
8819d00
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/runtime/ffi.mdx`:
- Around line 293-299: Document the nullable pointer contract in the JSCallback
examples by updating the callback parameter types and surrounding explanation so
they reflect that "ptr" may be Pointer | null. In the JSCallback sample(s) shown
in the ffi.mdx docs, adjust the signature and usage guidance to avoid
dereferencing the pointer without a null check, and make sure the text clearly
states when a callback may receive null so readers don’t copy an unsafe pattern.
In `@packages/bun-types/ffi.d.ts`:
- Around line 430-433: The callback return docs for the FFI pointer-typed return
behavior are missing DataView even though the type accepts it, so update the
documentation near the return type description in ffi.d.ts to explicitly mention
DataView alongside TypedArray; use the surrounding callback/return docs and
Pointer, CString, and JSCallback references to keep the wording aligned with the
public contract.
In `@src/js/bun/ffi.ts`:
- Around line 313-315: Replace the `instanceof __GlobalBunCString` check in the
`CString` handling path with a private brand check or static helper on
`__GlobalBunCString`, so detection cannot be affected by `Symbol.hasInstance` or
prototype tampering. Update the logic in the branch that returns `val.ptr` to
use the class’s internal brand/slot-based check, and keep the same behavior for
valid `CString` instances.
In `@test/js/bun/ffi/cc.test.ts`:
- Around line 155-163: The current CString test only covers the zero-offset case
and can miss the byteOffset bug in bun/ffi argument unwrapping. Extend the
existing `it("given a valid CString, returns a CString wrapping the same
pointer", ...)` case to also create a sliced `CString` with a non-zero offset
(for example via `new CString(ptr(arr), 2, 5)`), pass it through
`holder.library.symbols.ping`, and assert on the returned bytes/string content
rather than comparing base pointers. Keep the checks focused on observable
semantics so `src/js/bun/ffi.ts` is validated against offset-aware behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76953651-d014-4874-b70a-e4e121e14d98
📒 Files selected for processing (6)
docs/runtime/ffi.mdxpackages/bun-types/ffi.d.tssrc/js/bun/ffi.tstest/integration/bun-types/fixture/ffi.tstest/js/bun/ffi/cc.test.tstest/js/bun/ffi/ffi.test.js
A CString constructed with a byteOffset represents the string at ptr + byteOffset, so passing one as a pointer argument now forwards that address instead of the base pointer. The check also no longer relies on instanceof, matching how the function wrapper detects pointer carriers. Also mentions DataView in the callback return type docs and makes the JSCallback docs examples handle a null pointer argument.
|
Addressed all four review comments in 58fd5a7:
|
|
Closing: #35246 replaced the implementation this PR patches. Two of the |
What
Makes
bun:ffi's TypeScript surface infer exact call signatures from symbol definitions, and fixes the runtime bugs that surfaced while verifying the types against actual behavior.Types (
packages/bun-types/ffi.d.ts)dlopen,cc,linkSymbols,CFunction, andJSCallbacknow useconsttype parameters. Argument tuples infer exactly withoutas const, so arity is checked:Previously argument types were checked but arity was not (
Parameterscollapsed tonumber[]unless the caller wroteas const).CFunctionreturns a typed call signature derived from the definition instead ofCallableFunction.JSCallbackinfers the callback's parameters and return type from the definition. The mappings follow the native trampoline:cstringarguments arrive as a rawPointer | null(notCString),i64/u64arrive asbigint. The callback parameter uses a bivariant method signature so narrower handwritten annotations like(ptr: Pointer) => voidstay assignable.ptr/cstringarguments acceptArrayBufferandDataView, which the runtime converts.FFITypeStringToTypenow matches the runtime string map:"function","callback", and the previously missing"fn"resolve toFFIType.function(so string-spelled function arguments accept aJSCallback, like the enum spelling), and the other spellings the runtime accepts ("c_int","c_uint","isize","char*","void*","i64_fast","u64_fast") are representable.Runtime (
src/js/bun/ffi.ts)cc()never applied argument/return conversions. The wrapper-application loop readoptions[key], butccnests definitions underoptions.symbols[key](unlikedlopen(path, symbols)). Verified on 1.4.0-canary:returns: "cstring"produced a raw pointer number instead of aCStringidentity_u32(0xFFFFFFFF)returned4292870144(the JSValue encoding bug from FFIType.u32 misbehavior #7007 that theval|0wrapper exists to prevent)JSCallbackobject as a"function"argument jumped to a garbage pointer:panic(main thread): Segmentation fault at address 0xFFFFFFFFFFFFFFFFThis is also why the
// FIXME: bus errorskips incc.test.tsexisted: the skipped cstring tests crashed because the pointer-argument wrapper never ran.Pointer arguments now accept
CString, unwrapping to itsptr. The published types have always claimedFFIType.ptracceptsCString, but the wrapper threwTypeError: Unable to convert hello to a pointer.DataViewis accepted wherever TypedArrays are. The JS-side check used$isTypedArrayView, which excludes DataView, but the compiled stubs accept it (the JSType range check inFFI.hspans throughDataView, andJSVALUE_TO_TYPED_ARRAY_VECTORreads the ArrayBufferView vector).napi_env/napi_valuearguments pass through unmodified. Withccnow applying wrappers, the defaultval|0coercion would corruptnapi_valueJSValues (the native stub reads them raw via.asNapiValue). This also fixes the same latent bug fordlopen'd symbols with napi argument types.Also fixed
makeValidCaseincc.test.ts, which returned thelibraryvariable beforebeforeAllassigned it (alwaysundefined), unnoticed because every test using it was skipped.Tests
test/js/bun/ffi/ffi.test.js: newpointer argument conversionsuite over libcstrlen(CString, ArrayBuffer, DataView for ptr and buffer args, rejection of unconvertible values). Runs on Linux glibc, macOS, and Windows (msvcrt.dll), including ASAN builds.test/js/bun/ffi/cc.test.ts: un-skipped the ping/strlen cstring suites and addedcc applies the same conversions as dlopen(cstring returns, u32, ArrayBuffer, JSCallback function args, napi_value passthrough). These compile-success paths run under ASAN; the TinyCC setjmp conflict only affects its compile-error handling.test/integration/bun-types/fixture/ffi.ts: type-level assertions for inferred arity,CFunction, andJSCallbacksignatures, with@ts-expect-errordirectives that are unused against the old types.All fail on the unfixed build: the fixture produces 6 type-check diagnostics,
ffi.test.jsfails 3 tests, andcc.test.tssegfaults where the pointer wrapper is missing.bun-typesintegration test (tsc + tsgo, DOM and no-DOM) passes.Docs: updated the
JSCallbackexamples indocs/runtime/ffi.mdxto be type-correct under the stricter signatures (they also passedlengthasCString'sbyteOffsetparameter; nownew CString(ptr, 0, Number(length))).Overlap with other open PRs
The
cc()definition-lookup bug has been independently found and fixed in three other open PRs. Map of the overlap so this is easy to triage:options.symbols[key]lookup (identical semantics) plus one subprocess test for the JSCallback wild-jump case.val && val.ptr; this PR instead matches the narrower published type withinstanceof CString. Its other fixes (int16 clamp, double BigInt sign, native boundary checks) do not overlap with this PR.The overlapping hunks are semantically identical in all of them, so whichever lands first, the others rebase trivially. What only this PR contains: the type-level inference work (
consttype parameters, typedCFunction/JSCallback), DataView acceptance, the conversion test suites that run under ASAN, and themakeValidCasefixture repair.Fixes #235
Fixes #24518