Remove dead code from the WebCore bindings, IDL converters, and Rust FFI wrappers - #38005
Remove dead code from the WebCore bindings, IDL converters, and Rust FFI wrappers#38005robobun wants to merge 2 commits into
Conversation
…FFI wrappers Drops the inert ENABLE(BINDING_INTEGRITY) scaffolding from 19 generated-style bindings (the vtable check it existed for has been commented out since the files were imported), the EnumTraits tables for HTTPHeaderName and ExceptionCode, IDL converter specializations nothing instantiates (JSDOMConvertPromise.h and JSDOMConvertNull.h go away entirely), ExceptionOr<T&>, the unused opaque-root helpers, and a number of declarations, enumerators, macros and never-taken preprocessor branches in the JSC bindings. On the Rust side removes the unreachable Heap methods in bun_mimalloc_sys, Compressor/Decompressor::destroy in bun_libdeflate_sys, App::run/listen and their imports in bun_uws_sys, and two bun_css leftovers, plus 13 hawk.toml overrides for StatusFlag variants that no longer exist. A source lint pins the removed symbols.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 14 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 (71)
Comment |
|
Updated 1:05 AM PT - Aug 13th, 2026
❌ @autofix-ci[bot], your commit 5f986e2 has some failures in 🧪 To try this PR locally: bunx bun-pr 38005That installs a local version of the PR into your bun-38005 --bun |
|
Status (head 5f986e2): ready for a maintainer. Build 93944 finished with every job that ran green; its only red marks are the four darwin test shards, which never ran (macOS queue backlog, details below), so I am not retriggering.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it removes ~1,500 lines across 71 files in core JSC/WebCore bindings and IDL converter templates, a human look would still be worthwhile before landing.
What was reviewed:
- Confirmed
toJS<IDLPromise<...>>in JSSubtleCrypto short-circuits on void functors (JSDOMConvertBase.h:180/210), so removingJSDOMConvertPromise.his safe. - Confirmed
IDLNullremains defined inIDLTypes.handConditionalConverter<.., IDLNull, false>inJSDOMConvertUnion.hnever touches the removed converter. - Confirmed
jsFunctionBakeGetAsyncLocalStorageis still reached via$newCppFunctionin BakeSSRResponse.ts; only the never-readm_bakeGetAsyncLocalStorageLazyProperty was dropped. - Spot-checked
AbortController::opaqueRoot()returnsWebCoreOpaqueRootby value (matches the sole remaining overload), and thatOnLoadResult::wasMock,m_asUint8Array,DeclarationContext::Keyframes, and the removed BunCommonStrings entries have no remaining readers (including via token pasting / codegen).
Extended reasoning...
Overview
This PR removes ~1,501 lines of unreferenced code across 71 files: inert ENABLE(BINDING_INTEGRITY) scaffolding in 19 generated-style WebCore binding .cpp files, unused IDL converter template specializations (JSDOMConvert*.h), EnumTraits tables, opaque-root helper overloads, unused BunCommonStrings entries, declarations without definitions, unreferenced accessors/fields, dead Rust FFI wrappers in mimalloc_sys/libdeflate_sys/uws_sys, one CSS enum variant, and stale hawk.toml overrides. The only non-deletion is the trailing-comma fix on the last macro entry in BunCommonStrings.h. A new source-lint test pins every removal.
Security risks
None identified. This is pure removal of never-instantiated templates, never-called functions, never-read fields, and preprocessor-dead branches. No control flow, validation, or crypto logic is altered — the RSA-PSS change just unwraps #if 1, and the OpaqueRoot removals leave the one live addWebCoreOpaqueRoot(Visitor&, WebCoreOpaqueRoot) overload intact.
Level of scrutiny
Medium-high. While every individual removal is mechanical and the compiler/linker would catch most mistakes, the change spans template-heavy IDL converters (where a missing specialization surfaces as a confusing instantiation error only when someone later adds a caller), GC opaque-root machinery, and struct-layout-affecting field removals (JSOneShotDirectSink::m_asUint8Array, OnLoadResult::wasMock). The PR description is unusually thorough — every removal is enumerated with its verification method (rg across src/packages/scripts/test/codegen, hawk cross-crate analysis, checked against open PRs). I spot-checked the four riskiest claims (IDLPromise void short-circuit, IDLNull in unions, the Bake LazyProperty, opaque-root overload resolution) and all held.
Other factors
The PR includes a source-lint test that fails on main and passes here, and the description reports a full bun bd build passing plus targeted test runs. CI is still building (#93944). I'm deferring rather than approving because 71 files across core JSC bindings is above my comfort threshold for auto-approval — a maintainer may also want to weigh in on whether any of the removed IDL converter specializations or common strings were intentionally staged for near-term work, and whether the coordination with the several open dead-code PRs (#37062, #37149, #37229, #37328, #37332) is as claimed.
|
Thanks for the pass. On the two points left for a human:
|
2 participants
Removes 1,501 lines of code nothing references (one line outside the new lint is modified, the tail of a macro list), across the WebCore/JSC C++ bindings, a few Rust FFI wrapper crates, and stale hawk.toml overrides. No behavior change: everything removed was either never compiled into anything, never instantiated, or written and never read.
How the candidates were found
hawkanalysis configured inhawk.toml(release profile unioned over the 11 shipped targets) run against current main. Almost all of its 1218dead_publicfindings sit in files that one of the open dead-code PRs already touches; those were left alone, and the remainder was re-checked withrg(hawk only analyzes the release profile, so e.g.ntstatus::ACCESS_DENIED, which has a debug-only reader, was kept).rgacrosssrc/,packages/,scripts/,test/and freshly regeneratedbuild/debug/codegen/, and symbol by symbol against the diffs of the open PRs that touch the same files (no overlap;JSDOMConvert.his also touched by Remove dead code from webcore bindings, watcher, node-fallbacks, and misc crates #37062, on a different include line).src/js/was scanned as well and is clean (oxlint's unused checks keep it that way); nothing from there is in this PR.Removed
ENABLE(BINDING_INTEGRITY)scaffolding in 19 generated-style bindings (536 lines)JSAbortController,JSAbortSignal,JSBroadcastChannel,JSCloseEvent,JSCustomEvent,JSDOMFormData,JSDOMURL,JSMessageChannel,JSMessageEvent,JSMessagePort,JSPerformanceMark,JSPerformanceObserver,JSPerformanceObserverEntryList,JSPerformanceServerTiming,JSPerformanceTiming,JSTextEncoder,JSWebSocket,JSWorker,JSSubtleCrypto(.cpp). TheRELEASE_ASSERT(actualVTablePointer == expectedVTablePointer)these blocks exist for has been commented out since the files were imported, so the file-scope block only declared an unused_ZTV...vtable symbol and theif constexpr (std::is_polymorphic_v<T>)body intoJSNewlyCreatedwas empty after preprocessing (its only remaining statement sits underPLATFORM(WIN), which is never true in aBUILDING_JSCONLY__build).JSURLPattern.cppstill performs the check and is untouched;JSFetchHeaders.cpp/JSPerformance.cpphave the same blocks but are claimed by open PRs.IDL conversion layer (
webcore/JSDOMConvert*.h)JSDOMConvertPromise.h(whole file) and its include inJSSubtleCrypto.cpp: everytoJS<IDLPromise<...>>call there passes avoid-returning functor, whichJSDOMConvertBase.hshort-circuits without instantiating the converter;Converter<IDLPromise>had no callers. Cascade:DOMPromise::create(),promise()and the constructor inJSDOMPromise.h(only the staticwhenPromiseIsSettledremains in use).JSDOMConvertNull.h(whole file) and its includes inJSDOMConvert.h/JSDOMConvertUnion.h: no union namesIDLNull, andConditionalConverter<.., IDLNull, false>never touches the converter.Converter/JSConverterforIDLFloatandIDLUnrestrictedFloat(codegen only emits the double variants),JSConverter<IDLByteString>,JSConverter<IDLObject>,JSConverter<IDLAtomStringAdaptor<T>>, bothIDLRequiresExistingAtomStringAdaptor<T>converters, bothIDLCallbackInterface<T>converters,JSConverter<IDLCallbackFunction<T>>and the two-argumentConverter<IDLCallbackFunction<T>>::convertoverload (the one live caller,JSPerformanceObserver.cpp, uses the global-object overload), theUncachedString/OwnedStringoverloads of the DOMString / USVString converters (nothing constructs those adaptors), andVariadicConverter<IDLAny>/VariadicConverter<IDLInterface<T>>(their consumer,convertVariadicArguments, lives in a header nothing includes).CastedThisErrorBehavior::ReturnEarly(JSDOMCastThisValue.h).Other JSC / WebCore binding helpers
WTF::EnumTraits<HTTPHeaderName>(98 lines) andEnumTraits<ExceptionCode>:EnumTraits<E>::valuesis only consumed byEnumeratedArray,OptionSetandisZeroBasedContiguousEnum, none of which is used with either enum.ExceptionOr<T&>(never instantiated), theisolatedCopy(ExceptionOr<void>&&)/isolatedCopy(Exception&&)free functions (crossThreadCopyuses the member),ExceptionOr::m_wasReleased,Exception::extra().ImplType*/ImplType&forwarding overloads ofaddWebCoreOpaqueRoot, all threecontainsWebCoreOpaqueRootoverloads, thenullptr_tconstructor, and theroot(MessagePort*)/root(CryptoKey*)functions only those overloads could reach (plus the includes / forward declarations that existed for them). The one live user,JSAbortController, passes aWebCoreOpaqueRootvalue.BunCommonStrings.h:ConnectionWasClosed,OperationFailed,OperationTimedOut,ec,ed25519,rsa,rsaPss,jwkDsa,jwkG,systemError,x25519(each one expands to aLazyProperty, accessor, initializer and GC visit; thehttp*entries are reached via token pasting and stay).EventLoopTask(Function<void()>&&)(every construction site passes aFunction<void(ScriptExecutionContext&)>),m_bakeGetAsyncLocalStorage(initialized and visited, never read) and a duplicateLazyPropertyOfGlobalObjectalias inBakeAdditionsToGlobalObject.h.TaskSource(onlyPostedMessageQueue/WebSocketare used), eight ofEventListener::Type(JSEventListeneris the only subclass).#if 1 ... #elseinCryptoAlgorithmRSA_PSSOpenSSL.cpp,#if ENABLE(OFFSCREEN_CANVAS)/#if ENABLE(MEDIA_SOURCE)includes of headers that do not exist in this tree (JSEventTargetCustom.cpp,JSDOMURL.cpp).CryptoKeyOKP::platformExportSpki/platformExportPkcs8,SecretKeyJobCtx::result(),KeyPairJobCtx::deinit(), and the never-definedtemplate<typename Visitor> visitChildrendeclarations next toDECLARE_VISIT_CHILDRENinJSNodePerformanceHooksHistogram.handJSX509Certificate.h.JSNodePerformanceHooksHistogram::histogram()(+ a commented-out declaration),NodeVMSourceTextModule::cachedExecutable(),Worker::isOnline(),OnLoadResult::wasMockand its one store,JSOneShotDirectSink::m_asUint8Arrayand its one store (the flag reaches JS throughstartOptions),expectedEnumerationValues<BufferEncodingType>(noIDLEnumeration<BufferEncodingType>anywhere),G_FALSE/G_TRUEinSecretsLinux.cpp,BUN_WRAP_FWD_VOIDinworkaround-missing-symbols.cpp.Rust
bun_mimalloc_sys:Heap::malloc/calloc/reallocand themi_heap_calloc/mi_heap_reallocimports only they used (callers go through the rawmi_heap_*functions with a*mut Heap;MimallocArenadeliberately never hands out a&Heap).bun_libdeflate_sys:Compressor::destroy/Decompressor::destroy(the RAII wrappers free directly).bun_uws_sys:App::run,App::listenand theuws_app_run/uws_app_listenimports behind them (the server listens throughlisten_with_config),uws_app_listen_config_t::new, and a duplicateNewAppalias inApp.rs(the live one is inlib.rs).bun_css:DeclarationContext::Keyframes, and the emptygenerated_color_conversionsmarker module whoseuseno longer exists.hawk.toml: 13 overrides formysql::status_flags::StatusFlag::*variants that #37229 deleted; hawk now reports them asunknown_item. (The sixbun_platformones in the same state are removed by #37328.)test/internal/source-lints/dead-symbols-binding-integrity-idl-convert.test.tspins the removed symbols so they cannot quietly come back; it fails onmainand passes here.Verification
bun bd(full debug/ASAN build) passes with the removals; the new source lint fails withsrc/reset tomainand passes on the branch;bun bd test test/internal/source-lints/passes (87 tests).bun bd testpasses ontest/js/web/abort/abort.test.ts,workers/{message-channel,message-event,performance-observer-leak}.test.ts,encoding/text-encoder.test.js,html/FormData.test.ts,url/url.test.ts,broadcastchannel/broadcast-channel.test.ts,crypto/web-crypto.test.ts,streams/streams.test.js,test/js/node/perf_hooks/perf_hooks.test.ts,test/js/bun/perf_hooks/histogram.test.ts,test/js/node/crypto/x509.test.ts,test/js/node/zlib/zlib.test.js,test/js/bun/test/mock/mock-module.test.ts, and on everything intest/js/web/websocket/websocket.test.js,test/js/node/crypto/crypto.key-objects.test.ts,test/js/bun/css/color.test.tsandtest/js/web/workers/worker.test.tsexcept for tests that need the public internet or that spawn subprocesses and time out at their 5 s budget on this (heavily loaded) machine; the sevenworker.test.tsterminate() tests fail identically on a debug build withsrc/jscreset tomain, and the other timeouts hit a different subset on every run.cargo checkof the touched crates passes; the removed items had no readers in anycfg(hawk unions all 11 targets, and thergchecks are cfg-agnostic), so the per-targetcargo checkpass is left to CI.Left alone on purpose (follow-ups, not in this diff)
WorkerMessagingProxy::isOnline()is now unreferenced, but that header is touched by Remove dead code from libuv_sys, cares_sys, simdutf FFI, test_runner, and C++ bindings #37332 on the neighbouring lines.StringAdaptors.h(UncachedString/OwnedString) has no remaining construction sites, butIDLTypes.h(claimed by an open PR) still names the types.TextCodec::encodeside of the text codecs (~700 lines acrossTextCodecCJK/TextCodecSingleByte/TextCodecUserDefined/UnencodableHandling.h) is only reachable fromTextEncoding::encodeForURLParsing, which has no callers;TextEncoding.h/.cppare claimed by Remove dead code from libuv_sys, cares_sys, simdutf FFI, test_runner, and C++ bindings #37332, so this should go in one piece once that lands.DOMPromise/JSDOMPromise.cppas a whole become dead once Remove dead code from webcore bindings, watcher, node-fallbacks, and misc crates #37062 removesDeferredPromise::whenSettled.bun_install::read_without_launch(the shell-side.bunxfast path, whose caller was not carried over in the Rust port), a handful ofbun_jschelpers added four days ago in 9d519e8 (JSPromisesettle_task/resolve_task,VM::has_termination_request,LoopHandle::accepting_work/ref_keep_alive/unref_keep_alive,job.rsaccessors), andbun_sys::ErrorCase::LeakFdOnFail; these look like API surface still settling rather than leftovers, so they are not touched here.