Remove dead code from install, webcore, jsc, sha_hmac, and misc crates - #37089
Remove dead code from install, webcore, jsc, sha_hmac, and misc crates#37089robobun wants to merge 4 commits into
Conversation
- install: CacheBehavior enum had a never-constructed LoadFromMemory variant, leaving one variant threaded through 5 signatures and 8 call sites; removed the enum, the parameter, and the dead memory-only branch. Also removed the unused patch re-export alias. - runtime/node: Bun__versions_uws / Bun__versions_usockets no_mangle statics and their extern declarations. C++ reads these from the generated bun_dependency_versions.h since #22561; nothing references the symbols. - jsc: Zig_ErrorCodeParserError / Zig_ErrorCodeJSErrorObject statics (declared but never read from C++), the PARSER_ERROR sentinel const they carried, and the unused dangerously_set_ptr method plus its extern import emitted by js_class_module! (no instantiation calls it). - webcore: 6 never-constructed StartTag variants and the now-unreachable fallback arms; never-constructed ReadDuringJSOnPullResult::AmountRead. - sha_hmac: unused deprecated-API hashers (SHA512, SHA384, SHA512_256, RIPEMD160; only SHA1 and SHA256 have callers) and unused evp types MD5_SHA1 and Blake2. - wyhash: HashInt impls for u16/u64; the only caller instantiates u32. - clap: never-constructed Error::WriteFailed variant, its From impl, and the match arms it fed. - md: never-constructed LineType::Setextheader variant. - react_compiler: unreferenced default_true helper. - Unused re-export lines: sql_jsc (SslConfig alias, 4 facade tokens), bun_api (NodeLinker, NpmRegistryMap, PnpmMatcher), bundler_jsc (FsPath, ErrorableString, JsError), errno (Mode alias on all 3 platforms), spawn (BunSpawn / PosixSpawn aliases), runtime/ffi and runtime/valkey_jsc surface trims (with pub(crate) narrowing where items stay in-crate). - build scripts: stale src/*.c glob (asan-config.c was deleted), write-only BUN_DEP_* macro block in the generated versions header, unread Config.kqueue field. Verified: rg for each symbol across src/, src/codegen/, src/js/, and build/debug/codegen/ finds no remaining references; cargo check -p bun_bin, bun bd (full link), and bun run rust:check-all (10/10 targets) pass; smoke tests in touched areas pass (bun-cryptohasher 400, arraybuffersink 13, markdown-entrypoint 30, minimum-release-age 49, update-interactive 5).
WalkthroughThe change removes obsolete build fields, generated symbols, public re-exports, cache-mode parameters, runtime states, legacy errors, cryptographic implementations, and unused helper APIs. ChangesBuild declarations and generated versions
Manifest cache lookup
Public API and utility surface
Legacy errors and runtime bindings
Hash and cryptography implementations
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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. Comment |
|
Updated 10:43 PM PT - Aug 6th, 2026
✅ @robobun, your commit f9fc3ccc64fe5fa98cb6506e19510233cd95ffb1 passed in 🧪 To try this PR locally: bunx bun-pr 37089That installs a local version of the PR into your bun-37089 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The lint reads the source tree and fails if any deleted symbol reappears, mirroring the existing dead-symbols tests in test/internal/source-lints/. Also shortens three comments the removals had left wordy and applies rustfmt to the touched imports.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
clippy.toml names rsplit_once_char, split_once, and rsplit_once as the required replacements for the banned str::split_once family, so they are lint-policy infrastructure rather than dead code. Keeps the guidance in clippy.toml pointing at functions that exist.
59e3338 to
f9fc3cc
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/modules/NativeModuleList.h:1-27— Commit 59e3338 ("Restore bun_core::strings split/rsplit helpers") also added four unrelated new files —src/jsc/modules/NativeModuleList.h,src/runtime/server/DevErrorPage.rs,src/runtime/server/dev-error-page.html, andpackages/bun-error/schema.ts— none of which are wired up (nothing#includes the header,mod.rsdoesn't declare the .rs, nothing imports the .ts) and none of which are mentioned in the PR description.NativeModuleList.hduplicates theBUN_FOREACH_*_NATIVE_MODULEmacros already defined in_NativeModule.h:27-46(REVIEW.md: "Never copy a helper or constant table between modules"), so a dead-code-removal PR is landing ~380 lines of new dead code including a divergence-prone duplicate table. These look like accidentally-staged work from another branch — please drop them from this PR (they belong in whatever feature PR actually wires them up).Extended reasoning...
What the bug is
Commit 59e3338 is titled "Restore bun_core::strings split/rsplit helpers" and was pushed to address the earlier review comment about
clippy.tomlreferencing deleted helpers. Its commit message discusses onlysrc/bun_core/string/immutable.rs. Butgit show --stat 59e3338fshows it also added four wholly unrelated new files totalling ~378 lines:file lines status src/jsc/modules/NativeModuleList.h+39 nothing #includes it;NativeModuleDefaultSlot/m_nativeModuleDefaultsunreferenced anywheresrc/runtime/server/DevErrorPage.rs+207 not declared in src/runtime/server/mod.rs— cargo does not compile itsrc/runtime/server/dev-error-page.html+24 only referenced by the uncompiled .rs above packages/bun-error/schema.ts+108 nothing imports it; all bun-error consumers import from ../../src/api/schemaNone of these appear anywhere in the PR description, which otherwise exhaustively itemizes every single deletion down to individual enum variants and
HashIntimpls.Step-by-step verification
- NativeModuleList.h is a duplicate constant table.
rg 'NativeModuleList\.h|NativeModuleDefaultSlot|m_nativeModuleDefaults' src/returns only the file itself — nothing includes it, and the enum/constant/"cache array" it defines are unreferenced. Meanwhilesrc/jsc/modules/_NativeModule.hlines 27-46 already defineBUN_FOREACH_ESM_AND_CJS_NATIVE_MODULE/BUN_FOREACH_ESM_NATIVE_MODULE/BUN_FOREACH_CJS_NATIVE_MODULEverbatim. The new header's own comment says it exists "so ZigGlobalObject.h can size its cache array without pulling in _NativeModule.h", but ZigGlobalObject.h doesn't include it and has nom_nativeModuleDefaultsfield. - DevErrorPage.rs is not compiled.
rg 'DevErrorPage|dev_error_page' src/returns only the file itself and the html template.src/runtime/server/mod.rsdoes not declare it, so cargo never sees it — which is whybun bdandrust:check-allpass despite it referencing symbols likebun_core::runtime_embed_file!(Codegen, "bun-error/index.js"). - dev-error-page.html is only referenced by
include_bytes!in the uncompiled .rs. - packages/bun-error/schema.ts is orphaned.
rg "from ['\"]\./schema" packages/bun-error/returns nothing.index.tsx,runtime-error.ts,markdown.ts, andstack-trace-parser.tsall import from"../../src/api/schema"(the pre-existingsrc/api/schema.js+schema.d.ts), not from the new local file. The new file's header comment even says it is "Produced by src/runtime/server/DevErrorPage.rs" — the same uncompiled file.
Why this is blocking, not a nit
REVIEW.md is explicit on both counts:
- "Never copy a helper or constant table between modules" —
NativeModuleList.hcopies the native-module list out of_NativeModule.h. If someone later adds a native module to_NativeModule.h(the file every existing consumer includes),NativeModuleList.hsilently drifts. If both ever get included in one TU, the preprocessor emits macro-redefinition warnings. - "Delete dead code in the same PR that makes it dead… Do not stub empty files. Delete dead code." — this PR's stated purpose is dead-code removal, and it exhaustively documents every line it removes. Landing ~380 lines of new unreferenced/uncompiled code is the exact opposite of that purpose.
The PR's own verification methodology ("verified by
rgacrosssrc/… then by compiling") did not catch these because (a) an un-#included header and an undeclared.rsmodule don't participate in compilation, and (b) the PR only grepped for references to removed symbols, not to added ones. The comment-cop bot has already left six unresolved threads onNativeModuleList.h:6,31andDevErrorPage.rs:3,73,155,196.Evidence this is accidental
The four files form a coherent feature (a
Bun.serve({ development: true })HTML error page: Rust renderer + HTML template + JSON schema + a header refactor to size a per-global cache). They clearly belong to in-progress work on another branch. Commit 59e3338 was a small targeted fix (restore three string helpers + drop 5 lines from the source-lint test); the +378 unrelated lines rode in on the samegit add/git commit -a. The PR description — otherwise exhaustive down to "HashIntimpls foru16/u64" — mentions none of them.How to fix
Drop the four files from this PR:
git rm src/jsc/modules/NativeModuleList.h \ src/runtime/server/DevErrorPage.rs \ src/runtime/server/dev-error-page.html \ packages/bun-error/schema.tsThey should land in the feature PR that actually wires
DevErrorPageintoserver/mod.rs, pointspackages/bun-errorat the new schema, and makes_NativeModule.hinclude (rather than duplicate) the macro list. - NativeModuleList.h is a duplicate constant table.
|
The four files flagged above (NativeModuleList.h, DevErrorPage.rs, dev-error-page.html, bun-error/schema.ts) were committed by accident in 59e3338, which that review ran against. The branch was force-pushed as f9fc3cc with the same helper restore and without those files; the PR diff against the base no longer contains them. |
There was a problem hiding this comment.
Thanks for restoring the bun_core::strings split helpers in f9fc3cc — that was my only finding, and the accidental files are gone. I didn't find anything blocking on this revision, but with 38 files across install, jsc/FFI boundaries, webcore, and crypto, it's worth a maintainer glance.
What was reviewed on this pass:
StartTagvariant removal: confirmed the enum is only used symbolically (const-generic tags /START_TAGconsts), never as a wire/ABIu8, so the discriminant shift is inert.js_class_module!dangerously_set_ptrremoval: confirmed generate-classes.ts's own Rust output declares its own extern (generate-classes.ts:2473/2498) and none of the ~15 hand-written macro instantiations call it.sha::hashers::{SHA384,SHA512,SHA512_256}removal: confirmed CryptoHasher.rs'shashers::prefix resolves viause bun_sha_hmac::sha as hashersto the evp re-exports, not the removed submodule.errno::Mode,BUN_DEP_*,Setextheader: no remaining references anywhere.
Extended reasoning...
Overview
Dead-code sweep removing ~250 lines across 38 files: the CacheBehavior enum + parameter from install manifest lookups (all 8 call sites passed the same variant), unused #[no_mangle] statics (Bun__versions_uws/usockets, Zig_ErrorCode*) plus their C++ header declarations, the never-called dangerously_set_ptr wrapper from the js_class_module! macro, never-constructed enum variants (StartTag::{Empty,Err,...}, ReadDuringJSOnPullResult::AmountRead, LineType::Setextheader, clap::Error::WriteFailed), unused deprecated-API hashers in sha_hmac, unused HashInt impls, ~15 unused pub use re-export names, and stale build-script entries. A source-lint test guards against reintroduction.
Security risks
None identified. The crypto-adjacent changes (sha_hmac) remove only unreferenced hasher instantiations; the evp types backing Bun.CryptoHasher and the SHA1/SHA256 deprecated-API hashers used by websocket upgrade and S3 signing all stay. No validation, auth, or TLS logic is touched.
Level of scrutiny
Medium-high. Each removal in isolation is trivial, but the aggregate touches:
- FFI/ABI boundaries (
#[no_mangle]statics,extern "C"declarations in headers-handwritten.h) — mitigated by fullbun bdlink succeeding, which is decisive for link-time symbols. - A macro (
js_class_module!) that fans out into every hand-written JS class module. - The package manager's manifest-cache hot path (
by_name_hash_allow_expired). #[repr(u8)]enum variant removal that shifts discriminants.
The verification story is strong: cargo check, full build+link, rust:check-all across all 10 targets, and targeted smoke tests (bun-cryptohasher, arraybuffersink, minimum-release-age, update-interactive) all pass. The PR description exhaustively itemizes every deletion with its zero-reference proof.
Other factors
- My prior review flagged an unintentional deletion of
bun_core::strings::{rsplit_once_char,split_once,rsplit_once}(referenced byclippy.tomlas the mandated SIMD replacements for banned std methods). The author restored them in f9fc3cc and dropped the accidentalNativeModuleList.h/DevErrorPage.rsfiles. The current diff no longer touchesimmutable.rs. - I spot-checked the four removals I judged riskiest on this revision (StartTag discriminants,
dangerously_set_ptrvs generate-classes.ts's separate surface, CryptoHasher'shashers::alias resolution,errno::Mode) — all check out exactly as the PR description claims. - The
CacheBehavior::LoadFromMemorybranch removal is behavior-preserving because that variant was never constructed;by_name_hash_in_memory(the actual memory-only path) stays. - The comment-cop bot's remaining flags were argued as documenting invariants rather than justifying workarounds; those threads are resolved.
Deferring rather than approving because 38 files across install/jsc/webcore/sha_hmac plus C++ header edits is broad enough that a maintainer should skim the deletion list, even though I found nothing wrong.
1 participant
Dead code sweep over areas not covered by the open dead-code PRs (#37062, #37012, #36237, #36115, #35880, #35775, #35437): install, runtime/node, runtime/webcore, jsc, sql_jsc, and the sha_hmac/wyhash/clap/md/errno/spawn/api/bundler_jsc leaf crates, plus scripts/build. Net -250 lines. No overlap with the deletions in any open PR (the only shared file, headers-handwritten.h, is touched on different lines).
Every removal was verified by
rgacrosssrc/,src/codegen/,src/js/, andbuild/debug/codegen/(zero references outside the definition), then by compiling.Removed
install:
CacheBehaviorenum and parameter. TheLoadFromMemoryvariant is never constructed anywhere, so every one of the 8 call sites passesLoadFromMemoryFallbackToDiskand the enum carries no information. Removed the enum, thecache_behaviorparameter from the fourby_name*lookups, the dead memory-only branch inby_name_hash_allow_expired, theManifestLoadre-exports, and the argument at all call sites (outdated, update-interactive, lockfile, enqueue, populate-manifest-cache). The memory-only path callers actually use isby_name_hash_in_memory, which stays. Also removed the unusedpub use patch_install as patch;alias (all consumers usepatch_install::directly).runtime/node:
Bun__versions_uws/Bun__versions_usockets. Theseno_manglestatics (and theirextern "C"declarations in headers-handwritten.h) have no readers: BunProcess.cpp readsBUN_VERSION_USOCKETS/BUN_VERSION_UWSfrom the generatedbun_dependency_versions.hsince #22561. Their values were wrong anyway (both held the bun git sha). Since all C++ references resolve at link time, the fullbun bdlink passing is the proof nothing consumes the symbols.jsc: error-code sentinels and
dangerously_set_ptr.Zig_ErrorCodeParserErroris declared in headers-handwritten.h but no C++ reads it;Zig_ErrorCodeJSErrorObjectis not even declared. Removed both statics, the declaration, and thePARSER_ERRORconst whose only use they were (JS_ERROR_OBJECTstays, it is used widely). Separately, thejs_class_module!macro emitted adangerously_set_ptrwrapper plus its__dangerouslySetPtrextern import in every instantiation, and no instantiation calls it; the similarly named symbols in build codegen belong to generate-classes.ts's own separate Rust surface, which declares its own externs.webcore: never-constructed variants.
StartTag::{Empty, Err, ChunkSize, Ready, OwnedAndDone, Done}are never constructed (only the 8 sink tags are used asSTART_TAGconsts and const-generic args), so the two fallback_arms they fed were unreachable and are gone too; the remaining matches are exhaustive.ReadDuringJSOnPullResult::AmountReadis never produced byon_read_chunk(the code already said so in a comment).sha_hmac. The deprecated-OpenSSL3
sha::hashersmodule has exactly two consumers:SHA1(websocket upgrade) andSHA256(s3 signing). Removed the unusedSHA512,SHA384,SHA512_256,RIPEMD160hashers. CryptoHasher'shashers::SHA384etc. resolve through itsuse bun_sha_hmac::sha as hashers;alias to the evp re-exports, which stay. Also removedevp::MD5_SHA1andevp::Blake2(zero references;Algorithm::Blake2b256callsffi::EVP_blake2b256()directly).Smaller items.
HashIntimpls foru16/u64; the singlehash_intcaller passesu32.Error::WriteFailedis never constructed (no fallible-fmt path converts into it); removed the variant, itsFrom<fmt::Error>impl, and the unreachable diagnostic arm.LineType::Setextheadernever constructed or matched.default_truehelper with zero references.SslConfigalias and theCoerceTo/ExternColumnIdentifierValue/ThrowFmtArgs/ZigStringJscfacade tokens; bun_apiNodeLinker/NpmRegistryMap/PnpmMatcher; bundler_jscFsPath/ErrorableString/JsError; errnoModealias (all three platform files); spawnBunSpawn/PosixSpawnaliases; runtime/ffiabi_typesurface (formatters narrowed topub(crate), matching the workspaceunreachable_pubconvention) and runtime/valkey_jsc surface trims."src/*.c"glob (its last match,src/asan-config.c, was deleted in asan: keep MarkedArgumentBuffer inline storage on the real stack #29655; the pattern silently matches nothing), the write-only#define BUN_DEP_*block in the generated versions header (BunProcess.cpp reads only theBUN_VERSION_*constants), and theConfig.kqueuefield (computed, stored, never read).Verification
cargo check -p bun_bincleanbun bdfull build and link passes (link success is the decisive check for the removedno_manglestatics)bun run rust:check-all: 10 ok, 0 failed (no platform-gated false positives)bunx tsc -p scripts/build/tsconfig.json: same 18 pre-existing errors with and without this diff, none in touched filesVerified dead but deliberately left in place
These have zero references today but carry explicit staging markers or ABI roles, so they are notes rather than deletions: the h2
Connectionoutbound-send API (module-levelallow(dead_code), unit-tested), react_compiler's logger/diagnostic parity types (allowwith upstream-parity reasons),bun_opaque::FfiLayout(documented sealed marker), bake'sState::EvaluationFailure/BakeProdSourceMap(receiving ends of not-yet-wired dev-server and SSG paths; removing them strands live plumbing),BOM::Utf16Be/Utf32*(detection intentionally not ported yet),PercentEncodeError::OutOfMemory(staged for fallible-alloc), dev-serverMessageIdwire ids, and theRegularExpression::Flags/SSRKind::RegularABI mirrors.[review] gate passed · iteration 1 · 38 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