Remove dead code from shell, bake dev server, sql, install, cli, css, and websocket client - #37229
Conversation
… subprocess
All items verified to have zero references across src/, scripts/, packages/,
and regenerated build/debug/codegen output, then validated by cargo check and
a full debug build.
- shell: WriterTag::{Pipeline,Subshell,If} were never constructed; deleted
the variants, their dispatch arms, and the two on_io_writer_chunk handlers
only reachable through them. Also removed never-assigned states
(SubshellState::{Expanding,WaitWriteErr}, IfState::{WaitingWriteErr,Done},
CondExprState::Done, CatState::Done), Yield::Start,
ParseFlagResult::ShowUsage, and EntryKindHint::File.
- bake: route_bundle::State::EvaluationFailure was never constructed; deleted
the EnsureRouteCtx::on_failure chain, the always-None
Framework.evaluate_failure field, and ErrorPageKind (now a constant).
Dropped MessageId::{BrowserMessage,BrowserMessageClear,RequestHandlerError}
wire bytes the server never sent (generated.ts regenerated).
- sql: 28 unsent mysql CommandType variants, 13 unread StatusFlag variants,
and never-constructed error variants (mysql InvalidState, postgres
InvalidByteSequenceForEncoding/InvalidTimeFormat) with their arms.
- install: Subcommand::Scan (bun pm scan dispatches under Subcommand::Pm),
Step::{Copyfile,Linking,Patching}, the write-only lifecycle script time
log (empty entry struct, list never read, feeding timer always None), and
the file_count field flagged 'this is never read'.
- cli: PackError::MissingPackageJSON (never constructed) and
ExampleTag::Unknown (zero references).
- css: never-constructed crate_error::Error::CSSPrintError and two uncalled
inherent parse_with_options helpers.
- websocket: five never-constructed ErrorCode variants (explicit
discriminants keep the remaining C++ table values stable).
- subprocess/timer/watcher: uncalled Readable/Writable stubs, the orphan
WTFTimer__runIfImminent extern, and the never-sent fs-watcher
EventType::Abort.
- C++: KeyObject::getKeyObjectFromHandle decl+def pair with zero callers.
- js: two unread repl util-shim constants and a write-only minimatch debug
property.
- Cargo.toml: unused deps in bun_glob/bun_router and bundler's unused
bun_router dependency.
Adds a source lint pinning every removal so the symbols do not reappear.
|
Updated 3:42 PM PT - Aug 11th, 2026
@Jarred-Sumner, your commit fc75209 is building: |
WalkthroughThe pull request removes unused dependencies, obsolete public symbols, dead runtime paths, and redundant state transitions across installation, runtime, protocol, CSS, JavaScript, and C++ code. It adds source-lint coverage for removed symbols. ChangesDead code cleanup
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This interruption was autofix-ci's formatting commit (e6fd605: two collapsed blank lines from rustfmt, one prettier line wrap in the lint test). No functional changes; CI is running against that head. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install/PackageInstall.rs`:
- Around line 1099-1101: Update the copy helper functions used by the call sites
around copy, including the additional referenced sections, to return
crate::Result<()> instead of crate::Result<u32>. Remove real_file_count and all
increments, and return Ok(()) after successful copying while preserving existing
error propagation and InstallResult handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5fc7bf8-5212-4330-91ac-b5fa0b12ce77
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
src/bundler/Cargo.tomlsrc/css/crate_error.rssrc/css/selectors/parser.rssrc/glob/Cargo.tomlsrc/http_jsc/websocket_client.rssrc/install/PackageInstall.rssrc/install/PackageInstaller.rssrc/install/PackageManager.rssrc/install/PackageManager/CommandLineArguments.rssrc/install/PackageManager/PackageManagerLifecycle.rssrc/install/lifecycle_script_runner.rssrc/install/patch_install.rssrc/js/internal/fs/glob.tssrc/js/internal/repl/node-shims.jssrc/jsc/bindings/node/crypto/KeyObject.cppsrc/jsc/bindings/node/crypto/KeyObject.hsrc/router/Cargo.tomlsrc/runtime/api/bun/subprocess/Readable.rssrc/runtime/api/bun/subprocess/Writable.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/dev_server/mod.rssrc/runtime/bake/dev_server/route_bundle.rssrc/runtime/cli/create_command.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/publish_command.rssrc/runtime/node/node_fs_watcher.rssrc/runtime/shell/IOWriter.rssrc/runtime/shell/Yield.rssrc/runtime/shell/builtin/cat.rssrc/runtime/shell/builtin/rm.rssrc/runtime/shell/interpreter.rssrc/runtime/shell/states/CondExpr.rssrc/runtime/shell/states/If.rssrc/runtime/shell/states/Pipeline.rssrc/runtime/shell/states/Subshell.rssrc/runtime/timer/WTFTimer.rssrc/s3_signing/lib.rssrc/sql/mysql/StatusFlags.rssrc/sql/mysql/protocol/AnyMySQLError.rssrc/sql/mysql/protocol/CommandType.rssrc/sql/postgres/AnyPostgresError.rssrc/sql_jsc/mysql/protocol/any_mysql_error_jsc.rssrc/sql_jsc/postgres/error_jsc.rstest/internal/source-lints/dead-symbols-shell-bake-sql-install.test.ts
💤 Files with no reviewable changes (36)
- src/js/internal/fs/glob.ts
- src/sql/mysql/protocol/AnyMySQLError.rs
- src/bundler/Cargo.toml
- src/sql/mysql/protocol/CommandType.rs
- src/sql_jsc/mysql/protocol/any_mysql_error_jsc.rs
- src/runtime/timer/WTFTimer.rs
- src/js/internal/repl/node-shims.js
- src/runtime/shell/states/Pipeline.rs
- src/jsc/bindings/node/crypto/KeyObject.h
- src/install/PackageManager/CommandLineArguments.rs
- src/jsc/bindings/node/crypto/KeyObject.cpp
- src/install/PackageManager/PackageManagerLifecycle.rs
- src/css/crate_error.rs
- src/install/PackageInstaller.rs
- src/runtime/cli/pack_command.rs
- src/s3_signing/lib.rs
- src/runtime/bake/dev_server/mod.rs
- src/runtime/node/node_fs_watcher.rs
- src/glob/Cargo.toml
- src/sql/postgres/AnyPostgresError.rs
- src/runtime/shell/Yield.rs
- src/runtime/shell/states/If.rs
- src/sql_jsc/postgres/error_jsc.rs
- src/runtime/api/bun/subprocess/Writable.rs
- src/css/selectors/parser.rs
- src/http_jsc/websocket_client.rs
- src/runtime/shell/states/CondExpr.rs
- src/runtime/cli/publish_command.rs
- src/sql/mysql/StatusFlags.rs
- src/runtime/bake/dev_server/route_bundle.rs
- src/runtime/api/bun/subprocess/Readable.rs
- src/runtime/cli/create_command.rs
- src/runtime/shell/interpreter.rs
- src/router/Cargo.toml
- src/install/lifecycle_script_runner.rs
- src/install/patch_install.rs
The counts' only consumer was the file_count field removed earlier; the copy helpers now return Result<()>.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The five enum entries and their switch arms in WebSocket.cpp were unreachable: the Rust client never sent those discriminants.
|
CI status for merge review: build 90794 finished with 195 of 196 jobs passing. The single red job is ":darwin: 26 aarch64 - test-bun", which dies before any test runs (tart guest VM ssh auth failure, exit 255); the same pre-existing runner issue hit the previous build on a different host (darwin 14 aarch64), so it is independent of this diff and has been reported to CI infra. All test-level failures in both builds were known-flaky tests that passed on retry or when run alone; the two that stayed red in the first build (worker-transfer-terminate-stress on x64-asan, napi.test.ts on darwin x64) are pre-existing on main and were reported separately, and both passed in this build. The diff itself is green everywhere it ran: lints, clippy, miri, all build lanes, and every test lane except the broken darwin runner. Review feedback is addressed and all threads resolved. Ready for a maintainer. |
| Change = 1, | ||
| Error = 2, | ||
| Abort = 3, | ||
| Close = 4, |
There was a problem hiding this comment.
🟡 The C++ mirror of this enum still declares the removed variant: EventNames.cpp:50 (abort = 3,) and its switch arm at EventNames.cpp:65-66. Bun__domEventNameToJS's only caller is node_fs_watcher.rs:348 passing this Rust EventType, so DOMEventName is a 1:1 FFI mirror — the same half-prune pattern flagged for WebSocket ErrorCode/WebSocketErrorCode and fixed in b2ce4d9. Nothing breaks (Close = 4 keeps its explicit discriminant on both sides); flagging for consistency with that fix. Fix: delete abort = 3, and its case DOMEventName::abort: arm (the eventNames().abortEvent string itself is used elsewhere and should stay).
Extended reasoning...
What
The PR removes EventType::Abort = 3 from src/runtime/node/node_fs_watcher.rs:343 but leaves its C++ mirror in place:
src/jsc/bindings/webcore/EventNames.cpp:50—abort = 3,inenum class DOMEventNamesrc/jsc/bindings/webcore/EventNames.cpp:65-66—case DOMEventName::abort: return eventNames().abortEvent;
Why DOMEventName is a 1:1 FFI mirror of EventType
Bun__domEventNameToJS (EventNames.cpp:55) has exactly one caller in the entire tree: node_fs_watcher.rs:348, which passes the Rust EventType enum as its name argument (verified by grep across src/ — the only hits are the extern "C" definition, the Rust FFI import at node_fs_watcher.rs:353, and the call site at :348). The C++ DOMEventName enum exists solely to give that function a typed switch over the discriminants Rust sends; it has the same five entries with the same explicit values (rename=0, change=1, error=2, abort=3, close=4). This is a mirrored table across the FFI boundary.
Why REVIEW.md applies
REVIEW.md, Correctness: the bug class, not the bug: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep). Grep for every sibling site sharing the pattern." And One source of truth; update every consumer atomically: "When a fact lives in two places (mirrored tables, encode/decode pairs), derive one from the other."
This is byte-for-byte the same half-prune pattern that was flagged for the WebSocket ErrorCode/WebSocketErrorCode mirror during this PR's own review and fixed in commit b2ce4d9 (which removed the five orphaned entries from WebSocketErrorCode.h and their switch arms from WebSocket.cpp). The PR description's parenthetical — "C++ EventNames.cpp keeps its by-value string table entry" — is the same defense the WebSocket case had before b2ce4d9 overruled it. The fs-watcher mirror is another instance of the same class the PR already swept.
Why this is nit, not normal
Nothing breaks. Close = 4 keeps its explicit discriminant on both the Rust and C++ sides, so the remaining live values still line up byte-for-byte. Rust never sends 3 anymore, so the C++ case DOMEventName::abort: arm is unreachable — it was already unreachable before this PR (the Rust variant was never constructed), and the PR just made the desync explicit. Merging as-is causes zero behavioral change; this is purely a consistency issue with the WebSocket fix already applied in this PR.
Step-by-step proof
- Before this PR: Rust
EventType::Abort = 3exists at node_fs_watcher.rs:343 but is never constructed (per the PR's own word-boundary sweep). C++DOMEventName::abort = 3exists at EventNames.cpp:50 with a switch arm at :65-66 — unreachable, since Rust never sends3. - This PR deletes the Rust
Abort = 3line. C++abort = 3and its switch arm remain — still unreachable, now with no Rust counterpart at all. - The next Rust variant,
Close = 4, keeps its explicit discriminant and still matches C++close = 4(EventNames.cpp:51). Every live discriminant Rust can send (0,1,2,4) still hits the correct C++ case. Bun__domEventNameToJShas no other caller (grep confirms), so no other code path can reach theabortarm.
Fix
Delete abort = 3, from the DOMEventName enum (EventNames.cpp:50) and its case DOMEventName::abort: switch arm (EventNames.cpp:65-66). Do not touch eventNames().abortEvent itself — that AtomString is part of the WebCore event-name registry and is used elsewhere (AbortSignal etc.); only the fs-watcher-specific DOMEventName enum entry and its case arm are dead. Optionally extend the guard test's C++ section to cover EventNames.cpp the same way b2ce4d9 extended it for WebSocketErrorCode.h/WebSocket.cpp.
2 participants
Removes 477 lines of verified-dead code across the shell interpreter, bake dev server, SQL wire protocol, package manager, CLI, CSS, websocket client, subprocess glue, and build config (net -436 after the guard lint).
Every deletion was verified with word-boundary searches across
src/,scripts/,packages/, and regeneratedbuild/debug/codegen/output (including C++ callers,$zig/$cppstring references, andEnumStringby-name reverse parses), then validated bycargo checkon all 10 CI target triples and a full debug build. The workspace deniesdead_code, so what survives the lint is mostly never-constructedpubenum variants and the unreachable chains behind their match arms; that is what this PR removes.shell interpreter
WriterTag::{Pipeline,Subshell,If}were never constructed (everyChildPtr::newpassesBuiltin/Cmd/CondExpr; subprocess capture usesSubproc), so their dispatch arms and the twoon_io_writer_chunkhandlers reachable only through them were unreachable.SubshellState::{Expanding,WaitWriteErr},IfState::{WaitingWriteErr,Done}(theDonearm waspanic!("This code should not be reachable")),CondExprState::Done,CatState::Done(plus the localBranch::Doneit fed).Yield::Start(spawners callinterp.start_node()directly),ParseFlagResult::ShowUsage(noFlagParserimpl returns it),EntryKindHint::File(rm constructs onlyIdk/Dir).bake dev server
route_bundle::State::EvaluationFailurewas never constructed. Deleted its arm, bothEnsureRouteCtx::on_failureimpls (which would haveunwraped the always-NoneFramework.evaluate_failurefield), the field, andErrorPageKind(every live caller passedBundler, so the page title is now a constant).MessageId::{BrowserMessage,BrowserMessageClear,RequestHandlerError}: wire bytes the server never sends and no client handles.generated.tsregenerated via the bake codegen.sql
CommandType: kept the three commands bun sends (COM_QUERY,COM_STMT_PREPARE,COM_STMT_EXECUTE); the other 28 had zero references. The enum is only used asas u8in packet writers, no decode path.StatusFlag: keptSERVER_MORE_RESULTS_EXISTS(the only flagStatusFlags::hasis asked about). Decode is unaffected;StatusFlagsstores the rawu16.InvalidState, postgresInvalidByteSequenceForEncodingandInvalidTimeFormat. These names are also not producible via theEnumStringreverse parse inFrom<crate::Error>(no other sql error enum carries them). PostgresOverflowstays: mysql's constructedOverflowcould reach it through that by-name parse.install / cli
Subcommand::Scanplus its two arms (28 lines of help text):bun pm scandispatches underSubcommand::Pm.Step::{Copyfile,Linking,Patching}:InstallResult::failis only called with the other four variants.timerfield feeding it was never set toSome, so the whole chain (structs,append_concurrent, thePackageManagerfield, the timer field,manager_mut) was a write-only no-op left from the port.PackageInstall.file_countcarried a literal "TODO: this is never read" comment; error handling at the four assignment sites is preserved.PackError::MissingPackageJSON(never constructed; the live same-named variants are on different enums) andExampleTag::Unknown(zero references; the compiler confirmed by flagging the_ => unreachable!()arm as unreachable once it was gone).css / websocket / subprocess / timer / watcher / C++ / js
crate_error::Error::CSSPrintError(never constructed; the liveCSSPrintErrorspellings are the separatePrintErrenum) and two uncalled inherentparse_with_optionshelpers onGenericSelectorList/GenericSelector.ErrorCode::{HeadersTooLarge,CompressionFailed,ExpectedControlFrame,ProtocolError,ProxyConnectionRefused}: never constructed. Discriminants are explicit, so the remaining values still line up with the C++WebSocketErrorCodetable.WTFTimer__runIfImminent: anextern "C"export with zero references in C++, vendor/WebKit, or codegen (the imminent-GC path runs via__bun_run_wtf_timer).Readable::{on_close,on_ready}/Writable::on_readystubs with no callers (teardown goes throughSubprocess::on_close_ioand theUpstreamSourceimpl).EventType::Abortwas never sent (C++EventNames.cppkeeps its by-value string table entry).KeyObject::getKeyObjectFromHandle: declaration + definition pair with zero callers.internalBinding('util')shim constants and a write-only minimatch debug property.bun_glob/bun_routerand bundler's unusedbun_routerdependency; a dead root re-export inbun_s3_signing.Verification
cargo checkviabun run rust:check-all: 10 ok, 0 failed.bun bddebug build.bun bd teston the touched areas:bunshell.test.ts(418 pass), shell command tests,bun-pack.test.ts(76 pass),bun-install-lifecycle-scripts.test.ts(115 pass), websocket permessage-deflate,css.test.ts(1209 pass),bake/dev/esm.test.ts,repl.test.ts(148 pass),glob/match.test.ts,fs.watch.test.ts, spawn exit-code/stream tests. The only local failures are container artifacts (tests running as root, tests hitting external websocket endpoints, and PATH shims that expect a binary literally namedbun), identical under the released bun.test/internal/source-lints/dead-symbols-shell-bake-sql-install.test.tspinning every removal so the symbols do not come back.Follow-up note (not in this diff):
bun_install::npmdecodes a manifestfile_countthat may also be write-only, and the shellPromptBehaviour::Once { removed_count }field is written but unread pending-isupport; both left alone as plausible in-flight scaffolding.[review] gate passed · iteration 3 · 47 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 3
evidence per changed file