Make 13 hand-written extern "C" declarations agree between Rust and C++ - #38943
Conversation
… and lint for it Rust and C++ only meet in the linker, which matches names, not signatures, so a declaration that disagrees with the definition links and silently relies on the calling convention tolerating the difference. Thirteen such symbols exist; all of them predate the Rust port: - Bun__JSWrappingFunction__create: Rust passed a fifth `strong` argument the C++ definition never had. - ByteRangeMapping__getSourceID: C++ passed a second BunString argument the Rust definition never had. - ffi_vfprintf / ffi_vprintf / ffi_vsscanf: declared variadic in Rust, defined with a va_list parameter in C++. - URL__originLength: declared `-> u32`, defined returning size_t. - Bun__setExitCode, Bun__closeChildIPC, Bun__ensureProcessIPCInitialized, Bun__setTLSRejectUnauthorizedValue, Bun__setVerboseFetchValue: declared with a scalar return in C++, defined returning nothing in Rust (every caller ignores the value). - Bun__reportUnhandledError: returned a constant `undefined` that C++ declares as void and never reads; the impl now returns nothing. - WebCore__AbortSignal__signal: returned its argument, which the Rust declaration (void) never read; the C++ now returns void. test/internal/source-lints/extern-c-signatures.test.ts collects every hand-written site of every C-ABI symbol (Rust extern blocks including jsc_abi_extern! and #[link_name], #[unsafe(no_mangle)] definitions, HOST_EXPORT impls; C++ extern "C" declarations and definitions, extern "C" blocks, and headers.h's CPP_DECL/ZIG_DECL) and requires all sites of a symbol to agree on parameter count and, where both return types are scalars of known width, on return width. Anything it cannot parse is skipped. The source-lints workflow now also triggers on the C++ it reads.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
WalkthroughThe pull request aligns native and Rust FFI declarations with their callers. It updates return types, removes redundant parameters, models ChangesFFI declaration alignment
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. Scope is now the 13 signature corrections only. The source lint from the first revision was removed from this PR in 31377e1; 79e1d6e drops the workflow trigger change that only existed for it and shortens the Verification: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I checked the C++ call sites of the five functions whose declared return type changed to void (Bun__setExitCode, Bun__closeChildIPC, Bun__ensureProcessIPCInitialized in BunProcess.cpp; Bun__setTLSRejectUnauthorizedValue, Bun__setVerboseFetchValue in JSEnvironmentVariableMap.cpp) — every caller is a statement expression that already discards the result. WebCore__AbortSignal__signal has no C++ callers at all (only the Rust AbortSignal.rs wrapper, which was already -> ()).
Extended reasoning...
This PR adds ~960 lines of new source-lint infrastructure (hand-rolled regex scanners for Rust and C++ extern-"C" declarations) plus 13 FFI signature corrections across production Rust and C++. The signature changes are individually small and well-justified in the description, and I spot-checked the void-return conversions against their call sites — none read the return value. But the combination of a large new lint that every future extern-"C" edit will run against, plus scattered ABI-boundary edits, is not the kind of change I should approve without a human look. The one inline nit (mislabeled safe fn shapes-table entry) is minor and does not affect correctness of the lint today.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Defined in c-bindings.cpp. Only their addresses are taken (handed to | ||
| // TinyCC below); the `ap` parameter of the `v*` variants is a `va_list`, | ||
| // declared here as an opaque pointer. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
robobun
changed the title
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
.github/workflows/source-lints.yml:15-19— Commit 31377e1 ("Delete slop test") removedtest/internal/source-lints/extern-c-signatures.test.ts, so the net diff ships no test at all — the 13 extern-C signature fixes have zero regression coverage (REVIEW.md: "Every behavioral change ships an automated test in the same PR"), and the PR title's "and lint for it" plus the description's "Adds test/internal/source-lints/extern-c-signatures.test.ts" are now false. Either restore the lint (addressing whatever made it "slop", plus the previously-flaggedsafe fnshapes-entry issue) or update the PR title/description and add targeted regression tests for the signature changes. The workflow path-filter widening added here does incidentally benefitno-iostream-include.test.ts, so it's not dead — but its stated motivation in this PR ("the lint reads" C++) no longer applies.Extended reasoning...
What the bug is
The PR's second commit,
31377e13"Delete slop test" (authored by Jarred Sumner), deletes the 959-linetest/internal/source-lints/extern-c-signatures.test.tsthat the first commitd6d42a9fadded.ls test/internal/source-lints/confirms the file is absent from HEAD, and the PR's net changed-files list (10 files) contains no test file. Yet the PR title still says "…and lint for it", and the PR description devotes multiple paragraphs to documenting a test that no longer ships ("Addstest/internal/source-lints/extern-c-signatures.test.ts, which collects every hand-written site of every C-ABI symbol…"; "the lint fails on main with exactly the 13 symbols above… and passes on this branch"; "the lint takes about 2 s on a release bun"). Both the title and description are now materially false.Why this is blocking
REVIEW.md is explicit under Tests reviewers reject: "Every behavioral change ships an automated test in the same PR. 'Verified manually', unnamed 'existing tests', and benchmarks don't count, even for one-liners." The 13 extern-C signature corrections across
BunProcess.cpp,JSEnvironmentVariableMap.cpp,ZigSourceProvider.cpp,bindings.cpp,headers.h,virtual_machine_exports.rs,ffi_body.rs,expect.rs, andurl/lib.rsare behavioral changes to the FFI ABI contract, and they now ship with zero regression coverage. Nothing in the tree prevents the next PR from re-introducing a mismatched declaration — which is the exact class of undefined behaviour this PR set out to eliminate. REVIEW.md also states "Never silently weaken, skip, or delete an existing test or safety net. Every deletion needs a stated reason or replacement" — the commit message "Delete slop test" states no reason and provides no replacement.Step-by-step proof
git show --stat 31377e13→test/internal/source-lints/extern-c-signatures.test.ts | 959 ---(part of this PR's ranged3f975bd..31377e13per the coderabbit metadata).ls test/internal/source-lints/→ 22 test files,extern-c-signatures.test.tsnot among them.- The 10-file net diff (workflow yml + 9 source files) contains no
.test.ts. - Suppose a future PR reintroduces
extern "C" bool Bun__setExitCode(void*, uint8_t)inBunProcess.cpp(the pre-fix signature). The linker matches by name only, so it links; every C++ caller ignores the return value, so it passes existing runtime tests; and no lint exists to catch the mismatch. The invariant the PR title claims to enforce is unenforced. - The earlier review comment on line 907 (the
safe fnshapes entry pointing atURL__originLength, which is a plainfn, notsafe fn) was resolved by deleting the whole file rather than by fixing the entry — the underlying feedback was never addressed.
The workflow path-filter widening
One correction to a possible reading of this finding: the added paths in
.github/workflows/source-lints.yml(src/**/*.cpp,src/**/*.h,packages/bun-usockets/src/crypto/**) are not entirely dead.no-iostream-include.test.ts:25already setsroots = ['src', 'packages/bun-uws', 'packages/bun-usockets']and globs C++ headers/sources, so the widening incidentally closes a pre-existing under-triggering gap for that lint. However, the PR description's stated motivation for the widening — "widens .github/workflows/source-lints.yml's path filter to the C++ the lint reads" — refers to the deleted lint, so the rationale as written no longer applies.How to fix
Either (a) restore
extern-c-signatures.test.ts— addressing whatever made it "slop" (presumably size/complexity), plus fixing thesafe fnshapes entry to point at an actualsafe fnsibling likeURL__protocol— or (b) if the deletion is intentional: drop "and lint for it" from the PR title, remove the test documentation from the PR description, and add targeted regression tests for the 13 signature changes so the PR meets REVIEW.md's automated-test requirement. The workflow widening can stay in either case (it benefitsno-iostream-include.test.ts), but under option (b) its description-line rationale should be updated.
2 participants
Problem
Bun__JSWrappingFunction__create(Rust passes a 5thstrongargument,JSWrappingFunction.cpp:57takes 4),ByteRangeMapping__getSourceID(ZigSourceProvider.cpp:43passes a 2ndBunString,CodeCoverage.rs:845takes 1),ffi_vfprintf/ffi_vprintf/ffi_vsscanf(declared variadic inffi_body.rs, defined with ava_listparameter inc-bindings.cpp).URL__originLength(url/lib.rs:74saysu32,BunString.cpp:570returnssize_t);Bun__setExitCode,Bun__closeChildIPC,Bun__ensureProcessIPCInitialized(BunProcess.cpp),Bun__setTLSRejectUnauthorizedValue,Bun__setVerboseFetchValue(JSEnvironmentVariableMap.cpp) declared with a scalar return in C++ while the Rust definitions return nothing;Bun__reportUnhandledErrorreturns a constantundefinedthatZigGlobalObject.h:90declares asvoid;WebCore__AbortSignal__signalreturns its argument, which the Rust declaration (void) never reads.Fix
strong/sourceURLarguments are dropped (C++ never readstrong; Rust never readsourceURL, and theBun::toStringthat built it is a non-owning view, so nothing was leaked or needs releasing);ffi_v*declarations get ava_listparameter, spelled as an opaque pointer (only their addresses are taken, for TinyCC; on every target bun builds for ava_listargument travels as one pointer-sized value);URL__originLengthbecomesusize, and theas usizeat its only call site goes away;void(every C++ caller already discards the value);report_unhandled_errorstops returning its constant (no Rust callers; the C++ declaration and all eight C++ callers already treat it as void);WebCore__AbortSignal__signalreturns void inbindings.cppandheaders.h(no C++ callers; the Rust declaration was already void).bun bd(the regeneratedBun__reportUnhandledErrorthunk is now-> ()) andbun bd testontest/js/bun/test/expect-extend*.test.*andjest-extended.test.js(JSWrappingFunction),test/cli/test/coverage.test.tsplus a manualbun:jsccodeCoverageForFilerun (ByteRangeMapping__getSourceID),test/js/node/process/process.test.js,test/js/web/abort/abort.test.ts,test/js/bun/spawn/spawn.ipc.test.ts,test/js/node/child_process/child_process_ipc.test.js,test/js/node/events/event-emitter.test.ts,test/js/node/timers/node-timers.test.ts,test/js/web/fetch/fetch.tls.test.ts.cargo fmt --checkand clang-format are clean.ByteRangeMapping__getSourceIDaltogether as part of a larger coverage change; this PR only corrects its declaration.Background
extern "C"linkage: a Rustextern "C" { fn X(..); }item (or#[unsafe(no_mangle)] extern "C" fn Xdefinition) and a C++extern "C"declaration or definition are matched by the linker purely by the nameX; each compiler generates its calls and prologues from its own local copy of the signature, so the copies can disagree without any diagnostic.HOST_EXPORT: a// HOST_EXPORT(Sym)comment above a safe Rust fn makessrc/codegen/generate-host-exports.tsemit the#[unsafe(no_mangle)]thunk forSymwith the impl's parameters and return type, which is why changingreport_unhandled_error's Rust signature is what changes the exported symbol's.headers.hspellsextern "C"asCPP_DECL;bindings.cppincludes it, so itsWebCore__AbortSignal__signalline has to change together with the definition.How the 13 were found (lint output from the first revision; the lint itself is no longer in this PR)
The lint compared parameter counts and return widths of every hand-written
extern "C"site on both sides (1342 symbols declared in both languages); these 13 were the only disagreements in the tree.