Remove dead code: write-only fields and never-set options in 11 crates - #43745
Conversation
|
Warning Review limit reached
This review includes 18 billable files and costs up to $4.50.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 15 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (18)
Comment |
|
Status: ready for review. The diff is green. CI is red on one test that this change does not touch.
|
|
Updated 8:30 PM PT - Sep 21st, 2026
❌ @robobun, your commit 59e88d8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 43745That installs a local version of the PR into your bun-43745 --bun |
There was a problem hiding this comment.
LGTM — mechanical removal of write-only fields; I found no remaining reader for any of them.
What was reviewed:
- Grepped
src/for every removed field name (append_null_byte,has_terminated,root_progress_node,import_record_tag,stderr_data,is_server,virtual_hosted_styleon the outer struct, etc.) — no stray references remain, including incfg(windows)code inipc.rs. ResolveFunctionResult.resultdrop inVirtualMachine.rs:bun_resolver::Resultholds only borrowed&'static [u8]paths,Copyfds and a rawPackageJSONpointer, noDropimpl, soret.pathdoes not depend on it staying alive.- None of the touched structs are
#[repr(C)]or referenced viasize_of/offset_ofassertions, so no FFI layout expectation changes. csrf_jsc.rsstill uses the localencodingto encode the token aftergenerate, so behavior is unchanged there.
Extended reasoning...
The change deletes 16 write-only struct fields and one never-true printer option across 18 Rust files in 11 crates (+10/-105), touching Output, the bundler metafile, csrf options, the package manager, the parser/printer, VM module resolution, libarchive, data URLs, Windows IPC state, and S3 credentials. It touches no security-sensitive logic: the csrf change removes an unread copy of the encoding while the actual encoding path is unchanged, and the S3 field removed was on the outer wrapper that nothing named. I could not run cargo in this environment, so compile verification rests on CI, but grep confirms no reader survives for any removed field and the one lifetime-relevant deletion (the resolver Result no longer stored) is safe because the Result owns nothing. The diff is mechanical, follows an existing let _ = progress.start pattern, and adds no behavior.
### Problem - `hasExportStar` in `src/runtime/bake/hmr-module.ts` has no caller. Its only call site is a block that #18109 commented out on 2025-03-14. The `availableExportKeys` local above that block is read only by the commented-out code. - `firstConnection` in `src/runtime/bake/client/websocket.ts` is assigned once and never read. - No lint reports them. Earlier sweeps ran `tsc --noUnusedLocals` over `src/js` and `scripts` only, not over `src/runtime/bake`. ### Fix - Delete `hasExportStar`, the commented-out check, `availableExportKeys`, and `firstConnection`. 2 files, 40 lines removed. - Correct because the bundler already drops `hasExportStar`. The generated `bake.client.js`, `bake.server.js` and `bake.error.js` differ from `main` only by the two removed local declarations. - Verified: `tsc -p src/runtime/bake/tsconfig.json --noUnusedLocals` no longer reports either file. `test/bake/dev/esm.test.ts` (17 pass), `hot.test.ts` (11 pass) and `bundle.test.ts` (23 pass) with the debug build. Behaviour change: none ### Background - `hmr-module.ts` is the module loader that the dev server sends to the browser and to the SSR realm. `parseEsmDependencies` walks the dependency list of an ES module. Each entry carries the export names that the importer uses. - The removed check compared those names with the exports of the dependency and threw a `SyntaxError` for a missing one. It has been off for 18 months. A missing export fails at the use site. - A deletion has one possible place, so no other design was weighed. ### Downsides - The commented-out check was the only sketch of export verification in the HMR runtime. A person who wants to build it starts from the history of #18109. <details><summary>Notes</summary> #### Why this run is small Every other hit of this run is live, is platform code with a user on another target, or is a line that one of the 34 open dead-code pull requests already deletes. Each removed line here was checked against those diffs. #40492 and #43378 touch `websocket.ts` in other hunks. #### Scans of this run, all clean or already claimed - Debug objects linked again with `--gc-sections --print-gc-sections`, then `llvm-symbolizer` for `file:line`. 20,668 discarded functions in bun's objects. The Rust ones outside macros and trait impls are Windows or macOS helpers, or claimed (#40824, #40557, #40232). The C++ ones are claimed, are template instantiations, or have a Rust caller on another target (`bsd_socket_export`, `posix_spawnattr_reset_signals`). - Rust functions that never get a symbol (generic or `#[inline]`, never instantiated), found by comparing every `fn` line with the DWARF declaration lines of all emitted functions. 86 hits outside `cfg`, trait impls and `#[inline(always)]`. All are Windows-only, test-only (the outbound half of `api/bun/h2/connection.rs`), or claimed. - `clang -fsyntax-only -Wunused-function -Wunused-macros -Wunused-template -Wunused-member-function` over 588 translation units and the 69 unified ones (the build passes `-Wno-unused-function`). 3 functions and 6 macros. `formatStackTraceToJSValueWithoutPrepareStackTrace`, `hostName`, `G_TRUE`, `G_FALSE`, `MAX_LABELS` and `us_ioctl` are claimed (#40367, #43378, #40492, #40294). `us_quic_send_one` is used under the non-Linux `#if` branch. - A whole-program C++ reference index (`c-index-test -index-file`, 657 translation units, 32,018 symbols declared in bun's tree). 8,711 have no recorded reference. After filters for template-dependent uses, `extern "C"`, virtual methods and names that WebKit headers use, 78 remain. All are index artifacts (typedef struct tags, primary templates with used specializations, `requires` clauses) or one-line getters in files that open pull requests rewrite. - `tsc --noUnusedLocals` over `src/js`, `scripts`, `src/codegen`, `src/runtime/bake`, `src/node-fallbacks` and the sources of each package. The hits outside this change are claimed (#40122, #41169, #41385, #40492, #43778) or are loop variables. - Regex scans for struct fields with no read and for `bool` or `Option` fields that only ever get one constant. Nothing new after #43745. - Exports of `src/js` modules and builtin functions with no mention in another file: none. - Files that nothing includes, imports or names: none outside `.idl` copies (#41064). `src/symbols.txt`, `symbols.def` and the `package.json` scripts name nothing that is gone. No `#if 0`. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check <!-- robobun:evidence:end -->
Problem
BufferWriter.append_null_byteis nevertrue,TransposeState.import_record_tagis neverSome.dead_codepass countsx.f = vas a use off. hawk counts the initializer as a reference.Fix
#[deprecated]on 12,134 struct fields,cargo check --force-warn deprecatedfor Linux, Windows, and macOS, then a syn pass marks each use as read or write. 134 fields have no read. Most stay (Notes).bun bd,bun run rust:check-all(12 of 12 targets),cargo check --workspace --all-targets, and the tests in the Notes.written_without_trailing_zero()and an older stale comment.Background
MultiArrayList<T>keeps each field ofTin its own column. Code reads a column by name as a string (items::<"name", _>()), so the compiler sees no read. Those fields stay.BufferWriteris the JS printer's output buffer.done()could append a NUL byte. No caller asks for it.Downsides
bun_core::output::SourcebecomesSend + Sync, because its raw pointer fields are gone. It lives in athread_local!only.bun_resolver::Resultin_resolvenow drops when_resolvereturns. It has noDropeffect: paths are borrowed, fds areCopy.Notes
Removed, one line each
bun_core::output::Source:buffered_stream,buffered_error_stream,stream,error_stream(*mut io::Writer).init()cached them, nothing read them. The accessors of the same names return*_backing.new_interface(), which is a pointer cast with no side effect.bun_bundler:InputFileInfo.import_count(MetafileBuilder.rs).bun_install:SecurityScanSubprocess.stderr_data(an emptyVecthat is never filled, the scanner's stderr is inherited),PackageManager.root_progress_node. Theprogress.start(b"", 0)call stays, because it starts the progress root.bun_js_parser:PropertyOpts.async_range,ScanPassResult.approximate_newline_count,TransposeState.import_record_tag. Supportimport with { type: "json" }and others #16624 replaced the only setter of the tag withimport_loader, which stays.bun_jsc:VirtualMachine.has_terminated(its readers were debug panics inenqueue_task_concurrent, which Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 removed),ResolveFunctionResult.result(seven stores, no read). Nothing borrows from the stored value:pathandquery_stringpoint into the resolver's arena and the specifier.bun_js_printer:BufferWriter.append_null_byte, with the seven= falsestores inbun_jscandbun_runtime.bun_libarchive:BufferReadStream.reading.bun_runtime:WindowsState.is_server(ipc.rs, Windows only). Its last reader went away in more child-process #18688, before the Rust port.bun_csrf:GenerateOptions.encoding.csrf_jsc.rsencodes the token itself, andVerifyOptions.encodingstays.bun_resolver:DataURL.url(alwaysString::EMPTY).bun_s3_signing:S3CredentialsWithOptions.virtual_hosted_style. No code names it. Every reader usescredentials.virtual_hosted_styleon the innerS3Credentials.Passed the probe, kept on purpose
MultiArrayListcolumns: fields ofJSMeta,File,InputFile,BundledAst,Entry,Node,WatchItem,LineOffsetTable,ServerComponentBoundary, and others.ParseResult.source_contents_backing,LinkerContext.unique_key_buf,OutputFile.owned_src_path_text,HTTPResponseMetadata.owned_buf,PackageJSON.source_contentsandjson_tape,MatchedRoute.pathname_backing,SignResult.content_md5,KEventWaker.machport_buf.Repl.last_error(a GC protect),SecurityScanSubprocess.process,Watcher.thread.md::Options.hard_soft_breaksandunderline(Bun.markdown: implement the hardSoftBreaks and collapseWhitespace options #39495, Bun.markdown: implement the latexMath and underline options #39493),jsc::virtual_machine::Options.dns_result_order(Forward --inspect and --dns-result-order to the VM in compiled executables #40703),P.has_top_level_return(js_parser: make the ESM/CJS classification and the module/exports bindings agree #40840),ReactRefresh.last_hook_seenandforce_reset,DebugOptions.dump_environment_variables(the--dump-environment-variablesflag is parsed and ignored).AllocatorConfiguration.long_running,DumpStackTraceOptions.skip_*,NameOfSymbol.has_property_key_comment.PostgresSQLQueryFlags.is_done(set by the JSdone()call),WorkerPipe.done(leaves two empty reader callbacks),WTFTimer.repeat(leaves an unused FFI parameter),MaxHeapAllocator.len(leaves an emptyreset()),ReadToEndResult.err(callers drop read errors, which looks like a bug),Subcommand::PackwithPACK_PARAMS(bun pm packruns asSubcommand::Pm, so the pack help text is unreachable).Self-review
Three independent read-only passes tried to prove each deletion wrong (readers through raw pointers,
offset_of!, C++ layout mirrors, macros, othercfgs, unit tests, the Zig originals, drop effects). None found a reader. Concerns raised: theSend + Syncchange ofSource(now in Downsides), the style of the keptprogress.startcall (nowlet _ =, as ininstall_with_manager.rs),BufferWriter::written_without_trailing_zero()(nine callers, it only strips NUL bytes that the printer never appends now, a follow-up), and a comment aboveResolveFunctionResult.paththat names a function which no longer exists (it predates this change).Other scans of this run, all clean or already in an open pull request
--gc-sections --print-gc-sections, and a zero-mention identifier index oversrc/,packages/,scripts/andbuild/debug/codegen/.macro_rules!without an invocation, Cargo features that nothing enables,#if 0and never-true#ifblocks in the C++ bindings, patches that no dependency script applies, unused exports underscripts/,tsc --noUnusedLocalsoversrc/jsandscripts/.Overlap with the open dead-code pull requests
output.rs,VirtualMachine.rs,parser.rs,p.rs,js_printer/lib.rs,jsc_hooks.rs,PackageManager.rs,data_url.rs).Tests run with the debug build
test/js/bun/util/csrf.test.ts(31 pass),test/bundler/metafile.test.ts(65 pass),test/js/bun/archive.test.ts(108 pass),test/bundler/transpiler/transpiler.test.js(222 pass),test/js/bun/resolve/import-empty.test.js,test/js/bun/resolve/esModule.test.ts,test/cli/install/bun-install-security-provider.test.ts(43 pass),test/internal/source-lints/(195 pass). Also adata:URL import, andbun installunder a pty so that the progress root starts.No test is added. The change deletes fields that nothing reads, so there is no behavior to assert, and
test/internal/source-lints/CLAUDE.mdasks for no tests that pin dead symbols.