rust: fix cargo check --tests for bun_parsers and bun_sys after #34820/#35002 - #35068
rust: fix cargo check --tests for bun_parsers and bun_sys after #34820/#35002#35068robobun wants to merge 7 commits into
Conversation
Expr::get_boolean was removed as dead code in #35002 but had a cfg(test) caller in src/parsers/json.rs that the reachability analysis could not see. Rewrite the test to use the live get() + as_bool() API, which covers the same match arm. Separately, #34820 routed bun_highway::memmem through highway_memmem on all platforms, but that symbol lives in highway_strings.cpp which the standalone bench/test build cannot compile (it pulls in root.h). Add a libc memmem shim alongside the existing simdutf shim so the test binary links again.
WalkthroughUpdated Rust test helpers and native test shims, added conditional compilation coverage for selected Rust crates, and clarified ChangesRust test updates
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:42 AM PT - Jul 22nd, 2026
✅ @robobun, your commit 58e25b42184b6f41910f8d3cd14d636e6c40b5be passed in 🧪 To try this PR locally: bunx bun-pr 35068That installs a local version of the PR into your bun-35068 --bun |
|
Status: diff is green; ready for a maintainer. Reproduced on main:
With this patch, both All remaining CI reds on builds 77534 and 77639 are known-flaky tests in unrelated areas (install/migration, http/tls leak checks, repl, fetch-leak, es-module-lexer, no-orphans). This PR changes only |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical fixes to test-only/bench-only code, nothing in the shipped binary changes.
What was reviewed:
as_bool()replacement: verifiedExpr::as_boolatsrc/ast/expr.rs:323returnsOption<bool>over the sameEBoolean | EBranchBooleanarms; the existingbool=Some(true)assertion still holds.highway_memmemshim signature checked against both the Rust extern (src/highway/lib.rs:7-12) and the real impl (src/jsc/bindings/highway_strings.cpp:2023) — matches exactly.- Bench script wiring: new
.ois picked up by the existingar rcs ... "$SUP"/*.oglob; follows the siblingsimdutf_shimpattern.
Extended reasoning...
Overview
Three files touched, none of which ship in the binary:
src/parsers/json.rs— a single expression inside#[cfg(test)] mod testsswaps the removedExpr::get_boolean(&root, b"private")forroot.get(b"private").and_then(|e| e.as_bool()).src/parsers/benches/support/highway_shim.cpp— new 13-line bench-only shim providingextern "C" highway_memmemvia libcmemmem.scripts/bench-json-rust.sh— one addedbuildline to compile the shim into the existinglibbun_bench_cdeps.a.
Both breakages are well-explained and traceable: #35002's dead-code sweep removed Expr::get_boolean (only caller was cfg(test)-gated and invisible to the reachability analysis), and #34820 made bun_highway::memmem unconditionally reference the highway_memmem C symbol that the standalone bench build can't compile from highway_strings.cpp (which pulls in root.h).
Security risks
None. No runtime code paths, no user-facing surface, no untrusted input handling. The shim is dev-tooling-only and just forwards to libc memmem.
Level of scrutiny
Low. This is build-breakage repair confined to test/bench infrastructure. I verified the two substantive claims against source:
Expr::as_bool()exists (src/ast/expr.rs:323) and matchesData::EBoolean(b) | Data::EBranchBoolean(b) => Some(b.value), soroot.get(...).and_then(|e| e.as_bool())is semantically equivalent to the removed helper. The downstream assertionfull.contains("bool=Some(true)\n")is unchanged and still satisfied by{:?}onOption<bool>.- The shim's signature
void* highway_memmem(const uint8_t*, size_t, const uint8_t*, size_t)is byte-identical to the production definition athighway_strings.cpp:2023and matches the Rust extern atsrc/highway/lib.rs:7-12(*const u8, usize, *const u8, usize -> *const u8).
Other factors
- The new shim exactly mirrors the existing
simdutf_shim.cpppattern in the same directory, and the bench script wires it in the same way (compiled to$SUP/*.o, archived by the existing glob). - The bench script only targets Linux/Darwin (per the
CXXLIBbranch), where libcmemmemis available. - Author verified
cargo check -p bun_parsers --testsandscripts/bench-json-rust.sh --test(33/33 pass). - No prior reviewer comments to address; no CODEOWNERS concerns for bench scripts or cfg(test) blocks.
… bun_sys --tests Put highway_memmem next to the existing highway_index_of_char/any_char Rust shims in native_test_shims.rs instead of introducing a new C++ TU and bash build step; the symbol needs no C++ dependency. Also fix cargo check -p bun_sys --tests, broken by the same #35002 mechanism: File::stderr() was removed but had a cfg(test) caller in dropping_stdio_is_safe. Drop the stderr line since stdin/stdout already cover the Drop invariant being tested. Swept every test-bearing crate #35002 touched; bun_parsers and bun_sys were the only #35002-caused --tests regressions.
robobun
changed the title
cargo test -p bun_parsers after #34820 and #35002No other CI lane builds the test cfg, so refactors that remove a symbol whose only caller is behind #[cfg(test)] are otherwise invisible. This check is cargo-check only (no link/run) and runs in a few seconds on a warm tree.
…(Fd::stderr()) File::stderr() is gone, but Fd::stderr() is not, and callers can still wrap it with File::from_fd. On Windows is_stdio() checks stderr as a separate disjunct, so the Drop guard's stderr arm is a distinct code path worth keeping covered. Also drop the stale File::stderr() mention from the module doc.
…lter The neighboring linear-fifo.test.ts already checks vendor/lolhtml/Cargo.toml for the same reason (test-only lanes lack it and cargo cannot resolve the workspace without it). Also switch the unread stdout pipe to 'ignore', and widen the error filter to catch cargo's own 'error: ...' lines so a workspace-resolution failure shows up in the assertion rather than as a bare exit-code mismatch.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/parsers/native_test_shims.rs`:
- Around line 42-61: Update highway_memmem to handle needle_len == 0 before
constructing slices or calling windows, returning haystack directly to match
bun_highway::memmem’s empty-needle behavior. Preserve the existing search and
null-result behavior for non-empty needles.
In `@src/sys/file.rs`:
- Around line 489-495: Extend the stdio wrapper Drop test around
File::from_fd(Fd::stderr()) to assert stderr remains open after the wrappers are
dropped. Add the stderr-specific fstat check alongside the existing stdout
assertion, using Fd::stderr() and preserving the current repeated-loop coverage.
In `@test/internal/rust-tests-compile.test.ts`:
- Around line 31-50: Extend the rust compilation test near the existing cargo
check flow to add a link-time regression test for the native highway_memmem
shim, using the supported scripts/bench-json-rust.sh --test path or an
equivalent prepared cargo test --no-run target. Assert the link command succeeds
and retain the existing cargo-check coverage.
- Around line 1-11: Replace the narrative comments above the Rust regression
test with only the full issue URL for `#35002`. Apply the same reduction to all
referenced comment blocks, preserving the test implementation unchanged.
🪄 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: 7525f308-6c45-4ff1-8a7d-53d65e5fabd7
📒 Files selected for processing (4)
src/parsers/json.rssrc/parsers/native_test_shims.rssrc/sys/file.rstest/internal/rust-tests-compile.test.ts
There was a problem hiding this comment.
LGTM — all earlier feedback addressed; nothing in the shipped binary changes.
What was reviewed:
highway_memmemshim: confirmedbun_highway::memmemguards empty needle and short haystack before dispatch, matching the sibling shims' caller-precondition pattern.dropping_stdio_is_safe: stderr arm restored viaFile::from_fd(Fd::stderr())and now asserted withfstat(Fd::stderr()); module doc no longer names the removed constructor.- Guard test:
workspaceReadynow matcheslinear-fifo.test.ts's two-part check,stdout: "ignore", and the widened error filter also catches cargo's un-prefixederror:lines.
Extended reasoning...
Overview
Four files: a one-line rewrite in a #[cfg(test)] block of src/parsers/json.rs (removed Expr::get_boolean → live .get().and_then(|e| e.as_bool())), a new pure-Rust highway_memmem shim in the test-only src/parsers/native_test_shims.rs, a #[cfg(test)] fix + doc-comment reword in src/sys/file.rs, and a new test/internal/rust-tests-compile.test.ts guard that runs cargo check -p bun_parsers -p bun_sys --tests. Every Rust hunk is behind #[cfg(test)] or in a module that is only compiled for the crate's own test/bench binaries; the shipped bun binary is byte-identical.
Security risks
None. No user-facing surface, no parsing of untrusted input, no auth/crypto/permissions. The shim is unsafe extern "C" but is test-only, its sole caller (bun_highway::memmem, verified at src/highway/lib.rs:135-150) enforces both preconditions the SAFETY comment documents, and it never links into the product.
Level of scrutiny
Low. This is build-hygiene: restoring cargo check --tests after a dead-code sweep removed symbols whose only callers were cfg-gated. The replacements are mechanical (as_bool() matches the same EBoolean | EBranchBoolean arm at src/ast/expr.rs:323-326; File::from_fd(Fd::stderr()) constructs the same wrapper the removed File::stderr() did). The new guard test follows the sibling test/internal/linear-fifo.test.ts pattern exactly (same two-part workspaceReady skip, same repo-root resolution).
Other factors
Two prior review rounds from me and one from CodeRabbit were all addressed or reasonably declined: stderr coverage restored (bc406ca), lolhtml skip guard + stdout: "ignore" + widened error filter (5ce0a97), stderr fstat assertion (930c7fd). CodeRabbit's empty-needle concern was withdrawn after verifying the caller guard; the "add a link-time test" suggestion was declined because bench-json-rust.sh fetches from github.com and is out of the test-time budget — that reasoning holds. The PR body's mechgate evidence shows the guard test failing on main and passing on the branch under both ASAN-debug and release.
Same guard file as #35068; whichever lands first, the other appends its crate to CRATES.
There was a problem hiding this comment.
LGTM — restores cargo check --tests for the two crates broken by the #35002 dead-code sweep and #34820, with a guard test to catch the class going forward.
What was reviewed
- Verified the
highway_memmemshim signature matches theextern "C"declaration insrc/highway/lib.rs:7-12and that the sole caller (bun_highway::memmem) guards empty needle / short haystack before dispatch, sowindows(0)is unreachable. - Confirmed
Expr::as_bool(src/ast/expr.rs:323) covers the sameEBoolean | EBranchBooleanarms the removedget_booleandid. - Checked
native_test_shimsis#[cfg(test)]-gated insrc/parsers/lib.rs:11-12(bench-only otherwise), so nothing ships. - The new guard test now mirrors
test/internal/linear-fifo.test.ts's two-partworkspaceReadycheck and usesstdout: "ignore"; all earlier inline feedback and CodeRabbit findings are addressed or withdrawn.
Extended reasoning...
Overview
Four files touched, all confined to test-only surfaces:
src/parsers/json.rs— one call site inside#[cfg(test)] mod testsswaps the removedExpr::get_boolean(&root, b"private")for the liveroot.get(b"private").and_then(|e| e.as_bool()).src/parsers/native_test_shims.rs— adds a pure-Rust#[no_mangle] highway_memmemshim next to the existinghighway_index_of_char/highway_index_of_any_charshims; the module is included only under#[cfg(test)](lib.rs:11-12) and by the bench target.src/sys/file.rs— module-level doc comment reworded to stop referencing the removedFile::stderr();#[cfg(test)] fn dropping_stdio_is_safenow wraps stderr viaFile::from_fd(Fd::stderr())and assertsfstat(Fd::stderr()).is_ok()alongside stdout.test/internal/rust-tests-compile.test.ts— new guard test spawningcargo check -p bun_parsers -p bun_sys --tests --keep-going, skipped if cargo or the workspace prerequisites (codegen dir, vendor/lolhtml) are absent.
Security risks
None. No shipped-binary code changes: every Rust hunk is either a doc comment or behind #[cfg(test)], and the shim module is gated the same way. The new TS test spawns cargo locally with no network access and no untrusted input.
Level of scrutiny
Low. This is developer-tooling / test-infra maintenance recovering from a mechanical dead-code sweep. The robobun evidence in the PR body demonstrates the test fails on unpatched main (E0599 for both crates) and passes with the fix, on both ASAN-debug and release. The shim signature matches the extern declaration byte-for-byte, and the replacement APIs (as_bool, File::from_fd(Fd::stderr())) exist and preserve the original coverage.
Other factors
Three rounds of prior feedback are all resolved: my earlier inline comments (stderr coverage, lolhtml skip guard, undrained stdout pipe) were each addressed in bc406ca / 5ce0a97; CodeRabbit's four findings were either applied (stderr fstat assertion, 930c7fd) or withdrawn after the author's rebuttal (empty-needle guard, comment verbosity, link-time guard). The new test follows the sibling linear-fifo.test.ts pattern for skip-guarding cargo-workspace tests. No CODEOWNERS-sensitive paths, no design decisions, no outstanding threads.
|
Closing: all three breakages this PR fixes are gone on main. #36184 updated the |
1 participant
What does this PR do?
Restores
cargo check -p <crate> --testsfor the two crates broken on main, letsscripts/bench-json-rust.sh --testrun to completion again, and adds a guard test so the next workspace-wide refactor can't silently break them. None of these paths are exercised bybun bd, so they slipped through.E0599:
Expr::get_booleannot found (bun_parsers, from #35002)Expr::get_booleanwas removed by the workspace-wide dead-code sweep in #35002. Its only caller was inside a#[cfg(test)]block insrc/parsers/json.rs, which the reachability analysis (rooted at the two shipped artifacts) does not see. Rather than re-add the dead function, the test now usesroot.get(b"private").and_then(|e| e.as_bool()), which is the live API and exercises the sameEBoolean | EBranchBooleanmatch arm.E0599:
File::stderrnot found (bun_sys, from #35002)Same mechanism:
File::stderr()was removed as dead, its only caller is in#[cfg(test)] fn dropping_stdio_is_safe. Replaced withFile::from_fd(Fd::stderr())so theDrop-guard's stderr arm stays covered (it is a separateis_stdio()disjunct on Windows), and reworded the module doc that still named the removed constructor.I swept
cargo check -p <crate> --testsacross every test-bearing crate #35002 touched; these two were the only regressions it introduced. (bun_router --testsis also broken, but was already broken before #35002 and is out of scope here.)Link error: undefined
highway_memmem(bun_parserstest binary, from #34820)#34820 made
bun_highway::memmemcallhighway_memmemdirectly on all platforms. That symbol is defined insrc/jsc/bindings/highway_strings.cpp, which the standalone test/bench build cannot compile (it includesroot.h). Added a pure-Rust#[no_mangle]shim insrc/parsers/native_test_shims.rsnext to the existinghighway_index_of_char/highway_index_of_any_charshims. The only Rust caller (bun_highway::memmem) already guardsneedle.is_empty()andhaystack.len() < needle.len()before dispatching, so the shim does not need to.Guard test (
test/internal/rust-tests-compile.test.ts)Spawns
cargo check -p bun_parsers -p bun_sys --tests --keep-goingand asserts exit 0.bun bd,rust:check, andrust:clippyall build with the default cfg only, so nothing else in CI would catch a#[cfg(test)]break in these crates. Check-only (no link/run), ~3s on a warm build tree; skips ifcargois not on PATH or the codegen dir is not populated.How did you verify your code works?
Only
#[cfg(test)]blocks, the test-shim module (itself gated#[cfg(test)]/ bench-only), and a module doc comment are touched; nothing in the shipped binary changes.[review] gate passed · iteration 2 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 2
evidence per changed file