Remove dead code from llhttp/api.h, C++ bindings helpers, bun_core/bun_jsc Rust - #36576
Conversation
…erValues, JSDOMConvertJSON/WebGL, JSVMClientDataClient, headers-cpp.h, ares_build.h, TaskSource, headers-handwritten, JSDOMWrapper) and Rust (install LifecycleScriptTimeLog chain, bun_core String::github_action, jsc JSUint8Array::ptr/RefString::to_js/Errorable::value) Net -1295 lines. Every removed symbol was verified with zero references across src/ and build/debug/codegen/ (or, for whole files, zero #include anywhere in the tree), then confirmed by a full debug build and rust:check-all across all target triples.
|
Status: diff is green. Review nits addressed (WeakHashSet include, IDLJSON struct, sizegen.cpp description). Per Jarred's comment, reverted the |
WalkthroughThe pull request removes unused Rust APIs, JavaScriptCore and WebCore binding declarations, conversion helpers, generated headers, and lifecycle client tracking. It also adds source-tree lint checks for removed symbols. ChangesDead code cleanup
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two inline nits, I spot-checked the load-bearing deletion claims: addClient/JSVMClientDataClient have zero references; LifecycleScriptTimeLog/lifecycle_script_time_log are unreferenced; all four .github_action() call sites in VirtualMachine.rs are on jsc::ZigString (not bun_core::String); the removed helpers.h symbols, WritableEvent/ReadableEvent__, BUN_FFI_POINTER_OFFSET_TO_TYPED_ARRAY_VECTOR, and the deleted headers (llhttp/api.h, TaskSource.h, ares_build.h, HTTPHeaderValues.h) have no remaining includers/callers; and the JSDocumentWrapperType group appears only in commented-out DOMJITHelpers.h lines. The one textPlainContentType grep hit in CommonAtomStrings.h is an unrelated atom-string macro entry, not a call into the deleted namespace.
Extended reasoning...
Spot-checked the deletion claims that back the highest-risk removals (BunClientData VM-lifecycle field, PackageManager struct field, bun_core::String method) against src/ — all hold. The two inline findings are cosmetic (an inaccurate PR-description parenthetical about an already-uncompilable non-built file, and a now-superfluous header include); neither affects the build or runtime. Deferring rather than approving given the breadth (26 files spanning JSC bindings + install crate + bun_core), which warrants a human glance even for pure deletions.
…urce-lint: assert via surviving-file content, not file presence
![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-llhttp-helpers-install.test.ts`:
- Around line 22-28: Add direct regression assertions for the deleted
unreferenced headers api.h, ares_build.h, TaskSource.h, and HTTPHeaderValues.h
in the dead-symbols test, verifying each path is absent from the source tree.
Keep the existing indirect build-based checks for files with surviving
references unchanged.
🪄 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: ed252ea3-6963-4717-a9c3-5e66e51031f1
📒 Files selected for processing (3)
src/jsc/bindings/BunClientData.hsrc/jsc/bindings/IDLTypes.htest/internal/source-lints/dead-symbols-llhttp-helpers-install.test.ts
💤 Files with no reviewable changes (2)
- src/jsc/bindings/IDLTypes.h
- src/jsc/bindings/BunClientData.h
There was a problem hiding this comment.
All three earlier nits are addressed (WeakHashSet.h include, sizegen.cpp description, IDLJSON) and I found nothing new this pass. Deferring to a human given the breadth — 27 files / -1.3k lines across core JSC bindings (BunClientData dtor, helpers.h, headers-handwritten.h, IDLTypes.h) plus a PackageManager struct field.
Spot-checked: addClient/JSVMClientDataClient, the removed helpers.h overloads, HTTPHeaderValues/TaskSource.h/ares_build.h includes, WritableEvent__/ReadableEvent__, and LifecycleScriptTimeLog — all zero live references.
Confirmed the JSDOMWrapper.h node-type constants only appear in commented-out DOMJITHelpers.h:173-178, and bun_core/lib.rs re-exports only the kept ZigStringGithubActionFormatter.
Checked llhttp/api.h has no includers and BUN_FFI_POINTER_OFFSET_TO_TYPED_ARRAY_VECTOR has no remaining readers.
Extended reasoning...
Overview
Pure dead-code removal: -1296 / +80 across 27 files. Eight whole-file C++ header/source deletions (llhttp/api.h, headers-cpp.h, ares_build.h, TaskSource.h, HTTPHeaderValues.{h,cpp}, JSDOMConvertJSON.h, JSDOMConvertWebGL.{h,cpp}, JSVMClientDataClient.h), targeted symbol removals from helpers.h / headers-handwritten.h / JSDOMWrapper.h / BunClientData.{h,cpp} / IDLTypes.h, and Rust removals (LifecycleScriptTimeLog chain in install/, String::github_action, JSUint8Array::ptr, RefString::to_js, Errorable::value, one sizes.rs constant). A new source-lint test guards against reintroduction.
Security risks
None. This PR only deletes code and adds a read-only source-lint test; no new inputs, no auth/crypto/parsing surface touched.
Level of scrutiny
Medium-high, driven by breadth rather than logic complexity. Each individual removal is mechanically verifiable (grep + build), and the description documents per-item verification plus bun bd and rust:check-all across all targets. But the touched files are widely-included JSC bindings headers and a package-manager struct, so a wrong claim would have broad blast radius. I re-verified the load-bearing zero-reference claims (see message above) and all held; the only remaining live references to removed JSDOMWrapper.h constants are in commented-out code, matching the description.
Other factors
I've reviewed this PR three times previously; all three nits (stray <wtf/WeakHashSet.h> include, misleading "kept compilable" wording for sizegen.cpp, orphaned IDLJSON struct) were addressed in 91f57fe and 23b3d1a, and the source-lint test was extended to cover them. The bug-hunting system found nothing this run. The description also notes coordination with 11 other open dead-code PRs and a stated intent to leave the sizegen.cpp loop body for #36115 to rework. Given the scope and the number of core headers touched, a maintainer should give the final sign-off; I'm not blocking on anything specific.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Do we not log slow lifecycle scripts? We’re supposed to!
…ipt warning, not dead code The chain is currently non-functional on main (timer never started, Entry is empty, printAndDeinit was never ported from Zig) but it is the scaffolding for the "X's postinstall script took Yms" warning from #7719, which should be restored rather than deleted.
|
Good catch, reverted the Worth flagging though: on current main that feature is already non-functional, independent of this PR. Tracing it back:
So today PR is now net -1185 across 24 files (C++ bindings + |
robobun
changed the title
|
Can you implement it |
|
Already on it: a separate PR is being spun up to restore the #7719 slow-postinstall warning (port |
There was a problem hiding this comment.
LGTM — all three earlier nits are addressed (WeakHashSet include dropped in 91f57fe, IDLJSON removed in 23b3d1a, sizegen.cpp wording fixed) and the src/install/ LifecycleScriptTimeLog revert in 6057ada resolves the maintainer feedback.
Spot-checked post-revert: StringGithubActionFormatter has zero refs (only the ZigString variant is re-exported from bun_core/lib.rs); Zig::toStringView(ZigString) / Zig::toAtomString(ZigString) don't collide with the surviving Bun::toStringView(StringView) / JSString::toAtomString; JSVMClientDataClient/willDestroyVM, BUN_FFI_POINTER_OFFSET_TO_TYPED_ARRAY_VECTOR, and the deleted webcore headers all grep clean across src/. The ~JSVMClientData loop removal is provably a no-op since addClient() had no callers.
Extended reasoning...
Overview
Net -1185 lines across 24 files. Eight whole-file deletions under src/jsc/bindings/ (llhttp/api.h, headers-cpp.h, ares_build.h, TaskSource.h, HTTPHeaderValues.{h,cpp}, JSDOMConvertJSON.h, JSDOMConvertWebGL.{h,cpp}, JSVMClientDataClient.h), targeted symbol removals from helpers.h / headers-handwritten.h / JSDOMWrapper.h / BunClientData.{h,cpp} / IDLTypes.h, four Rust helper removals in bun_core::string / bun_jsc, and one new test/internal/source-lints/ regression test. The src/install/ LifecycleScriptTimeLog removal from an earlier revision was reverted in 6057ada per maintainer feedback, so no package-manager code is touched.
Security risks
None. Pure deletions of unreferenced declarations and never-included headers; no input parsing, no auth/crypto/permissions surface, no behavior change on any reachable path. The one non-trivial edit — dropping the m_clients.forEach(willDestroyVM) loop from ~JSVMClientData — is a provable no-op because addClient() had zero callers, so m_clients was always empty.
Level of scrutiny
Low-to-moderate. The change is large by line count but mechanically verifiable: each item is a grep-for-zero-references check plus a full build. I re-ran the reference checks for a representative sample across both languages (StringGithubActionFormatter, Zig::toStringView/toAtomString overload collision, JSVMClientDataClient, BUN_FFI_POINTER_OFFSET_TO_TYPED_ARRAY_VECTOR, the deleted webcore headers, the JSDOMWrapper.h node-type constants, WritableEvent/ReadableEvent typedefs) and all came back clean against current src/. The bun_core/lib.rs re-export list only names ZigStringGithubActionFormatter, so removing the String variant doesn't break the public re-export.
Other factors
- All three of my earlier inline nits are resolved (91f57fe, 23b3d1a, description update); the CodeRabbit source-lint suggestion was withdrawn; the maintainer's
src/install/concern was reverted in 6057ada. No outstanding threads. bun bdandbun run rust:check-allpass per the description; a stale reference in either language would fail the build outright.- Follows the established
test/internal/source-lints/dead-symbols-*.test.tspattern used by the ~11 sibling dead-code PRs referenced in the description; the new test asserts against surviving-file content rather thanexistsSync()per the harness constraint documented at lines 22-28. sizegen.cppremains uncompilable but that's pre-existing (never in a build rule; loop referenced undefinednames[]before this PR too) and is being reworked in #36115.
2 participants
Net -1185 lines (+68 / -1253) across 24 files. Every removed item was verified to have zero references across
src/andbuild/debug/codegen/, then confirmed by a fullbun bdbuild andbun run rust:check-all.No overlap with the 11 open dead-code PRs (checked file lists of #34965 #34759 #36474 #36178 #36237 #35559 #35775 #36318 #36115 #35437 #35880).
Whole-file deletions (C++, 1107 lines)
src/jsc/bindings/node/http/llhttp/api.h#included. Vendored upstream copy artifact; all 41LLHTTP_EXPORTdecls are duplicated verbatim inllhttp.h, andapi.cincludesllhttp.hnotapi.h. Only mentioned inllhttp/README.md.src/jsc/bindings/webcore/JSDOMConvertWebGL.{h,cpp}#if ENABLE(WEBGL). The .cpp#includes ~40 headers (JSANGLEInstancedArrays.hetc.) that don't exist in the repo, so the guard is provably inactive on every bun target.IDLWebGLAny/IDLWebGLExtensionused nowhere else.src/jsc/bindings/headers-cpp.hheadergen/sizegen.cpp, which isn't in any build rule. File itself has syntax errors (line 166#include ""ConsoleObject.h"", lines 172-182#include ""), so it cannot be compiling anywhere.src/jsc/bindings/webcore/HTTPHeaderValues.{h,cpp}textPlainContentType,formURLEncodedContentType,applicationJSONContentType,noCache,maxAge0) are called anywhere.src/jsc/bindings/webcore/JSDOMConvertJSON.hJSDOMConvert.h.IDLJSONis referenced nowhere outsideIDLTypes.h(type decl) and this file.src/jsc/bindings/ares_build.h#includes anywhere undersrc/. Superseded by the generatedbuild/<profile>/deps/cares/ares_build.hemitted byscripts/build/deps/cares.ts.src/jsc/bindings/webcore/TaskSource.h#included. Only referenced in commented-out code inWebSocket.cpp/JSDOMPromiseDeferred.cpp.src/jsc/bindings/JSVMClientDataClient.hBunClientDatabelow.C++ symbol removals
helpers.h(38 lines):Zig::toAtomString(ZigString),toStringNotConst,__dot_char/ZigStringCwd/BunStringCwd,toZigString(WTF::String*),toZigString(JSC::Identifier&)+(JSC::Identifier*),Zig::toStringView(ZigString). rg across src/ and codegen shows zero callers for each.headers-handwritten.h(22 lines):WritableEventtypedef + 8 consts,ReadableEventtypedef + 9 consts. Zero references anywhere.JSDOMWrapper.h(8 lines):JSTextNodeType,JSProcessingInstructionNodeType,JSDocumentTypeNodeType,JSDocumentFragmentNodeType,JSDocumentWrapperType,JSCommentNodeType,JSCDATASectionNodeType,JSAttrNodeType. Only referenced in commented-out code atwebcore/DOMJITHelpers.h:163-178. (JSNodeType/JSNodeTypeMask/JSElementType/JSAsJSONTypekept.)BunClientData.{h,cpp}(9 lines):addClient()is never called, som_clientsis always empty and the~JSVMClientDataforEach/clearloop is a no-op. RemovedaddClient,m_clients, the dtor loop, and the include ofJSVMClientDataClient.h.JSDOMConvert.h(2 lines): removed#includeof the two deleted headers.headergen/sizegen.cpp(2 lines): removed#include "headers-cpp.h". The file is not in any build rule and was already uncompilable (its loop referencesnames[]/sizes[]/aligns[], none of which were ever fully defined); leaving the loop untouched to minimise conflict with Remove dead code from node/crypto C++, js_parser, js_printer, bundler, sql, server #36115..Rust removals
bun_core::String::github_action+StringGithubActionFormatter(22 lines): all four.github_action()call sites inVirtualMachine.rsare onjsc::ZigString, notbun_core::String. TheZigStringvariant is kept.bun_jsc::JSUint8Array::ptr()+sizes::BUN_FFI_POINTER_OFFSET_TO_TYPED_ARRAY_VECTOR(14 lines): zero callers.bun_jsc::RefString::to_js()(9 lines): the sole externalRefStringuser (filesystem_router.rs) never calls.to_js(). Removed along with now-unusedJSGlobalObject/JSValue/JsResult/StringJscimports.bun_jsc::Errorable::value()(7 lines): identical body toErrorable::ok(); every caller usesok().Verification
bun bdpassesbun run rust:check-allpasses on all targetsbun bd test test/internal/source-lints/passes (62 tests)bun bd test test/js/node/inspector/passes (67 tests; exercisesBunDebugger.cpp)bun bd test test/cli/install/bun-install-lifecycle-scripts.test.tspasses (3 pre-existing env failures unrelated to this diff, reproduced on main)Followups (not in this diff)
src/jsc/bindings/CachedScript.his semantically vestigial (empty class, all callers passnullptr) but removing it requires editing signatures inScriptExecutionContext.h/JSDOMExceptionHandling.{h,cpp}.src/ast/lib.rsStringBuilderstub + thecount()method chain is a no-op cluster but removing it requires dropping the&mut StringBuilderparameter from threeclone_with_buildersignatures.src/runtime/api/bun/h2/connection.rssend_header_block/send_push_promise/send_data/encode_header/begin_header_block(~173 LOC) are only called from#[cfg(test)]; intentionally staged per theh2/mod.rsmodule doc for a future rewrite, so left alone.[review] gate passed · iteration 2 · 24 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 2
evidence per changed file