Remove dead code from napi/NodeVM C++, zlib, io event-loop, net.ts - #36742
Conversation
…io, net.ts
C++ (src/jsc/bindings/):
- napi: generateSourceCode() (26 LOC, zero callers); NapiWeakValue
isSet/isPrimitive/isCell/isString/cell()/primitive()/string() accessors
(23 LOC, only get() is used); NapiClass::destroy (unreachable with
DoesNotNeedDestruction)
- NodeVM: NodeVMGlobalObject::clearContextifiedObject,
NodeVMModuleRequest::addImportAttribute + specifier() setter,
NodeVM{SourceText,Synthetic}Module::hasModuleRecord,
NodeVMScript::cachedBytecode (all zero callers)
- JSBufferList: empty Constructor::initializeProperties, unreachable destroy
- JSStringDecoder: Constructor::initializeProperties (never called;
finishCreation does the same work)
- BunClientData: JSVMClientData's duplicate outputConstraintSpaces/
forEachOutputConstraintSpace/m_outputConstraintSpaces (only JSHeapData's
copies are called by BunGCOutputConstraint and subspaceForImpl)
- EventLoopTask: m_isCleanupTask field + isCleanupTask() + CleanupTaskTag
constructor (field written but never read; DeleteCallbackDataTask now uses
the primary templated constructor)
- JSEnvironmentVariableMap: jsSetterEnvironmentVariable (registered with
nullptr setter; only referenced in a comment)
- JSNextTickQueue::initialValues (never invoked by finishCreation)
- JSBuffer.cpp: commented-out jsBufferPrototypeToStringWithoutTypeChecks
DOMJIT block (23 LOC, unchanged since Jan 2025)
- blob.h: unused BlobRef type alias
- Bindgen/IDLTypes.h: unused IsIDLStrongAny trait
libuv (src/jsc/bindings/libuv/uv/):
- aix.h (32), os390.h (33), sunos.h (44), posix.h (31): bun never targets
AIX/zOS/Solaris/IBMi/Cygwin/QNX/Haiku/Hurd. Kept bsd.h (FreeBSD is a
real build profile). Matching #elif arms removed from unix.h and
dead comment block removed from uv-posix-polyfills.c.
Rust:
- src/zlib/error.rs (whole file, 31 LOC): unused Error/Result; the crate
uses ZlibError defined in lib.rs instead
- src/zlib/lib.rs: drop Byte/gzFile/struct_gzFile_s/voidpf from the
bun_zlib_sys re-export (zero Rust-side references)
- src/io/windows_event_loop.rs: FilePoll::ref_/activate/can_ref
(30 LOC, zero callers across all targets; verified with rust:check-all)
- src/install/lockfile: commented-out Stringifier::save stub in bun.lock.rs;
empty #[cfg(debug_assertions)] blocks in Buffers.rs left by commented-out
debug prints
TypeScript:
- src/js/node/net.ts: kpendingRead and kServerSocket Symbols (defined,
written once each, never read)
Verified with bun bd (builds clean) and bun run rust:check-all (10/10
targets pass).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe pull request removes unused APIs, members, platform branches, debug comments, and dead implementation paths across Rust, TypeScript, C++, and C. It adds a source-tree lint that prevents reintroduction of the removed symbols. ChangesDead code and API surface cleanup
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}
![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-napi-nodevm-libuv.test.ts`:
- Around line 76-83: Remove the deleted-header existsSync assertions in
test/internal/source-lints/dead-symbols-napi-nodevm-libuv.test.ts lines 76-83,
while retaining the uv/unix.h include assertions. Also remove the
src/zlib/error.rs existence check at lines 92-94, retaining the src/zlib/lib.rs
module-export assertion; rely on surviving-source checks instead.
🪄 Autofix (Beta)
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: 101496d1-7db5-4312-87f6-75ddfbf2fe93
📒 Files selected for processing (34)
src/install/lockfile/Buffers.rssrc/install/lockfile/bun.lock.rssrc/io/windows_event_loop.rssrc/js/node/net.tssrc/jsc/bindings/Bindgen/IDLTypes.hsrc/jsc/bindings/BunClientData.hsrc/jsc/bindings/DeleteCallbackDataTask.hsrc/jsc/bindings/EventLoopTask.hsrc/jsc/bindings/JSBuffer.cppsrc/jsc/bindings/JSBufferList.cppsrc/jsc/bindings/JSBufferList.hsrc/jsc/bindings/JSEnvironmentVariableMap.cppsrc/jsc/bindings/JSNextTickQueue.hsrc/jsc/bindings/JSStringDecoder.cppsrc/jsc/bindings/JSStringDecoder.hsrc/jsc/bindings/NodeVM.cppsrc/jsc/bindings/NodeVM.hsrc/jsc/bindings/NodeVMModule.cppsrc/jsc/bindings/NodeVMModule.hsrc/jsc/bindings/NodeVMScript.hsrc/jsc/bindings/NodeVMSourceTextModule.hsrc/jsc/bindings/NodeVMSyntheticModule.hsrc/jsc/bindings/blob.hsrc/jsc/bindings/libuv/uv/aix.hsrc/jsc/bindings/libuv/uv/os390.hsrc/jsc/bindings/libuv/uv/posix.hsrc/jsc/bindings/libuv/uv/sunos.hsrc/jsc/bindings/libuv/uv/unix.hsrc/jsc/bindings/napi.cppsrc/jsc/bindings/napi.hsrc/jsc/bindings/uv-posix-polyfills.csrc/zlib/error.rssrc/zlib/lib.rstest/internal/source-lints/dead-symbols-napi-nodevm-libuv.test.ts
💤 Files with no reviewable changes (30)
- src/jsc/bindings/JSStringDecoder.h
- src/jsc/bindings/libuv/uv/sunos.h
- src/jsc/bindings/Bindgen/IDLTypes.h
- src/jsc/bindings/uv-posix-polyfills.c
- src/jsc/bindings/EventLoopTask.h
- src/jsc/bindings/NodeVMSyntheticModule.h
- src/jsc/bindings/NodeVMScript.h
- src/jsc/bindings/libuv/uv/aix.h
- src/jsc/bindings/NodeVM.cpp
- src/jsc/bindings/JSBufferList.cpp
- src/jsc/bindings/NodeVMSourceTextModule.h
- src/jsc/bindings/NodeVM.h
- src/jsc/bindings/blob.h
- src/jsc/bindings/BunClientData.h
- src/jsc/bindings/JSNextTickQueue.h
- src/jsc/bindings/NodeVMModule.h
- src/jsc/bindings/libuv/uv/posix.h
- src/jsc/bindings/JSBuffer.cpp
- src/jsc/bindings/NodeVMModule.cpp
- src/jsc/bindings/libuv/uv/unix.h
- src/install/lockfile/bun.lock.rs
- src/jsc/bindings/napi.cpp
- src/js/node/net.ts
- src/zlib/error.rs
- src/jsc/bindings/JSStringDecoder.cpp
- src/io/windows_event_loop.rs
- src/jsc/bindings/libuv/uv/os390.h
- src/install/lockfile/Buffers.rs
- src/jsc/bindings/napi.h
- src/jsc/bindings/JSBufferList.h
…lint - JSEnvironmentVariableMap.cpp: the comment in jsSetterProxyEnvironmentVariable originally referenced the deleted jsSetterEnvironmentVariable; tighten it to describe the actual CustomValue-replacement behaviour instead of naming a function that no longer exists (and was never registered anyway). - dead-symbols-napi-nodevm-libuv.test.ts: assert against surviving source (unix.h includes, lib.rs mod declaration) instead of existsSync() on deleted paths. The verification harness's stash/restore step can leave deleted files on disk, and an unreferenced header/module on disk is harmless.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The commented-out debug prints removed in 9d58ed1 were the only readers of the three #[cfg(debug_assertions)] let _pos = stream.pos; bindings in load_generic_field! and the trees/dependencies load blocks.
io/windows_event_loop.rs:
- declare_scope!(FilePoll)/declare_scope!(KeepAlive): the only
scoped_log!(FilePoll, ..) calls were in the removed ref_/activate
methods; the KeepAlive scope was already orphaned (its callers moved
to crate::keep_alive).
- disable_keeping_process_alive doc: reworded to match the posix
counterpart instead of naming the removed ref().
io/posix_event_loop.rs:
- FilePoll::ref_: the equally-dead posix sibling of the removed Windows
method. All .ref_(ctx) call sites in the tree dispatch on KeepAlive;
FilePoll callers use enable_keeping_process_alive directly.
jsc/bindings/napi.h:
- namespace JSC { JSGlobalObject; JSSourceCode; } forward-decl block:
scaffolding for the removed generateSourceCode declaration.
JSGlobalObject is already provided via ZigGlobalObject.h; JSSourceCode
has zero uses in the header.
jsc/bindings/NapiRef.cpp:
- dropped the two isSet() comment lines that reference the removed
NapiWeakValue::isSet(); kept the .setString/.setObject assertion note
and the weakValueRef retention rationale.
…nitialValues() Siblings of the removed JSNextTickQueue::initialValues() with the same justification: both finishCreation bodies set every internal field directly and never call initialValues(). BunStreamSource's copy is kept (it has a live caller at BunStreamSource.cpp:57).
… touched - io/posix_event_loop.rs: declare_scope!(KeepAlive) (zero scoped_log! callers, posix sibling of the windows one removed in 6c7b226) - zlib/lib.rs: #[allow(non_camel_case_types, unused_imports)] above the trimmed pub use (all four remaining names are used in extern signatures; neither lint fires on a pub use anyway) - libuv/uv/unix.h: #if !defined(__MVS__) wrapper and the defined(_AIX) clause in the barrier guard (both reference platforms this PR already removed)
…stale uv_barrier_init comment The thiserror derive on the deleted Error enum was the crate's only consumer. The unix.h comment referenced src/unix/thread.c which bun never vendored (uv_barrier_init is a __bun_throw_not_implemented stub in uv-posix-stubs.c), and the _AIX clause removal in c52ae88 made the 'should match' claim false against upstream anyway.
There was a problem hiding this comment.
No correctness concerns after five review rounds — every deletion I spot-checked (EventLoopTask/DeleteCallbackDataTask constructor swap, NapiClass::destroy under DoesNotNeedDestruction, jsSetterEnvironmentVariable registered as nullptr, the three initialValues() statics, net.ts private Symbol()s) is provably unreferenced and the build/rust:check-all gates hold. Deferring only on breadth: 40 files across napi/NodeVM/JSC bindings, event-loop internals, and vendored libuv headers is more than I want to auto-approve, and the last unix.h reply explicitly asks for a human pass on that file.
What was reviewed:
- Checked
DeleteCallbackDataTaskstill binds the templatedEventLoopTask(T)ctor afterCleanupTaskTagremoval — identical body,m_isCleanupTaskwas write-only. - Verified
uv-posix-polyfills-posix.c(FreeBSD arm) is distinct from the deleteduv/posix.h; no dangling include. - Confirmed
JSVMClientData::m_outputConstraintSpacesduplicates were unused — both live callers go throughheapData(). - All prior inline findings (sibling
initialValues(), posixKeepAlivescope,__MVS__/_AIXguards,thiserrordep, stale unix.h comment) applied and resolved.
Extended reasoning...
Overview
Recurring dead-code sweep: ~416 deletions / 6 insertions across 40 files. C++ JSC bindings (napi, NodeVM*, JSBufferList, JSStringDecoder, BunClientData, EventLoopTask, JSEnvironmentVariableMap, blob, Bindgen/IDLTypes, JSBuffer), vendored libuv platform headers (aix/os390/sunos/posix.h deleted, unix.h/uv-posix-polyfills.c trimmed), Rust (src/zlib/error.rs whole-file, zlib re-exports, io/{posix,windows}_event_loop FilePoll methods + declare_scope!, lockfile debug scaffolding), TypeScript (net.ts write-only symbols), plus a source-lint regression test and Cargo.toml/Cargo.lock. Five follow-up commits addressed review feedback about orphaned scaffolding (siblings, stale comments, dead #[allow]/deps).
Security risks
None. Pure deletions of unreferenced symbols; no auth/crypto/parsing/validation touched. The only non-deletion is a 2-line comment condensation in JSEnvironmentVariableMap.cpp (shortening prose that referenced the now-deleted jsSetterEnvironmentVariable) and the DeleteCallbackDataTask constructor-argument change, which is behavior-identical.
Level of scrutiny
Medium-low per hunk (each is a grep-verified zero-caller removal backed by clean bun bd + rust:check-all 10/10), but high in aggregate: 40 files spanning napi (native-module ABI surface), NodeVM, JSC GC method-table shapes (destroy, initialValues), event-loop refcounting, and vendored libuv headers. REVIEW.md flags vendored-dep edits and JSC class shapes as areas needing extra care. The individual deletions are mechanical; the concern is breadth, not any single change.
Other factors
- Bug hunter found nothing on the current revision.
- I left five prior rounds of inline nits (all "sibling also dead" / "orphaned scaffolding") — every one applied and resolved.
- Robobun's final reply on
unix.hexplicitly requests a human pass before further edits to the vendored header, which aligns with deferring here. - The new
test/internal/source-lints/test is a source-tree regex lint (doesn't exercise the binary), which is appropriate for a deletion sweep but doesn't add behavioral coverage — smoke tests on vm/string_decoder/net/process-env are cited as unchanged. - No CODEOWNERS check performed; napi/NodeVM/libuv are areas a maintainer typically owns.
There was a problem hiding this comment.
No issues found; all seven prior review rounds' findings are resolved. Deferring for a human pass mainly on scope: 40 files across napi / NodeVM / event-loop (both platforms) plus edits to the vendored libuv posix headers (unix.h diverged from upstream, four platform headers deleted) — the author also flagged that header for a human look after the last micro-edit.
What was reviewed
EventLoopTaskCleanupTaskTagremoval:DeleteCallbackDataTask's lambda dispatches to the surviving templated ctor with identical semantics.destroy()removals onNapiClass/JSBufferList: both classes areDoesNotNeedDestruction, so the methods were unreachable.initialValues()removals: static, no template/method-table caller inJSInternalFieldObjectImpl;BunStreamSource's live instance is kept.kServerSocket/kpendingRead,jsSetterEnvironmentVariable,isCleanupTask: zero remaining references insrc/.
Extended reasoning...
Overview
Recurring dead-code sweep: ~416 deletions / 6 insertions across 40 files. C++ (src/jsc/bindings/: napi, NodeVM*, JSBufferList, JSStringDecoder, BunClientData, EventLoopTask, JSEnvironmentVariableMap, JSNextTickQueue, ModuleLoader, JSMockFunction, blob, Bindgen/IDLTypes, JSBuffer), vendored libuv posix headers (deletes aix.h/os390.h/sunos.h/posix.h, edits unix.h and uv-posix-polyfills.c), Rust (zlib/error.rs deleted, zlib/Cargo.toml drops thiserror, io/{posix,windows}_event_loop.rs FilePoll methods and declare_scope!s, lockfile debug scaffolding), TypeScript (net.ts write-only symbols), and a new source-lint test pinning each removal.
Security risks
None. Pure deletions of uncalled code; the only non-deletion additions are the source-lint test and a two-line comment shrink. No auth/crypto/permissions surface touched.
Level of scrutiny
Medium-high, driven by breadth rather than depth. Each individual removal is mechanical and independently verifiable (and was verified by bun bd + rust:check-all 10/10 + smoke tests on vm/string_decoder/net/process-env). But the sweep spans napi refcounting internals, JSC JSInternalFieldObjectImpl subclasses, both event-loop platform files, and — the main reason to defer — modifies vendored libuv headers, which future upstream syncs will have to reconcile. REVIEW.md's "Dependencies & vendoring" section flags vendor edits for a situational read, and robobun's own last comment on unix.h asked that further edits there wait for a human.
Other factors
- Seven review iterations (robobun + prior claude runs), all resolved; every finding was a completeness nit (siblings/scaffolding), not a correctness bug. The bug-hunting system found nothing this run.
- Spot-checked the one behaviour-adjacent change (
DeleteCallbackDataTaskre-routing to the primaryEventLoopTaskctor): the templated ctor's SFINAE (is_convertible<T, Function<void(ScriptExecutionContext&)>>) accepts the lambda and its body is byte-identical to the removedCleanupTaskTagctor. - Verified
JSBufferList::destroy(JSCell*) {}andNapiClass::destroywere unreachable: both classes areDoesNotNeedDestruction(the former viaJSNonFinalObject, the latter explicitly), andsubspaceForImpl<…, UseCustomHeapCellType::No>'sstatic_assertconfirms neither uses a destructing heap cell type — so the removed empty body was never dispatched. - Grepped
src/for remaining references tokServerSocket/kpendingRead/isCleanupTask/CleanupTask/jsSetterEnvironmentVariable: none. - No CODEOWNERS on any touched path.
|
CI status: build #87539 for 0a3dcec is finished: 193 jobs passed, 0 The only hard failure is
Local verification: |
|
Updated 12:52 AM PT - Aug 2nd, 2026
@Jarred-Sumner, your commit f32c435 is building: |
CI build 87459 crashed sql-close-pending-connection.test.ts on 6 lanes (debian x64-asan, ubuntu aarch64, alpine x64/aarch64, windows x64/aarch64) with an AllocatingScope assertion inside JSC::LocalAllocator::allocateSlowCase. Stack: JSPromise allocation -> GC slow path -> Heap::sweepInFinalize -> Listener::finalize -> deinit -> SocketGroup::close_all -> on_close -> JSValue::call -> allocation during sweep -> assert. The only net.ts change in this PR was removing kServerSocket (defined + written once, never read). The write clientHandle[kServerSocket] = handle stores the server's native Listener handle on each accepted socket's handle, which is a GC edge: the Listener stays reachable while any accepted socket handle is alive. server.close() in node:net stops accepting but does not close existing connections, so after a test's server local goes out of scope the accepted socket is the only thing keeping the Listener out of the finalizer. Without that edge the Listener is finalized while its socket group still holds the accepted socket; Listener::finalize -> close_all fires the socket's JS on_close callback during the sweep, which asserts. kServerSocket is restored with a comment documenting why it exists. kpendingRead stays removed (its only write was this[kpendingRead] = undefined, which retains nothing).
There was a problem hiding this comment.
No new issues found this pass — all earlier nits are applied and resolved. I'm not auto-approving because the sweep is broad (40 files across napi/NodeVM/JSC bindings, libuv vendored headers, event-loop Rust, net.ts) and already needed one revert (kServerSocket as a GC retention edge, a93d2fa), so a human sign-off on the remaining deletions is worth having.
Checked this round: DeleteCallbackDataTask still resolves to the templated EventLoopTask ctor after CleanupTaskTag removal; NapiClass::destroy/JSBufferList::destroy are unreachable under DoesNotNeedDestruction; the three initialValues() removals are static with no bytecode-intrinsic caller; uv-posix-polyfills-posix.c remains reachable via the __FreeBSD__ arm after the Cygwin/Haiku/QNX branch drop.
Extended reasoning...
Overview
Recurring dead-code sweep: 416 deletions / 6 insertions across 40 files. C++ JSC bindings (napi, NodeVM, JSBufferList/JSStringDecoder, BunClientData, EventLoopTask, JSEnvironmentVariableMap, three JSInternalFieldObjectImpl::initialValues() sites, blob.h, IDLTypes.h, JSBuffer.cpp commented block), four vendored libuv platform headers deleted plus edits to unix.h/uv-posix-polyfills.c, Rust (zlib/error.rs + Cargo.toml/lock, windows_event_loop.rs/posix_event_loop.rs FilePoll methods and log scopes, lockfile debug scaffolding), and net.ts (kpendingRead removed; kServerSocket restored with a two-line rationale after a CI crash). A source-lint regression test pins each deletion.
Security risks
None identified. Pure deletions of unreferenced symbols, commented-out blocks, and platform headers for OSes bun does not target. No auth/crypto/permissions/parsing paths touched. The one behavior-adjacent change — DeleteCallbackDataTask switching from the CleanupTaskTag ctor to the templated ctor — is byte-equivalent since m_isCleanupTask was never read.
Level of scrutiny
Moderate-to-high, warranting human review rather than bot approval. Individually each deletion is well-argued (grep evidence in the description, build + rust:check-all 10/10, smoke tests on vm/string_decoder/net/process env), but the sweep's own history on this PR shows the "zero readers" heuristic is not always sufficient: kServerSocket looked write-only yet its write was a load-bearing GC edge, caught only by CI crashes on 6 lanes. Several remaining deletions sit in similarly subtle territory — destroy(JSCell*) on GC types, JSInternalFieldObjectImpl subclass statics, napi weak-value accessors, Windows event-loop refcounting — where a maintainer familiar with each subsystem should confirm nothing depends on these via macro expansion, JSC method-table conventions, or planned work.
Other factors
I have already left five rounds of inline nits on this PR (sibling-site completeness, orphaned scaffolding, stale comments, the thiserror dep), all applied and resolved. The bug-hunting system found nothing new this run. The added source-lint test is a reasonable guard against reintroduction, though it is a text-match test rather than a behavioral one. CI build #87527 was still running at the last timeline update; final green on that build (particularly the Windows lanes for the windows_event_loop.rs changes) should be confirmed before merge. Given the breadth, the vendored-header deletions, and the one demonstrated near-miss, deferring to a human is the right call.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
don't modify vendored libuv headers
Restores src/jsc/bindings/libuv/uv/{aix,os390,sunos,posix,unix}.h and
src/jsc/bindings/uv-posix-polyfills.c to main. Vendored headers should
not be modified by dead-code sweeps.
|
Reverted all libuv header changes in 4dd5a99: Remaining diff is now 34 files (+97 / -311): C++ JSC bindings (napi, NodeVM, JSBufferList/JSStringDecoder, BunClientData, EventLoopTask, JSEnvironmentVariableMap, ModuleLoader, JSMockFunction, JSNextTickQueue, blob, Bindgen/IDLTypes, JSBuffer commented block), Rust (zlib/error.rs + Cargo.toml, io event-loop FilePoll methods and log scopes, lockfile debug scaffolding), and net.ts (kpendingRead only; kServerSocket kept per a93d2fa). |
robobun
changed the title
There was a problem hiding this comment.
No bugs found and all prior nits addressed in 0a3dcec, but deferring to a human given the breadth: 34 files across napi, NodeVM, event-loop, and JSC GC bindings, and this PR already surfaced one non-obvious GC-retention edge (kServerSocket, reverted in a93d2fa).
What was reviewed:
- Verified
initialValues()removals — onlyBunStreamSourcestill has a live caller; the three removed classes set fields directly infinishCreation. - Verified
JSVMClientData::m_outputConstraintSpacesduplicates — both live callers (BunGCOutputConstraint.cpp:126,BunClientData.h:218) go throughheapData, neverJSVMClientData. - Verified
jsSetterEnvironmentVariableandCleanupTaskTag/isCleanupTaskhave zero remaining references;DeleteCallbackDataTasknow uses the primary constructor with identical behaviour. NapiClass::destroy/JSBufferList::destroyare unreachable givenDoesNotNeedDestruction;FilePoll::ref_/activate/can_refhave no callers (all.ref_()sites dispatch on other types) and build passes 10/10 targets.
Extended reasoning...
Overview
Dead-code sweep removing ~311 lines across 34 files: unused C++ methods/members in src/jsc/bindings/ (napi, NodeVM, JSBufferList, JSStringDecoder, BunClientData, EventLoopTask, JSEnvironmentVariableMap, ModuleLoader, JSMockFunction, JSNextTickQueue, blob, Bindgen/IDLTypes, JSBuffer commented DOMJIT block), Rust dead items in src/zlib/, src/io/{posix,windows}_event_loop.rs, src/install/lockfile/, and one write-only private Symbol in src/js/node/net.ts. A source-lint regression test is added. The libuv header changes were reverted (4dd5a99) and kServerSocket was restored (a93d2fa) after CI caught a GC-retention crash.
Security risks
None identified. No auth/crypto/permission surfaces touched. The jsSetterEnvironmentVariable deletion is safe — it was never registered (the accessor was created with a nullptr setter), so there is no behaviour change to process.env write semantics. NodeVM changes remove uncalled accessors only (clearContextifiedObject, hasModuleRecord, etc.) and do not alter the sandbox boundary.
Level of scrutiny
Moderate-to-high. Each individual deletion is mechanical (grep-verified zero callers, C++/Rust build proves it), and the bug-hunting system found nothing on the final state. But the cumulative surface spans napi weak-value accessors, JSC destroy() static methods, GC output-constraint-space bookkeeping on JSVMClientData, and event-loop FilePoll lifetime methods. These are all memory-safety-adjacent, and this PR already demonstrated the risk class once: the initial kServerSocket removal looked write-only but was a GC retention edge, crashing 6 CI lanes. That was caught and reverted, but it argues for a human pass over the remaining deletions rather than a bot approval.
Other factors
- All my prior inline nits (posix
declare_scope!(KeepAlive), zlib#[allow],thiserrordep, source-lint posix entry, test filename) were applied. - No human reviewers have commented; all timeline participants are bots.
- I spot-checked the higher-risk deletions:
initialValues()is not called via any JSC template (onlyBunStreamSourceuses it, and it was kept);outputConstraintSpacesonJSVMClientDatahas no callers (both live sites dispatch onJSHeapData);isCleanupTask/CleanupTaskTaghave zero references and the sole consumer was updated in-diff. - The final commit (0a3dcec) is building on BuildKite #87539; I did not see confirmation of a green result in the timeline.
…C++ bindings (#36756) Removes 741 net LOC of unreferenced C++ from `src/jsc/bindings/` and `src/jsc/bindings/webcore/`. Every symbol was verified to have zero callers across `src/` and `build/debug/codegen/`, and the full debug build links cleanly. No overlap with any open dead-code PR (#35437, #35559, #35775, #35880, #36115, #36178, #36237, #36318, #36621, #36742). ### Whole files deleted - `webcore/DOMJITCheckDOM.h` (98 LOC): only includer was `JSEventDOMJIT.cpp` - `webcore/JSEventDOMJIT.cpp` (43 LOC): defined `checkSubClassSnippetForJSEvent`, whose sole reference in `JSEvent.cpp:242` was behind `#if 0` (nullptr used instead) - `webcore/DOMJITHelpers.cpp` (57 LOC): every function body was already commented out; compiled to an empty namespace - `webcore/JSDOMConvertSerializedScriptValue.h` (50 LOC): only includer was the `JSDOMConvert.h` umbrella; `IDLSerializedScriptValue<>` was never instantiated anywhere ### webcore/DOMJITHelpers.h Removed the entire `WebCore::DOMJIT` namespace body (~184 LOC: `branchIf*`, `toWrapper`, `tryLookUpWrapperCache`, `operationToJSNode`/`operationToJSContainerNode` declarations, and ~60 LOC of commented-out helpers). All 7 remaining includers (`generate-classes.ts` output, `JSBuffer.cpp`, `JSPerformance.cpp`, `JSTextEncoder.cpp`, `JSFFIFunction.cpp`, `JSSQLStatement.cpp`, `ZigGeneratedCode.cpp`) use only `JSC::DOMJIT::*` from JavaScriptCore headers, never `WebCore::DOMJIT::*`. The transitive `#include`s are kept. ### webcore/EventContext.{h,cpp} Removed `handleLocalEvents`, `node()`, `relatedTarget()`, `setRelatedTarget`, `isMouseOrFocusEventContext`, `isTouchEventContext`, `isWindowContext`, `isUnreachableNode`, the `(Type, Node&, ...)` constructor overload, the `Type` enum and `m_type` field, `m_relatedTarget`, `m_contextNodeIsFormElement`, and all `TOUCH_EVENTS` / commented-out blocks. Only `currentTarget()` / `closedShadowDepth()` / `target()` are reachable (via `EventPath::computePathUnclosedToTarget`). ### webcore/EventPath.{h,cpp} Removed the empty `EventPath(Node&, Event&)` constructor, `contextAt`, `eventTargetRespectingTargetRules`, the `buildPath` / `setRelatedTarget` declarations (never defined), the `Touch` forward decl and `TOUCH_EVENTS` block. ### webcore/EventListenerMap.{h,cpp} Removed `removeFirstEventListenerCreatedFromMarkup`, `copyEventListenersNotCreatedFromMarkupToTarget`, and their file-local static helpers. WebKit markup-listener transfer helpers with zero callers in Bun. ### ErrorCode.{h,cpp} - `Bun::toJS(JSGlobalObject*, ErrorCode)`: declared, never defined, never called - `INVALID_FILE_URL_HOST(..., const ASCIILiteral)` overload: not declared in the header, so the two call sites in `BunObject.cpp` bind to the `const WTF::String&` overload - `CRYPTO_JWK_UNSUPPORTED_CURVE(..., const WTF::String&)` overload: the only call site in `KeyObject.cpp` passes `(ASCIILiteral, const char*)`, matching the other overload - `Message::ERR_INVALID_ARG_TYPE(..., const ZigString*, const ZigString*, JSValue)` overload: zero callers ### DOMException.{h,cpp} Removed `create(const Exception&)` (zero callers) and the static `name(ExceptionCode)` / `message(ExceptionCode)` helpers (zero callers; `description(ec).name` is used directly where needed). ### CookieMap.{h,cpp} Removed `struct CookieStoreGetOptions` (zero references), `getAll()` (not in the `JSCookieMap` prototype table; `toJSON()` enumerates directly), and the private `CookieMap(Vector<Ref<Cookie>>&&)` constructor (zero `adoptRef` sites use it). ### DOMFormData.{h,cpp} Removed `clone()`; zero callers. ### Single-line declarations - `Cookie.h`: `isValidCookieValue` (declared, never defined; the trailing comment already said "this isn't needed") - `ImportMetaObject.h`: `createRequireFunction` (declared, never defined) - `JSCommonJSModule.h`: `setSourceCode` (declared, never defined), `clearSourceCode`, `idOrDot` - `Sink.h`: `numberOfSinkIDs` constexpr - `ProcessBindingTTYWrap.cpp`: duplicate forward declaration of `Process_functionInternalGetWindowSize` (already declared via `JSC_DECLARE_HOST_FUNCTION` in the header) ### Also scanned, nothing confidently dead `src/http/`, `src/ast/`, `src/semver/`, `src/event_loop/`, `src/bun_core/`, `src/threading/`, `src/runtime/bake/dev_server/`, `src/js/thirdparty/`. All recently swept and clean. ### Intentionally not touched (possible followups) - `InspectorHTTPServerAgent::{requestWillBeSent,responseReceived,bodyChunkReceived,requestFinished,requestHandlerException}` and `InspectorBunFrontendDevServerAgent::{clientErrorReported,graphUpdate}`: look like in-progress inspector scaffolding with matching Rust-side extern declarations; left alone - `webcore/streams/CrossRealmTransform.cpp` stubs: explicitly documented as frozen-ABI placeholders for transferable streams - `JSEventListener::wasCreatedFromMarkup()` and `m_wasCreatedFromMarkup`: now the only readers are gone, but removing the bitfield changes class layout; left for a separate pass - `webcore/ResourceLoadTiming.h`: only includers are `ResourceTiming.{h,cpp}` which #36621 modifies; avoided to prevent merge conflicts ### Verification - `rg -w <symbol> src/ build/debug/codegen/` returned only the definition for every removed item - `bun bd` builds and links - Smoke tests: `test/js/bun/cookie/cookie-map.test.ts`, `test/js/bun/globals.test.js`, `test/js/web/abort/abort.test.ts`, `test/js/web/fetch/body.test.ts -t FormData` all pass - `test/internal/source-lints/dead-symbols-domjit-eventpath-errorcode.test.ts` asserts the removed symbols do not reappear <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 4 · 29 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 3 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/dead-symbols-domjit-eventpath-errorcode.test.ts bun test v1.4.0 (1752533) test/internal/source-lints/dead-symbols-domjit-eventpath-errorcode.test.ts: 28 | ["src/jsc/bindings/webcore/JSDOMConvert.h", /JSDOMConvertSerializedScriptValue\.h/], 29 | ["src/jsc/bindings/webcore/JSEvent.cpp", /checkSubClassSnippetForJSEvent/], 30 | ["src/jsc/bindings/webcore/JSEvent.h", /checkSubClassSnippetForJSEvent/], 31 | ]; 32 | const resurrected = checks.filter(([file, re]) => re.test(src(file))).map(([file, re]) => `${file}: ${re.source}`); 33 | expect(resurrected).toEqual([]); ^ error: expect(received).toEqual(expected) - [] + [ + "src/jsc/bindings/webcore/DOMJITHelpers.h: namespace DOMJIT\b", + "src/jsc/bindings/webcore/DOMJITHelpers.h: branchIfNotWorldIsNormal|branchIfNotEvent|operationToJSNode", + "src/jsc/bindings/webcore/JSDOMConvert.h: JSDOMConvertSerializedScriptValue\.h", + "src/jsc/bindings/webcore/JSEvent.cpp: checkSubClassSnippetForJSEvent", + "src/jsc/bind ... (truncated) release without fix: 3 FAILED bun test v1.4.0-canary.1 (8fc0aeb) test/internal/source-lints/dead-symbols-domjit-eventpath-errorcode.test.ts: 28 | ["src/jsc/bindings/webcore/JSDOMConvert.h", /JSDOMConvertSerializedScriptValue\.h/], 29 | ["src/jsc/bindings/webcore/JSEvent.cpp", /checkSubClassSnippetForJSEvent/], 30 | ["src/jsc/bindings/webcore/JSEvent.h", /checkSubClassSnippetForJSEvent/], 31 | ]; 32 | const resurrected = checks.filter(([file, re]) => re.test(src(file))).map(([file, re]) => `${file}: ${re.source}`); 33 | expect(resurrected).toEqual([]); ^ error: expect(received).toEqual(expected) - [] + [ + "src/jsc/bindings/webcore/DOMJITHelpers.h: namespace DOMJIT\b", + "src/jsc/bindings/webcore/DOMJITHelpers.h: branchIfNotWorldIsNormal|branchIfNotEvent|operationToJSNode", + "src/jsc/bindings/webcore/JSDOMConvert.h: JSDOMConvertSerializedScriptValue\.h", + "src/jsc/bindings/webcore/JSEvent.cpp: checkSubClassSnippetForJSEvent", + "src/jsc/bindings/webcore/JSEvent.h: checkSubClassSnippetForJSEvent", + ] - Expected - 1 + Received + 7 at <anonymous> (/workspace/bun/test/internal/source-lints/dead-symbols-domjit-eventpath-errorcode. ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/dead-symbols-domjit-eventpath-errorcode.test.ts bun test v1.4.0 (1752533) test/internal/source-lints/dead-symbols-domjit-eventpath-errorcode.test.ts: (pass) webcore DOMJIT dead files and helpers do not reappear [15.85ms] (pass) webcore EventPath/EventContext/EventListenerMap dead members do not reappear [19.37ms] (pass) misc C++ bindings dead declarations do not reappear [28.64ms] 3 pass 0 fail 3 expect() calls Ran 3 tests across 1 file. [2.08s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 645ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/83] gen ErrorCode+*.h [2/83] gen JSEvent.lut.h Generating /workspace/bun/build/release/codegen/JSEvent.lut.h from /workspace/bun/src/jsc/bindings/webcore/JSEvent.cpp [3/83] cxx obj/unified/UnifiedSource-src_jsc_bindings_node-0.cpp.o [4/83] cxx obj/unified/UnifiedSource-src_jsc_bindings_v8-0.cpp.o [5/83] cxx obj/unified/UnifiedSource-src_jsc_bindings_node_http-0.cpp.o [6/83] cxx obj/unified/UnifiedSource-src_jsc_bindings-5.cpp.o [7/83] gen cpp.rs (cppbind) [8/83] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 239 extern-C blocks audited [8/83] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/bindings/Cookie.h | 1 - src/jsc/bindings/CookieMap.cpp | 18 -- src/jsc/bindings/CookieMap.h | 7 - src/jsc/bindings/DOMException.cpp | 8 - src/jsc/bindings/DOMException.h | 6 - src/jsc/bindings/DOMFormData.cpp | 8 - src/jsc/bindings/DOMFormData.h | 1 - src/jsc/bindings/ErrorCode.cpp | 29 ---- src/jsc/bindings/ErrorCode.h | 2 - src/jsc/bindings/IDLTypes.h | 2 - src/jsc/bindings/ImportMetaObject.h | 2 - src/jsc/bindings/JSCommonJSModule.h | 5 - src/jsc/bindings/ProcessBindingTTYWrap.cpp | 2 - src/jsc/bindings/Sink.h | 2 - src/jsc/bindings/webcore/DOMJITCheckDOM.h | 98 +---------- src/jsc/bindings/webcore/DOMJITHelpers.cpp | 57 +------ src/jsc/bindings/webcore/DOMJITHelpers.h | 185 --------------------- src/jsc/bindings/webcore/EventContext.cpp | 34 ---- src/jsc/bindings/webcore/EventContext.h | 116 +------------ src/jsc/bindings/webcore/EventListenerMap.cpp | 45 ----- src/jsc/bindings/webcore/EventListenerMap.h | 5 - src/jsc/bindings/webcore/EventPath.cpp | 18 +- src/jsc/bindings/webcore/EventPath.h | 37 ----- src/jsc/bindings/webcore/JSDOMConvert.h | 1 - .../webcore/JSDOMConvertSerializedScriptValue.h | 50 +----- src/jsc/bindings/webcore/JSEvent.cpp | 10 +- src/jsc/bindings/webcore/JSEvent.h | 4 - src/jsc/bindings/webcore/JSEventDOMJIT.cpp | 43 +---- ...dead-symbols-domjit-eventpath-errorcode.test.ts | 90 ++++++++++ 29 files changed, 101 insertions(+), 785 deletions(-) ``` </details> **gate history** · 5 passed · 2 rejected · iteration 4 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/bindings/Cookie.h 2 1 0 src/jsc/bindings/CookieMap.cpp 2 1 0 src/jsc/bindings/CookieMap.h 2 3 0 src/jsc/bindings/DOMException.cpp 2 3 0 src/jsc/bindings/DOMException.h 2 3 0 src/jsc/bindings/DOMFormData.cpp 1 1 0 src/jsc/bindings/DOMFormData.h 1 1 0 src/jsc/bindings/ErrorCode.cpp 1 1 0 src/jsc/bindings/ErrorCode.h 1 1 0 src/jsc/bindings/IDLTypes.h 1 1 0 src/jsc/bindings/ImportMetaObject.h 2 1 0 src/jsc/bindings/JSCommonJSModule.h 1 2 0 src/jsc/bindings/ProcessBindingTTYWrap.cpp 2 1 0 src/jsc/bindings/Sink.h 2 1 0 src/jsc/bindings/webcore/DOMJITCheckDOM.h 0 1 0 src/jsc/bindings/webcore/DOMJITHelpers.cpp 1 1 0 (+ 13 more files) ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
2 participants
Part of the recurring dead-code sweep. 311 deletions / 97 insertions across 34 source files. Every removed item was verified to have zero references across
src/andbuild/debug/codegen/, then confirmed by a cleanbun bdbuild andbun run rust:check-all(10/10 targets).C++ (
src/jsc/bindings/)napi
Napi::generateSourceCode()(napi.cpp, napi.h): 26 LOC, zero callers.rg generateSourceCode src/ build/debug/codegen/finds only the definition and declaration. Thenamespace JSC { JSGlobalObject; JSSourceCode; }forward-decl block that sat above it is dropped with it.NapiWeakValue::isSet/isPrimitive/isCell/isString/cell()/primitive()/string()(napi.h): accessor set never called; onlyget()is used.NapiRef.cppexplicitly avoidsisSet(); the two comment lines there that referenced it are dropped too.NapiClass::destroy(napi.h): unreachable withneedsDestruction = DoesNotNeedDestructionon the same class.NodeVM
NodeVMGlobalObject::clearContextifiedObject(): zero callers.NodeVMModuleRequest::addImportAttribute+specifier(WTF::String)setter: zero callers (only the getter is used).NodeVMSourceTextModule::hasModuleRecord/NodeVMSyntheticModule::hasModuleRecord: zero callers (moduleRecordIfExists()covers the same check).NodeVMScript::cachedBytecode(): zero callers; the field is accessed directly.JSBufferList / JSStringDecoder
JSBufferListConstructor::initializeProperties: empty body, never called.JSBufferList::destroy(JSCell*): unreachable on aJSNonFinalObjectwithoutNeedsDestruction.JSStringDecoderConstructor::initializeProperties: never called;finishCreation(cpp:552) already sets name/length/prototype.BunClientData
JSVMClientData::outputConstraintSpaces()/forEachOutputConstraintSpace()/m_outputConstraintSpaces: duplicates of theJSHeapDatamembers at the top of the file. Both real callers (BunGCOutputConstraint.cpp:126andBunClientData.h:227) dispatch onheapData, never onJSVMClientData.EventLoopTask
m_isCleanupTaskfield,isCleanupTask()getter,CleanupTaskTagconstructor: the bool is written in every constructor but never read.DeleteCallbackDataTasknow uses the primary templated constructor (identical behaviour).initialValues() statics
JSNextTickQueue::initialValues(),PendingVirtualModuleResult::initialValues()(ModuleLoader.h),MockWithImplementationCleanupData::initialValues()(JSMockFunction.h): never invoked; eachfinishCreationsets every internal field directly.BunStreamSource::initialValues()is kept (live caller at BunStreamSource.cpp:57).Misc
JSEnvironmentVariableMap.cpp:jsSetterEnvironmentVariable(registered withnullptr; only referenced in a comment, which is updated).JSBuffer.cpp: commented-outjsBufferPrototypeToStringWithoutTypeChecksDOMJIT block (23 LOC, unchanged since Jan 2025).blob.h: unusedBlobReftype alias (BlobRefPtris the one used).Bindgen/IDLTypes.h: unusedIsIDLStrongAnytrait.Rust
src/zlib/error.rs(whole file, 31 LOC): definesError/Resultthat nothing imports; the crate usesZlibErrorfromlib.rsinstead.rg 'bun_zlib::Error|bun_zlib::Result|zlib::error::'acrosssrc/returns nothing. Thethiserrordep it required is dropped fromsrc/zlib/Cargo.toml.src/zlib/lib.rs: droppedByte/gzFile/struct_gzFile_s/voidpffrom thebun_zlib_sys::sharedre-export (zero Rust-side references), and the now no-op#[allow(...)]above it.src/io/windows_event_loop.rs:FilePoll::ref_/activate/can_ref(30 LOC). No caller targetsFilePollfor any of these; all.ref_()/.activate()call sites in the tree dispatch onKeepAlive,Source,Pipe,Timer, orProgress. Thedeclare_scope!(FilePoll)/declare_scope!(KeepAlive)statics that only those methods logged through are dropped, and the staleref()reference in thedisable_keeping_process_alivedoc is reworded. Verified on all 10rust:check-alltargets including both Windows triples.src/io/posix_event_loop.rs: the posixFilePoll::ref_sibling (same justification), and thedeclare_scope!(KeepAlive)static with zeroscoped_log!callers.src/install/lockfile/bun.lock.rs: commented-outStringifier::savestub.src/install/lockfile/Buffers.rs: four empty#[cfg(debug_assertions)] { /* commented print */ }blocks on the save side, and the three orphanedlet _pos = stream.posbindings on the load side that fed them.TypeScript
src/js/node/net.ts:kpendingReadprivateSymbol()(defined, written once asthis[kpendingRead] = undefined, never read).kServerSocketwas initially removed too but restored in a93d2fa: the writeclientHandle[kServerSocket] = handleis a GC retention edge from accepted socket handle to native Listener, and removing it crashedsql-close-pending-connection.test.tson 6 CI lanes (build #87459) viaListener::finalizefiring JSon_closeduring a GC sweep. A two-line comment now documents why the symbol exists; the underlying Listener-finalizer-calls-JS-during-sweep issue is tracked separately.Verification
Smoke tests on
test/js/node/vm,test/js/node/string_decoder,test/js/node/net,test/js/sql/sql-close-pending-connection.test.ts, andtest/js/node/process -t envshow identical pass/fail counts with and without this diff.Notes
src/jsc/bindings/libuv/uv/*) are not touched (reverted in 4dd5a99 per review).napi.cppalso appears in Remove dead code from C++ webcore headers, src/js, and pretty_format #35775, which removes different symbols (napi_set_ref,napi_internal_get_version); no line overlap.src/zlib/Cargo.tomlalso appears in Remove unused Cargo boilerplate deps from 70 crates + misc dead C++/Rust #35880, which removes different deps (bstr/scopeguard/etc.); no deletion overlap.src/io/posix_event_loop.rsalso appears in Remove unused Cargo boilerplate deps from 70 crates + misc dead C++/Rust #35880 at lines 1562+; this PR touches line 562, no overlap.src/http/(HTTP client),src/valkey/,src/patch/,src/semver/,src/semver_jsc/.[review] gate passed · iteration 5 · 34 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 8 passed · 0 rejected · iteration 5
evidence per changed file