Skip to content

bun:ffi: accept FFIType.buffer and FFIType.buffer_length as numeric type tags - #38430

Open
robobun wants to merge 1 commit into
mainfrom
farm/02b14e16/ffi-numeric-buffer-types
Open

bun:ffi: accept FFIType.buffer and FFIType.buffer_length as numeric type tags#38430
robobun wants to merge 1 commit into
mainfrom
farm/02b14e16/ffi-numeric-buffer-types

Conversation

@robobun

@robobun robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • Delete ABIType::MAX and the <= MAX filter; the numeric branches now use ABIType::from_int alone.
  • from_int is an exhaustive match over the enum's discriminants and returns None for anything else, so it is already the range check; MAX was a second copy of the same fact, and the second copy is the one that went stale twice.
  • Nothing downstream changes: both spellings produce the same ABIType, so the existing rejections (Cannot return a buffer ..., buffer_length is an argument-only type ..., and reject_cc_unsupported_types_error for cc/viewSource/JSCallback) now apply to the numbers exactly as they do to the strings. Unknown numbers (22, -1, values that saturate in to_int32) still throw invalid ABI type.
  • Tests (test/js/bun/ffi/ffi.test.js): linkSymbols over JSCallbacks bound with [FFIType.buffer, FFIType.buffer_length] receives the view's data pointer (including subarray / DataView offsets) and byteLength; dlopen of the C fixture with the numeric spelling returns the same values as the existing string test; viewSource emits identical source for both spellings; numeric buffer / buffer_length return types and numeric buffer_length arguments in cc / viewSource / JSCallback get the type-specific errors rather than invalid ABI type; out-of-range numbers still get invalid ABI type.
  • Verified: the 6 new cases fail on the released binary (USE_SYSTEM_BUN=1, all with invalid ABI type) and pass with bun bd test; bun bd test test/js/bun/ffi/ is otherwise unchanged (the integer identities cases time out locally under ASAN on a loaded machine, with and without this change; they are green on main CI).

Background

  • ABIType is Bun's tag enum for FFI parameter and return types. A signature's args / returns entries may be strings (looked up in ABI_TYPE_LABEL) or numbers (the values exported as FFIType from bun:ffi, which are the enum discriminants); both paths produce an ABIType, and everything after parsing works on the enum.
  • buffer passes a TypedArray / DataView's data pointer; buffer_length passes that same view's byteLength, read at call time. Both are argument-only, which is why the return-type rejections exist, and buffer_length is only implemented by the engine-native binding, which is why cc / viewSource / JSCallback reject it.

…ype tags

Symbol signatures spelled with the numbers 20 (buffer) or 21
(buffer_length) were rejected with "invalid ABI type" while the string
spellings worked. generate_symbol_for_function filtered numeric tags
through ABIType::MAX, which was still napi_value (19): it was not bumped
when Buffer and BufferLength were added to the enum. ABIType::from_int
already returns None for unknown discriminants, so drop MAX and the
filter and let from_int be the only range check.
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

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

Next review available in: 10 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: 4b7700bb-9da0-4d89-93ca-c81cc431db64

📥 Commits

Reviewing files that changed from the base of the PR and between 01aa7cd and 98305a1.

📒 Files selected for processing (3)
  • src/runtime/ffi/abi_type.rs
  • src/runtime/ffi/ffi_body.rs
  • test/js/bun/ffi/ffi.test.js

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

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review.

Reproduced on the released binary (1.4.0) with viewSource({ f: { args: [FFIType.buffer], returns: "void" } }) and with dlopen(..., { f: { args: [FFIType.buffer, FFIType.buffer_length], returns: FFIType.u64 } }), both of which throw invalid ABI type while the string spellings work. The 6 new cases in test/js/bun/ffi/ffi.test.js fail on the released binary and pass with this branch's debug build.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — deletes the stale ABIType::MAX bound and relies on from_int's exhaustive match, so FFIType.buffer (20) and FFIType.buffer_length (21) now parse the same as their string spellings.

What was reviewed:

  • Confirmed ABIType::MAX has no other users in the tree, and from_int already returns None for anything outside 0–21, so the range check is preserved.
  • Checked the out-of-range test values against to_int32()'s saturating semantics — 2**31 and 2**32 + 20 saturate to i32::MAX, which from_int rejects.
  • Tests cover every entry point (dlopen/linkSymbols/CFunction/JSCallback/cc/viewSource) in both args and returns positions, plus the downstream type-specific rejections and out-of-range numbers.
Extended reasoning...

Overview

The PR fixes numeric FFIType tags 20 (buffer) and 21 (buffer_length/buffer_bytelength) being rejected with "invalid ABI type" while the equivalent string spellings work. The two numeric branches in generate_symbol_for_function (src/runtime/ffi/ffi_body.rs) filtered with int <= ABIType::MAX, and MAX was never bumped past NapiValue (19) when Buffer and BufferLength were added. The fix deletes ABIType::MAX entirely and lets ABIType::from_int — an exhaustive match over discriminants 0–21 that returns None for anything else — be the single range check. Net native change: 2 deleted lines in abi_type.rs, ~8 lines reshaped to let-else in ffi_body.rs. The rest is ~110 lines of new tests in test/js/bun/ffi/ffi.test.js.

Security risks

None. The string path already accepted "buffer" and "buffer_length" and produced the exact same ABIType variants; this change only makes the numeric spelling reach the same enum values. All downstream validation (argument-only rejection for both types as return values, reject_cc_unsupported_types_error for buffer_length in cc/viewSource/JSCallback) is unchanged and now applies uniformly. Out-of-range numbers still hit from_int's _ => None arm; I traced 2**31 and 2**32 + FFIType.buffer through is_any_int() (JSC's int52 check, so both qualify) → to_int32() (saturates to i32::MAX per JSValue.rs:759-763) → from_int(i32::MAX)None, so the test assertions are sound.

Level of scrutiny

Low-to-medium. The native change is a mechanical removal of a redundant guard that had gone stale twice — exactly the "one source of truth" pattern REVIEW.md calls out. from_int is visibly exhaustive over the enum in abi_type.rs, and a repo-wide grep confirms ABIType::MAX had no other consumers. The refactor to let Some(t) = ... else { return ... } is behavior-preserving and idiomatic.

Other factors

Test coverage is thorough and follows the review guidelines: every FFI entry point is exercised with the numeric spelling in both args and returns; positive assertions check actual data-pointer/byteLength values (including subarray/DataView offsets and Float64Array byte-width) rather than just "doesn't throw"; viewSource output is compared byte-for-byte against the string spelling; error-path assertions check the specific error message rather than bare toThrow(); out-of-range numbers are still asserted to fail. Resources are released via try/finally before assertions could leak them. The PR description states the new cases fail under USE_SYSTEM_BUN=1 and pass under bun bd. No outstanding reviewer comments (only a coderabbit rate-limit notice).

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 5:05 AM PT - Aug 14th, 2026

@robobun, your commit 98305a1 has some failures in Build #95688 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 38430

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

bun-38430 --bun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant