Remove dead code from resolver, install, event_loop, node_fs, fetch - #36872
Conversation
Removes unreferenced trait impls, enum variants, type aliases, and methods with zero callers across src/ and build/debug/codegen/. resolver/fs.rs: impl Clone/Default for Entry. The Clone impl's comment claimed BSSList::append needs ValueType: Clone, but OverflowList::append (bun_alloc/lib.rs:1887) has no such bound; all Entry construction goes through append_uninit + raw writes. resolver/result.rs: impl Default for DirEntryResolveQueueItem. The queue is [MaybeUninit<_>; 256] populated only via .write() with all fields explicit (resolver.rs:4140,4195,4233). resolver/lib.rs: impl Default for cache::Fs. Set::init() builds Fs with all four fields explicit; the two ..Default::default() nearby are for bun_sys::WindowsOpenDirOptions. install: PrintFormat::Info variant (never constructed; pm_trusted_command only passes Untrusted/Completed), OldV2VersionedURL type alias + its re-exports (zero references to the alias anywhere). event_loop/ManagedTask.rs: cancel() method. No caller invokes .cancel() on a ManagedTask. runtime/node/node_fs.rs: StatOrNotFound::to_js (duplicate of to_js_newly_created; FsReturn impl calls the latter). runtime/webcore/fetch: FetchRequestBodySinkJSSink type alias + re-export + the JSSink import that fed it. All callers spell JSSink<FetchRequestBodySink>. Verified with bun bd and bun run rust:check-all (10/10 targets). Adds a source-lint test following the existing dead-symbol test pattern.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
Disabled knowledge base sources:
WalkthroughThe PR removes obsolete public APIs and default implementations across event loop, install, resolver, Node filesystem, and fetch modules. It also removes the informational script format and legacy versioned URL alias. ChangesPublic API cleanup
Possibly related PRs
Suggested reviewers: Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — thanks for dropping the source-lint test file. The remaining inline nit (stale cancel() reference in the release_queued_tasks_for_shutdown doc comment) is non-blocking.
Extended reasoning...
Overview
Pure dead-code removal: 83 net lines deleted across 11 Rust files (unused Default/Clone impls in resolver, an unconstructed PrintFormat::Info variant, the OldV2VersionedURL alias, ManagedTask::cancel, a duplicate StatOrNotFound::to_js, and the FetchRequestBodySinkJSSink alias + its now-unused import). Since my earlier comment, commit 3e400f5 deleted test/internal/source-lints/dead-symbols-resolver-install-eventloop.test.ts, so the PR is now deletions-only with no test/** changes — my previous concern is resolved.
Security risks
None. No new code paths, no input handling, no auth/crypto/permissions surface. Removing an enum variant (PrintFormat::Info) and trait impls that the compiler confirms are unreferenced cannot introduce a security regression.
Level of scrutiny
Low-to-medium. The compiler is the ground truth for dead-code removal in Rust: bun run rust:check-all passed on all 10 targets, and targeted test suites (fs stat, resolve, pm trust) pass. Each removed item was individually justified in the PR description with the specific reason it's unreachable (e.g., DirEntryResolveQueueItem is only ever constructed via .write({ all fields }) into a MaybeUninit array; Entry construction goes through append_uninit + field-wise raw writes so Clone/Default are unused). I spot-checked that VersionedURLType<u32>::migrate() still compiles without the OldV2VersionedURL alias (it names the generic directly) and that the PrintFormat match remains exhaustive over the two surviving variants.
Other factors
The one inline nit this run — a stale doc comment on release_queued_tasks_for_shutdown in src/jsc/event_loop.rs that still names cancel() and already-gone SendQueue fields as its rationale — is documentation drift with zero runtime impact; the guard it documents is unchanged. It's worth cleaning up but doesn't block. The earlier open thread about the source-lint test file is moot now that the file is deleted.
| @@ -33,13 +33,6 @@ impl ManagedTask { | |||
| callback(ctx.unwrap().as_ptr()) | |||
| } | |||
There was a problem hiding this comment.
🟡 Removing ManagedTask::cancel() leaves the doc comment on release_queued_tasks_for_shutdown at src/jsc/event_loop.rs:735-742 stale — it still names cancel() (and the already-gone SendQueue.close_next_tick/after_close_task) as the load-bearing rationale for the task.tag != ManagedTask guard at line 756. Per REVIEW.md "grep for every sibling site sharing the pattern", that comment should be updated (and the guard's remaining justification, if any, restated) in this PR. Not a runtime bug — behavior is unchanged.
Extended reasoning...
What is stale
The PR deletes ManagedTask::cancel() from src/event_loop/ManagedTask.rs after verifying zero call sites. That's correct — nothing invokes .cancel() on a ManagedTask. But the doc comment on EventLoop::release_queued_tasks_for_shutdown at src/jsc/event_loop.rs:735-742 still documents cancel() as the reason ManagedTask entries are re-queued instead of freed at shutdown:
ManagedTaskentries are deliberately re-queued rather than freed: owners (e.g.SendQueue.close_next_tick/after_close_task) keep raw back-pointers that theycancel()fromDrop, and thoseDrops fire duringdestructOnExit(Subprocess::finalize→SendQueue::drop). Freeing the box here would leave those pointers dangling and makecancel()a heap-use-after-free.
With cancel() deleted, this comment now describes a UAF hazard around a method that structurally cannot be called. The task.tag != ManagedTask guard at line 756 is left with no live justification in prose.
Why the PR's verification missed it
The PR description says every symbol was checked with rg -w <symbol> src/ build/debug/codegen/ src/codegen/ for zero hits outside its own definition. A prose reference inside a /// doc comment doesn't show up as a caller to rg -w cancel (there are dozens of unrelated .cancel() hits on other types), and the referenced SendQueue.close_next_tick/after_close_task owners were already gone before this PR — rg 'close_next_tick|after_close_task' src/ returns only this comment. So the comment was already partially stale; removing cancel() is what makes its central claim reference a nonexistent method.
Step-by-step
- Before this PR:
ManagedTaskhas acancel()method that overwritesself.callbackwith a no-op. No code calls it. src/jsc/event_loop.rs:756special-casestask.tag != ManagedTaskso that__bun_release_task_at_shutdownnever consumes aManagedTask— they get re-queued and freed later indeinit()(line 783) via thecleanupfield.- The doc comment at :735-742 explains that guard by saying owners hold raw back-pointers and call
cancel()fromDropduringdestructOnExit, so freeing the box early would make thatcancel()a heap-UAF. - This PR deletes
cancel(). The named owners (SendQueue.close_next_tick,after_close_task) already exist nowhere insrc/. - After this PR: the guard at :756 and the free-in-
deinit()at :783 are still correct (behavior unchanged —ManagedTaskboxes are still freed viaheap::take+cleanupafterdestructOnExit), but the comment now points at a method that doesn't exist to explain why.
Why this matters per REVIEW.md
REVIEW.md → Correctness → "Fix the whole class in the same PR … Grep for every sibling site sharing the pattern" and → One source of truth; update every consumer atomically both apply: a comment that names a symbol as its load-bearing rationale is a consumer of that symbol. REVIEW.md → Code style → "Delete dead code in the same PR that makes it dead" is what this PR is doing; the stale comment is a leftover of the same class.
REVIEW.md also says "Before deleting odd-looking code, git-blame why it was written — it is usually load-bearing." The next reader trying to understand why ManagedTask is special-cased at :756 will follow this comment to a method that no longer exists and owners that no longer exist, and can't tell whether the guard is still needed.
Impact
None at runtime. release_queued_tasks_for_shutdown and deinit() behave identically before and after. This is purely a doc-comment/rationale drift.
Fix
Update the doc comment at src/jsc/event_loop.rs:735-742 to either (a) delete the cancel()/SendQueue paragraph and restate the actual remaining reason ManagedTask is deferred to deinit() (if one still exists — e.g. cleanup may call into subsystems that aren't safe until after destructOnExit, or an owner still holds a raw back-pointer through destructOnExit), or (b) if no rationale remains, note that and consider dropping the special-case in a follow-up. Either way, the comment should stop naming cancel().
|
Updated 11:05 PM PT - Aug 3rd, 2026
@Jarred-Sumner, your commit 3e400f5 is building: |
…ven-sh#36872) Removes 83 net lines of verified-unreferenced code across 11 Rust files. Each symbol was verified to have zero callers across `src/`, `build/debug/codegen/`, and `src/codegen/`; `bun bd` and `bun run rust:check-all` (10/10 targets) pass with them removed. ### Removed - **`src/resolver/fs.rs`**: `impl Clone for Entry` + `impl Default for Entry` (32 lines). The `Clone` comment claimed `BSSList::append` requires `ValueType: Clone`, but `OverflowList::append` at `src/bun_alloc/lib.rs:1887` has no such bound and all `Entry` construction goes through `append_uninit` + field-wise raw writes. No `.clone()` / `Entry::default()` anywhere. - **`src/resolver/result.rs`**: `impl Default for DirEntryResolveQueueItem` (15 lines). The queue is `[MaybeUninit<_>; 256]` populated only via `.write(DirEntryResolveQueueItem { <all fields> })` at resolver.rs:4140/4195/4233. - **`src/resolver/lib.rs`**: `impl Default for cache::Fs` (11 lines). `Set::init()` builds `Fs` with all four fields explicit; the two `..Default::default()` in this file are for `bun_sys::WindowsOpenDirOptions`. - **`src/install/lockfile/Package/Scripts.rs`**: `PrintFormat::Info` variant + its match arm (6 lines). `pm_trusted_command.rs` only ever passes `Untrusted`/`Completed`; `Info` is never constructed. - **`src/install_types/resolver_hooks.rs`** + re-exports in `install_types/lib.rs` and `install/lib.rs`: `OldV2VersionedURL` type alias (4 lines). Zero references to the alias name anywhere. - **`src/event_loop/ManagedTask.rs`**: `ManagedTask::cancel` (7 lines). No caller invokes `.cancel()` on a `ManagedTask`; the `.cancel()` calls in napi/s3/h3/ReadableStream are on unrelated types. - **`src/runtime/node/node_fs.rs`**: `StatOrNotFound::to_js` (6 lines). Exact duplicate of `to_js_newly_created`; the `FsReturn` impl for `StatOrNotFound` calls `to_js_newly_created` directly. - **`src/runtime/webcore/fetch/FetchRequestBodySink.rs`** + `fetch.rs`: `FetchRequestBodySinkJSSink` type alias + re-export + now-unused `JSSink` import (3 lines). Both callers (`FetchTasklet.rs:439,683`) spell out `JSSink<FetchRequestBodySink>`; not referenced by `generate-jssink.ts`. ### Verification ``` rg -w <symbol> src/ build/debug/codegen/ src/codegen/ # zero hits outside own definition for every item above bun bd # builds bun run rust:check-all # 10/10 targets ok (linux/macos/windows x x64/aarch64) bun bd test test/js/node/fs/fs.test.ts -t stat # 18 pass bun bd test test/js/bun/resolve/resolve.test.ts # 48 pass bun bd test test/cli/install/bun-pm.test.ts -t trust # 4 pass ``` ### Followups (not removed; listed for reference) Candidates that looked dead to `rg` but have cross-crate callers discovered at build time (kept): `AutoBitSet::{bytes,for_each}` (bundler computeChunks), `impl Default for cache::Entry` (bundler ParseTask), `NullDelimitedEnvMap::as_slice` (js_bun_spawn_bindings, shell subproc), `BlobOrStringOrBuffer::byte_length` (ValkeyCommand). Unremoved because reachable via `strum::EnumString` string parse: `RedisError::{InvalidArgument,InvalidArray,InvalidBigNumber,InvalidErrorString,InvalidNull,InvalidSimpleString}`, `AnyPostgresError::{InvalidByteSequenceForEncoding,InvalidTimeFormat}`. These are never directly constructed but are parse targets of `.name().parse()` and removing them would silently change error-code mapping if a source enum gains a matching variant name. ### Note on LOC This run's yield is well below the 1000 LOC target. The tree has been aggressively swept over the last three weeks: 10 merged + 5 open dead-code PRs, including commit 85ddc95 which removed ~39k lines in one pass. A full fan-out across `src/http`, `src/install`, `src/runtime/webcore`, `src/runtime/node`, `src/runtime/api`, `src/bun_core`, `src/jsc`, `src/collections`, `src/resolver`, `src/ast`, `src/semver`, `src/dotenv`, `src/patch`, `src/glob`, `src/watcher`, `src/event_loop`, `src/sql/postgres`, `src/valkey`, and the C++ bindings found ~40 candidates, of which ~15 proved genuinely unreferenced after build-time verification. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 12 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 2 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/dead-symbols-resolver-install-eventloop.test.ts bun test v1.4.0 (7ebba9e) test/internal/source-lints/dead-symbols-resolver-install-eventloop.test.ts: 25 | ["src/resolver/fs.rs", /impl Default for Entry \{/], 26 | ["src/resolver/result.rs", /impl Default for DirEntryResolveQueueItem \{/], 27 | ["src/resolver/lib.rs", /impl Default for Fs \{/], 28 | ]; 29 | const resurrected = checks.filter(([file, re]) => re.test(src(file))).map(([file, re]) => `${file}: ${re.source}`); 30 | expect(resurrected).toEqual([]); ^ error: expect(received).toEqual(expected) - [] + [ + "src/resolver/fs.rs: impl Clone for Entry \{", + "src/resolver/fs.rs: impl Default for Entry \{", + "src/resolver/result.rs: impl Default for DirEntryResolveQueueItem \{", + "src/resolver/lib.rs: impl Default for Fs \{", + ] - Expected - 1 + Received + 6 at <anonymous> (/workspace/bun/test/internal/source-lints/dead-symbols-resolver-install-eventloop.test.ts:30:23) (fail) dead resolver t ... (truncated) release without fix: 2 FAILED bun test v1.4.0-canary.1 (1498d7b) test/internal/source-lints/dead-symbols-resolver-install-eventloop.test.ts: 25 | ["src/resolver/fs.rs", /impl Default for Entry \{/], 26 | ["src/resolver/result.rs", /impl Default for DirEntryResolveQueueItem \{/], 27 | ["src/resolver/lib.rs", /impl Default for Fs \{/], 28 | ]; 29 | const resurrected = checks.filter(([file, re]) => re.test(src(file))).map(([file, re]) => `${file}: ${re.source}`); 30 | expect(resurrected).toEqual([]); ^ error: expect(received).toEqual(expected) - [] + [ + "src/resolver/fs.rs: impl Clone for Entry \{", + "src/resolver/fs.rs: impl Default for Entry \{", + "src/resolver/result.rs: impl Default for DirEntryResolveQueueItem \{", + "src/resolver/lib.rs: impl Default for Fs \{", + ] - Expected - 1 + Received + 6 at <anonymous> (/workspace/bun/test/internal/source-lints/dead-symbols-resolver-install-eventloop.test.ts:30:23) (fail) dead resolver trait impls do not reappear [1.03ms] 40 | ["src/runtime/node/node_fs.rs", /impl StatOrNotFound \{\n pub fn to_js\(&mut self,/], 41 | ["src/runtime/webcore/fetch/FetchRequestBodySink.rs", /\ ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/dead-symbols-resolver-install-eventloop.test.ts bun test v1.4.0 (7ebba9e) test/internal/source-lints/dead-symbols-resolver-install-eventloop.test.ts: (pass) dead resolver trait impls do not reappear [22.41ms] (pass) dead install/event_loop/runtime symbols do not reappear [18.66ms] 2 pass 0 fail 2 expect() calls Ran 2 tests across 1 file. [2.05s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 668ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/5] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 238 extern-C blocks audited [1/5] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_install_types v0.0.0 (/workspace/bun/src/install_types) �[1m�[92m Compiling�[0m bun_event_loop v0.0.0 (/workspace/bun/src/event_loop) �[1m�[92m Compiling�[0m bun_options_types v0.0.0 (/workspace/bun/src/options_types) �[1m�[92m Compiling�[0m bun_http v0.0.0 (/workspace/bun/src/http) �[1m�[92m Compiling�[0m bun_crash_handler v0.0.0 (/workspace/bun/src/crash_handler) �[1m�[92m Compiling�[0m bun_resolve_builtins v0.0.0 (/workspace/bun/src/resolve_builtins) �[1m�[92m Compiling�[0m bun_api v0.0.0 (/workspace/bun/src/api) �[1m�[92m Compiling�[0m bun_js_parser v0.0.0 (/workspace/bun/src/js_parser) �[1m�[92m Compiling�[0m bun_js_printer v0.0.0 (/workspace/ ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/event_loop/ManagedTask.rs | 7 ---- src/install/lib.rs | 4 +- src/install/lockfile/Package/Scripts.rs | 6 --- src/install_types/lib.rs | 8 ++-- src/install_types/resolver_hooks.rs | 1 - src/resolver/fs.rs | 32 --------------- src/resolver/lib.rs | 11 ------ src/resolver/result.rs | 15 ------- src/runtime/node/node_fs.rs | 6 --- src/runtime/webcore/fetch.rs | 2 +- src/runtime/webcore/fetch/FetchRequestBodySink.rs | 3 -- ...dead-symbols-resolver-install-eventloop.test.ts | 46 ++++++++++++++++++++++ 12 files changed, 52 insertions(+), 89 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/event_loop/ManagedTask.rs 1 1 0 src/install/lib.rs 1 1 0 src/install/lockfile/Package/Scripts.rs 1 1 0 src/install_types/lib.rs 1 1 0 src/install_types/resolver_hooks.rs 1 1 0 src/resolver/fs.rs 1 1 0 src/resolver/lib.rs 1 2 0 src/resolver/result.rs 1 1 0 src/runtime/node/node_fs.rs 1 1 0 src/runtime/webcore/fetch.rs 1 1 0 src/runtime/webcore/fetch/FetchRequestBodySink.rs 2 2 0 …e-lints/dead-symbols-resolver-install-eventloop.test.ts 0 1 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
2 participants
Removes 83 net lines of verified-unreferenced code across 11 Rust files. Each symbol was verified to have zero callers across
src/,build/debug/codegen/, andsrc/codegen/;bun bdandbun run rust:check-all(10/10 targets) pass with them removed.Removed
src/resolver/fs.rs:impl Clone for Entry+impl Default for Entry(32 lines). TheClonecomment claimedBSSList::appendrequiresValueType: Clone, butOverflowList::appendatsrc/bun_alloc/lib.rs:1887has no such bound and allEntryconstruction goes throughappend_uninit+ field-wise raw writes. No.clone()/Entry::default()anywhere.src/resolver/result.rs:impl Default for DirEntryResolveQueueItem(15 lines). The queue is[MaybeUninit<_>; 256]populated only via.write(DirEntryResolveQueueItem { <all fields> })at resolver.rs:4140/4195/4233.src/resolver/lib.rs:impl Default for cache::Fs(11 lines).Set::init()buildsFswith all four fields explicit; the two..Default::default()in this file are forbun_sys::WindowsOpenDirOptions.src/install/lockfile/Package/Scripts.rs:PrintFormat::Infovariant + its match arm (6 lines).pm_trusted_command.rsonly ever passesUntrusted/Completed;Infois never constructed.src/install_types/resolver_hooks.rs+ re-exports ininstall_types/lib.rsandinstall/lib.rs:OldV2VersionedURLtype alias (4 lines). Zero references to the alias name anywhere.src/event_loop/ManagedTask.rs:ManagedTask::cancel(7 lines). No caller invokes.cancel()on aManagedTask; the.cancel()calls in napi/s3/h3/ReadableStream are on unrelated types.src/runtime/node/node_fs.rs:StatOrNotFound::to_js(6 lines). Exact duplicate ofto_js_newly_created; theFsReturnimpl forStatOrNotFoundcallsto_js_newly_createddirectly.src/runtime/webcore/fetch/FetchRequestBodySink.rs+fetch.rs:FetchRequestBodySinkJSSinktype alias + re-export + now-unusedJSSinkimport (3 lines). Both callers (FetchTasklet.rs:439,683) spell outJSSink<FetchRequestBodySink>; not referenced bygenerate-jssink.ts.Verification
Followups (not removed; listed for reference)
Candidates that looked dead to
rgbut have cross-crate callers discovered at build time (kept):AutoBitSet::{bytes,for_each}(bundler computeChunks),impl Default for cache::Entry(bundler ParseTask),NullDelimitedEnvMap::as_slice(js_bun_spawn_bindings, shell subproc),BlobOrStringOrBuffer::byte_length(ValkeyCommand).Unremoved because reachable via
strum::EnumStringstring parse:RedisError::{InvalidArgument,InvalidArray,InvalidBigNumber,InvalidErrorString,InvalidNull,InvalidSimpleString},AnyPostgresError::{InvalidByteSequenceForEncoding,InvalidTimeFormat}. These are never directly constructed but are parse targets of.name().parse()and removing them would silently change error-code mapping if a source enum gains a matching variant name.Note on LOC
This run's yield is well below the 1000 LOC target. The tree has been aggressively swept over the last three weeks: 10 merged + 5 open dead-code PRs, including commit 85ddc95 which removed ~39k lines in one pass. A full fan-out across
src/http,src/install,src/runtime/webcore,src/runtime/node,src/runtime/api,src/bun_core,src/jsc,src/collections,src/resolver,src/ast,src/semver,src/dotenv,src/patch,src/glob,src/watcher,src/event_loop,src/sql/postgres,src/valkey, and the C++ bindings found ~40 candidates, of which ~15 proved genuinely unreferenced after build-time verification.[review] gate passed · iteration 0 · 12 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file