Remove dead code from webcore HTTP/timing C++ and install::dependency - #36621
Jarred-Sumner merged 5 commits into
Conversation
Deletes unused WebKit-derived C++ in src/jsc/bindings/webcore/ that Bun never reaches, plus one uncalled Rust trait method. Verified by rg across src/ and build/debug/codegen/ and a successful debug build. C++ whole-file deletions: - RFC7230.cpp/.h: global-namespace duplicate of WebCore::RFC7230 in HTTPHeaderField.cpp. HTTPParsers.cpp includes HTTPHeaderField.h and resolves RFC7230::isTokenCharacter to the WebCore::RFC7230 copy, so the ::RFC7230 copy is compiled but never referenced. - CommonAtomStrings.cpp/.h: initializeCommonAtomStrings() never called and none of the generated atoms (noneAtom, autoAtom, ...) referenced. - DOMPromiseProxy.h: header only #included vestigially by JSSubtleCrypto.cpp with no instantiation of DOMPromiseProxy<> or DOMPromiseProxyWithResolveCallback anywhere. - ServerTiming.cpp: contained only ServerTiming::isolatedCopy, whose sole caller was ResourceTiming::isolatedCopy (also removed here). C++ members removed: - HTTPHeaderField class + create() and every RFC7230 helper it transitively used (isDelimiter, isWhitespace, isVisibleCharacter, isInRange, isOBSText, isQuotedTextCharacter, isQuotedPairSecondOctet, isCommentText, isValidName, isValidValue, the SIMD nibble tables, vectorIsAllTokenCharacters, containsOnlyTokenCharacters). Only isTokenCharacter (both overloads) is kept for HTTPParsers.cpp. - NetworkLoadMetrics: parameterized ctor, updateFromFinalMetrics, emptyMetrics, isolatedCopy; AdditionalNetworkLoadMetricsForWebInspector parameterized create()/ctor and isolatedCopy. All uncalled. - ResourceTiming::isolatedCopy (both ref-qualifications), its private 4-arg ctor, and the long-dead commented-out factory stubs. - ServerTiming::isolatedCopy declarations. - HTTPHeaderMap: isolatedCopy (both), the #if USE(CF) set(CFStringRef) overload, addIfNotPresent, and the CommonHeader/UncommonHeader/ HTTPHeaderMap encode/decode templates. None instantiated. - FetchHeaders::filterAndFill: declared + defined, zero callers. Rust: - install::dependency::VersionExt::is_less_than (trait decl + impl for Version): zero callers; only is_less_than_with_tag is used (by the Yarn lock printer). Dependency::is_less_than is an unrelated function. Adds test/internal/source-lints/dead-symbols-webcore-timing-semver.test.ts to lint the source tree for reintroduction.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 26 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 (19)
Comment |
|
Updated 12:57 AM PT - Aug 2nd, 2026
@Jarred-Sumner, your commit d024c78 is building: |
|
Status: build passes locally ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The only consumers of crossThreadCopy() in this TU were the two HTTPHeaderMap::isolatedCopy() overloads removed earlier in this PR.
There was a problem hiding this comment.
No issues found — my earlier nit (leftover CrossThreadCopier.h include) was addressed in e90ebae. Deferring only because of scale: 20 files / -1447 LOC across the WebCore bindings is worth a quick human glance even for pure dead-code removal.
What was reviewed:
- Grepped
src/for every removed symbol (filterAndFill,addIfNotPresent,updateFromFinalMetrics,emptyMetrics,CommonAtomStrings,DOMPromiseProxy,RFC7230.h) — zero remaining references. - Confirmed
HTTPParsers.cpponly needsWebCore::RFC7230::isTokenCharacter, which is retained inHTTPHeaderField.h. - Verified all
.is_less_than(call sites insrc/install/resolve toDepSorter/TreeDepsSortCtx, not the removedVersionExt::is_less_than. - The
&→&reflow inHTTPHeaderMap.his autofix.ci clang-format only; no build-file references to the deleted.cppfiles.
Extended reasoning...
Overview
Pure dead-code removal across src/jsc/bindings/webcore/ (WebKit-derived C++ Bun never reaches) plus one uncalled Rust trait method in src/install/dependency.rs. Six whole files deleted (RFC7230.{h,cpp}, CommonAtomStrings.{h,cpp}, DOMPromiseProxy.h, ServerTiming.cpp); the rest are member/template removals (isolatedCopy, encode/decode, addIfNotPresent, filterAndFill, updateFromFinalMetrics, emptyMetrics, the HTTPHeaderField class, and unused RFC7230 helpers). One include dropped from JSSubtleCrypto.cpp. A source-lint test guards against reintroduction, following the existing test/internal/source-lints/dead-symbols-*.test.ts pattern. Net -1377 LOC.
Security risks
None. This is subtractive only — no new logic, no changed control flow in reachable code. The only surviving edit to a live path is removing an unused #include from JSSubtleCrypto.cpp, which does not affect the compiled crypto surface.
Level of scrutiny
Medium. Dead-code deletion is mechanical and self-verifying via the linker (bun bd succeeds), and this PR is the third in a series (#36474, #36576 already on main). I spot-checked the highest-risk claims: HTTPParsers.cpp resolves RFC7230::isTokenCharacter to the retained WebCore::RFC7230 overloads, so deleting the global-namespace duplicate is safe; every .is_less_than( call site in src/install/ is on DepSorter/TreeDepsSortCtx, not the removed VersionExt method; no CMake/build file lists the deleted .cpp files by name. The large HTTPHeaderMap.h diff hunk is mostly autofix.ci reflowing Type & → Type& — no semantic change.
Other factors
My previous review's only finding (leftover <wtf/CrossThreadCopier.h> include in HTTPHeaderMap.cpp) was fixed in e90ebae and the thread is resolved. The bug-hunting system found nothing this run. headers.test.ts passes and the new source-lint test follows the established directory convention. I'm deferring rather than approving purely on size — 20 files across the JSC/WebCore binding layer is enough surface that a maintainer glance is cheap insurance, but I have no substantive concerns.
The fail-before check restores main's src/ into the working tree and then
restores this branch's src/ on top; that second step does not remove files
that exist on main but not here, so a deleted ServerTiming.cpp was left
resurrected alongside this branch's ServerTiming.h (which drops the
isolatedCopy declarations) and the rebuild failed with an out-of-line
definition error. Reducing the .cpp to an include-only TU keeps the
filename present so the restore treats it as a modification rather than a
deletion. RFC7230.{cpp,h}, CommonAtomStrings.{cpp,h} and DOMPromiseProxy.h
are still deleted outright: when resurrected they compile as
self-contained pairs (or are not #included at all) and do not break the
build.
Also drop the existsSync() assertions from the source-lint test and add a
content check for ServerTiming.cpp. Whole-file deletions are now asserted
indirectly via surviving includers, matching the other
dead-symbols-*.test.ts files.
There was a problem hiding this comment.
No issues found — the earlier nit (leftover CrossThreadCopier.h include) was addressed in e90ebae. Deferring only because of surface area: -1377 LOC across 20 files including FetchHeaders/HTTPHeaderMap/HTTPHeaderField warrants a maintainer glance to confirm none of these WebKit-derived symbols are being intentionally kept for upstream-tracking.
What was reviewed:
- Grepped
src/for every removed symbol (filterAndFill,addIfNotPresent,updateFromFinalMetrics,emptyMetrics,DOMPromiseProxy,CommonAtomStrings,RFC7230.h) — zero remaining references. - Confirmed
HTTPParsers.cpponly usesRFC7230::isTokenCharacter, which is retained inHTTPHeaderField.{h,cpp}. - Confirmed
VersionExt::is_less_thanhas zero callers (allis_less_thanhits are onDependency,Indent,StableRef, orDepSorter— different types). - Checked
cmake/andscripts/for hardcoded references to the deleted.cpp/.hfiles — none;ServerTiming.cppkept as empty TU covers the glob case.
Extended reasoning...
Overview
Pure dead-code removal across src/jsc/bindings/webcore/ (C++ WebKit-derived HTTP/timing classes) plus one uncalled Rust trait method in src/install/dependency.rs. Net -1377 LOC. Whole-file deletions: RFC7230.{cpp,h}, CommonAtomStrings.{cpp,h}, DOMPromiseProxy.h. Member removals from HTTPHeaderField, HTTPHeaderMap, NetworkLoadMetrics, ResourceTiming, ServerTiming, FetchHeaders. A source-lint regression test is added following the existing test/internal/source-lints/dead-symbols-*.test.ts pattern. My earlier nit about the leftover <wtf/CrossThreadCopier.h> include was fixed in e90ebae, and 3cbd383 reverted ServerTiming.cpp from full deletion to an empty TU (config.h + own header only).
Security risks
None. This is deletion-only; no new parsing, no new input handling, no auth/crypto logic. The only kept RFC7230 helper (isTokenCharacter) is unchanged. The HTTPHeaderMap.h diff includes autofix.ci whitespace normalization (Type & → Type&) which is cosmetic.
Level of scrutiny
Medium-high. While deletion-only PRs are inherently lower risk (the linker catches missing symbols, and bun bd succeeds), FetchHeaders and HTTPHeaderMap are on the hot path for the JS Headers API and fetch(). I re-verified each removed symbol has zero references across src/, that HTTPParsers.cpp resolves RFC7230::isTokenCharacter to the retained WebCore::RFC7230 copy in HTTPHeaderField.h, and that the removed Rust VersionExt::is_less_than is distinct from the still-used Dependency::is_less_than. All checks pass.
Other factors
This is a continuation of the dead-code series (#36474 landed just before it). The verification methodology is thorough (rg across src/ + build/debug/codegen/, build passes, headers.test.ts 99/99). The added source-lint test matches sibling files in test/internal/source-lints/. I'm deferring rather than approving solely because of scale (20 files) and because a maintainer should confirm there's no intent to keep these WebKit-derived stubs for easier upstream diffing — that's a project-policy call, not a correctness one.
…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>
There was a problem hiding this comment.
Beyond the inline nit: verified no remaining #includes of RFC7230.h/CommonAtomStrings.h/DOMPromiseProxy.h anywhere under src/, and confirmed the removed VersionExt::is_less_than is distinct from DependencyExt::is_less_than — the Dependency::is_less_than(buf, a, b) call sites in lockfile/Package.rs resolve to the latter (different arg types) and are unaffected. Note: d024c78 deleted the source-lint test the description still cites as verification.
Extended reasoning...
This is my second pass on the PR. The prior CrossThreadCopier.h nit was addressed in e90ebae. This round's only finding is orphaned forward declarations in ResourceTiming.h — inert at compile and runtime. I additionally spot-checked the highest-risk deletions: (1) the whole-file removals have zero remaining includers in src/, (2) the Rust trait-method removal cannot collide with the same-named Dependency method because the receiver types differ, and (3) the many isolatedCopy hits elsewhere in src/jsc are on unrelated types (crypto params, Worker, SerializedScriptValue). Not approving given the ~1400 LOC sweep across HTTPHeaderMap/FetchHeaders/RFC7230 (live fetch/Headers surface) plus the mixed-in reformatting of HTTPHeaderMap.h — worth a human skim.
| public: | ||
| // static ResourceTiming fromMemoryCache(const URL&, const String& initiator const ResourceResponse&, const NetworkLoadMetrics&, const SecurityOrigin&); | ||
| // static ResourceTiming fromLoad(CachedResource&, const URL&, const String& initiator const NetworkLoadMetrics&, const SecurityOrigin&); | ||
| // static ResourceTiming fromSynchronousLoad(const URL&, const String& initiator const NetworkLoadMetrics&, const ResourceResponse&, const SecurityOrigin&); | ||
|
|
||
| const URL& url() const { return m_url; } |
There was a problem hiding this comment.
🟡 Deleting the commented-out fromMemoryCache/fromLoad/fromSynchronousLoad/updateExposure stubs removes the last textual references to CachedResource, ResourceResponse, and SecurityOrigin in this header — the forward declarations at lines 36/38/40 are now orphaned. Same pattern as the CrossThreadCopier.h include already dropped from HTTPHeaderMap.cpp in e90ebae; drop these three too (PerformanceServerTiming and ResourceLoadTiming are still used and should stay).
Extended reasoning...
What
After this PR, src/jsc/bindings/webcore/ResourceTiming.h still contains:
class CachedResource; // line 36
class PerformanceServerTiming; // line 37 — still used
class ResourceResponse; // line 38
class ResourceLoadTiming; // line 39 — still used
class SecurityOrigin; // line 40but nothing left in the header names CachedResource, ResourceResponse, or SecurityOrigin. The remaining class body uses only URL, String, ResourceLoadTiming, NetworkLoadMetrics, ServerTiming, Vector, Ref, and PerformanceServerTiming.
Step-by-step
- Before this PR, the only appearances of
CachedResource,ResourceResponse, andSecurityOrigininResourceTiming.hwere inside the commented-out declarations at the top of thepublic:block (fromMemoryCache/fromLoad/fromSynchronousLoad) and nearoverrideInitiatorType(updateExposure). - The diff removes all four commented-out lines (
-// static ResourceTiming fromMemoryCache(...),-// static ResourceTiming fromLoad(CachedResource&, ...),-// static ResourceTiming fromSynchronousLoad(...),-// void updateExposure(const SecurityOrigin&);). - Grepping the post-diff header for
CachedResource,ResourceResponse, orSecurityOriginyields only the forward-declaration lines themselves. PerformanceServerTiming(line 37) is still referenced byVector<Ref<PerformanceServerTiming>> populateServerTiming() const;andResourceLoadTiming(line 39) bym_resourceLoadTiming/resourceLoadTiming(), so those two stay.
Why existing code doesn't catch it
Unused forward declarations compile cleanly with no warning. The source-lint test added in this PR (dead-symbols-webcore-timing-semver.test.ts) checks ResourceTiming.h for reintroduction of fromMemoryCache and isolatedCopy but does not check for these forward declarations, so it won't flag them either.
Relationship to this PR
Strictly, these forward declarations were already dead pre-PR — comments don't require forward declarations to compile — so the PR didn't make them dead in a compiler sense. But this is a dead-code-removal PR that is directly editing ResourceTiming.h, and it just removed the only textual justification for those declarations existing. Leaving them behind is inconsistent with the PR's own cleanup elsewhere: it already dropped #include <wtf/CrossThreadCopier.h> from ResourceTiming.cpp and HTTPHeaderMap.cpp (the latter in response to review, e90ebae), and #include <wtf/NeverDestroyed.h> from NetworkLoadMetrics.cpp, all under the same "delete what became unreferenced" rationale. Per REVIEW.md — "Delete dead code in the same PR that makes it dead (required scope)" — these should go too.
Impact
None at runtime or compile time — forward declarations of unreferenced types are inert. This is purely a hygiene/consistency nit, not a blocking defect.
Fix
namespace WebCore {
-class CachedResource;
class PerformanceServerTiming;
-class ResourceResponse;
class ResourceLoadTiming;
-class SecurityOrigin;2 participants
Deletes unused WebKit-derived C++ in
src/jsc/bindings/webcore/that Bun never reaches, plus one uncalled Rust trait method. Net -1377 LOC (+70/-1447). Verified byrgacrosssrc/andbuild/debug/codegen/and a successfulbun bddebug build;test/js/web/fetch/headers.test.tspasses.C++ whole-file deletions
RFC7230.cpp/RFC7230.h(239 LOC): global-namespace duplicate ofWebCore::RFC7230inHTTPHeaderField.cpp.HTTPParsers.cppincludesHTTPHeaderField.hand resolvesRFC7230::isTokenCharacterto theWebCore::RFC7230copy, so the::RFC7230copy was compiled but never linked against.CommonAtomStrings.cpp/CommonAtomStrings.h(128 LOC):initializeCommonAtomStrings()is never called and none of the generated atoms (noneAtom,autoAtom,textPlainContentTypeAtom, ...) are referenced.DOMPromiseProxy.h(370 LOC): only#included vestigially byJSSubtleCrypto.cppwith no instantiation ofDOMPromiseProxy<>,DOMPromiseProxyWithResolveCallback, orIDLDOMPromiseProxyanywhere.ServerTiming.cpp(41 LOC): contained onlyServerTiming::isolatedCopy, whose sole caller wasResourceTiming::isolatedCopy(also removed here).C++ members removed
HTTPHeaderField.h/.cpp:class HTTPHeaderField+create()and everyWebCore::RFC7230helper it transitively used (isDelimiter,isWhitespace,isVisibleCharacter,isInRange,isOBSText,isQuotedTextCharacter,isQuotedPairSecondOctet,isCommentText,isValidName,isValidValue, the SIMD nibble tables,vectorIsAllTokenCharacters,containsOnlyTokenCharacters). OnlyisTokenCharacter(both overloads) is kept; it's the only exportHTTPParsers.cppactually uses.NetworkLoadMetrics.h/.cpp: parameterized ctor,updateFromFinalMetrics,emptyMetrics,isolatedCopy;AdditionalNetworkLoadMetricsForWebInspectorparameterizedcreate()/ctor andisolatedCopy. All uncalled.ResourceTiming.h/.cpp:isolatedCopy(both ref-qualifications), its private 4-arg ctor, and the commented-outfromMemoryCache/fromLoad/fromSynchronousLoad/updateExposurestubs.ServerTiming.h:isolatedCopydeclarations.HTTPHeaderMap.h/.cpp:isolatedCopy(both), the#if USE(CF)set(CFStringRef)overload,addIfNotPresent, and theCommonHeader/UncommonHeader/HTTPHeaderMapencode<>/decode<>templates. None instantiated.FetchHeaders.h/.cpp:filterAndFill: declared + defined, zero callers.Rust
install::dependency::VersionExt::is_less_than(trait decl + impl forVersion): zero callers; onlyis_less_than_with_tagis used (by the Yarn lock printer).Dependency::is_less_thanis an unrelated function on a different type.Verification
rgeach removed symbol acrosssrc/,build/debug/codegen/, andsrc/codegen/: zero references outside own definition/decl pair.bun bdsucceeds.bun bd test test/js/web/fetch/headers.test.ts: 99 pass.bun bd test test/js/bun/http/serve.test.ts: 280 pass; the 4 failures (requestIP v6, root-range port,#6583, loopback/bun:info) reproduce identically onmainin this environment.test/internal/source-lints/dead-symbols-webcore-timing-semver.test.tsadded to guard against reintroduction.Follow-ups (not in this diff)
AdditionalNetworkLoadMetricsForWebInspectorand theRefPtr<>field onNetworkLoadMetricsare never populated (nothing assigns a non-null value). Removing them would cascade intoNetworkLoadPriorityand theHTTPHeaderMap.hinclude; left for a separate pass.HTTPHeaderMap::append(const String&, const String&)appears to have zero callers (FetchHeaders::appenddoes not route through it) but left in place pending a more careful audit.[review] gate passed · iteration 1 · 20 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file