Remove dead code from bun_hash - #35225
Conversation
- XxHash3 wrapper: HashObject.rs calls bun_highway::xxhash3_64 directly since #31491, leaving the bun_hash::XxHash3 wrapper unreferenced. - Murmur hash_uint* helpers: the 12 hash_uint32/64(_with_seed) fns on Murmur2_32/Murmur2_64/Murmur3_32 have never been called outside their own self-referential tests (which only asserted they match hash()). Verified with rg across src/, build/debug/codegen/, src/codegen/, packages/, test/ and *.classes.ts. bun bd builds, cargo test -p bun_hash passes, rust:check-all passes on all targets, hash.test.js passes.
|
Warning Review limit reached
Next review available in: 4 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 |
|
No test added: this is pure dead-code removal with no observable behavior change, so there is no fail-before case a regression test could cover. The removed items ( Verification:
Same pattern as the earlier dead-code sweeps (#34878, #35052, #34837, #35152). |
There was a problem hiding this comment.
LGTM — pure dead-code deletion. The inline nit about the stale XxHash3 mention in the xxhash.rs module doc is cosmetic and non-blocking.
Extended reasoning...
Overview
Deletion-only sweep of src/hash/ (the bun_hash crate): removes the unreferenced XxHash3 thin wrapper (superseded by HashObject.rs calling bun_highway::xxhash3_64 directly), twelve hash_uint* convenience methods on the three Murmur variants that have zero callers workspace-wide, and the three #[cfg(test)] cases whose only purpose was asserting those helpers matched hash(&v.to_le_bytes()). The lib.rs re-export and intra-doc link are updated to match. I re-ran the greps: hash_uint has zero hits repo-wide, and the only remaining XxHash3 references are the crate-local struct in HashObject.rs (which does not import from bun_hash), a doc line in bun_highway, and the two stale doc strings the inline nit covers.
Security risks
None. Non-cryptographic hash utility crate; no changes to any code path that ships behavior — every removed function was unreferenced. The hash_with_seed implementations that back the JS-visible Bun.hash.* surface are untouched.
Level of scrutiny
Low. This is a mechanical dead-code removal with no additions beyond a one-line doc-table fix. The SMHasher verification tests for the live hash_with_seed entry points are retained, and the PR description reports cargo test -p bun_hash, rust:check-all (all 10 targets), and test/js/bun/util/hash.test.js all passing — so the JS-observable xxHash3 / murmur* outputs are demonstrably unchanged.
Other factors
The removed *_uint tests were self-referential (they only exercised the helpers being deleted), so no real coverage is lost — REVIEW.md's "never silently weaken an existing test" concern doesn't apply here. The one finding is a doc-header nit (xxhash.rs:1 still lists XxHash3); it's the same class of stale reference the PR already fixed in lib.rs and can be tidied in a follow-up or before merge without changing the verdict.
There was a problem hiding this comment.
LGTM — the stale XxHash3 doc header nit from the earlier pass is addressed in 740cc8b.
What was reviewed:
- Grepped
src/forhash_uint/hashUintandbun_hash::XxHash3— zero remaining references; theXxHash3inHashObject.rsis a local struct callingbun_highway::xxhash3_64directly. - Removed
*_uinttests were purely self-referential (hash_uint*(v) == hash(&v.to_le_bytes())); the SMHasher verification tests forhash_with_seedremain, so the JS-visible surface is still covered. lib.rsre-export and intra-doc table updated consistently;xxhash.rsmodule doc now matches its contents.
Extended reasoning...
Overview
Dead-code sweep of the bun_hash crate touching three files: src/hash/xxhash.rs (removes the 8-line XxHash3 wrapper struct + updates module doc), src/hash/murmur.rs (removes 12 hash_uint* convenience fns across Murmur2_32/Murmur2_64/Murmur3_32 plus their 3 self-referential #[cfg(test)] cases), and src/hash/lib.rs (drops XxHash3 from the re-export list and rewrites the intra-doc table row to point at bun_highway::xxhash3_64). Pure deletions plus doc-comment adjustments; no logic in any surviving function is modified.
Security risks
None. Non-cryptographic hash helpers with no callers; nothing user-facing changes. No parsing, no I/O, no allocation, no FFI surface touched.
Level of scrutiny
Low. This is mechanical dead-code removal following the same pattern as prior sweeps (#34878, #35052, #34837, #35152). The correctness question is "are these actually unreferenced?" — I re-ran the greps against the current tree: hash_uint/hashUint has zero hits in src/, and the only XxHash3 symbols left are (a) a doc line in bun_highway, (b) the Cargo.toml comment the author deliberately kept since it describes what bun_highway provides, and (c) the local struct XxHash3 in HashObject.rs which implements HashAlgorithm by calling bun_highway::xxhash3_64 directly (not the removed wrapper). No bun_hash::XxHash3 import exists anywhere.
Other factors
- The removed tests only asserted the removed helpers agree with
hash(&v.to_le_bytes())— they exercised nothing that survives, so deleting them alongside is correct per the "stated reason" rule. The SMHasher verification tests forhash_with_seed(the actual JS-exposed entry points) are retained. - The author ran
cargo test -p bun_hash,bun bd,bun run rust:check-all(all 10 targets), andbun bd test test/js/bun/util/hash.test.js— the last covers the fullBun.hashsurface end-to-end includingxxHash3and all three murmur variants. - My prior review's only finding (stale
XxHash3mention in thexxhash.rsmodule doc) was fixed in 740cc8b and the thread is resolved; I verified the current file content matches. - The PR description transparently flags additional probably-dead items (
Murmur*::hash,DEFAULT_SEED,Adler32::init/update) that were intentionally left in — reasonable scoping.
|
CI on 740cc8b (build 78397): the only red is |
Brings in #35002 (remove ~39k lines of dead Rust) and its follow-ups (#35019, #35052, #35152, #35225, #35293, #35326). The binary-size check compares against current main; this branch was 137 commits behind, so it still carried the dead code main dropped and registered as +630KB..+1.7MB on x64 while aarch64 linux showed -513KB/-601KB (different dead-code elimination outcomes per target). This stack's own native contribution is 19 files / +392 -45 lines; src/js is net -977 lines (domain.ts +692 vs fast-utf8-stream.ts -856 etc). Also: drop the hoisted pbkdf2 .bind handlers back to closures (review nit), and take main's expectations.txt since #34741 audited the stale ASAN entries.
Periodic dead-code sweep, this pass covers
src/hash/(thebun_hashcrate).Removed
XxHash3wrapper (src/hash/xxhash.rs, 8 lines): since Add runtime-dispatched SIMD xxHash3 via Highway #31491HashObject.rscallsbun_highway::xxhash3_64directly, so the thinbun_hash::XxHash3wrapper became unreferenced. Also dropped the re-export and fixed the stale intra-doc link insrc/hash/lib.rs.hash_uint*helpers (src/hash/murmur.rs, 12 fns):hash_uint32,hash_uint32_with_seed,hash_uint64,hash_uint64_with_seedon each ofMurmur2_32/Murmur2_64/Murmur3_32. These were ported over in Rewrite Bun in Rust #30412 and have zero callers anywhere in the workspace; their only references were three#[cfg(test)]cases that assertedhash_uint*(v) == hash(&v.to_le_bytes()), which are removed alongside. The SMHasher verification tests forhash_with_seedare kept.Verification
cargo test -p bun_hash: 10 passedbun bdbuildsbun run rust:check-all: 10/10 targets okbun bd test test/js/bun/util/hash.test.js: 20 passed (incl.xxHash3,murmur32v2,murmur32v3,murmur64v2)Probably also dead (not in this diff)
Murmur2_32::hash/Murmur2_64::hash/Murmur3_32::hashand theDEFAULT_SEEDconst: zero callers now that the*_uinttests are gone (HashObject.rsonly ever callshash_with_seed). Left in place since they're a 16-line convenience surface someone may still want.Adler32::init/Adler32::update(streaming form): onlyAdler32::hashis called externally.Also scanned
src/patch/andsrc/dotenv/during this pass; both are tight (no confidently-dead items).