Remove dead code from bun_sourcemap - #34878
Conversation
|
Warning Review limit reached
Next review available in: 20 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 (2)
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. Verification is |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
LGTM — pure dead-code removal, all four symbols verified unreferenced.
What was reviewed:
- Grepped
src/for each removed symbol (SourceMapper,append_source_mapping_url_remote,print_source_map_contentswithout_from_internal,FindCache::invalidate): zero callers. - Confirmed
print_source_map_contents_from_internal(the surviving variant) andinvalidate_allremain wired to their call sites inVirtualMachine.rs/SavedSourceMap.rs. - No
#[no_mangle]/extern "C"exports touched; nothing reachable from codegen or FFI.
Extended reasoning...
Overview
This PR deletes four unreferenced public items from src/sourcemap/ (net -44 lines): the SourceMapper<T> type alias, the free function append_source_mapping_url_remote, Chunk::print_source_map_contents (raw-VLQ variant), and FindCache::invalidate. No behavior is added or modified — every hunk is a pure deletion.
Security risks
None. Deletions only; no input handling, no FFI, no auth/crypto surface touched.
Level of scrutiny
Low. This is a mechanical dead-code cleanup in an internal Rust crate. I independently grepped the workspace for each symbol and confirmed the PR description's claims: only the _from_internal variant of print_source_map_contents is called (src/jsc/VirtualMachine.rs:2751), only invalidate_all is called on FindCache (src/jsc/SavedSourceMap.rs:315), and neither SourceMapper nor append_source_mapping_url_remote appears anywhere else. None of the removed items are #[no_mangle] / extern "C", so there's no risk of a hidden C++ caller. rust:check-all would fail on any missed reference.
Other factors
The one inline nit (bun_url now unused in src/sourcemap/Cargo.toml) is build-graph hygiene only — it doesn't affect the compiled binary or block merge. The PR description already documents verification (bun bd, rust:check-all on all 10 targets, sourcemap test suites), and the followup note about find_with_cache being write-only is correctly scoped out.
- SourceMapper<T>: unused type alias for SourceMapFormat<T>, carried over from the Zig port with no Rust callers - FindCache::invalidate: only invalidate_all is called (from SavedSourceMap::put_value); per-blob invalidation has no callers Verified with rg across src/, build/debug/codegen/, and *.classes.ts. bun bd and rust:check-all pass on all targets.
2f9f4b7 to
8adba4c
Compare
|
Narrowed in 8adba4c to avoid overlap with #32000: dropped the The |
There was a problem hiding this comment.
LGTM — pure dead-code removal, both symbols verified unreferenced.
Disregard my earlier inline comment about bun_url / append_source_mapping_url_remote — that function isn't removed by this PR (it's in #32000, as the description notes), so the Cargo.toml cleanup doesn't belong here.
What was reviewed:
rgconfirmsSourceMapperhas zero references anywhere insrc/.FindCache::invalidatehas no callers; the only consumer (SavedSourceMap.rs:315) callsinvalidate_all(). Unrelated.invalidate()hits in html_rewriter/dev_server are different types.- Neither item is
#[no_mangle]/extern "C", re-exported, or referenced from codegen/.classes.ts.
Extended reasoning...
Overview
Net -11 lines across two files in src/sourcemap/. Removes (1) pub type SourceMapper<T> = SourceMapFormat<T> — an unused type alias carried over from the Zig port, and (2) FindCache::invalidate(&mut self, data: *const u8) — per-blob cache invalidation that nothing calls (only invalidate_all is used, from SavedSourceMap::put_value).
Security risks
None. Pure deletion of unreferenced Rust items with no FFI exposure, no runtime behavior change, no user-reachable surface.
Level of scrutiny
Low. This is the canonical safe-to-auto-approve shape: deletion-only, verified dead by repo-wide grep, no extern/#[no_mangle], and bun run rust:check-all passes on all 10 targets per the PR description. If anything still referenced these, the build would fail — Rust's compiler is the test here.
Other factors
- I independently confirmed via
rgthatSourceMapperhas zero hits insrc/and that the onlyFindCache-relatedinvalidatecall isinvalidate_all()atsrc/jsc/SavedSourceMap.rs:315. The.invalidate()calls inhtml_rewriter.rsandbake/dev_serverare on unrelated types. - My earlier inline comment (about dropping
bun_urlfromsrc/sourcemap/Cargo.toml) was misplaced: it referenced the removal ofappend_source_mapping_url_remotefromlib.rs, but this PR does not touchlib.rs— that removal is in #32000, which the PR description explicitly calls out to avoid conflict. The comment does not apply here and can be ignored. - The PR description's followup note about
find_with_cache/FindCachebeing write-only is out of scope and correctly deferred. - No outstanding human reviewer comments; no test needed (no observable behavior to regress).
Removes two unreferenced public items from
src/sourcemap/(net -11 lines).Removed
SourceMapper<T>(Chunk.rs): type alias forSourceMapFormat<T>. Carried over from the Zig port; never referenced in Rust.FindCache::invalidate(InternalSourceMap.rs): per-blob invalidation. Onlyinvalidate_allis called (fromSavedSourceMap::put_value).Two other items (
Chunk::print_source_map_contentsandappend_source_mapping_url_remote) are also dead but already removed by #32000, so they are left out of this PR to avoid a conflict.Verification
rgfor each symbol acrosssrc/,build/debug/codegen/,*.classes.ts, andpackages/: zero hits outside the definition for both.#[no_mangle]/extern "C"exports touched.bun bdbuilds clean.bun run rust:check-allpasses on all 10 targets.bun bd test test/js/bun/sourcemap/: all pass.Followup (not in this PR)
InternalSourceMap::find_with_cacheand theFindCachestruct itself appear to be write-only:SavedSourceMapallocates a ~21KBFindCacheand callsinvalidate_all()on it input_value, but nothing ever callsfind_with_cacheto populate or read it; stack remapping goes through the uncachedfind(). Removing that would also touchSavedSourceMap's layout and a test that references FindCache by name, so I've left it for a separate pass.