Skip to content

Remove dead code from bun_format and napi.cpp - #45420

Open
robobun wants to merge 2 commits into
mainfrom
robobun/74e40cd1/dead-code-sweep
Open

robobun wants to merge 2 commits into
mainfrom
robobun/74e40cd1/dead-code-sweep

Conversation

@robobun

@robobun robobun commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • Two trait impls in bun_format have no user: Spanned for FormatExpr (src/format/js/format.rs) and Spanned for FormatSpecifier (src/format/js/print/import_declaration.rs). rustc's dead_code lint exempts trait impls, so the deny lints do not report them.
  • Bun__napi_get_version (src/jsc/bindings/napi.cpp) has no caller.

Fix

  • Delete the two impls and the function: 3 files, 18 lines.
  • Correct because nothing can reach them. No crate of the debug build instantiates either span method. A relink with --gc-sections drops Bun__napi_get_version, and rg finds only its definition. It is not in src/symbols.txt or src/symbols.def.
  • Verified: bun run rust:check-all (12 targets), cargo check --workspace with --all-targets and --release, bun bd, test/cli/format/format.test.ts, and the napi_get_version tests of test/napi/napi.test.ts.

Background

  • Spanned (bun_lint::span) gives a value its source range. FormatExpr and FormatSpecifier are private wrapper types of the formatter. The formatter asks the wrapped node for its range, not the wrapper.
  • To rustc a trait impl is always reachable, because generic code could call it. Only a whole-program view shows that nothing does.
  • Remove dead code from bun_format, bun_sema, bun_install, and bun_core #45402 removes six Spanned impls of the same kind. These two have no symbol in any object file, because no crate instantiates them, so a scan for dropped sections does not list them.
  • No other design applies: the change only deletes unreferenced code.

Downsides

  • None found. All 12 targets, the test targets and the release profile type-check without the impls. The lines that the 5,526 open PRs add use neither the impls nor Bun__napi_get_version.

Behaviour change: none

Notes

How the candidates were found

  • Linker: the debug link command plus -Wl,--gc-sections -Wl,--print-gc-sections. It drops 13,148 Rust function sections and 380 functions in handwritten C++ objects.
  • Compiler: 10,149 pub items got a temporary #[deprecated] mark (not committed). rustc then reports each use. Passes: the 12 CI targets, --all-targets on three of them, --release, and --cfg bun_asan --cfg bun_debug --cfg socket_fault_injection.
  • Trait impls: delete every candidate impl, run cargo check --workspace, put back each impl that an error names, repeat.
  • cargo mordant at the pinned rev with an empty baseline, on main and on the candidates. No file on main is over its unused_pub baseline.

Checks in detail

  • test/cli/format/format.test.ts with the debug ASAN build: 1015 pass, 0 fail with --timeout 120000. With the default 5 s timeout, 15 tests that start a JavaScript config loader time out on this machine.
  • test/napi/napi.test.ts -t napi_get_version: 2 pass.
  • cargo clippy -p bun_format --no-deps: clean.

What the scan found and where it went

Follow-up candidates, not in this diff

Delete two `Spanned` impls that nothing uses (`FormatExpr` and
`FormatSpecifier`) and `Bun__napi_get_version`, which has no caller.
@robobun

robobun commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:05 AM PT - Oct 17th, 2026

❌ @robobun, your commit 83386fc has 2 failures in Build #127070 (All Failures):

  • step failed outside runner - exit 1 on 🍎 any aarch64 - test-bun
  • ❌ CPU instruction violation on Windows x64 — 1 check(s) failed
  • The baseline build contains instructions not available on Nehalem (SSE4.2, no AVX/AVX2/AVX512).

    • Static instruction scan

    Static scan violations

    _powf_special  [AVX]  (1 insns)
    

    If these are runtime-dispatched behind a CPUID gate: add each symbol to scripts/verify-baseline-static/allowlist-x64-windows.txt with a comment pointing at the gate.

    If there's no gate: this is a real bug — a -march leaked into a subbuild.


🧪   To try this PR locally:

bunx bun-pr 45420

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

bun-45420 --bun

@robobun

robobun commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. The diff is green. CI is red on two lanes that this change does not touch.

CI on the head commit 83386fc (build 127070): 177 jobs pass, 2 fail.

  • windows x64 - verify-baseline: the static scan reports _powf_special [AVX] Vphsubw at 0x014519c364. The build of main at the same base commit d7562f2 fails with the same symbol and the same address (build 126987). Later builds of main pass that job.
  • darwin aarch64 - test-bun: the artifact download timed out after 120 s. No test ran.

How the dead code was confirmed:

  • Spanned for FormatExpr and Spanned for FormatSpecifier: no object file of the debug build has a symbol for either span method. cargo check --workspace passes without the impls on all 12 targets (bun run rust:check-all), with --all-targets, and with --release. bun_format has no cfg gate and both types are private to the crate.
  • Bun__napi_get_version: a relink of the debug build with -Wl,--gc-sections -Wl,--print-gc-sections drops it. rg Bun__napi_get_version finds only the definition.

PR: #45420

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 2 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: c4830f1e-4258-470e-aa16-e293883921f4
📥 Commits

Reviewing files that changed from the base of the PR and between 1cf3835 and 83386fc.

📒 Files selected for processing: 3
  • src/format/js/format.rs
  • src/format/js/print/import_declaration.rs
  • src/jsc/bindings/napi.cpp

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: e3f372e9-fa11-4e40-812c-ea389817c2c2
📥 Commits

Reviewing files that changed from the base of the PR and between 116dd64 and 1cf3835.

📒 Files selected for processing: 3
  • src/format/js/format.rs
  • src/format/js/print/import_declaration.rs
  • src/jsc/bindings/napi.cpp
💤 Files with no reviewable changes: 3
  • src/format/js/print/import_declaration.rs
  • src/jsc/bindings/napi.cpp
  • src/format/js/format.rs

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

Walkthrough

The change removes two Spanned implementations from JavaScript formatting types and removes the exported Bun__napi_get_version function from the N-API bindings.

Changes

Formatter span implementations

Layer / File(s) Summary
Remove formatter span delegation
src/format/js/format.rs, src/format/js/print/import_declaration.rs
FormatExpr and FormatSpecifier no longer implement Spanned by delegating to their wrapped values.

N-API version function

Layer / File(s) Summary
Remove N-API version function
src/jsc/bindings/napi.cpp
The exported Bun__napi_get_version function, which returned the N-API module version, was removed.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 1cf38

No actionable merge-blocking risk is established for these removals; merge after normal checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the main change: removing unused code from bun_format and napi.cpp.
Description check Passed The description explains the problem, lists the deleted code, states that behavior does not change, and provides detailed verification results. It does not use the exact template headings, but it cove…
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.
✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • 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.

@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.

Code review found no issues

No high-confidence issues detected in this change.

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