Repository navigation
Remove dead code from js_parser, bun_install, bun_sourcemap, the shell lexer, and ten WebCore files - #44363
Remove dead code from js_parser, bun_install, bun_sourcemap, the shell lexer, and ten WebCore files#44363robobun wants to merge 7 commits into
Conversation
|
Updated 9:21 PM PT - Oct 1st, 2026
✅ @robobun, your commit 4cd6788d8a4b50c770aaa95afacf5481c14ba73d passed in 🧪 To try this PR locally: bunx bun-pr 44363That installs a local version of the PR into your bun-44363 --bun |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe pull request changes binary-name iteration, CommonJS parser handling, sourcemap name parsing and lookup, shell lexer state, and the semver string API. It also removes commented-out code from WebCore bindings. ChangesInstall Bin Iteration
JavaScript Parser State
Sourcemap Name Handling
Shell Lexer State
Semver String API
WebCore Comment Cleanup
Possibly related PRs
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The selected WebCore cleanup does not change runtime behavior. No actionable merge-blocking risk remains; merge after normal checks pass. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — every deletion checks out as dead, and no behaviour changes.
What was reviewed:
- fold.rs: the removed
module.exports = {...}block requiredAssignTarget::Replaceand only ever reacheddeoptimize_common_js_named_exports(); return None; the survivingassign_target() != Nonecheck at src/js_parser/fold.rs:324-330 covers the same inputs, and the block's pre-exit evaluation was side-effect-free. - Grepped
src/for every removed symbol (stmt_expr_value,commonjs_replacement_stmts,restore_stmt_expr,next_in_dir,destination_node_modules,include_names,names_buffer,List::get_name,delimit_quote,bun_semver::String::init_append_if_needed) — no remaining references; the remaininginit_append_if_neededhits are allStringOrTinyString. - Confirmed the only
NamesIteratorconstructor (tree_printer.rs:769-779) is inside theMap | File | NamedFilearm, andJSSourceMapfillsself.namesfrom the payload and callsmapping::parsedirectly, somappings.nameswas never populated andget_namealways returnedNone. - WebCore C++ diff: every removed line is a
//comment or blank; no code lines touched.
Extended reasoning...
The change is a 24-file, +11/-424 dead-code removal across js_parser (CommonJS module.exports fold block and two write-only P fields), bun_install (unreachable Tag::Dir iterator path and its fields), bun_sourcemap (never-enabled include_names/names-array path plus the bun_semver dependency and Cargo.lock edge), the shell lexer's write-only delimit_quote flag, and comment-only cleanup in ten WebCore bindings. It touches no security-sensitive surface: no input validation, auth, crypto, or allocation ownership is altered, and the only measurable effect is memory_cost dropping the always-empty names buffer contribution. Approval rests on independently verifying each deadness proof by grep and by reading the surviving control flow, the absence of any lingering constructor or match site for the removed enum field, and the C++ diff consisting solely of comment and blank lines. No CODEOWNERS entry covers the changed files, no third-party review or objection is on the timeline, and the hunt ran dry.
|
Status: ready for review.
|
…ormance and WebSocket
An open pull request changes the line directly below it. The removal of the comment made the two pull requests conflict.
Behaviour change: none
Problem
module.exports = {...}check insrc/js_parser/fold.rs(110 lines). Each of its paths ends inp.deoptimize_common_js_named_exports(); return None;, and the check after it does the same for every input that enters it.Fix
falsein each store, and comment lines.bun bd,cargo clippy,cargo checkfor 12 targets, and 1,314 tests of the touched areas.Background
dead_code. But rustc counts a store to a field as a use.exports.foo = 1into an ESM export for that file.Downsides
ParsedSourceMap::memory_cost()reports 40 bytes less for each map:mapping::Listlost two fields that were always empty.module.exportsandbun addoutput for fivebinshapes match the base commit.Notes
Removed, one group each
module.exports = {...}check (143 lines infold.rs,p.rs,visit/mod.rs,visit/visit_stmt.rs). The check sits in the property-access fold. Its condition needsassign_target == AssignTarget::Replace. Its three exits (if deopt, inside theforover the properties, and the block after the loop) are allp.deoptimize_common_js_named_exports(); return None;. The check that follows it runs the same two statements whenassign_target != AssignTarget::None, which the first condition implies. What the block evaluates before it exits has no effect. With the block gone,P::stmt_expr_valuehas writers and no reader, so the field, its two stores and therestore_stmt_expr!macro go.P::commonjs_replacement_stmtshas no writer, so the field and the one branch that reads it go. History: the Zig original (src/js_parser/fold.zigbefore d451445) was afor ... elsewhoseelseran on each completion, so the rewrite after it never ran. The port dropped that rewrite and kept the test.bin::NamesIterator::next_in_dir(54 lines insrc/install/bin.rs,tree_printer.rs). The one constructor of the iterator is in the match arm for theMap,FileandNamedFiletags, so theTag::Dirarm ofnextnever runs. With it go the fieldsdone,dir_iteratoranddestination_node_modules. The Zig printer built the iterator in the same arm only.bun addprints no binary list for adirectories.binpackage, before and after.ParseUrlResultHint::All.include_names(125 lines insrc/sourcemap,SavedSourceMap.rs,JSSourceMap.rs,src/semver/lib.rs). The one constructor writesfalse. So the block that loaded the JSONnamesarray intomapping::Listnever ran,List.namesandList.names_bufferwere always empty, andList::get_namealways returnedNone.new SourceMap(payload)reads names fromJSSourceMap.names, which stays. That block held the last caller ofbun_semver::String::init_append_if_needed, so the function goes too, andbun_sourcemapno longer depends onbun_semver.Lexer::delimit_quote(8 lines insrc/shell_parser/parse.rs). Each store isfalseor a copy between the lexer, its sublexer and the backtrack snapshot. Nothing reads it.// LOG(Network, ...),// InspectorInstrumentation::...,// queueTaskKeepingObjectAlive(...), and// #includelines for headers that Bun does not have. Each removed line is a comment (87) or a blank line (21).git blamedates them from 2022-03-27 to 2025-12-26. An earlier sweep verified this set and held it (Remove dead code: 17 log scopes nothing logs to and four fields nothing reads, in 10 crates #44050).EventTarget.cpp. Remove dead code from the web streams bindings, the test runner formatter, and the bindgen codegen #41445 removes twelve inPerformance.cpp. Remove dead code from the class and sink generators, JSBuffer, the bake client, bun-error, and the build scripts #43378 removes one inWebSocket.cpp. WebSocket: bound every script-supplied value that an error message quotes #42216 changes the line directly below one inWebSocket.cpp.static_assert(!std::is_base_of<ActiveDOMObject, ...>)), and 8 files with changes that are not comments.Open PRs
git merge-tree): 383 PRs touch one of the 24 files or have more than 100 files. 253 of them conflict with main already. 130 merge cleanly with main. Two of those conflict with this branch:s_exprinvisit_stmt.rsand keeps tworestore_stmt_expr!()calls. To resolve: drop those calls.NamesIterator::next_in_dirchecked. To resolve: drop that hunk, the function is gone.bun_semver::String::init_append_if_needed, and js_parser: fold property access on multi-property object literals #36599 usesFlagsinfold.rs, whose import this PR drops with its last user.// LOGline ofWebSocket.cppthat this PR removes. The merge is clean and keeps their copy.Verification
The base is main at 9d9fdbe.
bun bdpasses (debug build with ASAN).cargo clippy --workspaceandcargo fmt --allare clean.cargo check --workspace --all-targetspasses, andbun run rust:check-allpasses for 12 of 12 targets.transpiler.test.js,runtime-transpiler.test.ts,esModule-annotation.test.js,bundler_cjs.test.ts,bundler_cjs2esm.test.ts, six sourcemap test files undertest/js/node/moduleandtest/js/bun/sourcemap,lex.test.ts,parse.test.ts,bunshell.test.ts,brace.test.tsandbun-pm.test.ts. 1,058 pass, 0 fail.EventTarget.cppandPerformance.cpp. At the final commitbun bdpasses, and 256 tests pass in 11 files:event-target.test.ts,web-globals.test.js,URLSearchParams.test.ts,performance.test.js,performance-entries.test.ts,perf_hooks.test.ts,text-encoder.test.js,structured-clone-fastpath.test.ts,websocket-blob.test.ts,websocket-subprotocol-strict.test.tsandwebsocket-close-code.test.ts.bun buildin four modes and a run throughrequireandimportfor 25 shapes ofmodule.exports(object literal, methods, spread, computed key, afterexports.x, nested in a function,+=,delete, in aforinit, and more), andbun addof five tarballs (binstring, named, map,directories.bin, none).Self-review
Five concerns, all addressed.
P::stmt_expr_valuein place as a written and unread field, to stay clear of js_parser: simplify an unused expression statement only when minifying, bundling or tree-shaking #36720. REVIEW.md asks for the deletion in the PR that makes the field dead, so it is deleted here and the conflict is named.bun_semver::String::init_append_if_needed.fold.rsblock, which runs and changes nothing: reworded.Verified and left out, because an open PR holds the same lines
Each of these was proved, applied, built and tested, then removed from the branch.
Bun.spawnstdio (313 lines): the memfd read-back for stdout and stderr (Readable::Memfd,ArrayBuffer::to_js_buffer_from_memfd, dead since spawnSync: drain piped stdio to EOF after the direct child exits #33832 left memfd to stdin), the error path ofStdio::as_spawn_option, and the shell'sWritable::MemfdandReadable::Memfd. Remove libuv on Windows #42819 rewrites the same functions.EMFILEandENFILEarm of thespawn_processmatch inspawn_maybe_sync(32 lines).spawn_processcannot return that value on any target. Without the arm, rustfmt indents the 50-line match again, in a file that 47 open PRs touch.package_dependentsandusage_countwith their scans. install: make yarn.lock migration linear in lock size #37244 deletes the same blocks.tag_print_help(74 lines): ten arms for tags that cannot reach it. cli: listbun pm diffinbun pm --help, keep one copy of the pm help text, restore the indent of the\-continued messages #39487 rewrites the same arms.bun_api::npm_registry::Parserand the log ofpnpm_matcher_from_expr(78 lines). install: .npmrc credentials the npm way — verbatim _auth, no URL-embedded credential in the request path, key-walk lookup for registries and tarballs #40423 edits the same call sites, and Remove cross-crate layering workarounds #33152 and Delete the schema::api mirror types and bun_api; one loader numbering across Rust/C++/JS #37095 remove the wrapper.JsSinkType::get_pending_errorwith its six guards (36 lines): no sink overrides it. JSSink write(): convert the chunk before resolving the native sink #41710 and s3: do not complete the upload when writer().end() gets an error #43134 edit the lines next to the guards.Parser::jsobjsandJSValueRaw(30 lines). shell: break words on a tab in the lexer #43861, shell: fail the parse when for/while/until/select/case/esac/function/do/done is used as a command #43921 and shell: reject!, brace groups, here-documents and stray then/elif/else/fi at parse time #43922 change the same call, and shell: remove unsafe from the interpreter, state nodes, IOWriter/IOReader and Builtin #40228 removes the cast._this_valueofJSValkeyClient::sendwith 35frame.this()arguments, and the parameter ofValkeyClient::shutdown. redis: forward PFADD elements and the NX|XX|GT|LT condition of the EXPIRE family #37571 and share one body per shape across the valkey command methods #39502 addsend_cmdcall sites, and socket, timer, sql, redis: replaceunsafewith typed ownership #40139 edits theshutdowncall.Found and not dead, or not a plain removal
P::keep_expr_symbol_namereturns its argument, so its ten call sites do nothing. Four of them evaluate an argument that panics on real input:new Bun.Transpiler({exports:{eliminate:["foo"]}}).transformSync("export let foo;")panics withcalled Option::unwrap() on a None value, and a destructuring default after a very deep member chain panics withindex out of bounds. The removal changes that behaviour, so it needs its own PR with tests.ScopeIteratorinload_npmrcalways counts 0, butensure_unused_capacity(0)can still grow the hash index of a map that has more than 8 entries.Leads from the same read, not proved a second time
JSValkeyClient::sendcannot returnErr:send_err_to_jsand 31 "Failed to send" strings.Exited.signalis 0 in each constructor: three "exited with a signal" branches inbun runandbunx.ParserOptions.suppress_warnings_about_weird_codeistruein each constructor.mapping::parse:estimated_mapping_countisNoneat all four call sites.ImportRecordFlags::WAS_ORIGINALLY_REQUIREis never set.VirtualMachine.arenahas four writers and no reader.GlobWalker::initandStatWatcher::initcannot fail.ResultValue::Emptyhas no constructor.[policy-decision:dep] gate passed · iteration 0 · 24 files touched
passes on PR (with fix)
Dependency/toolchain change; no test proof to run.diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file