Skip to content

Remove dead code from bun_lint, bun_react_compiler, and the node:fs Stats bindings - #46832

Open
robobun wants to merge 2 commits into
mainfrom
robobun/936a15e4/dead-code-sweep
Open

robobun wants to merge 2 commits into
mainfrom
robobun/936a15e4/dead-code-sweep

Conversation

@robobun

@robobun robobun commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • Delete the 18 impls and the getter. get_constructor moves to impl StatsSmall, its one instantiation. 13 files, 138 lines removed, 6 added.
  • Correct because an impl that a bound or a caller needs does not compile when it is gone, and all 12 targets compile.
  • Verified: bun run rust:check-all, cargo check --workspace --all-targets, clippy, bun bd, and the lint, react-compiler and node:fs Stats tests.

Behaviour change: none

Background

  • rustc treats a trait impl as reachable, because generic code can call it. The candidates come from a debug relink with --gc-sections --print-gc-sections.
  • The lint handles are keyed through IdSet and IdMap since bun lint: the rules that need types, compared with tsgolint function by function #45133, which removed the last FxHashMap keyed by TsSymbol or Signature.
  • Considered the 67 other impls that pass the same test: not taken in this run.

Downsides

  • A later PR that wants one of these impls must add it again. All 18 came with Add bun lint and bun format #44805 (2026-10-11), so the lint port can still reach some.
  • A deleted impl does not conflict in text with a new use. Before a merge, update the branch and wait for the cargo clippy job.
Notes

Removed

  • bun_lint, 15 impls:
    • Hash for CodePath and Segment (code_path/mod.rs), EsNode (selector/mod.rs), Reference (semantic/mod.rs), Signature (types/signature.rs) and TsSymbol (types/symbol.rs)
    • From<Pat> for Target, From<Expr> for Target and From<Target> for Node (utils/estree_compat.rs)
    • From<&Token> for TokenOrText (utils/ast_utils.rs). The by-value From<Token> stays.
    • From<f64> for Value and Emit for f64 (estree/value.rs)
    • From<DeclarationKind> for DeclarationKinds (semantic/mod.rs)
    • DoubleEndedIterator for TypeIter (types/ty.rs)
    • PartialEq<str> for Name (ast/name.rs). The &str, [u8] and &[u8] impls stay.
  • bun_react_compiler, 3 impls: Extend and FromIterator for IdMap, Clone for IdBitSet (hir/id_map.rs)
  • Bun__JSBigIntStatsObjectConstructor (NodeFSStatBinding.cpp), its declaration in src/runtime/node/Stat.rs, and the BIG arm of get_constructor. get_constructor is now on impl StatsSmall, so a call on the BigInt type does not compile.

What earlier sweeps said about these items

History of the 18 impls

Found dead, not taken

Checked against the open pull requests

  • Compared at 02:15 UTC on 2026-10-25, on main 179664e, with the heads of 5,754 open pull requests.
  • 13 of them touch one of the 13 files. None that merges with main gets a conflict from this branch. None deletes a line that this PR deletes. Rename Zig-prefixed identifiers to Bun/Rust equivalents #30747 edits the signature line of the getter. It conflicts with main already.
  • The self-review merged an earlier cut of this branch (these deletions plus five more) into each of 44 open pull requests that touch these crates and merge with main, and ran cargo check --workspace --all-targets on each merge. All pass. bun check: TypeScript 7.1 #46383 and Compile TypeScript to machine code ahead of time: bun build --compile --aot #45631 moved after that. Their new commits add no line in src/lint or src/react_compiler that names one of these types.
  • A search of the added Rust lines of all open pull requests found no call that names one of the removed conversions. That search cannot see .into(), .collect() or ==, so the merge instruction in Downsides still applies.

Method

  • Relink of the debug build with -Wl,--gc-sections -Wl,--print-gc-sections. The export list and the version script stay, so the NAPI, V8 and uv surface is still a root.
  • An impl is a candidate when no function of it has a symbol in the relinked binary. 254 impls outside the open dead-code pull requests qualified (no Debug, Display or Drop).
  • Each candidate was deleted and the workspace compiled. 85 compile away on Linux (all targets, with tests), Windows and macOS. The others are needed by a bound, a test, or another platform.

Verification on this commit

  • cargo check --workspace --all-targets --keep-going: pass
  • bun run rust:check-all: 12 ok, 0 failed, 0 skipped
  • cargo clippy --no-deps for bun_lint, bun_react_compiler and bun_runtime: pass
  • bun bd: pass
  • Tests with the debug build: fs-stats-constructor 7 pass, fs-stats-truncate 3 pass, lint native-rules 193 pass, lint react-compiler 26 pass, transpiler react-compiler 121 pass
  • On an earlier cut of this branch (these deletions plus five more): test/cli/lint/lint.test.ts 712 pass, test/cli/format/format.test.ts 1627 pass, quic-endpoint and quic-stream 22 pass.
  • The lint and format test files need --timeout above the default 5 s with a debug build on this machine. With it they pass.
  • cargo check --release cannot check bun_jsc against a debug codegen directory (bindgen_test is debug-only), so the release profile was checked only for the crates below bun_jsc. bun_lint and bun_react_compiler pass.

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

…tats bindings

18 trait impls that nothing uses:

- bun_lint: `PartialEq<str> for Name`, `Hash` for `CodePath`, `Segment`,
  `EsNode`, `Reference`, `Signature` and `TsSymbol`, `Emit for f64`,
  `From<f64> for Value`, `From<DeclarationKind> for DeclarationKinds`,
  `DoubleEndedIterator for TypeIter`, `From<&Token> for TokenOrText`,
  `From<Pat> for Target`, `From<Expr> for Target`, `From<Target> for Node`
- bun_react_compiler: `Extend` and `FromIterator` for `IdMap`,
  `Clone for IdBitSet`

`Bun__JSBigIntStatsObjectConstructor`: only `StatType::<true>::get_constructor`
reached it, and nothing calls that instantiation. `get_constructor` moves
to `impl StatsSmall`, so a call on the BigInt type no longer compiles.
@robobun

robobun commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review.

  • How the candidates were found: a relink of the debug build with --gc-sections --print-gc-sections lists the functions that nothing references. Each candidate impl was then deleted, and the workspace was compiled on all 12 targets.
  • Question for the owners of bun_lint: do the unused Hash, From, Emit and iterator impls on the lint handles go? A merge means yes. If the answer is no, I will reduce this PR to the BigIntStats getter.

@robobun

robobun commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 9:23 PM PT - Oct 24th, 2026

✅ @robobun, your commit f29c5a16f4f2278a20436afacbc6ae15d595f644 passed in Build #132372! 🎉


🧪   To try this PR locally:

bunx bun-pr 46832

That installs a local version of the PR into your bun-46832 executable, so you can run:

bun-46832 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — pure deletion of unused trait impls and one dead FFI pair; every removal checks out against the remaining callers.

What was reviewed:

  • Stats FFI: Bun__JSBigIntStatsObjectConstructor has no remaining declaration, definition or symbol-list entry anywhere in the repo; the only get_constructor caller is node_fs_binding.rs:165 on StatsSmall, and the BigIntStats structure is still reached via callJSStatsFunction<true> / Bun__createJSBigIntStatsObject, so no user-visible path changes.
  • Removed Hash impls: no HashMap/HashSet/derive(Hash) container keyed by CodePath, Segment, EsNode, Reference, Signature or TsSymbol (the semantic Symbol keeps its own Hash; Reference appears only as a map value).
  • TypeIter: no .rev()/next_back()/nth_back() on it (the one .rev() over union types is on the AST List, not the lint-types iterator); IdBitSet has no derive(Clone) owner and the save/restore site uses mem::take, not clone(); IdBitSet keeps its own Extend/FromIterator.
  • can_tokens_be_adjacent call sites all pass Token by value, so dropping From<&Token> has no caller; only one #[cfg]-gated file exists in src/lint and none of the react-compiler fixtures/cfg(test) blocks name a removed impl.
Extended reasoning...

The commit deletes 15 unused trait impls in bun_lint, three in bun_react_compiler, and the dead BigIntStats constructor getter plus its C++/Rust extern pair, with no logic added beyond relocating get_constructor onto StatsSmall. It touches no security-sensitive surface; the only runtime-visible path is the node:fs Stats constructor getter, whose single caller and symbol are unchanged. A missing impl or extern is a compile or link error rather than a silent behavior change, and grep-level verification across the repo (including cfg-gated code and CODEOWNERS, which does not cover these files) found no remaining user of anything removed. The bug hunt ran dry with no findings, so a human look is not needed.

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 179664e and bcd20ba.

Files selected for processing: 13
  • src/jsc/bindings/NodeFSStatBinding.cpp
  • src/lint/ast/name.rs
  • src/lint/code_path/mod.rs
  • src/lint/estree/value.rs
  • src/lint/selector/mod.rs
  • src/lint/semantic/mod.rs
  • src/lint/types/signature.rs
  • src/lint/types/symbol.rs
  • src/lint/types/ty.rs
  • src/lint/utils/ast_utils.rs
  • src/lint/utils/estree_compat.rs
  • src/react_compiler/hir/id_map.rs
  • src/runtime/node/Stat.rs
Files with no reviewable changes: 12
  • src/jsc/bindings/NodeFSStatBinding.cpp
  • src/lint/types/symbol.rs
  • src/lint/selector/mod.rs
  • src/lint/utils/ast_utils.rs
  • src/lint/ast/name.rs
  • src/lint/utils/estree_compat.rs
  • src/lint/types/ty.rs
  • src/lint/types/signature.rs
  • src/lint/code_path/mod.rs
  • src/lint/semantic/mod.rs
  • src/lint/estree/value.rs
  • src/react_compiler/hir/id_map.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Walkthrough

The pull request removes selected trait implementations from lint and React compiler code. It also removes the BigInt Stats constructor binding and adds a constructor method to StatsSmall.

Changes

Node Stats Constructor

Layer / File(s) Summary
Stats constructor selection
src/runtime/node/Stat.rs, src/jsc/bindings/NodeFSStatBinding.cpp
The generic constructor selection and BigInt Stats binding are removed. StatsSmall returns the Stats constructor.

Lint Trait and Conversion Implementations

Layer / File(s) Summary
AST and ESTree conversions
src/lint/ast/name.rs, src/lint/estree/value.rs, src/lint/utils/ast_utils.rs, src/lint/utils/estree_compat.rs, src/lint/semantic/mod.rs
Selected name comparison, numeric emission, and AST and ESTree conversion implementations are removed.
Hashing and iterator traits
src/lint/code_path/mod.rs, src/lint/selector/mod.rs, src/lint/semantic/mod.rs, src/lint/types/signature.rs, src/lint/types/symbol.rs, src/lint/types/ty.rs
Hash implementations are removed from code paths, nodes, references, signatures, and symbols. TypeIter no longer implements DoubleEndedIterator.

React Compiler Collection Traits

Layer / File(s) Summary
IdMap and IdBitSet traits
src/react_compiler/hir/id_map.rs
IdMap loses its Extend and FromIterator implementations. IdBitSet loses its Clone implementation.

Suggested reviewers: jarred-sumner

Priority: Low

Merge Risk: Minimal · up to f29c5

Merge Risk: ⚪ Minimal · up to f29c5

The cleanup preserves supported runtime behavior and workspace consumers.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly summarizes the removal of dead code from bun_lint, bun_react_compiler, and the node:fs Stats bindings.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description follows the repository template, with both required sections present: "What does this PR do?" (Problem, Fix, Background, Downsides, and a detailed Notes block) and "How did you verify …
Full details: Description check

Explanation

The description follows the repository template, with both required sections present: "What does this PR do?" (Problem, Fix, Background, Downsides, and a detailed Notes block) and "How did you verify your code works?" (a Verification section listing commands, test counts, and results). It states the change has no behaviour change and lists the removed impls, the deleted BigInt Stats constructor, and the moved getter. The description is thorough and matches the raw summary and PR objectives. It is long, but it covers the required information.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants