Skip to content

Remove dead code from bun_boringssl_sys and bun_lint_react_compiler - #47918

Open
robobun wants to merge 1 commit into
mainfrom
robobun/8b65b74e/dead-code-sweep
Open

robobun wants to merge 1 commit into
mainfrom
robobun/8b65b74e/dead-code-sweep

Conversation

@robobun

@robobun robobun commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • Delete the declaration, the [dev-dependencies] section with its three Cargo.lock lines, and the row: 14 lines in 4 files.
  • Correct because boringssl.rs:3-6 says the file declares only what the Rust crates use, and rust-test.ts:32 says a listed crate must have a #[test].
  • Verified on main b9f2ec3: bun bd, bun run rust:check-all (12 targets), bun run rust:test (50 of 50 test programs pass).
  • Self-reviewed: 42 concerns raised, 32 addressed, 4 rejected, 6 need a maintainer (see Notes).

Behaviour change: none

Background

  • A dev-dependency is a crate that only the tests of a package link.
  • scripts/rust-test.ts runs cargo test for a fixed list of crates that link without JavaScriptCore.
  • This run found about 330 more dead lines. Open pull requests already delete them or still use them, so they are not in this diff. The Notes list each with its owner.

Downsides

  • A later test in bun_lint_react_compiler must add its dev-dependencies and the row again.
  • None found for users. Binary size impact: none.
  • None found for open pull requests. At 2026-11-04 11:56Z none of the 6,055 open heads adds a caller of the declaration or a test to the crate, and none gains a conflicted file.
Notes

Removed (14 lines)

Checks

  • Base: main b9f2ec3. The results below are for that commit and expire with each rebase.
  • rg -w EVP_MD_do_all_sorted src packages scripts build/debug/codegen: no hit. rg 'bun_lint_react_compiler' scripts .buildkite test/internal: no hit. rg '#\[test\]|cfg\(test\)' src/lint/react_compiler: no hit.
  • bun bd builds. test/js/node/crypto/crypto.test.ts (467 pass) and test/js/node/crypto/hash-and-buffer-short-roads.test.ts (22 pass, 1 skip) pass on the debug build.
  • bun run rust:check-all: 12 of 12 targets pass. cargo check --workspace --all-targets passes on the host.
  • bun run rust:test: 50 of 50 test programs pass, 5,509 tests.
  • Open pull requests, 2026-11-04 11:56Z. The heads of all 6,055 were fetched, and the lines each adds against its merge base with main were searched.

Found dead in this run, and left alone

How the candidates were found

  • Rust: in a scratch tree every fn, const, static and type got #[deprecated] with its own note (91,738 items, and 2,097 trait methods in a second pass). cargo check --workspace then reports each resolved use. The passes: the 12 target triples, --all-targets, --all-features, and all custom --cfg flags together. An item is dead when every use of it is inside another dead item. Result: 161 items, of which open dead-code pull requests delete 124. 7 trait methods are never called, and open pull requests delete all 7.
  • C and C++: a second link of the debug build with -Wl,--gc-sections -Wl,--print-gc-sections, and clang -fsyntax-only -Wunused-function -Wunused-member-function -Wunused-template over 624 translation units. Result: 277 hand-written functions with no reference on linux-x64, of which open pull requests delete 179.
  • Read-only passes over src/js, packages/bun-usockets, packages/bun-uws, scripts/ and the Cargo manifests. Each found its area exhausted. The dev-dependencies come from the manifest pass.
  • Clean in this run: no .rs file outside a module tree, no macro_rules! that nothing invokes apart from the one in Remove dead code: 15 trait impls nothing uses and two C++ overloads #47264, and no non-Rust file under src/ that nothing names apart from the .idl copies in Remove dead code from the inspector agent, the WebKit IDL copies, and an orphaned package #41064.

Self-review

  • The first cut of this pull request had 345 deleted lines in 11 files: 13 of these 14 lines and the items above. The comparison for it read only the diffs of the open dead-code pull requests. The review read every open head, found the duplicates and the users listed above, and asked for this cut.
  • Addressed (32): the held and duplicated hunks are out, the branch is on the tip of main, the checks were run again on it, and the checks over every open head are stated with their time and commit.
  • Rejected (4):
  • Need a maintainer (6): whether main trims the WebIDL converter family or keeps it as upstream has it, whether a pull request that conflicts with main still holds an item, what to do with the 53 open dead-code pull requests that conflict with main, a ruling on the hand-run tools, a check in the builtins codegen for unread names, and where later runs read the holds of earlier runs.

[policy-decision:dep] gate passed · iteration 0 · 4 files touched

passes on PR (with fix)
Dependency/toolchain change; no test proof to run.
diff hotspot
Cargo.lock                         | 3 ---
 scripts/rust-test.ts               | 1 -
 src/boringssl_sys/boringssl.rs     | 4 ----
 src/lint/react_compiler/Cargo.toml | 6 ------
 4 files changed, 14 deletions(-)

gate history · 1 passed · 0 rejected · iteration 0

evidence per changed file
file                                reads  edits  tests
Cargo.lock                              0      0      3
scripts/rust-test.ts                    0      0      4
src/boringssl_sys/boringssl.rs          0      0      2
src/lint/react_compiler/Cargo.toml      0      0      2

- bun_boringssl_sys: the EVP_MD_do_all_sorted declaration. #47671 removed
  its last caller.
- bun_lint_react_compiler: the dev-dependencies bun_sema, bun_sema_parser
  and bun_sema_standalone, and the row of the crate in scripts/rust-test.ts.
  #47316 deleted the only tests of the crate.
@coderabbitai

coderabbitai Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

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 4 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
Commits

Reviewing files that changed from the base of the PR and between b9f2ec3 and b6a4124.

Files ignored due to path filters: 1
  • Cargo.lock is excluded by !**/*.lock
Files selected for processing: 3
  • scripts/rust-test.ts
  • src/boringssl_sys/boringssl.rs
  • src/lint/react_compiler/Cargo.toml
  • 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

@github-actions github-actions Bot added the claude label Nov 4, 2026
@robobun

robobun commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:54 AM PT - Nov 4th, 2026

❌ @robobun, your commit b6a4124 has 38 failures in Build #137518 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 47918

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

bun-47918 --bun

@robobun

robobun commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. Pull request: #47918.

How the dead code was confirmed on main b9f2ec3:

  • rg -w EVP_MD_do_all_sorted src packages scripts build/debug/codegen finds only the declaration. node:crypto, webcrypto: a fourth batch of Node compatibility fixes #47671 removed the last caller.
  • rg '#\[test\]|cfg\(test\)' src/lint/react_compiler finds nothing, and no source of the crate names bun_sema, bun_sema_parser or bun_sema_standalone. Before this change cargo test -p bun_lint_react_compiler ran 0 tests.
  • With the 14 lines removed: bun bd, bun run rust:check-all (12 targets) and bun run rust:test (50 of 50 test programs) pass.

CI: build 137518 has 28 red test files. All 28 are also red in build 137481 of main, without this diff. They are 27 files under test/aot/ and test/bundler/compile-aot-size.test.ts. None of them exercises the four changed files.

@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, a straightforward dead-code removal.

What was reviewed:

  • EVP_MD_do_all_sorted: no remaining reference outside vendor/ (including cfg-gated Rust and C++), so the extern declaration is unused.
  • bun_lint_react_compiler: no #[test], cfg(test), doctest blocks, or bun_sema* imports remain after a47a315 deleted code_generation.rs, so the dev-deps and the rust-test.ts row are consistent with the list's own rule.
  • Hand-edited Cargo.lock: the remaining six entries match Cargo.toml exactly and stay sorted; bun_sema_standalone is still a path dep of several other crates, so cargo would not drop its package entry.
Extended reasoning...

The change is a 14-line pure deletion across four files: one unused extern "C" declaration in the BoringSSL FFI block, an unused [dev-dependencies] section with its three Cargo.lock edges, and one row in the rust:test crate list. It touches the crypto FFI surface only by removing a declaration nothing calls, so there is no behavioral or security-relevant change. The removals were cross-checked against repo-wide searches and the commits that orphaned the code (01ec085 and a47a315), the lockfile edit matches the manifest, and none of the files fall under CODEOWNERS; the bug hunt exited on dry_streak with no findings.

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