Remove dead code from bun_options_types and bun_http_types - #35152
Conversation
Deletes pub items with zero references anywhere in the workspace: bun_options_types: - jsx::Pragma::parse_package_name: never called - bundle_enums::LoaderOptionalExt trait + impl: never imported or called - bundle_enums::ImportKindExt trait + impl: never imported or called - schema::api::ImportKind enum: only referenced by the ImportKindExt impl above - standalone_path::BASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIX (cfg(windows)): duplicate of the const in bun_standalone_graph, which all callers use bun_http_types: - MimeTypeList::COUNT: never read - Method::Optional::contains: never called Also drops the sealed::Sealed impls for LoaderOptional/ImportKind (only the removed traits bounded on them) and tidies the doc comments in bun_ast that pointed readers at the removed extension traits. Verified with rg across src/, build/debug/codegen/, and src/codegen/. bun bd and bun run rust:check-all pass.
|
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 (9)
Disabled knowledge base sources:
WalkthroughChangesThe pull request removes obsolete public APIs across AST, options types, HTTP method handling, MIME type lists, JSX helpers, and standalone path constants, including related schema declarations, extension traits, re-exports, and documentation references. Public API cleanup
Suggested reviewers: Comment |
|
Updated 2:06 PM PT - Jul 22nd, 2026
@Jarred-Sumner, your commit f79e101 is building: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — pure dead-code removal, all deletions verified unreferenced.
What was reviewed:
- Grepped
src/for every removed symbol (parse_package_name,LoaderOptionalExt,ImportKindExt,api::ImportKind,MimeTypeList::COUNT,Optional::contains) — zero hits. - Confirmed
BASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIXcallers all resolve to thebun_standalone_graphcopy, not the removedstandalone_pathone. - Checked that surviving imports (
bun_core::stringsin jsx.rs,const_formatdep) are still used elsewhere in the crate.
Extended reasoning...
Overview
Deletes seven unreferenced items across bun_options_types and bun_http_types: Pragma::parse_package_name, the LoaderOptionalExt/ImportKindExt extension traits and their sealed::Sealed impls, schema::api::ImportKind, the Windows-only duplicate standalone_path::BASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIX, MimeTypeList::COUNT, and Method::Optional::contains. Also drops the now-dead LoaderOptional import and re-exports, and trims three doc comments in bun_ast that pointed at the removed traits. Net +7/-114, deletion-only.
Security risks
None. No runtime logic, FFI, parsing, or user-input handling is touched — every change is a removal of code with zero call sites.
Level of scrutiny
Low. This is mechanical dead-code cleanup in leaf type crates. The only real risk is accidentally deleting something with a hidden caller (FFI export, codegen consumer, cfg-gated call site). I independently verified each deletion:
rg -wacrosssrc/returns zero hits forparse_package_name,LoaderOptionalExt,ImportKindExt,api::ImportKind, andMimeTypeList::COUNT.- All three
BASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIXreferences (server_body.rs,standalone_graph_jsc.rs,StandaloneModuleGraph.rs) go throughStandaloneModuleGraph::, confirming theoptions_types::standalone_pathcopy was an unused duplicate. - The one
.contains()inServerConfig.rs:400operates on the destructuredEnumSet<Method>, not onOptional— matches the PR's claim. - None of the removed items are
#[no_mangle]/extern "C", so no C++ linkage can depend on them.
Other factors
- The
use bun_core::stringsimport injsx.rsremains live viastrings::count_charinmember_list_to_components_if_different; theconst_formatworkspace dep remains live viacompile_target.rs. No newly-orphaned imports. - The trimmed doc comments in
bun_astaccurately drop only the references to removed items; the survivingLoaderExtpointer inloader.rsis still correct. - Author reports
rust:check-allpasses on all 10 targets, which covers thecfg(windows)deletion; the Rust compiler would catch any missed reference regardless. - No behavioral change is possible, so no new tests are expected per REVIEW.md's dead-code guidance.
There was a problem hiding this comment.
LGTM — pure dead-code removal, all seven deletions verified to have zero remaining references. The inline nit about LoaderOptional becoming orphaned is worth folding in, but it's non-blocking.
Extended reasoning...
Overview
Deletes seven unreferenced pub items across bun_options_types and bun_http_types (traits, an enum, a const, two functions), drops the now-unused Sealed impls and import, trims three doc comments in bun_ast that pointed at the removed extension traits, and adds test/internal/source-lints/removed-dead-symbols.test.ts to guard against reintroduction. Net +7/-114.
Security risks
None. Pure deletions of uncalled Rust items; no runtime code paths change. No auth/crypto/FFI/unsafe touched.
Level of scrutiny
Low. Each removal was independently spot-checked with rg -w across src/:
parse_package_name,LoaderOptionalExt,ImportKindExt— zero hits.BASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIX— the surviving canonical const inbun_standalone_graph::StandaloneModuleGraphis what all three call sites (server_body.rs,standalone_graph_jsc.rs,StandaloneModuleGraph.rs) resolve to; the deletedcfg(windows)copy inoptions_typeswas never imported.MimeTypeList::COUNT,Method::Optional::contains— zero hits.schema::api::ImportKind— only consumer wasImportKindExt::to_api, removed in the same diff.
The PR description reports bun bd and rust:check-all (all 10 targets, covering the cfg(windows) deletion) pass, which is the load-bearing verification for a deletion-only change.
Other factors
- The new source-lint test follows the established pattern in
test/internal/source-lints/(8 sibling files) — reads a source file and asserts a regex does not match. No new test infrastructure. - The one inline finding (
bun_ast::LoaderOptionalleft orphaned after its last consumer was deleted) is a valid completeness nit per REVIEW.md's "delete dead code in the same PR that makes it dead", but it's ~10 lines of harmless unreferenced code, not a functional issue. Fine to fold in or handle as a follow-up. - No prior human review comments to address; CI build is in progress.
LoaderOptionalExt::from_api was the only consumer of LoaderOptional::NONE and LoaderOptional::from_loader. With that trait gone, the struct has zero references beyond its own definition and the crate-root re-export.
There was a problem hiding this comment.
LGTM — the follow-up commit removed LoaderOptional as flagged, and all eight deleted symbols now verify as zero-reference.
What was reviewed
- Re-grepped each removed symbol (
parse_package_name,LoaderOptional{,Ext},ImportKindExt,api::ImportKind,BASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIX,MimeTypeList::COUNT,Optional::contains) acrosssrc/— no remaining references. - Confirmed
ServerConfig.rs:400set.contains(Method::GET)resolves toEnumSet::contains, not the removedOptional::contains. - Confirmed all three
BASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIXcall sites resolve to thebun_standalone_graphcopy, not the removedcfg(windows)duplicate. - New test follows the existing
test/internal/source-lints/pattern.
Extended reasoning...
Overview
Pure dead-code deletion across two small type-only Rust crates (bun_options_types, bun_http_types) plus bun_ast, removing eight pub items with zero workspace references: jsx::Pragma::parse_package_name, LoaderOptionalExt + bun_ast::LoaderOptional, ImportKindExt + schema::api::ImportKind, a cfg(windows)-only duplicate constant, MimeTypeList::COUNT, and Method::Optional::contains. Also trims three doc comments that pointed at the removed items and drops the corresponding Sealed impls / re-exports. Adds a source-lint test guarding against reintroduction. Net +87 / -114.
Security risks
None. This is deletion-only against unreferenced Rust items in type-definition crates. No FFI symbols (verified none are #[no_mangle]/extern "C"), no runtime behavior touched, no user input handling.
Level of scrutiny
Low. The change is mechanical and self-verifying: rustc compiles the workspace across all 10 targets (rust:check-all), so any remaining reference to a deleted item would fail the build. I independently re-grepped every removed identifier across src/ and confirmed zero hits. The one non-obvious case — set.contains(Method::GET) in ServerConfig.rs — is a pattern-matched EnumSet<Method>, so it dispatches to EnumSet::contains, not the removed Optional::contains(&self, &Optional). The BASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIX deletion is Windows-only and the three live call sites all path through StandaloneModuleGraph::, which resolves to the canonical copy in bun_standalone_graph.
Other factors
My prior review flagged that removing LoaderOptionalExt orphaned bun_ast::LoaderOptional; the author addressed this in 3f4e095 (struct + re-export removed, entry added to the lint test), and the thread is resolved. The new removed-dead-symbols.test.ts sits alongside seven existing files in test/internal/source-lints/ and follows the same read-source-and-regex pattern. The gate evidence shows the lint test failing on main and passing on the branch. No outstanding reviewer comments.
|
CI status: the diff itself is green. The remaining red in builds 77945 and 77964 is unrelated to this diff:
Ready for review/merge. |
| //! Data-only enum + pure predicates. `to_api()` / `from_api()` / `API_NAMES` | ||
| //! live in `bun_options_types::LoaderExt` (would back-edge into the schema | ||
| //! crate). `to_mime_type` / `from_mime_type` live in `bun_http_types` (would | ||
| //! back-edge into `bun_http::MimeType`). |
There was a problem hiding this comment.
🟡 The PR description's robobun evidence block is stale: it uses test/internal/source-lints/removed-dead-symbols.test.ts as the entire fails-on-main/passes-on-PR gate proof and lists it as +85 in the diff hotspot ("10 files changed"), but commit f79e101 deletes that test with no stated reason — the final diff is 9 files with no test/ entry. The resolved LoaderOptional thread also says an entry was "added to removed-dead-symbols.test.ts", which is no longer true. Please either restore the test if the deletion was accidental, or update the description to drop the stale evidence block/diffstat and note why the source-lint was dropped (per REVIEW.md "Every deletion needs a stated reason or replacement" and CLAUDE.md #11).
Extended reasoning...
What the issue is
Commit f79e101 — the tip of this PR — deletes test/internal/source-lints/removed-dead-symbols.test.ts with a bare commit message ("Delete test/internal/source-lints/removed-dead-symbols.test.ts") and no stated reason. That file was added earlier in this same PR (da9816a) and extended in 3f4e095. However, the PR description was not regenerated after the deletion, so the auto-generated robobun evidence block still presents the deleted test as the only proof that the change is correct.
Where the description is now inconsistent with what ships
Four places in the PR body/timeline reference the deleted file:
- The fails-on-main / passes-on-PR gate — both collapsibles run
removed-dead-symbols.test.tsand show its 8 assertions ("8 pass / 0 fail") as the entire "[review] gate passed" proof. Nothing in that block still exists in the final diff. - The diff hotspot table — lists
.../source-lints/removed-dead-symbols.test.ts | 85 +++++++...and totals "10 files changed, 93 insertions(+), 129 deletions(-)". The actual final diff (git diff 47597ab2..HEAD --shortstat) is 9 files with notest/entry; the manually-written summary at the top of the description ("Net: +7 / -114") is closer to reality than the evidence block. - The evidence-per-changed-file table — lists the test file with "0 reads / 2 edits".
- The resolved LoaderOptional review thread — the robobun reply says the fix "added an entry to
removed-dead-symbols.test.ts", which is now false.
Step-by-step proof
git log --onelineon the PR head shows, in order: 7b6eb3f (dead-code removal) → da9816a (adds the test) → 3f4e095 (removesLoaderOptional, extends the test) → f79e101 (deletes the test).ls test/internal/source-lints/on the PR head listsdead-code-escapes.test.tsand siblings but notremoved-dead-symbols.test.ts.- The PR's changed-files metadata lists exactly 9 files, all under
src/;git diff 47597ab2..HEAD --shortstatconfirms 9 files changed. - The PR description's
<!-- robobun:evidence:begin -->block was generated at 3f4e095 (its build hashes are3f4e09509) and was never regenerated after f79e101. - Therefore a reviewer reading the description will conclude an 8-assertion regression guard ships with this PR when in fact no test ships at all.
Why this matters
REVIEW.md states "Every deletion needs a stated reason or replacement", and CLAUDE.md #11 says "NEVER overstate what you got done or what actually works in commits, PRs". The description currently overstates what ships: it presents test evidence for a test that isn't in the final diff. This is not a code defect — the 9 dead-code deletions themselves are correct, bun bd builds, and rust:check-all passes on all 10 targets — but the PR body materially misrepresents the artifact a reviewer is approving.
Impact
Nothing breaks at runtime if merged as-is. The risk is purely process/review: a maintainer skimming the evidence block would approve believing a regression guard exists against these symbols reappearing, when the PR actually relies on the pre-existing test/internal/source-lints/dead-code-escapes.test.ts (or nothing) instead.
How to fix
Either:
- (a) Restore
removed-dead-symbols.test.tsif f79e101 was accidental (e.g. a rebase mishap), or - (b) If the deletion was intentional (e.g. maintainer preferred relying on the existing
dead-code-escapes.test.tsin the same directory, or decided a per-symbol source-lint isn't worth the maintenance): regenerate/edit the PR description to drop the stale robobun evidence block and diff hotspot, and add one line stating why the source-lint test was dropped. Optionally amend the resolved LoaderOptional thread reply to strike the "added an entry to removed-dead-symbols.test.ts" claim.
Anchoring this comment on src/ast/loader.rs since the deleted test file is not present in the final diff to comment on directly; the LoaderOptional entry in that test was the last thing added to it before deletion.
Brings in #35002 (remove ~39k lines of dead Rust) and its follow-ups (#35019, #35052, #35152, #35225, #35293, #35326). The binary-size check compares against current main; this branch was 137 commits behind, so it still carried the dead code main dropped and registered as +630KB..+1.7MB on x64 while aarch64 linux showed -513KB/-601KB (different dead-code elimination outcomes per target). This stack's own native contribution is 19 files / +392 -45 lines; src/js is net -977 lines (domain.ts +692 vs fast-utf8-stream.ts -856 etc). Also: drop the hoisted pbkdf2 .bind handlers back to closures (review nit), and take main's expectations.txt since #34741 audited the stale ASAN entries.
2 participants
Dead code sweep of two small Rust crates. Every item below has zero references anywhere in
src/,build/debug/codegen/, orsrc/codegen/(confirmed withrg -w). None are#[no_mangle]/extern "C"or named in.classes.ts.bun_options_typesjsx::Pragma::parse_package_name(jsx.rs): never called.bundle_enums::LoaderOptionalExttrait + impl: never imported by any crate;LoaderOptional::from_apihas no callers.bundle_enums::ImportKindExttrait + impl: never imported; the onlyto_api()calls onImportKind-shaped values targetlog::Kind::to_api, notbun_ast::ImportKind.schema::api::ImportKindenum: only referenced by theImportKindExtimpl above.standalone_path::BASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIX(cfg(windows)only): duplicate of the const inbun_standalone_graph::StandaloneModuleGraph, which all three call sites (server_body.rs,standalone_graph_jsc.rs,StandaloneModuleGraph.rs) resolve to.bun_http_typesMimeTypeList::COUNT: never read.ALL.len()is available if a count is ever needed.Method::Optional::contains: never called. Theset.contains(Method::GET)inServerConfig.rsisEnumSet::contains.Also drops the
sealed::Sealedimpls forLoaderOptional/ImportKind(only the removed traits bounded on them), the now-unusedLoaderOptionalimport inbundle_enums.rs, and trims three doc comments inbun_astthat pointed readers at the removed extension traits.Verification
bun bdbuilds cleanbun run rust:check-allpasses on all 10 targets (covers thecfg(windows)deletion)bundler_edgecase.test.ts,bun-serve-static.test.ts,transpiler.test.js -t jsxall passNet: +7 / -114.
[review] gate passed · iteration 1 · 10 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 1
evidence per changed file