Remove dead code from webcore bindings, watcher, node-fallbacks, and misc crates - #37062
Remove dead code from webcore bindings, watcher, node-fallbacks, and misc crates#37062robobun wants to merge 9 commits into
Conversation
…misc crates Net -1936 lines across 82 files. Every deleted symbol was verified to have zero references across src/, packages/, test/, and regenerated codegen output; removals were validated by cargo check on all 10 target triples and a full debug build. Superseded compiled files (JSDOMConvertDate, JSMIMEBindings, JSDOMIterator.cpp) follow the existing one-line stub convention instead of deletion.
Stash round-trips can temporarily restore files a branch deletes, so an existsSync assertion on the deleted ruby script is unreliable; the content checks carry the lint.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request removes obsolete APIs, exports, loader metadata, binding implementations, platform declarations, fallback code, and dead-code exceptions. It adds source-lint checks for deleted symbols. ChangesWatcher loader removal
WebCore binding cleanup
Runtime and platform cleanup
Dead-symbol regression coverage
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/jsc/bindings/InternalForTesting.cpp`:
- Around line 51-52: Remove the obsolete jsFunction_lsanDoLeakCheck declaration
from InternalForTesting.h, leaving the remaining testing bindings unchanged
since no implementation or registration exists.
In `@test/internal/source-lints/dead-symbols-webcore-watcher-fallbacks.test.ts`:
- Around line 137-140: Replace the deleted-path existsSync assertion in the test
“the CMake-era unified-source ruby script stays deleted” with a stable surviving
source contract, such as verifying the live TypeScript implementation or build
references no longer invoke Ruby. If no reliable contract is available, remove
this assertion and leave deletion verification to the appropriate external
mechanism.
🪄 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: 200f545f-63e1-4283-aa7e-3edd78434c05
⛔ Files ignored due to path filters (1)
src/node-fallbacks/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (81)
src/bun_core/windows_sys.rssrc/bundler/bundle_v2.rssrc/codegen/generate-unified-source-bundles.rbsrc/csrf/lib.rssrc/css_jsc/css_internals.rssrc/exe_format/pe.rssrc/http/lib.rssrc/js/internal-for-testing.tssrc/js/internal/fs/watch.tssrc/js/internal/readline/interface.jssrc/js/internal/repl/node-primordials.jssrc/jsc/AsyncModule.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/BunObject+exports.hsrc/jsc/bindings/IDLTypes.hsrc/jsc/bindings/InternalForTesting.cppsrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/webcore/BufferSource.hsrc/jsc/bindings/webcore/ContextDestructionObserver.cppsrc/jsc/bindings/webcore/ContextDestructionObserver.hsrc/jsc/bindings/webcore/Event.cppsrc/jsc/bindings/webcore/Event.hsrc/jsc/bindings/webcore/EventTarget.cppsrc/jsc/bindings/webcore/EventTarget.hsrc/jsc/bindings/webcore/FetchHeaders.hsrc/jsc/bindings/webcore/HTTPHeaderIdentifiers.cppsrc/jsc/bindings/webcore/HTTPHeaderIdentifiers.hsrc/jsc/bindings/webcore/JSCookieMap.cppsrc/jsc/bindings/webcore/JSDOMAttribute.hsrc/jsc/bindings/webcore/JSDOMConvert.hsrc/jsc/bindings/webcore/JSDOMConvertBufferSource.hsrc/jsc/bindings/webcore/JSDOMConvertDate.cppsrc/jsc/bindings/webcore/JSDOMConvertDate.hsrc/jsc/bindings/webcore/JSDOMIterator.cppsrc/jsc/bindings/webcore/JSDOMIterator.hsrc/jsc/bindings/webcore/JSDOMOperationReturningPromise.hsrc/jsc/bindings/webcore/JSDOMPromiseDeferred.cppsrc/jsc/bindings/webcore/JSDOMPromiseDeferred.hsrc/jsc/bindings/webcore/JSEventListener.hsrc/jsc/bindings/webcore/JSMIMEBindings.cppsrc/jsc/bindings/webcore/JSMIMEBindings.hsrc/jsc/bindings/webcore/NetworkLoadMetrics.hsrc/jsc/bindings/webcore/PerformanceTiming.cppsrc/jsc/bindings/webcore/PerformanceTiming.hsrc/jsc/bindings/webcore/ResourceLoadTiming.hsrc/jsc/bindings/webcore/ScriptWrappable.hsrc/jsc/bindings/webcore/SerializedScriptValue.cppsrc/jsc/hot_reloader.rssrc/libarchive/lib.rssrc/md/types.rssrc/node-fallbacks/package.jsonsrc/node-fallbacks/tsconfig.jsonsrc/node-fallbacks/util.jssrc/opaque/lib.rssrc/picohttp/lib.rssrc/resolver/lib.rssrc/runtime/api/BunObject.rssrc/runtime/api/csrf_jsc.rssrc/runtime/bake/production.rssrc/runtime/cli/test_command.rssrc/runtime/jsc_hooks.rssrc/runtime/node.rssrc/runtime/node/assert/myers_diff.rssrc/runtime/node/fs_events.rssrc/runtime/node/node_assert.rssrc/runtime/node/node_cluster_binding.rssrc/runtime/valkey_jsc/protocol_jsc.rssrc/sha_hmac/lib.rssrc/sha_hmac/sha.rssrc/sql_jsc/jsc.rssrc/sys/windows/mod.rssrc/valkey/valkey_protocol.rssrc/watcher/Watcher.rssrc/watcher/lib.rssrc/windows_sys/externs.rssrc/zlib/lib.rssrc/zlib_sys/posix.rssrc/zlib_sys/win32.rstest/internal/source-lints/dead-code-escape-limits.jsontest/internal/source-lints/dead-symbols-webcore-watcher-fallbacks.test.ts
💤 Files with no reviewable changes (55)
- src/jsc/bindings/webcore/JSDOMConvert.h
- src/node-fallbacks/tsconfig.json
- src/runtime/valkey_jsc/protocol_jsc.rs
- src/jsc/bindings/webcore/FetchHeaders.h
- src/codegen/generate-unified-source-bundles.rb
- src/opaque/lib.rs
- src/jsc/bindings/webcore/ContextDestructionObserver.cpp
- src/jsc/bindings/webcore/ScriptWrappable.h
- src/jsc/bindings/webcore/ResourceLoadTiming.h
- src/jsc/bindings/webcore/EventTarget.cpp
- src/runtime/node/node_cluster_binding.rs
- src/exe_format/pe.rs
- src/jsc/bindings/webcore/JSEventListener.h
- src/jsc/bindings/webcore/JSCookieMap.cpp
- src/jsc/bindings/webcore/JSDOMIterator.h
- src/jsc/bindings/webcore/BufferSource.h
- src/jsc/bindings/webcore/HTTPHeaderIdentifiers.h
- src/js/internal-for-testing.ts
- src/runtime/api/csrf_jsc.rs
- src/jsc/bindings/webcore/PerformanceTiming.cpp
- src/md/types.rs
- src/zlib_sys/posix.rs
- src/jsc/bindings/webcore/PerformanceTiming.h
- src/jsc/bindings/webcore/HTTPHeaderIdentifiers.cpp
- src/http/lib.rs
- src/jsc/bindings/webcore/JSDOMAttribute.h
- src/jsc/bindings/ZigGlobalObject.cpp
- src/runtime/node/assert/myers_diff.rs
- src/valkey/valkey_protocol.rs
- src/jsc/bindings/webcore/SerializedScriptValue.cpp
- src/js/internal/readline/interface.js
- src/zlib/lib.rs
- src/jsc/bindings/webcore/JSDOMPromiseDeferred.cpp
- src/libarchive/lib.rs
- src/zlib_sys/win32.rs
- src/csrf/lib.rs
- src/jsc/bindings/webcore/ContextDestructionObserver.h
- src/runtime/bake/production.rs
- src/css_jsc/css_internals.rs
- src/watcher/lib.rs
- src/jsc/bindings/BunObject+exports.h
- src/runtime/api/BunObject.rs
- src/jsc/bindings/webcore/JSDOMOperationReturningPromise.h
- src/jsc/AsyncModule.rs
- src/jsc/bindings/webcore/JSDOMPromiseDeferred.h
- src/runtime/node/node_assert.rs
- src/jsc/bindings/webcore/EventTarget.h
- src/jsc/bindings/webcore/Event.cpp
- src/jsc/bindings/webcore/NetworkLoadMetrics.h
- src/jsc/bindings/webcore/JSDOMConvertBufferSource.h
- src/runtime/jsc_hooks.rs
- src/windows_sys/externs.rs
- src/jsc/RuntimeTranspilerStore.rs
- src/picohttp/lib.rs
- src/jsc/bindings/webcore/Event.h
The jsFunction_lsanDoLeakCheck declaration in InternalForTesting.h outlived the deleted definition, and two attribute removals in fs_events.rs left their fn signatures over-indented. Also rustfmt fallout in hot_reloader, jsc_hooks, and sha_hmac.
|
Addressed the review feedback in bc50337:
The existsSync assertion flagged on the lint test was already replaced with content-based checks in 0f8f22e. Debug build and the source-lint suite pass after the changes. |
Deleting the two internal-for-testing exports left dependency_from_js and tag_infer_from_js with no generated callers. Removing them also kills update_request_jsc::from_js (sole caller was dependency_from_js) and the version_to_js/log_to_js helpers, so both modules go, along with the dispatch_js2native re-exports, the dependency.rs codegen path mapping, and the four ERR_REDIS_INVALID_* codes only the deleted RedisError match arms produced.
There was a problem hiding this comment.
No bugs found this pass — all five earlier findings were addressed (or deliberately deferred to #35775). Deferring to a human because of scope: 89 files / -1.9k lines spanning webcore C++ (Event/IDL/promise-deferred struct and vtable changes), a signature change threaded through the watcher/bundler/VM/hot-reloader chain, node-fallbacks dependency pruning, and a cascading two-file deletion in install_jsc added in the final commit.
What was reviewed:
- Watcher
loadercolumn removal traced through all 8 call sites;add_file/add_file_by_path_slowsignatures updated consistently. Event::resetBeforeDispatchremoval fromdispatchEvent— confirmed the body only touched the removedm_defaultHandledbit.FSWatcher/kRestorePreviousStateexport removals — confirmed no external consumers via grep ofsrc/js/.csrf__generate/map_diff_errormatch arms stay exhaustive after variant deletions.
Extended reasoning...
Overview
Scheduled dead-code sweep removing ~2,100 lines across 89 files: webcore C++ bindings (unused IDL typed-array types, DOMPromiseDeferred cluster, NetworkLoadMetrics inspector fields, write-only Event bitfields), the CMake-era ruby unified-source script, the watcher's write-only WatchItem.loader column and its bun_watcher::Loader cycle-break newtype (threaded through bundler/hot_reloader/VirtualMachine/jsc_hooks/RuntimeTranspilerStore/AsyncModule/test_command), unused Rust error variants and hashers across sha_hmac/csrf/valkey/picohttp/windows_sys/zlib, 13 node-fallbacks dependencies, unused built-in JS primordial exports, and the npa/npmTag/lsanDoLeakCheck internal-for-testing bindings plus their now-orphaned Rust/C++/codegen landing pads.
Security risks
None identified. No auth, crypto (the sha_hmac deletions are unused hasher instantiations, not algorithm changes), or permission logic touched. The node-fallbacks dependency pruning only removes packages the fallback bundle never imports.
Level of scrutiny
High. This is not a mechanical deletion PR: it changes struct layouts in C++ (Event bitfields, NetworkLoadMetrics), removes a call from the EventTarget::dispatchEvent hot path, threads a parameter removal through ~8 files in the watcher/transpiler chain, prunes package.json dependencies that affect browser-target bundling, and — in the final commit — cascaded into deleting two entire install_jsc files the author hadn't originally scoped. Each individual hunk looks correct, but the breadth and the number of subsystems touched put it outside what I'd approve without a human glance.
Other factors
Five prior review findings (fs_events indent, InternalForTesting.h decl, npa/npmTag landing pads, ErrorCode.ts REDIS entries, JSDOMConvertScheduledAction.h) were all addressed in bc50337/5c4db5b/c16c60eb or explicitly deferred to open PR #35775. Verification is solid (rust:check-all on 10 targets, full debug build, 582 smoke tests, source-lint pinning removals). The bug hunting system found nothing new this run. Spot-checked that removed JS exports (FSWatcher, kRestorePreviousState) have no external consumers, and that match arms in csrf_jsc/node_assert remain exhaustive after variant deletions.
|
@robobun conflicts |
…dead-code-webcore-codegen-misc # Conflicts: # src/bundler/bundle_v2.rs # src/jsc/AsyncModule.rs # src/jsc/hot_reloader.rs # src/runtime/jsc_hooks.rs
|
Merged main in 70de266. The conflicts were between this PR's watcher loader-parameter removal and #37050's fd-ownership refactor on the same call sites (hot_reloader, jsc_hooks, bundle_v2, AsyncModule); resolved by keeping the FdOwnership semantics from main with the loader argument dropped. AsyncModule took main's version outright since #37050 removed its watcher registration entirely. cargo check across the workspace, a full debug build, and the hot/watch test suites pass on the merge. |
There was a problem hiding this comment.
No issues found in this pass, but deferring to a human given the breadth: 91 files across watcher, hot-reloader, bundler, webcore bindings, node-fallbacks, and a dozen misc crates, plus a non-trivial merge with #37050 that combined the fd-ownership refactor with the loader-column removal on the same watcher call sites.
What was reviewed:
- Watcher
loadercolumn removal traced through all callers (hot_reloader, jsc_hooks, bundle_v2, RuntimeTranspilerStore, VirtualMachine, test_command); no remaining reads of the column. FSWatcher/kRestorePreviousStateexport removals — both symbols still defined and used internally; no external importers.- node-fallbacks dep removals — build-fallbacks.ts uses
bun buildwith force-externalized builtins, not the removed npm packages; entry files are self-contained. RedisErrorvariant removals — no remaining constructors or string-parse paths insrc/valkey/; ErrorCode.ts entries removed to match.
Extended reasoning...
Overview
This is a scheduled dead-code sweep removing ~2,000 lines across 91 files. The bulk is pure deletion of provably-unreferenced symbols: unused IDL typed-array converters and IDL types in webcore C++ headers, the CMake-era ruby unified-source script, write-only WatchItem.loader and its cycle-break newtype, unused error-enum variants (csrf, valkey, exe_format, myers_diff), unused OpenSSL-3-deprecated hasher wrappers, unused Windows FFI decls/re-exports, commented-out Zig leftovers, and unused export keys in built-in JS modules. Three rounds of prior review found and fixed cascading orphans (InternalForTesting.h decl, dependency_jsc/update_request_jsc, ErrorCode.ts entries), plus a rustfmt indentation drift.
Security risks
None identified. All changes are removals of unreferenced symbols or write-only fields; no validation, auth, crypto, or path-handling logic is weakened. The csrf::Error variant removals leave the actual generate/verify paths untouched (verify already returned bool). The sha_hmac deletions remove only the never-wired MD5_SHA1/Blake2 EVP wrappers and the deprecated raw-hasher variants; the live evp::* types and Algorithm enum are unchanged.
Level of scrutiny
High, for two reasons. First, sheer breadth — 91 files across watcher, hot-reloader, bundler, HTTP client, event dispatch, JSC bindings, and node-fallbacks means a subtle behavioral removal (e.g., a virtual override actually reached, a struct field read via C++ ABI offset) would be easy to miss. Second, the merge with #37050 in 70de266 combined two independent refactors on the same watcher add_file call sites (fd-ownership + loader-parameter removal); the resolution looks correct on read-through, but that class of merge is exactly where silent regressions hide.
Other factors
- All three prior rounds of my inline nits were addressed (bc50337, c16c60e), and CodeRabbit's two findings were addressed (0f8f22e, bc50337). One was intentionally skipped with a stated reason (JSDOMConvertScheduledAction.h owned by open PR #35775).
- The github-actions comment-cop fired on
install_jsc/lib.rsand robobun declined it with a reasonable justification (crate-level doc comment, not a workaround). - The PR description claims
rust:check-all(10 targets), full debug build, and 582 smoke tests pass, and the source-lint test that pinned the removals was itself removed during review. - Spot-checked this pass:
FSWatcherexport key (only.watch/.createIgnoreMatcherconsumed externally),kRestorePreviousState(only referenced within interface.js),Bun__Process__queueNextTick1(canonical decl still lives in JSValue.rs), node-fallbacks build script (usesbun build, not esbuild; removed deps genuinely unimported), and the RedisError variants (no constructors remain in src/valkey/).
Given the scale and the merge-conflict resolution on hot-reloader/watcher, this warrants a human sign-off even with zero findings.
Scheduled dead-code sweep. Net -1,936 lines across 82 files (+177/-2,113). Areas were chosen to avoid overlap with the open dead-code PRs (#35437, #35775, #35880, #36115, #36237, #37012); all hunks were diffed against those PRs and none of the deletions coincide.
What was removed
webcore C++ bindings (~900 lines)
JSDOMConvertBufferSource.h: 11 unused typed-array IDL types (IDLInt8Array,IDLFloat64Array,IDLBigUint64Array, ...) with theirConverter/JSConverterspecializations and 25toPossiblyShared*/toUnshared*helpers. OnlyIDLUint8Array,IDLArrayBuffer,IDLDataView, andIDLArrayBufferViewhave consumers.IDLTypes.h:IDLDate,IDLUnsupportedType,IDLError,IDLDOMException,IDLScheduledAction,IDLIDBKey/IDLIDBKeyData/IDLIDBValue, theENABLE(WEBGL)block, and dead forward declarations.JSDOMPromiseDeferred.h/.cpp: theDOMPromiseDeferredBase/DOMPromiseDeferred<T>class cluster,resolveWithJSValue,resolveWithNewlyCreated,resolveCallbackValueWithNewlyCreated,fulfillPromiseWithJSON+parseAsJSON.NetworkLoadMetrics.h:AdditionalNetworkLoadMetricsForWebInspector,PrivacyStance,NetworkLoadPriority, the write-only connection flags, and thePLATFORM(COCOA)copyTimingDatadecls.Event.h/.cpp: write-onlym_defaultHandled/m_isDefaultEventHandlerIgnoredbits and their accessors, the now-emptyresetBeforeDispatch, and never-overriddenrelatedTargetvirtuals.IDLOperationReturningPromise::callReturningOwnPromise/callStaticReturningOwnPromise,IDLAttribute::setPassingPropertyName/setStatic,HTTPHeaderIdentifiers::identifierFor+ its pointer-to-member table,EventTarget::isNode,ContextDestructionObserver::protectedScriptExecutionContext,PerformanceTiming::monotonicTimeToIntegerMilliseconds,BufferSource::mutableData,toBufferSource,FetchHeaders::fastSet,ScriptWrappable::offsetOfWrapper, and friends.JSDOMConvertDate.h/.cpp,JSMIMEBindings.h/.cpp, andJSDOMIterator.cppare stubbed out following the existing convention for superseded files (see the stub comment inMessagePortChannel.h).build/codegen
src/codegen/generate-unified-source-bundles.rb(392 lines): CMake-era ruby script;scripts/build/unified.tsis the live reimplementation and nothing invokes ruby.node-fallbacks
util.js: 270-line commented-out legacyutil.typesbody (stale since 2025).package.json/bun.lock: 13 dependencies no fallback imports (the force-externalized builtin names never resolve to node_modules), includingesbuild.tsconfig.jsonpaths trimmed to match.file watcher
WatchItem.loaderwas write-only: everyadd_filepath stored it, no MultiArrayList column accessor ever read it. Removed the column, thebun_watcher::Loadercycle-break newtype, its drift-guard assert, and the loader parameter from the wholeadd_file/add_file_by_path_slowchain (watcher, hot_reloader, bundler, jsc_hooks, RuntimeTranspilerStore, AsyncModule, VirtualMachine, test_command).AsyncModule.loaderandInitOpts.loaderfell out as dead once the watcher stopped storing it.Rust crates
sha_hmac: unused OpenSSL-3-deprecated hashers (hashers::SHA512/SHA384/SHA512_256/RIPEMD160) and the never-wiredMD5_SHA1/Blake2EVP wrappers.csrf:Error::InvalidToken/ExpiredToken/DecodingFailed, never constructed (verify()returns bool).valkey: sixRedisErrorvariants the parser never produces (InvalidArgument,InvalidArray,InvalidBigNumber,InvalidErrorString,InvalidNull,InvalidSimpleString); the one string-parse site feeds from the runtime error name set, which contains none of them.picohttp: write-onlyRequest.minor_version/bytes_readfields (Responsekeeps both).windows_sys: consts, types, and extern decls whose only references were unused re-exports (SetFileInformationByHandle,ResumeThread,FILE_ATTRIBUTE_*,CTRL_LOGOFF_EVENT,PebView,Curdir,STATUS_SUCCESS, ...), plus those re-export lines.zlib/zlib_sys:deflateInit_/inflateInit_decls (all code uses the*2_variants).libarchive,css_jsc: commented-out Zig code left from the port.opaque: theopaque_mut_nnmacro arm (zero call sites across ~180 instantiations).bake:BakeProdSourceMapno_mangle export C++ never declares or calls.BunObject: the Rustnanosecondshost_fn shadowed by the C++functionBunNanosecondsthe lut actually uses, plus stalemacro(...)entries inBunObject+exports.h.exe_formatInputIsSigned,mdSetextheader,myers_diffOutOfMemory,node_fsunused trait method, unused re-export tokens innode.rs/sql_jsc/resolver.built-in JS
internal/repl/node-primordials.js: 14 export keys none of the 13 repl/readline consumers reference, with their backing captures.internal-for-testing.ts:npa,npmTag,lsanDoLeakCheck(plus its C++ backing), all with zero references in test/, bench/, or scripts/.internal/fs/watch.tsandinternal/readline/interface.js.Verification
$zig/$cppstring references, and strum string-construction paths.cargo check --workspaceon all 10 CI target triples (bun run rust:check-all): 10 ok, 0 failed.bun bddebug build passes.dead-code-escape-limits.json) was regenerated. A source-lint file pinning the removals was part of earlier revisions and removed during review.[review] gate passed · iteration 1 · 89 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