Remove dead code from analytics, perf, clap, picohttp, exe_format, create_command - #35369
Conversation
|
Warning Review limit reached
Next review available in: 24 seconds 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 ignored due to path filters (1)
📒 Files selected for processing (21)
Comment |
|
Updated 11:06 PM PT - Jul 23rd, 2026
⏳ @robobun, your commit 8084a63 is still building in
|
|
This is a pure dead-code removal: every deleted symbol had zero callers (verified with rg across src/, src/codegen/, and build/debug/codegen/), so there is no behavior change to write a fail-before regression test against. The verification is The six previous merged dead-code sweeps (#35225, #35293, #35152, #34878, #34837, #35052) also landed without test/ changes for the same reason. |
There was a problem hiding this comment.
LGTM — the three earlier nits are addressed in 978b845, and the remaining inline note about dev_dependencies/dependencies in create_command.rs is a non-blocking cleanup (those locals were already write-only before this PR).
Extended reasoning...
Overview
Pure dead-code sweep: net -1844 lines across 22 files in bun_analytics, bun_perf, bun_clap, bun_picohttp, bun_exe_format, bun_libarchive, bun_api, plus three load_tracy() no-op call sites and ~290 lines of commented-out Zig in create_command.rs. The only additions are doc-comment rewrites where the deleted items were referenced (ArgIter, Help.msg, phr_chunked_decoder, NativeEndianInt) and a visibility narrowing on c::phr_header / c::phr_parse_response to pub(super).
Verification
Rust dead-code removal is compiler-verified: the PR reports bun bd and bun run rust:check-all pass on all 10 targets, so every removed symbol is provably unreferenced from compiled code (including cfg-gated platform branches). I additionally spot-checked the higher-risk removals with repo-wide greps at HEAD:
phr_parse_response/phr_parse_request/phr_parse_headers/phr_decode_chunked_is_in_datare-exports — only the internalc::phr_parse_responsecall inResponse::parse_partsremains; no external importers.load_tracy/tracy_trace/BUN_TRACY_PATH/tracy::— zero hits.has_flag— onlyFilePollFlag::has_flaginsrc/io/remains, unrelated to clap.ShellIterator/ShellIteratorError— zero hits.PEFile::validate/get_bun_section_length/ removedErrorvariants — zero hits (theBunSectionNotFoundmatches are inelf.rs, a different enum).EventName/help_full/help_ex/get_value_simple/write_takes_value_suffix— zero hits.Request::parse/Headers::parse— remaining hits areUpdateRequest::parse/PostgresRequest::parse, unrelated.
The removed picohttp FFI declarations (phr_parse_request, phr_parse_headers, phr_decode_chunked_is_in_data) are Rust-side extern declarations of C functions, not exports, so removing them cannot affect the C side.
Security risks
None. No auth, crypto, permissions, or input-validation code is touched; every hunk is either a deletion of an uncalled item, a Cargo dep drop, or a comment update.
Level of scrutiny
Medium — the change is large by line count but mechanically uniform (deletions + comment fixups), and correctness is fully checked by the multi-target compile. This matches six prior merged dead-code sweeps (#35225, #35293, #35152, #34878, #34837, #35052) that also landed without test/ changes.
Other factors
The three nits from my earlier pass (stale _state doc comment, orphaned dos_header_offset field, stale Help.msg intra-doc links) were all addressed in 978b845 and the threads resolved. The one new inline nit — the now-unjustified dev_dependencies/dependencies write-only locals in create_command.rs — is a completeness suggestion (same class as the dos_header_offset fix already applied) with no runtime effect, so it doesn't block.
There was a problem hiding this comment.
LGTM — all four prior nits addressed in 978b845 and 310be1a.
What was reviewed: Spot-checked the higher-risk removals against the current tree: has_flag hits are all FilePollFlag::has_flag (unrelated); phr_parse_response re-export has no external callers (internal c:: use kept); tracy_trace!/tracy_trace_named! macros unused despite #[macro_export]; EventName/SchemaInt/BufReader/ShellIterator/Seek/get_bun_section_length/validate/Names::matches have zero references. rust:check-all on all 10 targets confirms no cfg-gated caller was missed.
Extended reasoning...
Overview
Periodic dead-code sweep across 22 files, net -1844 lines. Removes unreferenced items from bun_analytics (peechy decoder scaffolding, EventName, error.rs), bun_perf (the entire 490-line tracy.rs module + load_tracy() no-op stub + its three call sites), bun_clap (ShellIterator, the help/help_ex/help_full family, Names::matches, has_flag), bun_picohttp (Request::parse/clone, Headers::parse, unused FFI re-exports), bun_exe_format (validate/get_bun_section_length/get_dos_header/get_pe_header + associated error variants + dos_header_offset field), bun_libarchive (Seek enum), create_command.rs (~290 lines of commented-out Zig + write-only dev_dependencies/dependencies locals), plus Cargo dep pruning in three crates and a doc-comment update on NativeEndianInt.
Security risks
None. Pure deletions of unreferenced symbols and comments; no new logic, no changed control flow, no user-input handling touched.
Level of scrutiny
Medium — the diff is large and spans HTTP-parser FFI, PE manipulation, and CLI parsing, but every hunk is a removal of a symbol with zero call sites. The Rust compiler is the primary safety net here: any live reference would fail bun bd or rust:check-all, both of which pass on all 10 targets per the PR body. I independently grepped the tree for each removed public item (tracy_trace, load_tracy, phr_parse_request/phr_parse_headers/phr_decode_chunked_is_in_data, struct_phr_* re-exports, has_flag, ShellIterator, ParseRequestError/ParseHeadersError, BUN_TRACY_PATH, Seek, get_bun_section_length, validate, Names::matches, SchemaInt/BufReader/EventName) and confirmed zero non-self references. The has_flag case in particular checked out — the four remaining hits are FilePollFlag::has_flag in src/io/, unrelated to clap.
Other factors
I reviewed this PR twice previously and left four nits (stale _state doc comment on phr_chunked_decoder, orphaned dos_header_offset field, stale Help.msg intra-doc links, write-only dev_dependencies/dependencies locals). All four were addressed in 978b845 and 310be1a, and the fixes match what was suggested. The follow-up commit also collapsed a now-redundant nested len_u32() > 0 check in create_command.rs — a correct simplification since the Prune.prune() call that could shrink the property list between the two checks was removed. No new issues found this run. This matches the pattern of six previously-merged dead-code sweeps that also landed without test changes.
…d create_command - analytics: EventName enum, Reader/BufReader/SchemaInt decoder infra, error module (nothing in the crate is fallible), 9 unused Cargo deps - perf: tracy.rs module (zero runtime callers; load_tracy() was an empty stub), 8 unused Cargo deps, BUN_TRACY_PATH env var - clap: ShellIterator/ShellIteratorError (test-only), help/help_ex/ help_full/print_param/write_takes_value_suffix chain (superseded by simple_help), Names::matches, Args::has_flag/ComptimeClap::has_flag, get_value_simple - picohttp: Request::parse/clone, Headers::parse, ParseRequestError, ParseHeadersError, unused c:: re-exports and FFI decls - exe_format/pe: PEFile::validate, PEFile::get_bun_section_length, get_dos_header/get_pe_header helpers, 4 orphaned Error variants - libarchive: Seek enum - api: 10 unused Cargo deps - runtime/cli/create_command: ~290 lines of commented-out Zig package.json injection logic (disabled since Sep 2023) - dotenv/run_command/repl_command/bundle_v2: removed no-op load_tracy() call sites Verified with rg across src/, src/codegen/, build/debug/codegen/; bun bd passes; bun run rust:check-all passes on all 10 targets.
… dos_header_offset field
…in create_command
310be1a to
bd9d714
Compare
There was a problem hiding this comment.
LGTM — pure dead-code deletion, all prior nits addressed.
What was reviewed: Spot-checked the riskiest removals for remaining callers — load_tracy/tracy_trace! (none), picohttp FFI re-exports (phr_parse_response now crate-private, only internal use remains), ComptimeClap::has_flag (only FilePollFlag::has_flag hits remain, unrelated), removed PE Error variants (elf.rs BunSectionNotFound is a separate enum). bytes_eq import in comptime.rs still used by find_param_index. The four earlier nits (_state doc comment, dos_header_offset field, Help.msg intra-doc links, dev_dependencies/dependencies write-only locals) are all fixed in the current diff.
Extended reasoning...
Overview
Periodic dead-code sweep removing ~1,844 lines across 22 files: unused enums/traits/functions in bun_analytics (peechy decoder scaffolding, EventName, error.rs), the entire bun_perf::tracy module + its stub load_tracy() call sites, unused bun_clap help/flag machinery (help/help_ex/help_full/ShellIterator/has_flag/Names::matches), unused bun_picohttp request/header parsers + FFI decls, unused bun_exe_format PE validation methods + error variants, bun_libarchive::Seek, ~290 lines of commented-out Zig in create_command.rs, and unused Cargo dependencies from three crates.
Security risks
None. Pure deletions with no new code paths. The removed PEFile::validate was itself never called, so its removal doesn't weaken any active check. No auth/crypto/permission code touched.
Level of scrutiny
Moderate — the diff is large but the compiler is the primary safety net for dead-code removal: bun bd + rust:check-all on all 10 targets passing means no cfg-gated caller was missed. The residual risk is FFI/codegen/dynamic-dispatch callers the compiler can't see; I grep-verified the removed picohttp C re-exports, load_tracy, has_flag, and PE error variants have no remaining references in src/. The PR description documents rg verification across src/, src/codegen/, and build/debug/codegen/ per symbol.
Other factors
This is my third pass on this PR. The first two rounds raised four nits (stale _state doc comment in picohttp, orphaned dos_header_offset field, broken [\help`]/[`help_ex`]intra-doc links onHelp.msg, and write-only dev_dependencies/dependencieslocals increate_command.rs) — all addressed in follow-up commits and confirmed present in the current diff. This is the 7th such sweep following an established merged pattern (#35225, #35293, #35152, #34878, #34837, #35052), none of which required test/` changes. No behavior change to regression-test.
There was a problem hiding this comment.
No issues found — the four earlier nits are all addressed and I re-verified the removed symbols (tracy/load_tracy, picohttp phr_parse_request/phr_parse_headers/phr_decode_chunked_is_in_data, clap ShellIterator/has_flag) have zero remaining references in src/. Deferring only because -1844 lines across 22 files — including dropping the tracy profiling subsystem and PE validate() — is large enough that a maintainer should confirm none of it was intentionally kept for future wiring.
What was reviewed:
- Grepped every removed public symbol against
src/— no hits outside deleted definitions;bytes_eqimport incomptime.rsstill has live users (find_param_index). - Confirmed
c::phr_parse_responseremainspub(super)and reachable fromResponse::parse_parts; only the unused re-exports were dropped. - Checked the collapsed
create_command.rsdependency-detection blocks still sethas_dependencieson the same condition (the removed inner re-check was redundant post-Prune-removal).
Extended reasoning...
Overview
Periodic dead-code sweep removing ~1844 lines across 22 files: the entire tracy.rs profiling module (490 lines, unreachable — load_tracy() was an empty {} stub), unused peechy decoder scaffolding in bun_analytics, unused clap help/shell-iterator machinery, unused picohttp request/headers parse paths, PE validate()/get_bun_section_length(), ~290 lines of commented-out Zig in create_command.rs, and associated Cargo dep pruning. Follow-up commits addressed four prior nits (restored _state doc mention, dropped dos_header_offset field, fixed stale Help.msg intra-doc links, removed write-only dev_dependencies/dependencies locals).
Security risks
None. Pure deletion of unreferenced code; no new logic, no input handling changes. The removed PE validate() was never called by the compile pipeline (which validates inline in init()).
Level of scrutiny
Moderate. Mechanically straightforward — every removal is a symbol with zero callers, verified by rust:check-all across 10 targets and my own grep pass. But the diff is large and removes whole subsystems (tracy, PE validation helpers) that a maintainer may have been keeping around deliberately for future re-wiring even if currently disconnected. That's a judgment call outside what I can verify from the code alone.
Other factors
- Six prior dead-code sweeps (#35225, #35293, #35152, #34878, #34837, #35052) landed with the same shape and no test changes, so precedent supports this.
- All four inline nits from earlier runs were addressed in c777455 / bd9d714 and the threads are resolved.
- I confirmed the
has_flaghits insrc/io/areFilePollFlag::has_flag, unrelated to the removed clap method (matching the PR description's claim). - The
create_command.rssimplification correctly preserves thehas_dependenciesside effect while dropping the write-onlyOption<Expr>locals and the now-redundant innerlen_u32() > 0re-check.
|
CI status after one re-roll: Build 79292 (bd9d714): 4 failures, all unrelated
Build 79335 (8084a63, re-roll): 2 failures, both unrelated
This diff is Rust-only deletions of zero-caller symbols; none of the failures touch the changed crates. |
…eate_command (#35369) Periodic dead-code sweep. Net -1844 lines across 22 files. ### bun_analytics - `EventName` enum: zero references anywhere (only `strum` user in the crate) - `Reader` trait / `BufReader` / `SchemaInt` re-export / `eof()`: peechy decoder scaffolding with zero callers; nothing decodes analytics schemas at runtime - `error.rs`: the crate has no fallible surface once the decoder infra is gone - Cargo deps: dropped `strum`, `bstr`, `scopeguard`, `const_format`, `enum-map`, `enumset`, `bun_sys`, `thiserror`, `bun_errno` ### bun_perf - `tracy.rs` (490 lines): no runtime caller exists. `load_tracy()` in `env_loader.rs` was an empty `{}` stub and its three call sites were no-ops - `BUN_TRACY_PATH` env var: only read by `tracy.rs` - Cargo deps: dropped `strum`, `bstr`, `scopeguard`, `const_format`, `enum-map`, `enumset`, `bitflags`, `bun_paths` ### bun_clap - `ShellIterator` / `ShellIteratorError` / `State` enum (+ tests): zero non-test references - `help` / `help_ex` / `help_full` / `print_param` / `write_takes_value_suffix` / `get_value_simple`: zero callers. All `--help` output goes through `simple_help` / `simple_help_bun_top_level` - `Names::matches`: zero callers (doc comment referenced `has_flag`/`find_param`, neither of which call it) - `Args::has_flag` / `ComptimeClap::has_flag`: zero callers (the `has_flag` hits in `src/io/` are `FilePollFlag::has_flag`, unrelated) ### bun_picohttp - `Request::parse` / `Request::clone`: zero callers. All `Request` construction in `src/http/` is via struct-init - `Headers::parse`: zero callers (`Headers` struct + `Display` impl are kept; `WebSocketUpgradeClient` uses them) - `ParseRequestError` / `ParseHeadersError`: only referenced by the removed `parse` methods - `c::phr_parse_request` / `c::phr_parse_headers` / `c::phr_decode_chunked_is_in_data` FFI decls and their re-exports - `struct_phr_header` / `struct_phr_chunked_decoder` type aliases, `phr_header` / `phr_parse_response` re-exports ### bun_exe_format - `PEFile::validate`: zero callers - `PEFile::get_bun_section_length`: zero callers (runtime uses `Bun__getStandaloneModuleGraphPELength` from C++ instead) - `PEFile::get_dos_header` / `get_pe_header`: only used by `validate` - `Error::{InvalidSectionData, BunSectionNotFound, InvalidBunSection, SizeOfImageMismatch}`: only constructed by the removed methods ### bun_libarchive - `Seek` enum: defined, never referenced ### bun_api - Cargo deps: dropped `strum`, `bstr`, `scopeguard`, `const_format`, `enum-map`, `enumset`, `libc`, `bitflags`, `bun_collections`, `bun_install_types` ### src/runtime/cli/create_command.rs - ~290 lines of commented-out Zig code for the old `bun create` package.json injection path (bun-framework-next / bun-macro-relay / react-refresh / Prune / InjectionPrefill). Disabled in commit ffd21e9 (Sep 2023) and carried over verbatim during the Rust rewrite ### Verification - `rg` across `src/`, `src/codegen/`, `build/debug/codegen/` for each removed symbol: zero hits outside own definition - `bun bd`: passes - `bun run rust:check-all`: passes on all 10 targets (linux/macos/windows × x64/aarch64, + musl/freebsd) - `cargo check -p <crate> --tests`: passes for all modified crates - Smoke tested `bun --help`, `bun run --help`, `bun install --help` ### Followup candidates (not in this diff) - `bun_core::fmt::CountingWriter`: only remaining user was `help_full`; kept because it's a general utility - `bun_dotenv::instance()` wrapper fn: zero callers (the static is read directly) - `bun_csrf::Error::{InvalidToken, ExpiredToken, DecodingFailed}`: never constructed
Periodic dead-code sweep. Net -1844 lines across 22 files.
bun_analytics
EventNameenum: zero references anywhere (onlystrumuser in the crate)Readertrait /BufReader/SchemaIntre-export /eof(): peechy decoder scaffolding with zero callers; nothing decodes analytics schemas at runtimeerror.rs: the crate has no fallible surface once the decoder infra is gonestrum,bstr,scopeguard,const_format,enum-map,enumset,bun_sys,thiserror,bun_errnobun_perf
tracy.rs(490 lines): no runtime caller exists.load_tracy()inenv_loader.rswas an empty{}stub and its three call sites were no-opsBUN_TRACY_PATHenv var: only read bytracy.rsstrum,bstr,scopeguard,const_format,enum-map,enumset,bitflags,bun_pathsbun_clap
ShellIterator/ShellIteratorError/Stateenum (+ tests): zero non-test referenceshelp/help_ex/help_full/print_param/write_takes_value_suffix/get_value_simple: zero callers. All--helpoutput goes throughsimple_help/simple_help_bun_top_levelNames::matches: zero callers (doc comment referencedhas_flag/find_param, neither of which call it)Args::has_flag/ComptimeClap::has_flag: zero callers (thehas_flaghits insrc/io/areFilePollFlag::has_flag, unrelated)bun_picohttp
Request::parse/Request::clone: zero callers. AllRequestconstruction insrc/http/is via struct-initHeaders::parse: zero callers (Headersstruct +Displayimpl are kept;WebSocketUpgradeClientuses them)ParseRequestError/ParseHeadersError: only referenced by the removedparsemethodsc::phr_parse_request/c::phr_parse_headers/c::phr_decode_chunked_is_in_dataFFI decls and their re-exportsstruct_phr_header/struct_phr_chunked_decodertype aliases,phr_header/phr_parse_responsere-exportsbun_exe_format
PEFile::validate: zero callersPEFile::get_bun_section_length: zero callers (runtime usesBun__getStandaloneModuleGraphPELengthfrom C++ instead)PEFile::get_dos_header/get_pe_header: only used byvalidateError::{InvalidSectionData, BunSectionNotFound, InvalidBunSection, SizeOfImageMismatch}: only constructed by the removed methodsbun_libarchive
Seekenum: defined, never referencedbun_api
strum,bstr,scopeguard,const_format,enum-map,enumset,libc,bitflags,bun_collections,bun_install_typessrc/runtime/cli/create_command.rs
bun createpackage.json injection path (bun-framework-next / bun-macro-relay / react-refresh / Prune / InjectionPrefill). Disabled in commit ffd21e9 (Sep 2023) and carried over verbatim during the Rust rewriteVerification
rgacrosssrc/,src/codegen/,build/debug/codegen/for each removed symbol: zero hits outside own definitionbun bd: passesbun run rust:check-all: passes on all 10 targets (linux/macos/windows × x64/aarch64, + musl/freebsd)cargo check -p <crate> --tests: passes for all modified cratesbun --help,bun run --help,bun install --helpFollowup candidates (not in this diff)
bun_core::fmt::CountingWriter: only remaining user washelp_full; kept because it's a general utilitybun_dotenv::instance()wrapper fn: zero callers (the static is read directly)bun_csrf::Error::{InvalidToken, ExpiredToken, DecodingFailed}: never constructed