Repository navigation
Remove dead code from bun_semver and bun_semver_jsc - #35293
Conversation
- Delete src/semver_jsc/SemverString_jsc.rs: the SemverStringJsc trait is never imported or used. The only would-be caller (install_jsc/dependency_jsc.rs) has its own inline helper. - Drop the JsResult and SemverStringJsc re-exports from semver_jsc/lib.rs; nothing outside the crate references them. - Remove 7 unused Cargo dependencies from each of bun_semver and bun_semver_jsc (strum, scopeguard, const_format, enum-map, enumset, libc, bitflags). None are referenced in any .rs file in either crate. - Trim the stale doc comment in dependency_jsc.rs that referenced the deleted trait.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (5)
WalkthroughChangesSemver JSC cleanup
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — pure dead-code removal, verified the removed symbols and deps have no remaining references. The inline nit about the stale src/semver/lib.rs:544 comment is non-blocking.
What was reviewed:
- Grepped
src/forSemverStringJsc,SemverString_jsc,bun_semver_jsc::JsResult— zero hits post-change; only consumer ofbun_semver_jscisBunObject.rscallingSemverObject::create. - Confirmed none of the 7 dropped Cargo deps (nor their macros/derives) appear in any
.rsfile undersrc/semver/orsrc/semver_jsc/; all retained deps (bstr,bun_core,bun_jsc,bun_semver) are still referenced inSemverObject.rs. Cargo.lockdiff matches the twoCargo.tomledits exactly (no unrelated churn).
Extended reasoning...
Overview
Dead-code sweep of two small crates: deletes the unused SemverStringJsc extension trait file, removes two unused re-exports from src/semver_jsc/lib.rs, drops 7 unused workspace deps from each of src/semver/Cargo.toml and src/semver_jsc/Cargo.toml, trims a stale doc comment in src/install_jsc/dependency_jsc.rs, and regenerates Cargo.lock. Net +1/-53.
Security risks
None. This is subtractive only — no new code paths, no logic changes, no data handling. The one modified .rs file (dependency_jsc.rs) only shortens a doc comment.
Level of scrutiny
Low. Rust's compiler is the authoritative check here: if any removed symbol or dep were live, bun bd and rust:check-all (which the PR reports passing on all 10 targets) would fail. I independently confirmed via grep that SemverStringJsc / SemverString_jsc / bun_semver_jsc::JsResult have zero references in src/, that the only external use of bun_semver_jsc is SemverObject::create in BunObject.rs, and that none of the dropped crate names or their macros appear in either crate's source. The Cargo.lock hunk contains exactly the expected removals for bun_semver and bun_semver_jsc and nothing else.
Other factors
The bug hunter's one finding is a nit: a leftover migration breadcrumb comment at src/semver/lib.rs:544 now points at deleted code. It's the same class of cleanup this PR already did in dependency_jsc.rs, but it's comment-only, has no runtime effect, and lives in a file outside the diff — not worth blocking on. The PR description documents thorough verification (cargo check, bun bd, rust:check-all, semver.test.ts), and the change is small and mechanical enough that human review would add little.
The comment pointed readers at bun_semver_jsc for the String to_js bridge, which the previous commit removed.
There was a problem hiding this comment.
LGTM. Pure dead-code removal — the inline nit about the leftover breadcrumb at src/semver/lib.rs:11-12 is comment-only and non-blocking.
Extended reasoning...
Overview
Deletion-only sweep of bun_semver / bun_semver_jsc: removes the unused SemverStringJsc extension trait and its module, drops two dead re-exports from src/semver_jsc/lib.rs, prunes 7 unused Cargo deps from each crate (with matching Cargo.lock shrinkage), trims a stale doc comment in src/install_jsc/dependency_jsc.rs, and — after the prior review round — removes the stale toJS breadcrumb at src/semver/lib.rs:544. Net +1/-53.
Security risks
None. No executable logic is added or changed; every hunk is a deletion of an unused symbol, an unused dep entry, or a comment.
Level of scrutiny
Low. I re-verified the PR's dead-code claims: rg for SemverStringJsc|SemverString_jsc|bun_semver_jsc::JsResult across src/ → 0 hits; the sole external reference to bun_semver_jsc is bun_runtime's SemverObject::create call in BunObject.rs, which is untouched; and none of the removed dep names/macros appear in any .rs under either crate. The Cargo.lock diff exactly mirrors the two Cargo.toml edits. rust:check-all and bun bd passing per the description confirm no cfg-gated code depended on the removed items.
Other factors
The prior review comment (stale breadcrumb at line 544) was addressed in e03ffa6 and the thread is resolved. This run flagged one more sibling of the same class at src/semver/lib.rs:11-12 — a two-line comment-only nit that doesn't affect correctness and can be folded in before merge or left for a follow-up.
The comment claimed *_jsc extension traits live in bun_semver_jsc, but the only such trait (SemverStringJsc) was removed earlier in this PR.
There was a problem hiding this comment.
LGTM — both prior nits (stale breadcrumb comments in src/semver/lib.rs) were addressed in e03ffa6 and e1e412e.
What was reviewed:
- Confirmed
SemverStringJsc/SemverString_jsc/bun_semver_jsc::JsResulthave zero references insrc/; the only remaining consumer ofbun_semver_jscisBunObject.rscallingSemverObject::create. - Confirmed none of the seven dropped Cargo deps (nor their macros —
bitflags!,defer!,formatcp,EnumSetType, etc.) appear in any.rsfile undersrc/semver/orsrc/semver_jsc/. Cargo.lockchanges are exactly the dep removals for these two crates; no version drift elsewhere.
Extended reasoning...
Overview
Pure dead-code sweep of bun_semver / bun_semver_jsc: deletes the never-imported SemverStringJsc extension trait file, drops two unused re-exports from src/semver_jsc/lib.rs, removes seven unused workspace deps from each crate's Cargo.toml (with the corresponding Cargo.lock entries), and trims four stale migration-breadcrumb comments (one in dependency_jsc.rs, three in src/semver/lib.rs — the latter two added in follow-up commits after earlier review). Net +1/-53.
Security risks
None. This is deletion-only with no behavioral change; no user-facing surface, parsing, auth, or FFI is touched. The only executable-code change is removing an unreferenced trait impl.
Level of scrutiny
Low. The Rust compiler is the load-bearing safety net here — a missing dep or a still-referenced symbol would fail cargo check / bun bd, and the PR reports bun run rust:check-all passing on all 10 target triples plus bun bd test test/cli/install/semver.test.ts (which exercises SemverObject::create, the one live path in bun_semver_jsc). I independently verified via grep that (a) no reference to SemverStringJsc, SemverString_jsc, or bun_semver_jsc::JsResult exists anywhere under src/, (b) the sole external reference to bun_semver_jsc is bun_runtime's SemverObject::create call, and (c) none of the seven removed deps or their derive/macro names appear in any .rs file under either crate.
Other factors
Both of my prior inline comments (stale toJS breadcrumb at lib.rs:544 and the SemverObject re-export breadcrumb at lib.rs:11-12) were addressed and resolved; the author also swept for remaining siblings and noted the one unrelated deinit/Drop breadcrumb in SemverQuery.rs is still accurate. The Cargo.lock hunk contains only the two bun_semver* package entries with the seven deps removed from each — no unrelated churn. No outstanding reviewer comments.
|
CI on build 78744: the 5 red tests ( |
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.
Dead-code sweep of the
bun_semver/bun_semver_jsccrates. Net change: +1 / -53 lines.Removed
src/semver_jsc/SemverString_jsc.rs(whole file): theSemverStringJscextension trait is never imported. The only dependent ofbun_semver_jscisbun_runtime, whose sole reference isbun_semver_jsc::SemverObject::create. The one place that wants abun_semver::String→ JS conversion (src/install_jsc/dependency_jsc.rs) doesn't depend onbun_semver_jscand has its own inline helper.pub use bun_jsc::JsResultandpub use SemverString_jsc::SemverStringJscfromsrc/semver_jsc/lib.rs: no external references (rg 'bun_semver_jsc::JsResult|bun_semver_jsc::SemverStringJsc' src/→ 0 hits).src/semver/Cargo.tomlandsrc/semver_jsc/Cargo.toml:strum,scopeguard,const_format,enum-map,enumset,libc,bitflags(7 each). None of these crate names, nor their macros/derives (bitflags!,defer!,formatcp,EnumSetType,EnumMap,c_int, etc.), appear in any.rsfile under either crate.src/install_jsc/dependency_jsc.rsthat referenced the deleted trait (and made a claim about stubbed JSC types that is no longer accurate).Verification
rgacrosssrc/andbuild/debug/codegen/for every removed symbol and dep name: only definition-site hits.cargo check -p bun_semverpasses.bun bdbuilds.bun run rust:check-allpasses on all 10 target triples.bun bd test test/cli/install/semver.test.tspasses (28/28, coversBun.semverwhich routes throughbun_semver_jsc::SemverObject).