Remove dead code from webcore C++, ZigSourceProvider, react_compiler, bun_core/fmt - #36390
Conversation
… bun_core/fmt
C++ (webcore): strip ~2900 lines of commented-out WebCore DOM type
listings that have been stale since the initial 2022 WebKit import.
These were never compiled (pure comment lines inside live files):
- DOMIsoSubspaces.h, DOMClientIsoSubspaces.h: 795 commented m_subspaceFor*
lines each (Touch, GPU*, ApplePay*, MediaKey*, WebXR*, etc.)
- EventHeaders.h, EventTargetHeaders.h, EventFactory.cpp,
EventTargetFactory.cpp: commented #include and switch-case arms for
DOM event types Bun does not implement
- EventPath.cpp: commented shadow-DOM retargeting (RelatedNodeRetargeter,
buildPath, retargetTouch*)
- PerformanceTiming.h: commented m_* field list
- JSDOMGlobalObject.cpp: commented callerGlobalObject /
legacyActiveGlobalObjectForAccessor
C++ (bindings): remove never-called functions (verified via rg across
src/, build/debug/codegen/, and src/codegen/):
- ZigSourceProvider: forEachSourceProvider (declared, never defined),
sourceMappingForSourceURL, freeSourceCode, updateCache, cacheBytecode,
commitCachedBytecode, isBytecodeCacheEnabled, readOrGenerateByteCodeCache,
readCache. The last six are non-const so they never overrode the
JSC::SourceProvider const virtuals; their bodies unconditionally
returned on entry.
- ConsoleObject: logToSystemConsole, setLogToSystemConsole, consoleAgent,
setDebuggerAgent, setPersistentScriptProfilerAgent (never called),
warnUnimplemented, internalAddMessage (declared, never defined), and
the backing m_consoleAgent / m_debuggerAgent / m_scriptProfilerAgent /
m_profiles / m_profileRestoreBreakpointActiveValue fields (never read).
- AsyncContextFrame: 5-arg profiledCall overload and run() (only the
4-arg profiledCall has a caller, in NodeHTTP.cpp).
Rust: remove functions with zero callers (verified via rg + cargo check
on all targets):
- react_compiler: Environment::{for_outlined_fn, get_property_type_numeric,
get_fallthrough_property_type, take_outlined_functions,
identifier_name_for_id}; ProgramContext::{is_already_compiled,
mark_compiled} and the backing already_compiled field.
- bun_core/fmt: CountingWriter + Null (fmt::count delegates to
bun_alloc::fmt_count instead), hex_int_upper (hex_int_lower is the
only used variant), parse_num (deprecated alias of parse_ascii,
zero callers).
- css: MediaList::clone_in (unused alias of deep_clone).
JS: net.ts kServerSocket and kpendingRead symbols were written once and
never read (private non-.for Symbols).
Verified with bun bd (full link) and bun run rust:check-all
(10 targets green).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
WalkthroughChangesDead symbol cleanup
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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-webcore-commented.test.ts`:
- Around line 1-7: Update the header comment in
dead-symbols-webcore-commented.test.ts to explicitly document that zero callers
were verified across both src/ and build/debug/codegen/. Preserve the existing
explanation of the lint’s purpose and ensure the note remains
contributor-facing.
🪄 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: 243d34a5-b527-420f-9f78-d00e3d80fc71
📒 Files selected for processing (22)
src/bun_core/fmt.rssrc/bun_core/lib.rssrc/css/media_query.rssrc/js/node/net.tssrc/jsc/bindings/AsyncContextFrame.cppsrc/jsc/bindings/AsyncContextFrame.hsrc/jsc/bindings/ConsoleObject.cppsrc/jsc/bindings/ConsoleObject.hsrc/jsc/bindings/JSDOMGlobalObject.cppsrc/jsc/bindings/ZigSourceProvider.cppsrc/jsc/bindings/ZigSourceProvider.hsrc/jsc/bindings/webcore/DOMClientIsoSubspaces.hsrc/jsc/bindings/webcore/DOMIsoSubspaces.hsrc/jsc/bindings/webcore/EventFactory.cppsrc/jsc/bindings/webcore/EventHeaders.hsrc/jsc/bindings/webcore/EventPath.cppsrc/jsc/bindings/webcore/EventTargetFactory.cppsrc/jsc/bindings/webcore/EventTargetHeaders.hsrc/jsc/bindings/webcore/PerformanceTiming.hsrc/react_compiler/hir/environment.rssrc/react_compiler/imports.rstest/internal/source-lints/dead-symbols-webcore-commented.test.ts
💤 Files with no reviewable changes (19)
- src/jsc/bindings/webcore/EventHeaders.h
- src/jsc/bindings/webcore/PerformanceTiming.h
- src/css/media_query.rs
- src/jsc/bindings/ConsoleObject.cpp
- src/jsc/bindings/webcore/EventTargetFactory.cpp
- src/js/node/net.ts
- src/jsc/bindings/webcore/EventTargetHeaders.h
- src/jsc/bindings/webcore/EventFactory.cpp
- src/jsc/bindings/AsyncContextFrame.cpp
- src/jsc/bindings/JSDOMGlobalObject.cpp
- src/bun_core/fmt.rs
- src/react_compiler/hir/environment.rs
- src/jsc/bindings/AsyncContextFrame.h
- src/react_compiler/imports.rs
- src/jsc/bindings/ZigSourceProvider.h
- src/jsc/bindings/ConsoleObject.h
- src/jsc/bindings/ZigSourceProvider.cpp
- src/jsc/bindings/webcore/DOMClientIsoSubspaces.h
- src/jsc/bindings/webcore/DOMIsoSubspaces.h
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 5:19 PM PT - Jul 29th, 2026
@Jarred-Sumner, your commit e24d7e1 is building: |
…ol writes
Follow-up to the review on this PR:
- ConsoleObject.h: remove Inspector*Agent forward-decls/aliases and the
<wtf/Vector.h> / InspectorConsoleAgent.h includes that only served
the deleted m_* fields.
- ZigSourceProvider.{h,cpp}: remove BytecodeCacheGenerator /
UnlinkedFunction* / CodeSpecializationKind aliases and the
BytecodeCacheError.h include that only served the deleted cache
methods.
- AsyncContextFrame.{h,cpp}: remove the 5-arg call(..., NakedPtr<Exception>&)
overload; its only caller (5-arg profiledCall) was removed in the
previous commit and all remaining call sites (generate-jssink.ts,
NodeVM.cpp, NodeVMSyntheticModule.cpp, ServerRouteList.cpp, napi.cpp,
BunPlugin.cpp, the 4-arg profiledCall) use the 4-arg form.
- environment.rs: drop the now-empty Name resolution helpers section
divider.
Restore net.ts kServerSocket / kpendingRead. CI build #85379 showed sql
test crashes (validateIsNotSweeping in JSCell.cpp) that I could not
reproduce locally in 20+ runs including with aggressive GC; the symbol
writes were the only JS-side property write this PR removed, so they are
reverted out of caution even though the sql tests do not exercise
net.Server's accept path.
There was a problem hiding this comment.
LGTM — 6e64351 addressed the newly-dead supporting decls from the previous round. The two inline nits (leftover SourceCode/CachedBytecode aliases, stale ## JS section in the PR body) have zero functional impact and don't need to block.
Extended reasoning...
Overview
Pure dead-code removal across 21 files (~3400 lines deleted, +91 for the source-lint guard). Three buckets: (1) ~2900 lines of commented-out WebCore DOM type listings that have been comment-only since the 2022 WebKit import; (2) ~150 lines of never-called C++ bindings functions (ZigSourceProvider bytecode-cache stubs whose bodies unconditionally return;, ConsoleObject inspector-agent fields/accessors, AsyncContextFrame::run and 5-arg overloads); (3) ~200 lines of zero-caller Rust helpers in react_compiler, bun_core/fmt, and css. A source-lint test in test/internal/source-lints/ guards against reintroduction.
Security risks
None. This removes uncompiled comments and unreferenced functions; no input handling, auth, crypto, or serialization paths are touched.
Level of scrutiny
Medium-low. The line count is large but the change is mechanical. The one non-obvious correctness question — whether the removed ZigSourceProvider::{cacheBytecode, updateCache, commitCachedBytecode, ...} were live virtual overrides — is answered in the PR body (they were non-const so never matched the base's const virtuals) and, more directly, their bodies all unconditionally return; on the first line, so removal is behavior-preserving regardless. cachedBytecode() const final and m_cachedBytecode remain, which is the actual live bytecode-cache path used by SourceProvider::create.
Other factors
- My previous review flagged four sets of transitively-orphaned declarations; 6e64351 addressed all of them (Inspector*Agent forward-decls/includes, ZigSourceProvider aliases +
BytecodeCacheError.h, 5-argAsyncContextFrame::call, empty environment.rs section divider) and also reverted the net.ts change after review. - Re-verified zero remaining references for the removed Rust symbols (
for_outlined_fn,take_outlined_functions,get_property_type_numeric,get_fallthrough_property_type,identifier_name_for_id,is_already_compiled,mark_compiled,MediaList::clone_in) acrosssrc/; theIndexSetimport inimports.rsremains used by five other sites afteralready_compiledis dropped; the onlyparse_numhit left is an unrelated local macro inseq.rs. - Build verification is thorough:
bun bdclean,rust:check-all10/10 targets, smoke tests pass, and the source-lint test fails-before/passes-after. - The two remaining inline findings are cosmetic (unused
usingaliases, commented-out .cpp bodies whose .h side was already removed, and a stale PR-description section) — none affect the compiled binary.
…ASCIILiteral call() bodies ZigSourceProvider.cpp: the file-scope SourceCode and CachedBytecode using aliases have no unqualified consumers; all remaining uses are JSC:: prefixed. AsyncContextFrame.cpp: the commented-out ASCIILiteral call() overload bodies had their matching commented header declarations removed earlier in this PR; drop the .cpp side too.
There was a problem hiding this comment.
LGTM — 96fd65f addresses the last two leftovers (ZigSourceProvider.cpp SourceCode/CachedBytecode aliases and the AsyncContextFrame.cpp commented ASCIILiteral bodies), and the PR body now matches the 21-file diff. All three rounds of feedback resolved.
Extended reasoning...
Overview
Dead-code sweep across 21 files, net -3446/+93. Roughly 85% (~2900 lines) is deletion of pure //-commented WebCore DOM type listings in DOMIsoSubspaces.h, DOMClientIsoSubspaces.h, Event*Headers.h, Event*Factory.cpp, EventPath.cpp, PerformanceTiming.h, and JSDOMGlobalObject.cpp — comment-only lines that have never compiled since the 2022 WebKit import. The remaining ~500 lines remove never-called functions from ZigSourceProvider (dead-on-entry return; bodies that were non-const and thus never overrode the const JSC::SourceProvider virtuals), ConsoleObject (unread m_*Agent fields, undeclared internalAddMessage/logToSystemConsole, uncalled warnUnimplemented), AsyncContextFrame (run() and the 5-arg call/profiledCall overloads), react_compiler::Environment/ProgramContext, and bun_core::fmt (CountingWriter/Null/hex_int_upper/parse_num). A source-lint test guards against reintroduction.
Security risks
None. Pure deletion of comments and zero-caller functions; no new code paths, no input handling, no auth/crypto surface.
Level of scrutiny
Medium. The line count is large but the change is conceptually mechanical: comment deletion (zero risk by construction) plus removal of functions with verified zero references across src/, src/codegen/, and build/debug/codegen/. The touched files include critical infrastructure (ZigSourceProvider = module loading, AsyncContextFrame = async_hooks callback dispatch), but the removed SourceProvider methods were dead-on-entry (return; as first statement) and non-const so could not have been reached via virtual dispatch from the const base virtuals; the removed AsyncContextFrame overloads had their only caller (the 5-arg profiledCall) removed in the same PR, and every remaining AsyncContextFrame::call site (generate-jssink.ts, NodeVM, NodeVMSyntheticModule, ServerRouteList, napi, BunPlugin, NodeHTTP) uses the surviving 4-arg form.
Other factors
- Three prior review rounds from me on this PR; all feedback addressed (6e64351 swept the transitively-dead Inspector*Agent/BytecodeCacheGenerator declarations and the 5-arg
call; 96fd65f swept the last twousingaliases and commented ASCIILiteral bodies; PR body synced to the 21-file diff). - The bug-hunting system found nothing on the current revision.
- Author reports
bun bd(debug+ASAN) links clean andbun run rust:check-allis 10/10 green; smoke tests for react-compiler, globals/BroadcastChannel, console-log, and sql-close-pending-connection pass; the source-lint test fails-before/passes-after. - The one uncertain change (net.ts symbol writes) was reverted out of caution and moved to Followups.
- Removing
ConsoleObject::m_consoleAgentis strictly an improvement — it was an uninitialized raw pointer (no{ nullptr }default) whose only accessor is now gone.
|
CI status on 96fd65f (build #85400, all lanes in): 16 tests flaky (passed on retry or passed alone). Two tests red on every retry, neither touched by this diff:
The sql-close-pending-connection / sql.test.ts crashes from build #85379 are gone after 6e64351 reverted the net.ts symbol-write removal; see the Followups note in the PR body. Diff is green on the code it touches; ready for review. |
|
@robobun fix conflicts |
…dead-code-webcore-comments-react-compiler # Conflicts: # src/bun_core/lib.rs
|
Conflicts resolved in 873f9ee (merge of main). The conflict was in |
2 participants
Net: -3366 / +19 across 17 files (the +19 is the restored
/* ... */LGPL header inEventPath.cpp, previously wrapped in//).No file overlap with the other open dead-code PRs (#34965, #34759, #36178, #36237, #35775, #35559, #36318, #36115, #35437).
C++ webcore: commented-out DOM type listings (~2900 LOC)
These blocks have been pure comment lines since the initial 2022 WebKit import and were never compiled. They listed WebCore DOM classes Bun does not implement (Touch/GPU/ApplePay/MediaKey/WebXR/RTC/IndexedDB/etc.).
DOMIsoSubspaces.h,DOMClientIsoSubspaces.h: 795 commentedm_subspaceFor*/m_clientSubspaceFor*lines each. Live lines untouched.EventHeaders.h,EventTargetHeaders.h: commented#includelines for DOM event types Bun does not have.EventFactory.cpp,EventTargetFactory.cpp: commentedcase *InterfaceType:arms for the same.EventPath.cpp: commented shadow-DOM retargeting (RelatedNodeRetargeter,buildPath,retargetTouch*). Bun has no Node/ShadowRoot.PerformanceTiming.h: commentedm_*field list.JSDOMGlobalObject.cpp: commentedcallerGlobalObject/legacyActiveGlobalObjectForAccessor.C++ bindings: never-called functions (~200 LOC)
Verified zero references across
src/,build/debug/codegen/, andsrc/codegen/.ZigSourceProvider:forEachSourceProvider(declared, never defined),sourceMappingForSourceURL,freeSourceCode,updateCache,cacheBytecode,commitCachedBytecode,isBytecodeCacheEnabled,readOrGenerateByteCodeCache,readCache. The last six are non-const so they never overrode theJSC::SourceProviderconst virtuals (base:virtual void cacheBytecode(...) const { }etc.); their bodies unconditionallyreturn;on entry anyway. Also removed theBytecodeCacheGenerator/UnlinkedFunction*/CodeSpecializationKind/SourceCode/CachedBytecodealiases andBytecodeCacheError.hinclude that only served those methods.ConsoleObject:logToSystemConsole/setLogToSystemConsole/internalAddMessage(declared, never defined),consoleAgent/setDebuggerAgent/setPersistentScriptProfilerAgent/warnUnimplemented(never called), the backingm_consoleAgent/m_debuggerAgent/m_scriptProfilerAgent/m_profiles/m_profileRestoreBreakpointActiveValuefields (never read), and theInspector*Agentforward-decls/aliases and<wtf/Vector.h>/InspectorConsoleAgent.hincludes that only served them.AsyncContextFrame:run(), the 5-argprofiledCalloverload, the 5-argcall(..., NakedPtr<Exception>&)overload, and the commented-out ASCIILiteral overloads. All remaining call sites (generate-jssink.ts, NodeVM.cpp, NodeVMSyntheticModule.cpp, ServerRouteList.cpp, napi.cpp, BunPlugin.cpp, NodeHTTP.cpp) use the 4-arg form.Rust (~130 LOC)
Zero callers across
src/andbuild/debug/codegen/;cargo checkon all 10 targets is green.react_compiler:Environment::{for_outlined_fn, get_property_type_numeric, get_fallthrough_property_type, take_outlined_functions, identifier_name_for_id}.bun_core/fmt:hex_int_upper(onlyhex_int_loweris used).(
CountingWriter/Null/parse_numin fmt.rs,MediaList::clone_inin css, andProgramContext::{is_already_compiled, mark_compiled}/already_compiledin react_compiler/imports.rs were originally removed here but main landed the same deletions independently before the merge in 873f9ee, so they dropped out of this diff.)Verification
bun bd(debug+ASAN) builds and links clean.bun run rust:check-all: 10 ok, 0 failed (linux/macos/windows/freebsd/android, x64+aarch64).test/bundler/transpiler/react-compiler.test.ts(32 pass),test/js/bun/globals.test.js+test/js/web/broadcastchannel/broadcast-channel.test.ts(31 pass),test/js/web/console/console-log.test.ts(4 pass),test/js/sql/sql-close-pending-connection.test.ts(5 pass).Followups (not included, "maybe dead")
src/jsc/bindings/headers-cpp.h(190 LOC): only included bysizegen.cpp(which another open PR touches) and contains visibly broken syntax. Left for that PR.SerializedScriptValue.cpphas ~580 LOC of commented DOM-type serialization (Blob/File/ImageData/DOMPoint) from 2023; left alone since it may be reference for future structured-clone work.src/js/node/net.tskServerSocket/kpendingReadsymbol writes: write-only private Symbols, originally removed here but reverted in 6e64351 after CI build #85379 showed sql test crashes (validateIsNotSweeping) that disappeared after the revert. TheclientHandle[kServerSocket] = handlewrite appears to be a GC retention edge keeping the server handle alive while accepted clients exist; the sql tests use a net.Server-based unix-socket proxy. Not safe to remove without deeper investigation.ConsoleObject.cppstill includesInspector{ScriptProfiler,Debugger,Console}Agent.h(no remaining references) andEventPath.hretains orphanedbuildPath/setRelatedTargetprivate declarations and commented shadow-DOM includes. No functional impact; can go in the next sweep.