ast: make cargo test -p bun_ast link natively - #37611
Conversation
The crate's test binary references seven mimalloc entry points through the TapeAlloc::Arena arm of the tape allocator (mi_heap_malloc, mi_heap_malloc_aligned, mi_malloc_usable_size, mi_is_in_heap_region, mi_free_size and mi_free_size_aligned in the dev profile; mi_free and mi_heap_malloc in release), but mimalloc is only linked into the full bun binary, so `cargo test -p bun_ast` failed at link time on every host while the Miri lane, which never links, stayed green. Add src/ast/native_test_shims.rs, mounted #[cfg(test)], defining those symbols. Nothing in the test binary references mi_heap_new or mi_heap_main, so no test can hold an arena and the shims abort if reached instead of allocating. test/internal/rust-ast-cargo-test.test.ts runs `cargo test -p bun_ast --lib` in both profiles on hosts with cargo and a configured checkout.
|
Warning Review limit reached
Next review available in: 25 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 |
|
Status: reproduced on main (97e21e5) with |
There was a problem hiding this comment.
LGTM — test-only shims following the established src/parsers/native_test_shims.rs pattern, gated behind #[cfg(test)] so nothing here ships in the real binary. The one inline nit (wrong test filename in the doc comment) is worth fixing but doesn't block.
Extended reasoning...
Overview
This PR fixes cargo test -p bun_ast failing to link natively because the crate's test binary references seven mimalloc symbols (mi_heap_malloc, mi_free, etc.) that only exist in the full bun link. It adds src/ast/native_test_shims.rs — a #[cfg(test)]-mounted module defining those seven symbols as aborting stubs — plus a two-line hook in lib.rs and a new test/internal/rust-ast-cargo-test.test.ts that runs cargo test -p bun_ast --lib in both dev and release profiles to lock the fix in.
Security risks
None. The shim module is compiled only into the crate's own cargo test binary via #[cfg(test)]; it is never part of the shipped bun executable. The shims themselves abort via unreachable!() and cannot be reached (no mi_heap_new/mi_heap_main reference exists in the test binary, so no arena can be constructed). The new test spawns cargo locally with inherited env plus CARGO_TERM_COLOR=never — no network, no untrusted input.
Level of scrutiny
Low. This is test-infrastructure only, following an established workspace pattern verbatim: src/parsers/native_test_shims.rs uses identical #[unsafe(no_mangle)] extern "C" fn shims for highway/stack-check symbols, and the PR description cites two more recent instances (#37592, #37599). The new test file is a near-copy of test/internal/rust-windows-sys-link.test.ts (same workspaceResolvable gate, same skip conditions, same spawn/drain/assert shape), extended with test.each over dev/release and an assertion that the load-bearing tape test actually ran. I verified the bun_alloc::mimalloc::Heap import path resolves via pub use bun_mimalloc_sys::mimalloc in src/bun_alloc/lib.rs.
Other factors
The one finding is a doc-comment nit: the module comment at native_test_shims.rs:10-11 says rust-windows-sys-link.test.ts links both profiles, but that test runs a single dev-profile --no-run; the correct reference is this PR's own rust-ast-cargo-test.test.ts. It's a self-referential slip with no runtime effect, so it doesn't warrant holding the PR. The PR description's verification section is thorough (fails-without-fix confirmed per profile, each shim's necessity confirmed by deletion, clippy/fmt/miri clean), and the 180s test timeout is justified inline (cold target dir + fat-LTO release link).
|
Updated 10:21 PM PT - Aug 11th, 2026
✅ @robobun, your commit 48eb7a2b896d906f3e337633a83b40954f265846 passed in 🧪 To try this PR locally: bunx bun-pr 37611That installs a local version of the PR into your bun-37611 --bun |
|
Addressed the review so far: the doc comment pointed at the wrong test file (d607aec), and the module and test header comments are trimmed to what they need to say (9ba4b94, b8007c4). No code changes since the first revision; |
There was a problem hiding this comment.
LGTM — the doc-comment nit from the earlier pass is addressed, and no new issues in b8007c4.
What was reviewed:
#[cfg(test)]gate on the module mount inlib.rs— never compiled into the shipping binary.- Shim shape matches the
src/parsers/native_test_shims.rsprecedent;bun_alloc::mimalloc::Heapimport path is used elsewhere in-tree. - Test mirrors
rust-windows-sys-link.test.ts(sameworkspaceResolvableguard, concurrent pipe drain, exit-code-last); the asserted test namee::json_tape_tests::tape_rooted_at_a_mutable_borrow_still_accepts_writesexists atsrc/ast/e.rs:2564. - Checked that aborting shims can't silently mask a real call — the test runs (not
--no-run), so a reached shim would fail the row.
Extended reasoning...
Overview
Three files: a two-line #[cfg(test)] mod native_test_shims; mount in src/ast/lib.rs, a new 51-line src/ast/native_test_shims.rs defining seven #[unsafe(no_mangle)] extern "C" mimalloc symbols that all call unreachable!(), and a new test/internal/rust-ast-cargo-test.test.ts that spawns cargo test -p bun_ast --lib in dev and release profiles. The purpose is to make the crate's standalone test binary link — mimalloc is only present in the full bun link, and the TapeAlloc::Arena arm of the JSON tape allocator keeps references to mi_* symbols live even though tests only exercise the Global arm.
Security risks
None. The shim module is gated behind #[cfg(test)] and is never compiled into the production bun binary. The shims themselves are diverging (unreachable! → panic/abort), so even in the test binary they cannot corrupt state. The new TS test spawns a local cargo from which() inside the repo root with no network I/O and no untrusted input.
Level of scrutiny
Low. This is developer-tooling / test-infrastructure only, and it follows an existing in-repo pattern verbatim: src/parsers/native_test_shims.rs does the same thing for highway/stackcheck symbols, and test/internal/rust-windows-sys-link.test.ts is the template the new test copies (identical workspaceResolvable check, skipIf(isWindows || !cargo || ...), CARGO_TERM_COLOR: "never", concurrent Promise.all drain). No production code path, allocator behavior, or user-facing API is touched.
Other factors
My earlier review flagged one nit — the module doc comment referenced the wrong test file — which was fixed in d607aec and then trimmed further in 9ba4b94/b8007c46 in response to the comment-cop bot; all threads are resolved and the final header now matches the parsers precedent's shape. I verified the asserted test name exists (src/ast/e.rs:2564), the bun_alloc::mimalloc::Heap import path is real (also used at src/runtime/jsc_hooks.rs:2362), and the test's assertions are non-vacuous (it checks a specific test line ran, not just exit code). The 180s timeout is justified inline for cold-tree fat-LTO. Nothing outstanding.
Any of the json_tape_tests keeps the arena arm live, so the native run check matches the group rather than pinning a single test's name, and also requires libtest's overall ok line. The shim helper's comment now names both arena constructors the binary leaves unreferenced.
|
Self-review follow-ups landed in 60444cb and 48eb7a2: the test now matches the |
There was a problem hiding this comment.
LGTM — the earlier doc-comment nit is addressed and nothing new turned up on this pass.
What was reviewed:
#[cfg(test)]gating on the module mount insrc/ast/lib.rs— the shims cannot reach the shipped build.- Shim shape matches the precedent in
src/parsers/native_test_shims.rs;bun_alloc::mimalloc::Heapresolves via thebun_mimalloc_sysre-export. - Test file mirrors
rust-windows-sys-link.test.ts(same skip predicate, same pipe-draining pattern, spawningcargosoprocess.envrather thanbunEnvis correct); thejson_tape_testsgroup it asserts on exists atsrc/ast/e.rs:2439.
Extended reasoning...
Overview
Three files: a two-line #[cfg(test)] mod native_test_shims; mount in src/ast/lib.rs, a new 51-line src/ast/native_test_shims.rs defining seven #[unsafe(no_mangle)] extern "C" mimalloc symbols that all diverge via unreachable!, and a new test/internal/rust-ast-cargo-test.test.ts that runs cargo test -p bun_ast --lib in dev and release profiles. The purpose is to let bun_ast's standalone test binary link when the TapeAlloc::Arena match arm keeps <&MimallocArena as Allocator> live but mimalloc itself is only linked into the full bun binary.
Security risks
None. The shim module is behind #[cfg(test)] and never compiles into libbun_rust.a or the shipped binary. Every shim body is unreachable!(...), so even in the test binary they abort loudly rather than allocate; the PR's argument that nothing references mi_heap_new/mi_heap_main (so no MimallocArena can be constructed to reach them) is sound and matches the linker's undefined-symbol list. No user-facing surface, no untrusted input, no FFI beyond satisfying the linker.
Level of scrutiny
Low. This is test-infrastructure plumbing that follows an established in-tree precedent verbatim: src/parsers/native_test_shims.rs is the same pattern (#[cfg(test)]-mounted no_mangle externs for symbols only the full link provides), and test/internal/rust-windows-sys-link.test.ts is the same test shape (same which("cargo") + workspaceResolvable skip predicate, same Bun.spawn with concurrent pipe drains, same CARGO_TERM_COLOR: "never"). The only deltas from the precedent test — a test.each over dev/release and a 180s timeout — are each justified inline (release pins mi_free, cold target dir compiles ~30 crates and fat-LTO-links).
Other factors
I confirmed bun_alloc::mimalloc::Heap resolves (re-export of bun_mimalloc_sys::mimalloc::Heap at src/mimalloc_sys/mimalloc.rs:52) and that mod json_tape_tests exists at src/ast/e.rs:2439, so the regex assertion is not vacuous. The test spawns cargo, not bunExe(), so spreading process.env rather than bunEnv matches the sibling file and is correct. The one prior finding from this reviewer (wrong test filename in a doc comment) was fixed across d607aec..48eb7a2, and all comment-cop and inline threads are marked resolved. CI was green on b8007c4 and the delta since is assertion/comment-only.
|
Heads-up for sequencing: #37592 adds |
Repro
On a configured checkout (
bun run build --configure-only), on main (97e21e5):bun run rust:miri -p bun_astpasses (20 tests), so the crate's entry inMIRI_CRATESdid not catch this: Miri never links. Of the other crates in that list, all link natively exceptbun_ptr(#37592) andbun_clap(#37599), which reach highway kernels instead;bun_astis the only one whose missing symbols are mimalloc's.Cause
A standalone test binary links the crate's Rust dependencies and nothing else; mimalloc (and the
#[global_allocator], which lives inbun_bin) exist only in the full bun link. lld only reports undefined symbols referenced from sections that survive--gc-sections, and--why-liveroots both chains ine::json_tape_tests(added in #33311):JsonTapestoresVec<_, TapeAlloc>, the tests append to and drop such tapes, and<TapeAlloc as Allocator>::{allocate, deallocate}contain theTapeAlloc::Arenaarm next to theGlobalarm the tests take. That arm is<&MimallocArena as Allocator>, which reachesmi_heap_malloc[_aligned](plusmi_malloc_usable_sizeunderdebug_assertions) andbun_alloc::basic::mi_free_checked, whose debug half ismi_is_in_heap_region+mi_free_size[_aligned]and whose release half ismi_free. Hence the two different sets per profile, seven symbols in total.TapeAllocforwards only those two methods, so nothing else of the arena is live.This is not what #37575 addresses: that is link.exe failing on references from dead code, and it leaves Linux unchanged. These references are live.
Fix
src/ast/native_test_shims.rs, mounted#[cfg(test)]from lib.rs, defines the seven symbols. This is the workspace's existing answer for test binaries that reference externs only the full binary provides (src/parsers/native_test_shims.rs, the simdutf stubs insrc/paths/string_paths.rs, #37592, #37599): the crate whose tests make the references live defines them, and any new reference stays a link error naming the symbol. Nothing here is compiled into bun itself, andTapeAllocis unchanged: theArenaarm is howbun_parsersuses the tape, not something to restructure around the test link, and the tape tests exist precisely to exerciseJsonTape.The shims abort rather than allocating. Nothing in the test binary references
mi_heap_newormi_heap_main(neither appears in the linker's list, and they are the only two ways to construct aMimallocArena), so no test can obtain an arena and none of the seven can be called; the link just needs them resolved. A test that does create anArenain future fails at link time onmi_heap_new, before any stub could run, which is the point to decide whether the crate wants a real allocator behind these. System-backed stubs would have been platform-specific code (malloc_usable_sizevsmalloc_sizevs_msize, aligned frees thatmi_freecannot tell apart on Windows) that nothing executes.Not done in
bun_alloc: dependencies are never built with the downstream crate'scfg(test), so the only way to do it there is a cargo feature turned on frombun_ast's dev-dependencies. The shipped build (cargo build -p bun_bin --lib, resolver 2) could not pick that up, but multi-package test invocations can, and some of those link the real mimalloc (scripts/bench-json-rust.shrunscargo test -p bun_parsersagainst a mimalloc archive;bun_parsersandbun_react_compilertest binaries genuinely needmi_heap_newand friends), so a unified feature would produce duplicate definitions exactly where the real allocator is wanted.bun_astis also the only crate that needs stubs for these symbols; the other crates that referencemi_*from tests need the real thing, so there is nothing to share.Verification
cargo test --locked -p bun_ast: links, 20 passed; same with--release. The crate has no doctests, benches or integration tests, so--libis the whole surface.bun run rust:miri -p bun_ast: 20 passed, as before.cargo fmt --check,cargo clippy -p bun_ast --tests,cargo check -p bun_ast,test/internal/source-lints: clean.test/internal/rust-ast-cargo-test.test.tsrunscargo test -p bun_ast --libin both profiles and asserts that ajson_tape_testscase and libtest's overallokline were printed. With the module unmounted both rows fail with the errors above (dev: the six symbols, release:mi_free+mi_heap_malloc); mounted, both pass (bun bd test test/internal/rust-ast-cargo-test.test.ts). Removing any one of the seven shims fails a row: six of them the dev row,mi_freeonly the release row, which is why the release row exists; it costs about 10s warm here (fat LTO link of the test binary) against about 2s for the dev row, and the file skips on the test-only CI lanes likerust-windows-sys-link.test.tsdoes (no cargo or no configured checkout). The explicit timeout is for a cold target dir. It also skips on Windows: link.exe reports references from dead code too, so there abun_coredependent's test binary either fails on ~100 unrelated externs or (build: make cargo test link on Windows hosts via /FORCE:UNRESOLVED in the generated .cargo/config.toml #37575) links with unresolved symbols forced through, and neither outcome says anything about these shims.Like #37592 and #37599 this adds a per-crate file under
test/internal/; no CI lane runs nativecargo testfor workspace crates (only Miri), so these stay local guards. A singlecargo test --no-runover theMIRI_CRATESlist in the Miri workflow would cover all of them once the three crates link, but that is a cross-PR change and not part of this one.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/internal/rust-ast-cargo-test.test.ts