Remove dead code from install, event_loop, dns, mysql protocol, C++ bindings, and JS internals - #36970
Conversation
…indings, and JS internals
WalkthroughThis PR removes unused symbols and export members across Rust, JavaScript, C++, build flags, and MySQL protocol code. It also updates related parsing and dispatch code, and adds source-lint tests that fail if the removed symbols return. ChangesDead symbol removal and regression coverage
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/internal/source-lints/dead-symbols-install-sql-bindings.test.ts`:
- Around line 87-115: Extend the dead-symbol test around the checks array to
positively verify that ErrorStackTrace.cpp still defines
Bun__errorInstance__finalize, rather than only asserting deleted symbols are
absent. Add a source-presence assertion using the existing headFile/checking
pattern, or an equivalent Darwin link check, while preserving the current
resurrected-symbol behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b0571ee-1945-4683-88a6-8d7357936a4d
📒 Files selected for processing (43)
scripts/build/flags.tssrc/dns/lib.rssrc/event_loop/EventLoopTimer.rssrc/install/error.rssrc/install/hosted_git_info.rssrc/install/lockfile/bun.lock.rssrc/install/migration.rssrc/install/pnpm.rssrc/install/resolution.rssrc/install/windows-shim/bun_shim_impl.rssrc/js/internal/fixed_queue.tssrc/js/internal/http1_server_fallback.tssrc/js/internal/inspector/cdp.tssrc/js/internal/promisify.tssrc/js/internal/repl/node-shims.jssrc/js/internal/sql/query.tssrc/js/internal/sql/shared.tssrc/js/internal/sql/sqlite.tssrc/js/internal/streams/iter/from.tssrc/js/internal/streams/iter/utils.tssrc/js/internal/trace_events.tssrc/jsc/bindings/BunObject.hsrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/BunString.cppsrc/jsc/bindings/ErrorStackTrace.cppsrc/jsc/bindings/ImportMetaObject.cppsrc/jsc/bindings/JSBakeResponse.cppsrc/jsc/bindings/NodeFSStatFSBinding.cppsrc/jsc/bindings/NodeValidator.cppsrc/jsc/bindings/NodeValidator.hsrc/jsc/bindings/dh-primes.hsrc/jsc/bindings/headers-handwritten.hsrc/jsc/bindings/sliceAnsi.cppsrc/jsc/bindings/sqlite/JSSQLStatement.cppsrc/jsc/bindings/webcore/JSAbortSignal.cppsrc/runtime/bake/BakeGlobalObject.cppsrc/runtime/dispatch.rssrc/sql/mysql/protocol/EOFPacket.rssrc/sql/mysql/protocol/LocalInfileRequest.rssrc/sql/mysql/protocol/OKPacket.rssrc/sql/mysql/protocol/StmtPrepareOKPacket.rssrc/sql_jsc/mysql/MySQLConnection.rstest/internal/source-lints/dead-symbols-install-sql-bindings.test.ts
💤 Files with no reviewable changes (36)
- src/js/internal/http1_server_fallback.ts
- scripts/build/flags.ts
- src/js/internal/streams/iter/utils.ts
- src/js/internal/sql/query.ts
- src/jsc/bindings/sqlite/JSSQLStatement.cpp
- src/runtime/bake/BakeGlobalObject.cpp
- src/jsc/bindings/headers-handwritten.h
- src/jsc/bindings/JSBakeResponse.cpp
- src/install/windows-shim/bun_shim_impl.rs
- src/dns/lib.rs
- src/jsc/bindings/NodeFSStatFSBinding.cpp
- src/js/internal/inspector/cdp.ts
- src/sql_jsc/mysql/MySQLConnection.rs
- src/jsc/bindings/ImportMetaObject.cpp
- src/jsc/bindings/webcore/JSAbortSignal.cpp
- src/jsc/bindings/NodeValidator.h
- src/install/resolution.rs
- src/js/internal/streams/iter/from.ts
- src/install/lockfile/bun.lock.rs
- src/install/hosted_git_info.rs
- src/jsc/bindings/dh-primes.h
- src/js/internal/sql/shared.ts
- src/install/error.rs
- src/jsc/bindings/BunProcess.cpp
- src/install/pnpm.rs
- src/js/internal/promisify.ts
- src/js/internal/trace_events.ts
- src/js/internal/repl/node-shims.js
- src/js/internal/sql/sqlite.ts
- src/event_loop/EventLoopTimer.rs
- src/jsc/bindings/BunObject.h
- src/jsc/bindings/sliceAnsi.cpp
- src/install/migration.rs
- src/jsc/bindings/NodeValidator.cpp
- src/jsc/bindings/BunString.cpp
- src/js/internal/fixed_queue.ts
There was a problem hiding this comment.
Thanks for the fix in d5567c6 — the Representation::Ssh slice is now anchored correctly and guarded against going vacuous. I didn't find further issues, but this is a 43-file dead-code sweep across install, the event loop, DNS, MySQL wire structs, C++ JSC bindings, built-in JS exports, and build defines, and the PR itself demonstrates the risk profile (the weak-referenced Bun__errorInstance__finalize was caught only by the darwin LTO link). A maintainer should sign off on the breadth here.
What was reviewed:
- Confirmed the reworked
reprStart/reprEndslice withexpectguards can fail whenSsh,is reintroduced. - Spot-checked
BunString__toWTFString,WITH_BORINGSSL, andBUN_SINGLE_THREADED_PER_VM_ENTRY_SCOPE— no remaining references outside the new lint test. - Verified MySQL packet decoders still consume the same wire bytes (reads kept, only stores removed);
LocalInfileRequestnow uses#[derive(Default)]which zero-initspacket_sizeas before.
Extended reasoning...
Overview
This PR removes verified-unreferenced symbols across ~43 files: Rust enum variants and struct fields in bun_install, bun_event_loop, bun_dns, and the MySQL wire protocol; C++ host functions, forward declarations, and stale commented blocks in src/jsc/bindings/; unused export default entries in src/js/internal/; and four compile defines in scripts/build/flags.ts. It adds a source-lint test pinning each removal and a positive presence check for the WebKit weak-referenced finalizer.
Security risks
None identified. This is deletion-only apart from a one-line comment and the new lint test. No auth, crypto, permission, or input-validation surface is touched (the dh-primes.h change removes only an unused forward declaration).
Level of scrutiny
High. Individually each deletion is mechanical, but the aggregate spans build configuration, extern "C" link surface, and hot-path timer dispatch. The PR's own history shows why: Bun__errorInstance__finalize was initially removed and only restored after a platform-specific LTO link failure — exactly the class of reference (weak, cross-repo, platform-gated) that grep-based verification misses. The four removed build defines were checked against the pinned WebKit checkout per the description, but I could not independently verify that here (vendor/WebKit is not present in this checkout).
Other factors
- All three prior review threads (comment-cop, my vacuous-slice finding, CodeRabbit's presence-check request) are resolved with corresponding commits.
- The PR description explicitly lists three additional dead-but-not-removed items "pending maintainer judgment", which itself signals the author expects a human pass.
- Verification per the description includes
rust:check-allacross 10 targets, a fullbun bdbuild, and targeted smoke tests — reasonable coverage, but the breadth (43 files, ~430 lines removed) and the extern-C / build-define changes put this outside what I'll approve without a maintainer look.
|
CI state: the diff itself is green across lanes. The remaining red on debian 13 x64-asan is unrelated to this change: test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts is a pre-existing BoringSSL RSA keygen leak that also fails on main (reported separately), and test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js hit a JSC exception-scope assert during worker terminate that does not reproduce locally under an ASAN debug build in 60 runs, touches no code in this diff, and does not appear in recent builds. The other failures passed on retry. Ready for review. |
…nd TS input types - node-shims.js: remove kEmptyObject/isWritable/addAbortListener (zero requirers; main deleted them in #36970 and the source lint pins the removal) - repl.test.ts: Bun-owned completion test for require("node:") and import("node:") (node:test offered, node:undici/node:bun:ffi not), and module-typescript/commonjs-typescript rows in the --input-type eval matrix
2 participants
Removes code verified to have zero references across
src/,scripts/,test/, and freshly regeneratedbuild/debug/codegen/output. Every candidate was grepped for bare-name, quoted-string, and$-prefixed references before deletion; items referenced from generated bindings,.classes.tsfiles, attribute-macro exports (uws_callback(export = ...)), orextern "C"surfaces were left alone. The vendored WebKit tree is part of the reference scan as well:Bun__errorInstance__finalizewas initially removed here, then restored once the darwin LTO link surfaced its__attribute__((weak))reference from JSC'sErrorInstance.cpp(weak references satisfy non-LTO links silently).Rust
bun_install::Error: variantsFileTooBig,ProcessFdQuotaExceeded,ReadOnlyFileSystem,FileSystem,FileBusywere never constructed (the same-named live variants belong tobun_runtime's separate error enum;node_fs.rsmaps onto that one)hosted_git_info::Representation::Ssh: every ssh-flavored protocol maps toSshurlFromTextLockfileError::InvalidSemverplus its only mention, an unreachable match arm inbun.lock.rs(ParseError::InvalidSemverstays and is still produced)MigratePnpmLockfileError::{PnpmLockfileInvalidOverride, PnpmLockfileInvalidPatchedDependency}: never produced by the pnpm migrationFailReason::InvalidShimDataSize: no size check produces itbun_event_loop::EventLoopTimer::TimerCallbackstruct, itsTagvariant, and the dispatch arm inruntime/dispatch.rs: nothing ever constructed one, so the tag could never be dispatchedbun_dns::Family::Unix: neither the string map nor the JS numeric mapping yields it (AF_UNIXon the result path is a different, live match)OKPacket::{warnings, info, session_state_changes},EOFPacket::warnings,StmtPrepareOKPacket::warning_count,LocalInfileRequest::filename. The wire reads stay so packet parsing consumes the same bytes; only the dead stores and their zero-initializers are gone.C++ bindings
NodeValidator.cpp: host functionsjsFunction_validateString/jsFunction_validateFunction/jsFunction_validateBooleanand their declarations. Their$newCppFunctionbindings were removed in an earlier sweep (Remove dead code from FFI sys crates, bun_core, built-in JS, codegen, and build scripts #36937 removed the sibling trio); theV::validate*overloads they forwarded to are live and stay.ImportMetaObject.cpp:jsFunctionRequireResolveand its only calleefunctionRequireResolve(static, 76 lines; the liverequire.resolveis built elsewhere)BunString.cpp:BunString__toWTFString(no Rust-side caller; the regeneratedcpp.rsdrops the import)sliceAnsi.cpp: never-instantiatedstruct HyperlinkInfo(wrapAnsi.cpp'sHyperlinkStateis the live one)NodeFSStatFSBinding.cpp:getStatFSPrototype<bool>, a template with zero instantiationsfunctionBunPeek/functionBunPeekStatus(BunObject.h),callBakeResponse/constructBakeResponse(JSBakeResponse.cpp),jsSqlStatementGetHasMultipleStatements(JSSQLStatement.cpp),bn_set_words(dh-primes.h)ErrorCaptureStackTraceexperiment in BunProcess.cpp, thedeletePropertyblock in JSAbortSignal.cpp, thesetOnEachMicrotaskTickblock in BakeGlobalObject.cppBuilt-in JS internals
Export-default entries no requirer ever destructures (verified against every
require()site, C++getDirectlookups, andtest/imports of internal modules); backing functions that are still used in-file stay:internal/repl/node-shims.js:isWritable,runScriptInThisContext,kEmptyObject,addAbortListener,promisify(none of repl.js / internal/repl/* touch them)internal/streams/iter/from.ts:normalizeAsyncSource,normalizeSyncSource,normalizeSyncValue,primitiveToUint8Arrayinternal/sql/sqlite.ts:SQLCommand,commandToString,parseSQLQuery,SQLiteQueryHandle(sole requirer pulls onlySQLiteAdapter)internal/sql/shared.ts:parseDefinitelySqliteUrl,buildDefinedColumnsAndQuery,normalizeSSLModeinternal/http1_server_fallback.ts:createHttp1FallbackResponseHandle,kHttp1ActiveRequestssetTid(trace_events),FixedCircularBuffer(fixed_queue),EXECUTION_CONTEXT_ID(inspector/cdp),defineCustomPromisify(promisify),SQLQueryStatus(sql/query),allUint8Array(streams/iter/utils)Build config and orphaned files
scripts/build/flags.ts: definesIS_BUILD,WITH_BORINGSSL=1,STATICALLY_LINKED_WITH_BMALLOC=1,BUN_SINGLE_THREADED_PER_VM_ENTRY_SCOPE=1have zero readers insrc/,packages/, or the pinned WebKit checkout (WebKit reads the lowercaseSTATICALLY_LINKED_WITH_bmalloc, which is not what we were defining)src/runtime/ffi/libtcc1.a.macos-aarch64: prebuilt 30KB archive from 2022 with zero references (libtcc1.cis embedded viainclude_bytes!and compiled at runtime)Verification
cargo check --workspaceandbun run rust:check-all(10 ok, 0 failed) so platform-gated uses would have surfacedbun bddebug build, which regenerates codegen and relinks the C++ sidetest/js/bun/repl/repl.test.ts(148 pass),test/cli/install/migration/migrate.test.ts(20 pass),test/js/sql/wire-frames.test.ts,test/js/sql/sql-mysql-clean-reentry.test.tsagainst MariaDB,test/js/sql/adapter-override.test.ts, fixed-queue node tests, plus module-load smokes for repl/stream/trace_events/http2/utiltest/internal/source-lints/suite passes, including the newdead-symbols-install-sql-bindings.test.tsthat pins these removalsCharacterSetcollation table plus theBun__CryptoHasherExtern__*helpers were left alone after verification showed they are referenced (label()at MySQLConnection.rs:661, C++ wrappers in CryptoUtil.cpp)Verified-dead but deliberately not removed (for a future pass, pending maintainer judgment)
read_without_launch/FromBunShellContext(~120 lines): zero callers, but the crate docs describe it as the staged in-process path for the shellsrc/runtime/bake/incremental_visualizer.html+memory_visualizer.html(~808 lines): the/_bun/incremental_visualizerroute that served them was never ported from the Zig dev server, while the websocket topic plumbing they rely on is live and testedsrc/jsc/bindings/webcrypto/*.idl(29 files, ~1055 lines): nothing in the build reads.idl, but they are maintained alongside the handwritten bindings as spec reference (SubtleCrypto.idl was edited in July)[review] gate passed · iteration 2 · 43 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 2
evidence per changed file