bun:ffi: unify integer argument coercion on modular wrap

[robobun]

What

The JS-side argument wrappers in bun:ffi applied a different out-of-range policy for each integer width: char/i8/i32 wrapped (ToInt32), u8/i16/u16/u32 saturated (with an off-by-one upper bound on i16: >=32768?32768), i64/u64 threw RangeError on a fractional number, and u64 saturated negative numbers to 0n. Which value reached C depended on the declared type and none of it was documented.

This unifies on one policy: modular wrap, matching the conversion a TypedArray element store performs (ToInt32/ToUint32/ToBigInt64/ToBigUint64) and what N-API and the WebAssembly JS API do.

Repro

// echo.c: uint8_t echo_u8(uint8_t v){return v;}  etc.
s.echo_u8(300)   // 255  -> now 44   (Uint8Array([300])[0])
s.echo_u8(-1)    // 0    -> now 255
s.echo_i16(40000)// -32768 (sat, via off-by-one) -> now -25536
s.echo_u32(-1)   // 0    -> now 4294967295
s.echo_i64(5.7)  // RangeError: Not an integer -> now 5n
s.echo_u64(-1)   // 0n   -> now 18446744073709551615n

Fix

ffiWrappers in src/js/bun/ffi.ts:

• char, i8, u8, i16, u16, i32, u32: plain val | 0. ToInt32 truncates fractions and wraps modulo 2^32; the generated stub's implicit narrow to the C parameter width then wraps modulo 2^N. | 0 also guarantees the Int32Tag encoding the trampoline's raw *argsPtr read relies on (FFIType.u32 misbehavior #7007), and for unsigned widths the signed int32 bit pattern is reinterpreted by the C cast (-1 | 0 reaches uint32_t as 0xFFFFFFFF). The per-type saturation clamps and the second uint16_t override are removed.
• i64/i64_fast and u64/u64_fast: BigInt passes through; a number is truncated toward zero (NaN/Infinity become 0). Safe integers (non-negative only for u64, because toUInt64NoTruncate clamps negative doubles) stay as Number to avoid a BigInt allocation; everything else becomes a BigInt, which toBigInt64/toBigUInt64 wrap modulo 2^64.

Documented under FFI types in docs/runtime/ffi.mdx.

Not in this PR

The two other low-severity cells from the same round are tracked by existing PRs and left alone here:

• bool-typed JSCallback returns of a truthy non-true value reaching C as false: bun:ffi: coerce JSCallback return values to the declared return type #33095 addresses JSCallback return coercion.
• cc() skipping FFIBuilder entirely (so none of these wrappers run under cc): bun:ffi: infer exact signatures from symbol definitions, fix cc argument/return conversions #32075.

How did you verify your code works?

New test/js/bun/ffi/ffi-int-coercion.test.ts compiles a small dlopen fixture with an echo function per width and asserts the wrapped result equals the corresponding TypedArray element store for out-of-range, fractional, negative-to-unsigned, and NaN/Infinity inputs, plus an in-range identity sweep. 7 of the 9 tests fail on the unfixed build (saturation/throw cases above); all pass with the fix. Existing test/js/bun/ffi/ suite is unchanged.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/ffi/ffi-int-coercion.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ad7da5a7-ff10-424d-8266-58cb3baf86fc

📥 Commits

Reviewing files that changed from the base of the PR and between 47597ab and 5ff99ec.

📒 Files selected for processing (3)

• docs/runtime/ffi.mdx
• src/js/bun/ffi.ts
• test/js/bun/ffi/ffi-int-coercion.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 5:41 PM PT - Jul 22nd, 2026}

❌ @robobun, your commit 5ff99ec has 2 failures in Build #78115 (All Failures):

• test/js/node/test/parallel/test-gc-http-client-connaborted.js - timeout on 🐧 25.04 x64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 25.04 x64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 13 x64

---

🧪   To try this PR locally:

bunx bun-pr 35180

That installs a local version of the PR into your bun-35180 executable, so you can run:

bun-35180 --bun

[claude]

This changes user-facing coercion semantics for every integer FFI argument type (saturate/throw → modular wrap), which is an API design decision a maintainer should sign off on — especially since bun:ffi values reach native code directly. The implementation and test coverage look solid; deferring for the policy call rather than a correctness concern.

Beyond the inline nit, I checked that the new 64-bit wrapper can't hit BigInt()'s RangeError on non-integers (Math.trunc runs first, and NaN/±Infinity are gated to 0n), and that ffiWrappers.fill(int32) still leaves the later double/float/bool/ptr/function/buffer overrides intact.

Extended reasoning...

Overview

The PR rewrites the ffiWrappers table in src/js/bun/ffi.ts so all integer argument types use a single coercion policy (modular wrap, matching TypedArray element stores) instead of the previous per-type mix of wrap/saturate/throw. It removes the duplicate i64_fast/u64_fast/uint16_t overrides, adds a docs section under FFI types, and adds a new dlopen-based test file that compiles echo functions per width and asserts round-trip equals the corresponding TypedArray store.

Security risks

FFI is inherently safety-sensitive: the coerced value is written raw into the C call frame. The change doesn't widen the set of values that can reach C (everything was already reachable via the wrap paths on i8/i32), but it does change which value reaches C for out-of-range inputs — e.g. u32(-1) now becomes 0xFFFFFFFF instead of 0. Any user code that relied (knowingly or not) on the old saturation to clamp a size/index argument will now pass a very different value to native code. That's a legitimate compat/API concern for a maintainer to weigh, not a bug in the PR itself.

Level of scrutiny

High. This is a deliberate behavior change to a user-facing API surface (bun:ffi argument coercion), which the repo guidance explicitly flags as needing maintainer agreement. The module is marked experimental, which lowers the compat bar, but the choice of wrap vs. saturate vs. throw is exactly the kind of design call that shouldn't land on bot approval alone.

Other factors

• The PR description notes the new test file was not run locally ("no test proof · deferring to CI"), so CI needs to be green before merge.
• One nit was filed inline (const dir = tempDir(...) should be using dir so the compiled .so is cleaned up).
• I verified the 64-bit wrapper's BigInt(n) call cannot throw: Math.trunc guarantees an integer double, and the > -Infinity && < Infinity guard routes NaN/±Infinity to 0n. I also confirmed the fill(int32) baseline is followed by all the same non-integer overrides (double, float, bool, cstring/pointer, buffer, function) as before, so no wrapper was accidentally dropped.

[claude]

Beyond the inline nit, I also checked: the ffiWrappers.fill(int32) still runs before the later double/float/bool/ptr/function overrides so none are clobbered; JSVALUE_TO_INT64 in src/runtime/ffi/FFI.h handles both int32-tagged and double Numbers, so the new i64 wrapper returning Number for safe integers (where it previously always returned BigInt) is safe and covered by the in-range test; and the generated stub's (uintN_t)JSVALUE_TO_INT32(...) cast in abi_type.rs provides the modulo-2^N narrowing the val|0 path relies on. Note this is a deliberate user-facing coercion change (saturate → wrap) to an experimental API — worth a maintainer sign-off on the policy itself.

Extended reasoning...

Verified the wrapper table ordering, the native-side Number handling for the new i64/u64 fast path, and the C-stub narrowing cast that backs the val|0 simplification. The one inline finding is a test-harness convention nit; the substantive change is a policy decision on out-of-range coercion semantics.

[claude]

No bugs found; both earlier nits (tempDir disposal, cc/gcc/clang probe) are addressed. Deferring to a maintainer because this is a user-visible behavior change to bun:ffi argument coercion — swapping saturation/throw for modular wrap is a policy call on public API surface, and worth a human sign-off even with the module marked experimental.

What was reviewed:

• ffiWrappers rewrite: val|0 fill for ≤32-bit widths still produces the Int32Tag encoding the trampoline requires; unsigned reinterpretation via the C cast is sound.
• 64-bit wrappers: traced NaN/±Infinity → 0n, negative numbers to u64 → BigInt path (avoids the UB toUInt64NoTruncate clamp), and BigInt(Math.trunc(n)) cannot throw on any finite double.
• Test file: cc probe + skipIf, using dir, and per-width TypedArray reference values look correct.

Extended reasoning...

Overview

This PR rewrites the ffiWrappers table in src/js/bun/ffi.ts to apply a single coercion policy (modular wrap, matching TypedArray element assignment) to every integer argument width, replacing the previous per-width mix of saturate / wrap / throw. It adds a docs section under FFI types and a new test/js/bun/ffi/ffi-int-coercion.test.ts that compiles a C echo fixture and asserts each width matches the corresponding TypedArray store.

Security risks

None identified. The wrappers run in JS before the trampoline; | 0 and the BigInt paths are pure value coercions with no allocation-size or bounds implications. FFI itself is inherently unsafe, but this change doesn't widen the attack surface.

Level of scrutiny

Medium-high. bun:ffi is flagged experimental in the docs, so breaking changes are permissible, but this is still a deliberate API-semantics change: echo_u8(300) moves from 255 to 44, echo_u32(-1) from 0 to 4294967295, and echo_i64(5.7) from throwing to 5n. That's exactly the kind of "API design" decision REVIEW.md says needs maintainer agreement. The implementation is small and well-reasoned (the #7007 Int32Tag invariant is preserved, and the u64 path correctly routes negatives through BigInt to avoid the undefined C cast / toUInt64NoTruncate clamp), but whether wrap-over-saturate is the right policy is a maintainer call, not a mechanical fix.

Other factors

• The two nit-level findings from earlier review passes (using dir for tempDir disposal, Bun.which("cc")||gcc||clang probe with skipIf) have both been applied and the threads are resolved.
• Test coverage is thorough across widths, boundaries, fractions, NaN/Infinity, and out-of-range BigInt, with an in-range identity sweep. The PR body states 7/9 tests fail on the unfixed build.
• I checked that the arrow-wrapper codegen shape (val=>${wrapper})(pN) still works for both the expression-body (val|0) and block-body (64-bit) wrapper strings.
• CI build #78115 for the latest commit is still in flight per the robobun status comment.

[robobun]

CI build #78115: the new test/js/bun/ffi/ffi-int-coercion.test.ts passes on every lane. Remaining red is unrelated to this diff:

• test/js/node/test/parallel/test-net-connect-memleak.js and test-gc-http-client-connaborted.js: pre-existing GC-timing failures on main (both reported for triage)
• proxy-stress-errors.test.ts, test-fs-promises-file-handle-readFile.js, test-stdin-from-file-spawn.js: flaky, passed on retry or other lanes

The change is confined to the ffiWrappers table in src/js/bun/ffi.ts plus docs; none of the above touch the FFI path. Ready for review on the policy choice (wrap vs. saturate).

[Jarred-Sumner]

(Claude wrote this comment.)

Superseded by #35246: the per-type argument-coercion wrappers in ffi.ts were removed and integer coercion now happens in the JSC engine, so the modular-wrap unification here no longer has code to apply to. Closing.