highway: add a scalar feature so standalone cargo test binaries link; run native cargo test in CI - #37599
highway: add a scalar feature so standalone cargo test binaries link; run native cargo test in CI#37599robobun wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 8 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 ignored due to path filters (1)
📒 Files selected for processing (10)
Comment |
|
Updated 1:17 AM PT - Aug 12th, 2026
✅ @robobun, your commit aa57c6420517abff10e16044f35eee3bc8d61b2b passed in 🧪 To try this PR locally: bunx bun-pr 37599That installs a local version of the PR into your bun-37599 --bun |
|
Status: ready for review. CI is green on aa57c64 (all Buildkite lanes, clippy, format, source lints; the Miri job now also runs the new native Reproduced on main at 97e21e5 with Gate test: Related: #37592 (bun_ptr, same bug class; can switch to the feature once this lands), #37575 (Windows side). The bun_collections pool-test race found while building the crate list was handed off separately. |
There was a problem hiding this comment.
LGTM — test-only shim following the existing src/parsers/native_test_shims.rs pattern; nothing here compiles into the shipping binary.
What was reviewed
- Shim signature/return-convention checked against
src/highway/lib.rs(found_at:haystack_len= not found) and the C++ decl inhighway_strings.cpp— matches. #[cfg(test)]gating on the module means the#[no_mangle]symbol only exists in the crate's test binary, so no collision with the real kernel in the full link.long_paramsaddition: after stripping--, both new tokens exceed the 16-byteSCALAR_CUTOFF, so the shim is exercised on both found and not-found paths; expectedArgresults line up.- New
.test.tsmirrorsrust-windows-sys-link.test.ts(same skipIf gating, pipe draining, env spread).
Extended reasoning...
Overview
Fixes cargo test -p bun_clap failing to link on highway_index_of_char by adding a #[cfg(test)]-gated scalar shim (src/clap/native_test_shims.rs), extending the existing long_params unit test with a ≥16-byte flag so the shim is actually called, and adding a test/internal/ guard that runs the real cargo link. No production code paths change — StreamingClap::normal is untouched.
Security risks
None. The new symbol is compiled only into the crate's own test binary (module is behind #[cfg(test)]), the unsafe block reconstitutes a slice from a pointer/length pair the sole caller (bun_highway::index_of_char) derives from a live &[u8], and the SAFETY comment states this. The TS test spawns cargo locally with no network access.
Level of scrutiny
Low. This is test infrastructure following two in-tree precedents byte-for-byte: the shim body is identical to src/parsers/native_test_shims.rs:17-24, and the guard test is a near-copy of test/internal/rust-windows-sys-link.test.ts. Nothing user-facing or runtime-linked is touched.
Other factors
Verified the shim's contract against src/highway/lib.rs — scalar_only gates on len < SCALAR_CUTOFF (16), and found_at treats result == haystack_len as not-found, which the shim's .unwrap_or(haystack_len) honors. The new test tokens (ee-longer-than-sixteen-bytes, 28 bytes after prefix strip; and the =2 form, 30 bytes) both clear the cutoff, so the PR's claim that the shim is executed rather than merely linked holds. The 600s timeout on the TS test is unusual but explicitly justified (cold cargo build of bun_core) and matches the sibling test's rationale; the skipIf correctly gates on cargo availability and a configured workspace. The PR description documents the sibling issues (#37592 for bun_ptr, bun_ast tracked separately), so scope is deliberately bounded to bun_clap.
|
Heads-up for sequencing: #37592 adds |
A crate's cargo test binary links only its Rust dependencies; the highway C++ kernels behind bun_core::strings exist only in the full bun link. Since the byte search in StreamingClap::normal moved onto bun_core::strings, cargo test -p bun_clap has failed to link (undefined symbol: highway_index_of_char). The Miri lane kept passing because Miri never links and bun_highway already switches to scalar code under cfg(miri). Key that switch to a `scalar` cargo feature as well, and enable it from bun_clap's [dev-dependencies], so the crate's test binary references no kernel at all. The bun build never enables the feature. The clap test helper that hand-rolled a substring search for the same reason now uses strings::contains. Add scripts/rust-test.ts (bun run rust:test), which runs cargo test for each crate that is expected to link natively, one invocation per crate so feature unification cannot hide a missing dev-dependency, and run it in the Miri workflow, which already has the configured tree and toolchain. The configure prerequisite logic it shares with rust-miri.ts moves to scripts/rust-workspace.ts. test/internal/rust-native-cargo-test.test.ts runs the same script locally.
3bd59a7 to
2a35b29
Compare
…ad of repeating it
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it introduces a new workspace-wide pattern (the bun_highway/scalar feature enabled via dev-dependencies), adds a new CI step to the Miri workflow, and has a stated sequencing dependency with #37592, a human look would still be worthwhile.
What was reviewed:
src/highway/lib.rs: everycfg(miri)→cfg(any(miri, feature = "scalar"))site is a mechanical 1:1 rewrite; thescalar_only()predicate folds to a constant so DCE removes the FFI references, and the memmem wrappers use#[cfg]blocks — no kernel is reachable with the feature on.streaming.rstest_err:strings::containsis semantically equivalent to the hand-rolled search it replaces (quotedis always ≥2 bytes soindex_of's empty-needle behaviour doesn't apply).rust-workspace.tsextraction is byte-for-byte the logic that was inrust-miri.ts;rust-miri.tsbehaviour is unchanged.rust-test.tsruns onecargo test --locked -pper crate to avoid feature unification, collects failures, and exits nonzero — checked that a failing crate is reported and doesn't short-circuit the loop.
Extended reasoning...
Overview
Eleven files: a scalar cargo feature on bun_highway that reuses the existing Miri scalar-fallback paths so standalone cargo test binaries link without the highway C++ kernels; bun_clap enables it via [dev-dependencies]; the naive substring search in test_err is replaced with strings::contains (test-only). Tooling side: scripts/rust-workspace.ts extracts the configure-prerequisite check from rust-miri.ts, new scripts/rust-test.ts runs cargo test --locked -p <crate> per crate, .github/workflows/miri.yml gains a bun run rust:test step and path-filter entries, package.json gains rust:test, and test/internal/rust-native-cargo-test.test.ts runs the same script under bun bd test. Cargo.lock gains one edge (bun_clap → bun_highway).
Security risks
None. No runtime code path in the shipped binary changes: the scalar feature is off in bun_bin's dependency graph (only reachable from bun_clap's dev-dependencies), and cfg!(feature = "scalar") compiles to a constant false there. The streaming.rs change is inside #[cfg(test)]. The scripts spawn cargo/bun/ninja with fixed arguments in the repo directory.
Level of scrutiny
Medium. The src/ edits are mechanical (fifteen identical cfg rewrites plus a test helper simplification) and provably inert in the production build. The tooling is internal dev/CI infrastructure. What raises this above "rubber-stamp" is that it establishes a pattern the rest of the workspace is expected to adopt (per-crate [dev-dependencies] bun_highway = { features = ["scalar"] }), adds a step to a GitHub workflow that runs on every PR touching the listed crates, and the author flagged a merge-order interaction with #37592 that a maintainer should be aware of when landing.
Other factors
The PR description documents that an earlier revision (per-crate #[no_mangle] shims) was reviewed and this centralized approach was the suggested alternative, so the design has had prior input. The comment-cop bot flagged the scalar_only doc comment length; the author trimmed it to four lines pointing at Cargo.toml and those threads are resolved. The new test follows harness conventions (bunEnv, bunExe(), concurrent pipe drains, skipIf on missing prerequisites, stderr asserted before exit code). The rust-miri.ts refactor is behaviour-preserving by inspection. No CODEOWNERS cover the touched paths.
Repro
On a configured checkout (
bun run build --configure-only), on main:bun run rust:miri -p bun_clappasses (9 tests), so the crate's entry inMIRI_CRATESdid not catch this.Cause
A standalone test binary links the crate's Rust dependencies and nothing else; the highway C++ kernels behind
bun_core::stringsexist only in the full bun link. Since #37052,StreamingClap::normalsplits--name=valuewithstrings::index_of_char_usize, and the streaming tests reachnormal, so the reference is live and lld reports it (lld only reports references from sections that survive--gc-sections, which is why this is a different problem from the dead-reference one #37575 handles on Windows). Miri kept passing because Miri never links and #37078 madebun_highwaytake scalar paths undercfg(miri).Fix
That
cfg(miri)switch is the right place to solve the native case too, so this keys it to a cargo feature as well:bun_highwaygains ascalarfeature; everymirigate insrc/highway/lib.rsbecomesany(miri, feature = "scalar"). With it on, no search wrapper references a kernel.cargo tree -p bun_bin -i bun_highway -e featuresshows onlydefault, i.e. the bun build itself is unaffected (the flag is a constant-falsecfg!inscalar_only, as before).bun_clapenables it from[dev-dependencies](the one-lineCargo.lockchange is that edge). Cargo only activates a crate's dev-dependencies when building that crate's own tests, so nothing else sees the feature. Thetest_errhelper, which hand-rolled a substring search for exactly this reason (strings: route all byte search through highway, deny the paths around it #37052), now usesstrings::contains, so the clap tests also exercise thememmemside of the gating natively.An earlier revision of this PR instead added a per-crate
#[no_mangle]stand-in for the kernel (the shape ofsrc/parsers/native_test_shims.rs). Review of that revision pointed out that the central switch already exists, and that shims are hand-copied, name-matched reimplementations that every affected crate repeats; with the feature, a crate needs one manifest line and runs the same scalar code the Miri lane runs. The remaining in-tree shims (src/parsers/native_test_shims.rs, which does not currently link natively for other reasons; the highway half of #37592; thelinear_fifo.rsbyte-search lint exception) can move over separately.CI coverage
No lane ran a native
cargo testfor any crate, which is how #37052 could break this:test/internal/rust-windows-sys-link.test.tsand friends skip on the test-only lanes (no configured workspace), clippy does not build tests, Miri does not link. So:scripts/rust-test.ts(bun run rust:test) runscargo test --locked -p <crate>for the crates whose tests are expected to link natively. One invocation per crate on purpose: features unify within an invocation, so a combined-p a -p brun would turnscalaron for every crate and hide a missing dev-dependency. The list is the Miri set minusbun_ast(mimalloc symbols, tracked separately),bun_ptr(ptr: make cargo test -p bun_ptr link natively and run the Miri crate set natively in CI #37592) andbun_collections, whose pool tests turn out to race against thread-local pool teardown when libtest runs them in parallel (2 of 30 native runs fail on unmodified main; handed off separately, and it should be added back once fixed). The other 11 were stable over 15 consecutive runs here..github/workflows/miri.ymlruns it as a step before Miri: that job already has the configured tree and the clang/lld the generated.cargo/config.tomlpoints at, and bun_core's is the only build script in these crates' graphs.src/highway/**and the new scripts are added to its path filter.rust-miri.tshad inline moves toscripts/rust-workspace.tsso both scripts share it;rust-miri.tsis otherwise unchanged.test/internal/rust-native-cargo-test.test.tsruns the same script underbun bd test. It needs cargo and a configured checkout like its siblings, and is skipped on Windows, where every bun_core dependent's test binary fails to link for the dead-reference reason until build: make cargo test link on Windows hosts via /FORCE:UNRESOLVED in the generated .cargo/config.toml #37575 lands. Without thesrc/changes it fails (on main proper with the link error above; with this PR'sCargo.lockand main's manifests,--lockedrejects the lockfile first), with them it passes.Verification
cargo test --locked -p bun_clap: links, 9 passed.bun run rust:test: all 11 crates pass;bun run rust:test -p bun_ptrreports the failure correctly.bun run rust:miri(full set, through the refactored script): all 14 crates pass with the same counts as before.cargo fmt --all --check;cargo clippy -p bun_highwaywith and without--features scalar, and forbun_clap/bun_core: clean.bun test test/internal/source-lints/: 75 pass.bun bdrebuilt the debug binary with the feature off.bun bd test test/internal/rust-native-cargo-test.test.tspasses; fails with thesrc/changes stashed.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-native-cargo-test.test.ts