Remove 7 stale module-level dead_code escapes from the node:http2 engine and react_compiler::hir - #42278
Remove 7 stale module-level dead_code escapes from the node:http2 engine and react_compiler::hir#42278robobun wants to merge 4 commits into
Conversation
…e and react_compiler::hir The six modules under src/runtime/api/bun/h2 each start with #![allow(dead_code)], and react_compiler/hir/mod.rs lists dead_code in its module-level allow. Nothing in those modules is dead any more: with the escapes removed, cargo check passes for every shipped target in dev, and for the host in release. The escapes were not harmless. rustc skips the modules, and the cross-crate hawk analysis treats every item under allow(dead_code) as a reachability root, so 12.3k lines were invisible to both.
|
Status: ready to merge. The diff is green. The two red CI runs are on tests this PR does not touch. How this was checked:
CI:
The |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughThe change removes ChangesDead-code lint cleanup
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The dead-code cleanup and allowlist enforcement are ready to merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — pure lint-attribute deletion with no code path touched.
What was reviewed:
- Confirmed all seven hunks only remove
dead_codefrom module-level#![allow(...)]attributes; no other tokens change. - Checked
src/react_compiler/hir/mod.rs: the survivingreason = "ported from facebook/react react_compiler_hir; uses std collections by design"still reads correctly for the remainingclippy::disallowed_types/disallowed_methods/unreachable_pubentries. - No CODEOWNERS entry covers these paths; change aligns with the repo's "delete dead code / don't hide it" rule.
<details>
<summary>Extended reasoning...</summary>
Overview
This PR deletes seven stale module-level dead_code lint escapes: six standalone #![allow(dead_code)] lines from the node:http2 engine modules (src/runtime/api/bun/h2/{connection,flow_control,hpack,settings,stream,wire}.rs) and one dead_code entry from the multi-lint #![allow(...)] block in src/react_compiler/hir/mod.rs. The diff is attribute-only — not a single line of executable code, type, or import is added or modified.
Security risks
None. Removing a lint suppression has zero runtime effect; the compiled output is byte-identical. There is no user input handling, no auth/crypto/permission surface, and no data flow change of any kind.
Level of scrutiny
Low. This is exactly the kind of mechanical cleanup the approval guidelines call out as approvable without human review. The only real risk is that unsuppressing the lint surfaces new warnings (which under the workspace's dead_code = "deny" would fail the build). The PR description states cargo check, bun run rust:check-all across all 12 targets, and bun bd all pass, and CI would catch a build break immediately if that claim were wrong. Because the workspace denies dead_code, a green build is itself the proof that nothing dead was hiding under these escapes — no separate test is needed for an attribute-only change per REVIEW.md's "refactor guilty until proven behavior-preserving" bar (the bar here is a clean rust:check-all).
Other factors
I verified the remaining reason string in react_compiler/hir/mod.rs still accurately describes the surviving allows (disallowed_types/disallowed_methods for std collections, unreachable_pub for the port), so no stale-comment issue. CODEOWNERS covers none of the touched paths. No prior reviews or outstanding objections exist on the timeline. The change directly serves REVIEW.md's "Delete dead code in the same PR that makes it dead… Public items escape dead-code lints" policy by re-enabling the lint and unblocking the hawk cross-crate reachability tool from treating these modules as roots.
</details>
…faces and the standalone shim dead-code-escapes.test.ts skipped module-level #![allow(...)] blocks on the assumption that they only appear on codegen surfaces. The seven escapes the previous commit removed show that they also land on handwritten modules, where they hide the file from rustc and root every item in it for hawk. The lint now fails when a file outside a short allowlist carries one.
The check only failed when a file outside the allowlist gained an escape. An allowlisted file that dropped its escape left a stale entry behind, and that entry would let the escape come back unnoticed. Compare the two sets for equality, like the item-level inventory above it does.
|
Updated 12:31 AM PT - Sep 11th, 2026
❌ @robobun, your commit 461fa43 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 42278That installs a local version of the PR into your bun-42278 --bun |
There was a problem hiding this comment.
LGTM — the module-level check is now bidirectional (toEqual on both sorted lists), so a stale allowlist entry will fail the test just like a new escape would, and each allowlisted path carries a one-line justification.
What was reviewed:
- The seven
#![allow(dead_code)]deletions are pure lint-suppression removals with no code-path change; correctness is enforced by the workspacedead_code = "deny"plus CI's cross-target build. MODULE_ESCAPEregex: the(?<![\w:])dead_code\blookbehind avoids false hits on namespaced lint names, and[^\]]fences the match inside a single attribute like the existingESCAPEpattern.- The new test reuses the same
strippedsource (line comments removed) as the item-level scan, so commented-out attributes won't count.
<details>
<summary>Extended reasoning...</summary>
Overview
This PR deletes six #![allow(dead_code)] inner attributes from the node:http2 engine modules under src/runtime/api/bun/h2/ and drops the dead_code entry from the multi-lint #![allow(...)] in src/react_compiler/hir/mod.rs. It then extends test/internal/source-lints/dead-code-escapes.test.ts with a MODULE_ESCAPE regex and a bidirectional allowlist assertion so that any new (or stale) module-level dead_code escape breaks CI. My prior review flagged that the first version of this check was one-directional and that the react_compiler/diagnostics allowlist entry lacked a justifying comment; commit 84a1fe8 addressed both by switching to expect(moduleLevel.toSorted()).toEqual([...MODULE_ESCAPE_ALLOWED].toSorted()) and adding per-entry comments.
Security risks
None. The Rust changes remove lint suppressions only — no runtime code paths, allocations, or FFI surfaces are touched. The test change scans repository source files with a regex and compares against a hard-coded set; it takes no external input.
Level of scrutiny
Low. The native changes are one-line deletions whose only possible failure mode is a compile error (the workspace denies dead_code), which the multi-target CI build catches deterministically. The test-file change follows the exact pattern already established in the same file for item-level escapes, and the bidirectional toEqual shape matches REVIEW.md's "assert the strongest invariant" guidance.
Other factors
Exit reason was dry_streak and no findings were reported this run. There are no outstanding CHANGES_REQUESTED reviews from other parties. Both points from my earlier review have been addressed in the latest push, and REVIEW.md's dead-code guidance ("delete dead code in the same PR that makes it dead"; "do not add tests to check dead code stays dead") is satisfied here — the PR removes suppressions rather than adding them, and the new test guards escape-hatch inventory (an established local pattern in this file), not dead-symbol behavior.
</details>
### Problem - 49 hand-written trait impls across 12 crates have no user: no call, and no bound that needs them. - rustc's `dead_code` lint exempts trait impls, so the workspace's deny lints never report them. ### Fix - Delete the 49 impls and `hash_array_list` (`src/css/generics.rs`), whose only caller was a removed impl. 25 files, 436 lines removed. - Candidates come from the linker: in a relink of the debug build with `--gc-sections`, an impl block with no kept function address is a candidate. The compiler decides next: an impl that a bound, a supertrait, another target, or a unit test needs does not compile away, so it stays. That kept 128 of 233 candidates. - 56 more stayed by choice: open PRs use them, docs describe them, or they are std ergonomics impls on shared containers. See the Notes. - Verified: `bun run rust:check-all` (12 targets), `cargo check --release` (3 targets), `cargo check --tests`, `cargo clippy`, `bun bd`, 11 test files, and a scan of all open PRs for users. ### Background - To rustc a trait impl is always reachable: generic code could call it. Only a whole-program view shows that nothing does. - The debug build uses opt-level 0 and one section per function, so `--gc-sections` drops every function with no caller. Callers in generic instantiations and macro-expanded code count. - That link proves linux only. `cargo check` covers the other targets. - #39795 removed 92 impls this way. <details><summary>Notes</summary> **Removed impls, by file** - `src/ast/e.rs` - `impl From<f64> for Number` - `src/bun_alloc/baby_vec.rs` - `impl<'a, 'b, T: Copy> Extend<&'b T> for BabyVec<'a, T>` - `impl<'a, 'b, T> IntoIterator for &'b BabyVec<'a, T>` - `impl<'a, 'b, T> IntoIterator for &'b mut BabyVec<'a, T>` - `impl<'a, T> core::borrow::Borrow<[T]> for BabyVec<'a, T>` - `impl<'a, T> AsRef<[T]> for BabyVec<'a, T>` - `src/bun_alloc/lib.rs` - `impl<const N: usize> BSSAppendable for [&[u8]; N]` - `src/bun_core/atomic_cell.rs` - `unsafe impl<U> Atom for *const U` - `src/bun_core/fmt.rs` - `impl From<InvalidCharacter> for crate::CrateError` - `src/bun_core/output.rs` - `impl<T: fmt::Display> FmtTuple for &[T]` - `src/bundler/Chunk.rs` - `impl core::ops::Index<usize> for CompileResultSlots` - `src/bundler/bundle_v2.rs` - `impl Ord for StableRef` - `impl PartialOrd for StableRef` - `src/collections/array_hash_map.rs` - `impl<K, V, C, A: MapAllocator> ArrayHashMapExt for ArrayHashMap<K, V, C, A>` - `src/css/generics.rs` - `impl<'bump, T: DeepClone<'bump>> DeepClone<'bump> for &'bump T` - `impl<T: CssEql, const N: usize> CssEql for [T; N]` - `impl CssHash for ()` - `impl<T: CssHash, const N: usize> CssHash for [T; N]` - `impl<'bump, T: CssHash> CssHash for ArrayList<'bump, T>` - `impl CssHash for bun_ast::Loc` - `impl<T: IsCompatible, const N: usize> IsCompatible for [T; N]` - `impl<'bump, T: ToCss> ToCss for ArrayList<'bump, T>` - `impl ToCss for CustomIdent` - `impl ToCss for DashedIdent` - `impl ToCss for Ident` - `src/css/media_query.rs` - `impl crate::generic::ToCss for MediaList` - `src/css/properties/mod.rs` - `impl crate::generics::ParseWithOptions for css_values::length::Length` - `impl<S, const P: u8> crate::generics::Parse for GenericBorder<S, P>` - `src/css/rules/supports.rs` - `impl crate::generics::CssEql for SupportsCondition` - `impl css::generic::ToCss for SupportsCondition` - `src/css/selectors/parser.rs` - `impl<Impl: SelectorImpl> Default for GenericSelector<Impl>` - `impl<Impl: BunSelectorImpl> CssEql for GenericSelectorList<Impl>` - `impl<Impl: BunSelectorImpl> CssHash for GenericSelectorList<Impl>` - `impl<Impl: BunSelectorImpl> CssEql for GenericComponent<Impl>` - `impl<Impl: BunSelectorImpl> CssHash for GenericComponent<Impl>` - `src/css/values/alpha.rs` - `impl crate::generics::CssHash for AlphaValue` - `src/install/lockfile/lockfile_json_stringify_for_debugging.rs` - `impl<const N: usize> JsonScalar for &[u8; N]` - `src/install/resolution.rs` - `impl Default for Tag` - `src/install_types/resolver_hooks.rs` - `impl<I: VersionInt> Default for ResolutionValue<I>` - `impl Default for Resolution` - `src/jsc/JSPropertyIterator.rs` - `impl IntoIterObject for *const JSObject` - `src/jsc/JSValue.rs` - `impl FromAny for ()` - `impl FromAny for &str` - `impl<T: FromAny> FromAny for Option<T>` - `src/jsc/host_fn.rs` - `impl<T> IntoHostConstructReturn for *mut T` - `src/react_compiler/hir/mod.rs` - `impl From<FloatValue> for f64` - `src/runtime/bake/DevServer.rs` - `impl From<OpaqueFileId> for OpaqueFileIdOrOptional` - `src/runtime/error.rs` - `impl From<Error> for bun_jsc::CrateError` - `src/tcc_sys/tcc.rs` - `impl<ErrCtx> Default for Config<ErrCtx>` **Candidates that stayed (184 of 233)** - 128: the workspace does not compile without the impl (a bound, a supertrait, another target, or a unit test needs it). - 8: open PRs use them. The six `SmallList` impls (`Deref`, `DerefMut`, `IntoIterator` x2, `FromIterator`, `Extend`) for #39497, #38692 and #36605. `Default for Wyhash` for #40372. `AsRef<[u8]> for Utf8Bytes` for #40708. This branch merged with #39497, #38692 and #40708 passes `cargo check --workspace`. #36605 and #40372 conflict with main on their own. - 19: std ergonomics impls on widely shared types. This is a design call, so it needs a maintainer decision and its own PR: `Interned` (`Deref`, `AsRef<[u8]>`, `Borrow<[u8]>`), `ZStr` (`AsRef<ZStr>`, `PartialEq<[u8]>`, `PartialEq<&[u8; N]>`), `StoreStr` (`PartialEq<[u8]>`), `StoreSlice` (`AsRef<[T]>`, `From<&[T]>`, `From<ArenaVec>`), `IntoIterator for &HashMap` and `&mut HashMap`, `BoundedArrayAligned` (`Deref`, `DerefMut`, `fmt::Write`), `AsRef<[T]> for RawSlice`, `Deref for ArenaString` (also the only caller of `ArenaString::as_str`), `Display for GenericIndex`, `Display for CowSliceZ`. - 21: docs or comments describe the impl as part of a design. `ArrayHashContext<Box<[u8], A>> for BoxedSliceContext<C>` (its doc comment records it as the fix for lookups reached through `Deref`), `Default for Ordinal` (the default is `INVALID`, not 0), `IntoLogWrite for &mut W`, `ThrowFmtArgs for fmt::Arguments`, `PoolStorage for UnwiredStorage`, `StringBuilder for semver_string::Builder`, `fmt::Write for FmtAdapter`, `AsFd for &File`, and the css blanket impls that the `css_derive` module docs and the `Parse` section of `src/css/generics.rs` name (`Option<T>`, `SmallList`, `bool`, `str`, `[u8]`, `[T]`, `Size2D`, `Rect`). - 6: next to a hunk of an open dead-code PR (#40690, #41335, #43010, #42278), left alone to avoid a conflict. - 2: the removal leaves something else half-used. See the follow-up list. **Method details** - Link: the debug link command from `build.ninja` plus `-Wl,--gc-sections`. `-rdynamic`, `--dynamic-list` and the version script stay, so the exported NAPI/V8/uv surface is still a root. - Liveness: `llvm-nm` lists the 669,524 kept function symbols and `llvm-symbolizer` maps each address to `file:line`. An impl block is a candidate when no kept address falls inside it and a rust-analyzer SCIP index shows that linux compiles every method in it. - Limit of the finder: it reads impl blocks from source text. Impls that a `macro_rules!` type list generates are not candidates, so unused arms of such lists are not in this PR. - Restore loop: delete all candidates, run `cargo check --workspace --keep-going --message-format=json`, put back each impl that an error names, repeat until clean. Targets in the loop: linux-gnu, windows-msvc, darwin, freebsd, android, musl, then `--tests`. - Method resolution: on linux the link proves that no call to a removed impl remains, so no call can resolve differently. On other targets a call that used a removed impl fails to compile. - Open PRs: main has no merge queue, so a green PR that uses a removed impl would break main when it merges. I fetched the heads of all 5474 open non-draft PRs, took the Rust lines each one adds against its merge base, and searched them for the types and traits of the 49 impls. No PR uses one. - Base: main at 18fe86e. The liveness data is from cf71211. The commits between them do not touch the 25 files, and every check above ran again on 18fe86e. - `cargo check --workspace --tests` reports one error, `src/bundler/options.rs:2328` (`expected ContentHash, found integer`). Clean main has the same error. This PR does not touch it. - Three tests fail in the local debug build with and without this change: `ffi.test.js` "FTL-compiled call site" (5 s timeout under ASAN), and two `serve.test.ts` tests that depend on the container (root can bind low ports, loopback check). **Other scans in this run** - Free functions, inherent methods, types, consts and statics: a name scan, the linker scan, and a rust-analyzer zero-reference scan for linux, windows and darwin. Every hit was platform code with a caller on another target, test-only code, or a deletion that an open PR already carries. - Also clean or already claimed: `.rs` files outside every mod tree, unused Cargo dependencies, extern declarations without a Rust caller, commented-out code in C++/TS, unused `src/js` modules and internal exports, linker-dropped C++ functions in `src/jsc/bindings`. **Follow-up candidates, not in this diff** - 25 hand-written `Debug` impls that nothing formats. They are a debugging aid, so this PR keeps them. - `Clone for JsPoster` has no caller, and it is the only reader of the `JsPosterVTable::clone` slot. Removing both is a small refactor in `src/event_loop` and `src/jsc/VmHandle.rs`. - `Display for ErrorLocation` (`src/css/error.rs`) has no caller, and it is the only reader of `ErrorLocation::filename`. - The 19 std ergonomics impls above, if a maintainer wants shared containers to go through inherent methods only. - `Bun__napi_get_version` in `src/jsc/bindings/napi.cpp` has no caller. - The class generator emits `<Type>Class__call` shims for classes that set `call: true` and `noConstructor: true` (the seven `Expect*` matchers in `jest.classes.ts`). Nothing references those shims. </details>
This branch has not been deployed
2 participants
Problem
dead_codelint escapes are stale. Six are the#![allow(dead_code)]lines at the top ofsrc/runtime/api/bun/h2/{wire,settings,flow_control,hpack,stream,connection}.rs. One is thedead_codeentry in the#![allow(...)]block ofsrc/react_compiler/hir/mod.rs.hawktreats every item underallow(dead_code)as a reachability root. It never reports these modules, and it keeps alive anything they reference.Fix
test/internal/source-lints/dead-code-escapes.test.tsnow requires the set of files with a module-levelallow(dead_code)to equal a short allowlist (two generated surfaces, the standalone shim). It fails onmainwith the seven files listed and passes here.dead_codeisdenyfor the workspace and the build still passes:cargo checkin dev and release on the host,bun run rust:check-allon all 12 targets, andbun bd(which adds--cfg bun_debugand--cfg bun_asan).hawkon linux-x64 with the escapes removed reports nodead_publicitem in either directory.test/js/node/http2/h2-conformance.test.ts,node-http2-settings-ack-ordering.test.ts,node-http2-continuation.test.ts,test/bundler/transpiler/react-compiler.test.ts.Background
dead_code = "deny". An#[allow(dead_code)]is a deliberate escape.test/internal/source-lints/dead-code-escapes.test.tspins the item-level escapes per file. It does not count module-level#![allow]blocks, so these seven were never audited.hawk(hawk.toml,tools/hawk/) is the cross-crate analysis that findspubitems no other crate uses. rustc cannot see those.src/runtime/api/bun/h2/is thenode:http2engine thath2_frame_parser.rsdrives.src/react_compiler/hir/is the HIR of the React Compiler port.Notes
This run of the dead-code sweep found no other deletion that an open PR does not already hold. What it covered, on
81f97bb020:hawkonx86_64-unknown-linux-gnu,aarch64-apple-darwinandx86_64-pc-windows-msvc. 367 items are dead on all three. They are FFI struct fields, enum variants that mirror external code tables (already listed inhawk.toml), items an open PR deletes, or false positives (constants used only as array lengths, modules used only through a re-export, helpers named fromgenerate-classes.tstemplates).hawk --fixto narrow visibility in a scratch tree, thencargo checkwith lints capped at warn. 36dead_codehits:MultiArrayListcolumn structs, holder fields, items used only from#[cfg(test)], or items an open PR deletes.--gc-sections --print-gc-sections. 92 discarded C++ functions have an out-of-line definition insrc/orpackages/. 76 are in open PRs. The rest have a caller on Windows or macOS, are exported API (napi_*,v8::), or are template instantiations. 1,158 discarded C-ABI symbols: the same result.clang -fsyntax-only -Wunused-function -Wunused-template -Wunused-member-function -Wunused-const-variableover all 664 translation units (the build passes-Wno-unused-function). 13 hits. Four are in open PRs. The others are used on another platform or in another translation unit.c-index-test -index-fileover the same 664 translation units: 23,572 declarations insrc/andpackages/, 7,320 with no reference. After a textual cross-check 62 names remain. All are macro-pasted names, JSC template hooks, or mirrors of the v8, libuv and llhttp headers..rsfile outside a module tree. No unused Cargo dependency. No internal JS module that nothing requires. 179 lines of commented-out code in total.Not in the diff:
src/react_compiler/diagnostics/mod.rshas the same stale entry. Remove dead code from bun_css, bun_react_compiler, bun_jsc, and smaller crates #40690 already removes it, so the new lint allows that file for now. Remove dead code from bun_css, bun_react_compiler, bun_jsc, and smaller crates #40690 also adds a separate inventory of module-level escapes (module-lint-escapes.test.ts) that pins the sixh2files at one each. Whichever of the two PRs merges second has to regenerate that inventory.src/install/windows-shim/main.rsallowsdead_codefor the whole standalone shim crate. That one is justified:bun_shim_impl.rsis compiled twice and each build uses a different half.[auto-merge] gate passed · iteration 3 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 3
evidence per changed file