docs(ffi): cover cc, viewSource, Node-API types, and fix CFunction usage - #31535
docs(ffi): cover cc, viewSource, Node-API types, and fix CFunction usage#31535robobun wants to merge 20 commits into
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughDocs and runtime changes add ChangesFFI API Documentation and Implementation Enhancements
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 1:32 AM PT - Jun 30th, 2026
❌ @robobun, your commit df6f525 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 31535That installs a local version of the PR into your bun-31535 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/js/bun/ffi/cc.test.ts`:
- Around line 107-113: The test currently creates temp dirs using
tempDirWithFiles (see variable dir and the calls at the snippet around hello.c
and again in the 146-163 region); replace those uses with harness.tempDir using
the file-tree overload (tempDir({...}) style) so the temporary directories and
files are created via the repo's harness helper. Locate every invocation of
tempDirWithFiles in this test (including the hello.c block and the later suite
at lines 146-163) and change them to tempDir(...) from harness, preserving the
same file contents and names but switching to the file-tree overload API.
- Around line 164-177: The test currently swallows all errors from the cc(...)
call, hiding real failures; update the try/catch around the cc({ source:
path.join(dir, "get_type.c"), symbols: { get_type: {...} } }) invocation so the
catch only suppresses the specific Node-API-header-missing error (inspect the
error message or error.code for the known indicator) and rethrow any other
errors; apply the same tightening to the other catch that currently sets res =
undefined so only known header-unavailable failures downgrade the test,
otherwise propagate the error.
🪄 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: da79579b-6485-427f-ad93-309dadf809fd
📒 Files selected for processing (4)
docs/runtime/c-compiler.mdxdocs/runtime/ffi.mdxsrc/js/bun/ffi.tstest/js/bun/ffi/cc.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/c-compiler.mdx`:
- Line 69: Update the table row for the u64 alias to include size_t in the
listed C types so the docs match the runtime/type mappings; specifically modify
the row that currently reads "| u64 | `uint64_t` | `uint64_t`, `usize` |" to add
`size_t` (so the third column contains `uint64_t`, `usize`, `size_t`) so u64
documents both `usize` and `size_t`.
🪄 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: 48ee3b3f-5e9f-4947-94da-4af09f2f10f3
📒 Files selected for processing (6)
docs/runtime/c-compiler.mdxdocs/runtime/ffi.mdxpackages/bun-types/ffi.d.tssrc/js/bun/ffi.tstest/integration/bun-types/fixture/ffi.tstest/js/bun/ffi/cc.test.ts
There was a problem hiding this comment.
No issues in this revision — all earlier feedback is addressed. Deferring to a human since the scope grew from docs into runtime cc() behavior (making FFIBuilder reachable changes arg coercion for every cc symbol, plus the shared ffiWrappers napi entries affect dlopen/linkSymbols too).
Extended reasoning...
Overview
What began as a docs-drift fix for bun:ffi now spans six files: two docs pages (ffi.mdx, c-compiler.mdx), the public type definitions (packages/bun-types/ffi.d.ts), the runtime module (src/js/bun/ffi.ts), a bun-types integration fixture, and test/js/bun/ffi/cc.test.ts. The docs/types/test portions are straightforward. The substantive part is four runtime fixes in src/js/bun/ffi.ts: (1) cc() now reads symbol definitions from options.symbols[key] instead of options[key], which makes FFIBuilder reachable for cc for the first time; (2) identity ffiWrappers entries for napi_env/napi_value; (3) self-referential "18"/"19"/"20" keys in the FFIType map; (4) a displayPath guard for array source. A size_t alias was also added across runtime/types/docs.
Security risks
bun:ffi is inherently unsafe-by-design (raw pointers, JIT-compiled C wrappers), and the module already carries a "do not rely on in production" warning. The changes here don't introduce new attack surface — they tighten argument coercion and fix a wrapper that was previously dead code. The ffiWrappers[napi_value] = "val" identity wrapper passes JSValues through unchanged, which is what the native side already expected; no new injection vector. I see no security regression.
Level of scrutiny
Medium-high. The docs and .d.ts JSDoc edits are low-risk. But fix #1 is a real behavioral change: every cc() symbol with non-empty args now goes through FFIBuilder's coercion table where it previously bypassed it entirely. The PR itself discovered three knock-on regressions from that change (napi coercion, numeric-enum reverse lookup, size_t lookup) over the course of review, which is exactly why a human should confirm the surface is now fully covered. Fixes #2/#3 also touch the shared dlopen/linkSymbols path.
Other factors
I left ~13 inline comments across earlier revisions; every one is resolved and the current diff reflects all of them (including the most recent size_t-in-docs and JSDoc-sync nits, addressed in 06eb05a/8caa30c). The new regression tests in cc.test.ts are self-contained (no node-gyp dependency) and directly cover each runtime fix. The bug-hunting system found nothing in this revision. The one open thread I'd note for the human reviewer: CI build #58860 showed build-rust failures across platforms on an earlier commit — likely infra, since this PR touches no Rust, but worth confirming green before merge. The tempDirWithFiles style nit from CodeRabbit was resolved without a code change (it matches the file's existing convention).
There was a problem hiding this comment.
No new issues found — the docs and type changes look clean, but the runtime change in src/js/bun/ffi.ts (making FFIBuilder reachable for every cc() symbol with args, plus the napi/size_t wrapper additions) is a behavioral change to the FFI hot path that's worth a human sign-off.
Extended reasoning...
Overview
This PR spans six files: two docs pages (ffi.mdx, c-compiler.mdx), the public type definitions (packages/bun-types/ffi.d.ts), the runtime FFI module (src/js/bun/ffi.ts), a bun-types integration fixture, and test/js/bun/ffi/cc.test.ts. It started as a docs-drift fix but, over ~14 rounds of review, grew to include four interrelated runtime fixes in cc(): reading symbol definitions from options.symbols[key] (which makes FFIBuilder reachable for cc for the first time), identity ffiWrappers for napi_env/napi_value, self-referential FFIType["18"|"19"|"20"] keys, a size_t alias, and a displayPath guard for array sources. Each fix has a dedicated regression test.
Security risks
FFI is inherently safety-sensitive (raw pointers, JIT-compiled C, native calls), but the changes here don't introduce new attack surface — they tighten argument coercion and fix a wrapper-skipping bug. The napi_value identity wrapper passes JS values through unchanged, which is what the native side already expected. No new user-controlled input reaches native code that didn't before.
Level of scrutiny
The docs/d.ts/test portions are mechanical and low-risk. The src/js/bun/ffi.ts change is the part that warrants human review: switching cc() from options[key] to options.symbols[key] flips a previously-dead branch live for every cc() symbol with non-empty args, newly subjecting all such calls to ffiWrappers coercion. The review history itself demonstrates the subtlety — each fix surfaced a new edge case (napi coercion → numeric enum lookup → size_t → array-source label). The final state looks correct and is well-tested, but a maintainer familiar with the FFI codegen should confirm there are no remaining arg-type strings the native side accepts that FFIBuilder will now reject.
Other factors
All prior inline comments (mine and CodeRabbit's) are resolved, and the current bug-hunting pass found nothing. CI showed a failure on an earlier commit (8036821) and was retriggered on e18f75a; I'd confirm it's green before merge. The PR description also notes that test/napi/napi-value-ffi.test.ts has it.todo cases this unblocks — un-skipping those is flagged as a follow-up.
|
CI note for maintainers: this diff is green. Every red on build #67155 (rebased onto Final state of #67155:
The FFI regression tests in My one CI re-roll is spent (HEAD |
|
Heads up: #32075 also carries the cc |
Updates the bun:ffi documentation to match the exported API: - Add a "Compiling C from JavaScript (cc)" section with a C11 example, cross-linking the dedicated C Compiler page for the full reference. - Add void, usize, and isize to the FFIType table (usize/isize are aliases of u64/i64; void is the default return type and invalid as an argument). All three are accepted by the runtime ABI parser. - Add a Node-API types section explaining napi_env/napi_value and linking the C Compiler page's worked examples. - Add a "Viewing generated bindings" section documenting viewSource. - Add read.intptr to the pointer-reading table. - Drop the incorrect `new` keyword from the CFunction examples; it is a factory function, not a constructor. Also fixed the CFunction JSDoc example in ffi.d.ts.
Covers the bun:ffi type surface documented in this PR: - CFunction is a factory function — assert it is callable without `new`, exposes close(), and is NOT constructable (`new CFunction(...)` is a type error). This guards against regressing it back into a constructor. - cc() returns a Library whose symbols and close() are typed correctly. - viewSource() returns string[] for a symbol map and string for a single callback definition. These run as part of test/integration/bun-types/bun-types.test.ts, which packs bun-types and type-checks the fixtures.
Revert the ffi.d.ts JSDoc tweak and the bun-types fixture additions so this PR is documentation-only. The issue is a docs fix (docs/runtime/ffi.mdx); a JSDoc comment and prose aren't unit-testable, and docs-only changes don't carry tests in this repo.
- Drop `isize` from the i64 alias column. Only `"usize"` is in the TypeScript string types; `"isize"` works at runtime but fails type-checking. The note now explains this and points to `i64` for type-checked code. - Relabel the `read` table header from `FFIType` to `Type` and add a footnote: `intptr` is a `read` helper only, not an `FFIType`, so it can't be used in args/returns.
`cc` read each symbol definition from `options[key]`, but `cc` nests the
definitions under `options.symbols` (unlike `dlopen`/`linkSymbols`, where
`options` is the symbol map itself). So `options[key]` was always
`undefined`, `FFIBuilder` was never invoked, and a `cstring` return was
handed back as a raw pointer number instead of a `CString`.
Read from `options.symbols[key]` instead. Now `cc({ symbols: { f: {
returns: "cstring" } } })` returns a `CString`, matching the documented
behavior and the ffi.d.ts JSDoc example.
Also document the `include` option on the C Compiler docs page, which the
ffi docs cross-link references.
`cc` accepts an array of source files, which it normalizes in place while leaving `path` as an array. The stack-trace label (now reachable after the cstring fix) called `path.includes`/`path.split`, which assume a string. Use the first file (`path[0]`) for the label so array sources produce a sensible name instead of a comma-joined string (and never hit `.split` on an array).
The `cstring` fix made `FFIBuilder` reachable for `cc` symbols with arguments, which applies the `ffiWrappers` coercion table. `napi_value` and `napi_env` had no entry, so they fell back to the default `val|0` coercion — turning a `napi_value` argument into `0` (or throwing on a BigInt) before it reached native, where it is read as a raw JSValue. Add identity wrappers so both pass through untouched. This also fixes the same pre-existing breakage for `dlopen`/`linkSymbols` symbols that take a `napi_value` argument. Adds a self-contained cc regression test that compiles a napi_typeof-based get_type(napi_env, napi_value) and asserts the argument type is reported correctly (number/string/boolean/bigint) instead of always "number".
The get_type regression test caught every cc() error and turned itself into a no-op, which could mask unrelated compile/link regressions. Narrow the catch to the "include file '...node_api.h' not found" case and rethrow anything else.
- Rework the napi_value-argument regression test to use an identity function. `napi_value` is an opaque pointer, so the C source needs no Node-API headers and links nothing — it just round-trips the raw JSValue. This removes the header-availability dependency (and the try/catch that could hide real failures) and sidesteps the Windows x64 "TCC can't link napi" issue, since nothing napi is linked. Asserts the returned value is identical (Object.is) to the input, which fails when the argument is coerced. - Drop the unused `#include <stdio.h>` from the cc docs example; the function only returns a string literal, and the include can fail to resolve where the host lacks C dev headers.
The `FFIType` map carried self-referential numeric keys `"0"`–`"17"` so `FFIType[FFIType.i32]` resolves, but was missing `"18"`/`"19"`/`"20"` for `napi_env`/`napi_value`/`buffer`. `FFIBuilder` resolves each argument's wrapper via `ffiWrappers[FFIType[arg]]`, so the numeric enum form (`args: [FFIType.napi_value]`) looked up `FFIType[19]` → undefined and threw `Unsupported type 19`. The string form (`"napi_value"`) was unaffected. Add the three missing keys. This also fixes the same reverse-lookup hole for `dlopen`/`linkSymbols`. Extends the cc regression test to cover the numeric enum form.
The C Compiler page's "Primitive types" table says it mirrors the dlopen FFIType list, but was missing the `void` row and the `usize` alias added to the ffi page. Sync the two entries so they no longer disagree.
The numeric-form test created its own temp dir but never removed it, leaking a directory per run. The C source is identical to the one compiled in beforeAll, so reuse that path (cleaned up by afterAll) and drop the extra dir.
`cc` with `returns: "cstring"` returns a `CString` (a boxed `String`), which `console.log` renders as `[String: "Hello, World!"]`. Interpolate it so the example prints the bare string, matching the comment and the dlopen example at the top of the page.
The compiler accepts `"size_t"` (→ `uint64_t`) but it was missing from the JS `FFIType` map, so once `FFIBuilder` became reachable for `cc`, a `"size_t"` arg/return hit the `ffiWrappers[FFIType[type]]` lookup and threw `Unsupported type size_t`. Add `size_t: 8` alongside `usize: 8` (same as the native label map), and add a cc regression test. Also fixes the same gap for `dlopen`/`linkSymbols`.
The runtime `FFIType` map now maps `size_t`, but `FFITypeStringToType` didn't, so `args: ["size_t"]` / `returns: "size_t"` failed type-checking even though it works at runtime. Add `["size_t"]: FFIType.uint64_t` to match, and cover it in the bun-types fixture.
The `cc` and `CFunction` JSDoc `@example` blocks still showed the patterns
corrected in ffi.mdx: drop the unused `#include <stdio.h>`, interpolate the
CString result (`console.log(`${hello()}`)` prints the bare string), and
call `CFunction(...)` without `new` (it's a factory, not a constructor).
`size_t` is now a valid alias of `u64`/`uint64_t` in both the runtime map and the TypeScript string types, so list it in the FFIType tables on both the FFI and C Compiler pages.
darwin-aarch64-test-bun lanes expired (infra); diff is docs + ffi JS only and passes all build lanes + darwin-x64 tests.
|
Rebased onto |
e18f75a to
df6f525
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Closing: superseded. Most of this PR has been overtaken by #35246, which rewrote
The docs items that are still missing on |
1 participant
Closes #31534
The
bun:ffidocumentation (docs/runtime/ffi.mdx) had drifted from the exported API insrc/js/bun/ffi.ts/packages/bun-types/ffi.d.ts: several methods andFFITypes were undocumented, andCFunctionwas shown withneweven though it's a factory function.While documenting
cc, review surfaced several real runtime bugs incc, which this PR also fixes (with tests).Docs (
docs/runtime/ffi.mdx)cc— added a "Compiling C from JavaScript (cc)" section with a C11 example, cross-linking the C Compiler page for the full option reference.FFITypetable — addedvoid,usize, andisize, with notes on the runtime-vs-TypeScript-types nuance forisizeand thatvoidis return-only.napi_env/napi_value.viewSource— added a "Viewing generated bindings" section.readtable — addedread.intptr, relabeled the header toType, and added a footnote clarifyingintptris a reader-only helper.CFunction— removed the incorrectnewkeyword from both examples.#### includesection (it documented the otherccoptions but was missinginclude).Bug fixes (
src/js/bun/ffi.ts)cstringreturns —ccread each symbol definition fromoptions[key], butccnests them underoptions.symbols(unlikedlopen/linkSymbols). Sooptions[key]was alwaysundefined,FFIBuilderwas never invoked, and acstringreturn came back as a raw pointer number instead of aCString. Fixed to readoptions.symbols[key].napi_value/napi_envargument coercion — makingFFIBuilderreachable (fix Fix ?? operator #1) meantccsymbols with args now run theffiWrapperscoercion table, which had no entry fornapi_value/napi_env— so they fell back to the defaultval|0coercion, mangling anapi_valueargument into0(or throwing on BigInt). Added identity wrappers so both pass through untouched.napi_env/napi_value/bufferreverse-lookup — theFFITypemap was missing self-referential keys"18"/"19"/"20", so the numeric enum form (args: [FFIType.napi_value]) failedFFIBuilder'sffiWrappers[FFIType[arg]]lookup and threwUnsupported type 19. Added the keys.sourcelabel — the newly-reachable stack-trace label assumedpathis a string, butccalso accepts an array of files. Guarded withdisplayPath(first file).Fixes 2 and 3 touch the shared
FFIBuilderpath, sodlopen/linkSymbolssymbols that takenapi_value/napi_envarguments benefit from the same correction.Tests (
test/js/bun/ffi/cc.test.ts)Self-contained regression tests (no node-gyp needed —
napi_valueis an opaque pointer, so the fixtures need no Node-API headers and link nothing):cstringreturn yields aCStringequal to"Hello, World!"(not a number).napi_valueidentity function round-trips the exact same JS value (Object.is) — covering both the string ("napi_value") and numeric (FFIType.napi_value) forms.All verified fail-before / pass-after with
bun bd test.Notes
intalias forint32_t(issue item 6) was already present — left unchanged.cc/napi_env/napi_valuehave a dedicated page (docs/runtime/c-compiler.mdx); the ffi docs cross-link it rather than duplicating.test/napi/napi-value-ffi.test.tshasit.todocases fordlopen/cc+napi_valueargs that these fixes unblock on POSIX; un-skipping them is a sensible follow-up since that file requires a node-gyp addon build.