bun:ffi: remove ABIType::param_typename, a copy of ABIType::typename - #39124
Conversation
Both wrote typename_label() to the writer. The parameter and return positions of the generated prototype have always used the same C type name, so print_source_code now calls typename for both and the reimplemented_helper baseline entry for abi_type.rs goes away.
|
Warning Review limit reached
Next review available in: 32 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 (3)
Comment |
There was a problem hiding this comment.
LGTM — pure dead-code removal with no behavior change.
What was reviewed:
- Confirmed the deleted
param_typenamebody was byte-identical totypename(both writeself.typename_label()), so the swap atffi_body.rs:2138cannot change generated C. - Grepped
src/for remainingparam_typenamereferences — none; the single caller is updated. - Checked the mordant baseline edit removes only the one
reimplemented_helper:src/runtime/ffi/abi_type.rsentry it resolves, and the comment update accurately drops the reference to the never-portedparam_typename_label.
Extended reasoning...
Overview
This PR deletes ABIType::param_typename from src/runtime/ffi/abi_type.rs — a method whose body was identical to ABIType::typename (both do writer.write_all(self.typename_label())). Its sole caller in Function::print_source_code (src/runtime/ffi/ffi_body.rs:2138) is switched to typename, which is already what the adjacent return-type emission at line 2128 uses. The corresponding reimplemented_helper baseline entry is dropped from mordant-baseline.toml, and a stale comment mentioning param_typename_label is trimmed.
Security risks
None. This touches only the string spelling of C type names in FFI-generated wrapper source, and the substitution is between two functions with identical implementations. No input handling, allocation, or control flow changes.
Level of scrutiny
Low. The change is mechanical: replacing a call to f(x) with a call to g(x) where f and g had identical bodies, then deleting f. I verified via grep that no other param_typename callers exist in src/, and the preloaded abi_type.rs shows typename still present with the same body the deleted method had. The mordant baseline removal is self-checking — CI's mordant lane will fail if the duplicate somehow remains.
Other factors
The PR description documents byte-identical viewSource output against the released binary and passing ffi.test.js / cc.test.ts under the debug build. This aligns directly with REVIEW.md's "if your fix makes two functions byte-identical, delete one" and "delete dead code in the same PR that makes it dead." The author already flagged the merge-order interaction with #38057, so no hidden coordination risk. No CODEOWNERS entry covers src/runtime/ffi/.
### Problem - mordant's `reimplemented_helper` finding baselined for `src/jsc/webcore_types.rs`: `Blob::is_bun_file` and `Blob::needs_to_read_file` had the same signature and the same body (the store is `Some` and its data is `Store::File`), so a change to one would have silently missed the other. - The duplication is inherited, not a porting mistake: `Blob.zig` already had `isBunFile` and `needsToReadFile` with the same body, and neither side has changed since the port (#30412). Nothing relies on them differing. ### Fix - Delete `is_bun_file` and keep `needs_to_read_file`, which is what the ~40 other call sites (and the `blob_needs_to_read_file` hook that `bun_sql_jsc` goes through) already use. - Point the two `is_bun_file` callers at it: `Response.rs` (`getCompleteWebRequestOrResponseBodyValueAsArrayBuffer` returns `undefined` for a file-backed body) and `CryptoHasher.rs` (the synchronous hashers reject `Bun.file()` input). Same predicate, so no behavior change. - Remove the `reimplemented_helper:src/jsc/webcore_types.rs` entry from `mordant-baseline.toml`, and the two stale comments in `webcore/Blob.rs` that named the deleted method. The entry is removed by hand rather than by regenerating the file: a full `bun run rust:mordant:baseline` on Linux also drops two unrelated entries (`always_unwrapped_option` in `PackageInstall.rs`, `narrowed_two_ways` in `node_crypto_binding.rs`) that this PR has no business touching. - No new test: there is no observable behavior to pin (the two predicates were byte-for-byte the same), so no test can distinguish before from after. The `Bun.file()` rejection at the `CryptoHasher` call site is already covered by `bun-cryptohasher.test.ts` ("Bun.file in CryptoHasher is not supported yet"), and the mordant run below is the check for the finding itself. Same shape as #39124, #39116 and #39135 from this batch. - Verified: - `bun run rust:mordant` (full workspace) on this branch: clean, no `target/mordant/over-baseline.txt`. With main's `webcore_types.rs` restored on top of the trimmed baseline it reports `reimplemented_helper ... over the mordant baseline (0 recorded for src/jsc/webcore_types.rs)` and `1 finding(s) over the baseline in bun_jsc`, so the entry was this site and the lint no longer fires. - The `mordant` workflow also runs on this PR since the baseline changed; with the entry removed it fails if the finding is still reported. - `bun bd test test/js/bun/util/bun-cryptohasher.test.ts` (covers the `CryptoHasher` call site: `Bun.file()` input still throws): 402 pass. - `bun bd test` on `test/js/web/fetch/{blob,blob-write,body,blob-file-name-ownership,response}.test.ts`, `test/js/web/structured-clone-blob-file.test.ts` and `test/js/bun/io/bun-write.test.js` (`--timeout 60000`, since several of these spawn a debug+ASAN child and exceed the 5s default): everything passes except `blob.test.ts` "Bun.file(path).slice(start, end) streams only the slice", which fails identically on a clean checkout of main (88a6398) and is being handled separately; it is a `FileReader` streaming bug unrelated to this predicate. - Two open PRs add `is_bun_file()` calls in `Blob.rs` (#32434, #33659); whichever lands after this one needs those calls renamed to `needs_to_read_file()`. That is a compile error, not a silent change. ### Background - `Blob` (`src/jsc/webcore_types.rs`) is a view (offset + size) onto a refcounted `Store`, whose `data` is one of `Bytes` (in memory), `File` (a path or fd; what `Bun.file()` creates) or `S3`. `needs_to_read_file()` asks whether the blob is `File`-backed, i.e. whether its bytes have to be read off disk before anything in memory can look at them; synchronous consumers use it to bail out. - mordant is the advisory Rust lint pack the `rust-lints` workflow runs (`bun run rust:mordant`). `mordant-baseline.toml` holds the per-(lint, file) counts of findings that predate the job, so CI only fails when a count goes up; once a baselined finding is fixed its entry is deleted. Co-authored-by: Alistair Smith <hi@alistair.sh>
Problem
ABIType::param_typename(src/runtime/ffi/abi_type.rs:229) had the same signature and body asABIType::typename(abi_type.rs:220): both writeself.typename_label()to the writer. A change to one would silently miss the other. This is thereimplemented_helperfinding baselined forabi_type.rsinmordant-baseline.toml.paramTypenamewas added in fix(ffi): fix ffi with largeuint32_tvalues #7009 already delegating totypenameLabel(), the same astypename. The variant that would have differed (paramTypenameLabel, which spelleduint32_tparameters asint32_t) was never called by anything and was dropped as dead code whenffi.zigwas ported. Largeuint32_targuments do not depend on it:print_source_codeloads every small integer argument as the rawint64_tencoding and passes it to a prototype whose parameter type (uint32_t) truncates it to the low 32 bits, so the parameter and return positions have always used the same C type name.Fix
param_typename; the only caller (Function::print_source_codein src/runtime/ffi/ffi_body.rs, the prototype's parameter list) now callstypename.reimplemented_helper:src/runtime/ffi/abi_type.rsentry frommordant-baseline.tomland drop the mention of the never-portedparam_typename_labelfrom the table comment.viewSourceprototype for a symbol taking all 20 argument types is byte-identical to the one the releasedbunemits (see below).bun bd test test/js/bun/ffi/ffi.test.js --timeout 120000: 157 pass (the--timeoutis only because the exhaustive integer identity tests exceed the 5s default under a debug+ASAN build; CI passes its own larger per-test timeout).bun bd test test/js/bun/ffi/cc.test.ts test/js/bun/ffi/ffi-viewSource-non-object.test.ts test/js/bun/ffi/ffi-error-messages.test.ts: 35 pass.mordantworkflow runs on this PR since the baseline changed; with the entry removed, it fails if the finding is still reported.param_typenamecaller inffi_body.rs; whichever of the two lands second needs that one call renamed totypename.Background
bun:ffi'scc()/dlopen()fallback path generates a C wrapper per symbol and compiles it with the bundled TinyCC.print_source_codeemits the prototype of the user's function (<ret> name(<param> arg0, ...)) and the wrapper that unpacksEncodedJSValues and calls it;typenamesupplies the<ret>and<param>spellings from the per-variantABI_TABLE.mordantis the dylint pack run bybun run rust:mordant;mordant-baseline.tomlholds per-(lint, file) counts of pre-existing findings, and CI fails only on findings above the baseline.Prototype comparison, released bun vs this branch
Both binaries emit: