Remove dead code from node:http2, the HTTP/2 parser, JSC bindings, uSockets, and the builtin-name tables - #38900
Conversation
…ockets, and the builtin-name tables node:http2 bound every entry of `constants` as a local; 178 of the 240 bindings were never read, nor were `Socket`, two ServerHttp2Session fields and the native assertSettings binding that http2.ts stopped using when it grew its own assertSettings(). With the binding gone the js2native codegen no longer names `js_assert_settings`, so the Rust host function, its facade re-export and the Zig-era BUN__HTTP2_* declarations go too. Also removed: the four InspectorHTTPServerAgent command stubs (the HTTPServer inspector domain only defines enable/disable, so nothing can dispatch to them), declaration-only leftovers in BunObject.cpp, JSBuffer.cpp and CryptoUtil.h, the IsIDLEnumeration helper and the globalBuiltinFunction macro, two uncalled uSockets TLS helpers plus the darwin Security.framework teardown hook, ExprData::is_e_string, ArrayHashMap::get_adapted, Behavior::eq, js_parser::FunctionKind (its Stmt variant was never constructed), nine builtin-name entries with no users, the unused $stream* defines, and scripts/find-dead-exports.ts, which the hawk setup in tools/hawk/ replaced. A source lint pins every removed symbol.
|
Warning Review limit reached
Next review available in: 3 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 (26)
Comment |
There was a problem hiding this comment.
Beyond the inline nit, I checked the parts of this diff that could change behavior rather than just delete symbols:
validate_function_namedropping thekind == FunctionKind::Exprguard is behavior-preserving — the only call site isparse_fn_expr(parse_fn_stmtnever called it), so the guard was always true.- None of the nine removed
BunBuiltinNames.hentries have a surviving*PublicName()/*PrivateName()accessor use anywhere insrc/. - The
MAX_*_F64constantsjs_assert_settingsused are still referenced byload_settings_from_jsin the same file, so its removal does not orphan them.
Extended reasoning...
Verified the one non-pure-deletion hunk (the FunctionKind::Expr guard on the generator-named-yield diagnostic in p.rs) is equivalent because parse_fn_stmt never invoked validate_function_name; grepped src/ for every removed builtin-name accessor and found none; confirmed the h2 f64 constants remain live. The inline nit about dangling Socket type annotations stands on its own, and the PR's breadth across 26 files in six subsystems is enough that a human pass is still worthwhile.
| @@ -67,7 +67,6 @@ type Http2ConnectOptions = { | |||
| createConnection?: Function; | |||
| }; | |||
| const TLSSocket = tls.TLSSocket; | |||
There was a problem hiding this comment.
🟡 Removing const Socket = net.Socket leaves the bare Socket type annotation dangling at 5 sites in this file (lines 1901, 3998, 4450, 4954, 6396) — there is no ambient/global Socket type in scope, so those annotations now reference an undeclared identifier. No runtime effect (the builtin bundler strips types), but the PR's tsc --noUnusedLocals justification is wrong for this const since tsc counts type-position uses; rewrite the 5 annotations to net.Socket (and drop the source-lint line that pins the const's deletion), or keep the const.
Extended reasoning...
What the issue is
The PR removes const Socket = net.Socket; from src/js/node/http2.ts (previously line 70) on the grounds that it "was never read" per tsc --noUnusedLocals and grep. However, the identifier Socket is still used as a type at 5 sites in the file:
src/js/node/http2.ts:1901—[bunHTTP2Socket]: TLSSocket | Socket | null;src/js/node/http2.ts:3998—#socket_proxy: Proxy<TLSSocket | Socket>;src/js/node/http2.ts:4450—constructor(socket: TLSSocket | Socket, options?: Http2ConnectOptions, server?: Http2Server)src/js/node/http2.ts:4954—#socket_proxy: Proxy<TLSSocket | Socket>;src/js/node/http2.ts:6396—function connectionListener(socket: Socket)
A const bound to a class value (net.Socket) is usable in type position — it names the class's instance type. Removing the const removes the only binding those annotations resolved against.
Why nothing else provides Socket
Grepping src/js for a Socket type/interface/class declaration finds exactly two interface Socket declarations, both module-scoped, not global:
src/js/builtins.d.ts:10— insidedeclare module "bun" { interface Socket { ... } }src/js/private.d.ts:257— insidedeclare module "node:net" { interface Socket { ... } }
Neither augments the global scope, so after this change bare Socket at the 5 sites above is an undeclared identifier. This is asymmetric with the sibling const TLSSocket = tls.TLSSocket on the line immediately above, which the PR keeps and which is used identically in type position at the same sites.
Why the PR's stated justification does not apply here
The PR description says these were found via "tsc --noUnusedLocals, re-checked by grep". But tsc --noUnusedLocals does count type-position references of a value binding as uses — a const X = SomeClass referenced only as foo: X is not flagged. So either tsc was not actually run on this file (src/js builtins are not part of a checked tsconfig project), or the grep missed the type-position hits. Either way, the "never read" claim in the PR description is inaccurate for this particular const.
Impact
None at runtime. src/codegen/bundle-modules.ts strips TypeScript types from builtins before bundling, and the PR verified bun bd builds and test/js/node/http2/node-http2.test.js (357 tests) passes. This is purely a type-hygiene issue in an internal builtin: 5 annotations now reference an undeclared name, and the new source-lint (test/internal/source-lints/dead-symbols-http2-settings-inspector-stubs.test.ts:42 — expect(http2).not.toMatch(/^const Socket = net\.Socket;$/m)) locks the const's deletion in, so the dangling references are pinned rather than caught.
Step-by-step proof
- After this PR,
src/js/node/http2.tsline 69 isconst TLSSocket = tls.TLSSocket;and the followingconst Socket = net.Socket;line is gone. - At line 1901,
[bunHTTP2Socket]: TLSSocket | Socket | null;—TLSSocketresolves to the const on line 69;Sockethas no binding. - Search the file for
import.*Socket,type Socket,interface Socket,class Socket— none. The file's onlySocket-named declaration was the removed const. - Search the ambient
.d.tsfiles referenced bysrc/js—builtins.d.tsandprivate.d.tseach declareinterface Socketinside adeclare module "..."block, so neither contributes a globalSocket. - Therefore all 5 type annotations reference an undeclared identifier.
How to fix
Rewrite the 5 annotations to net.Socket in this PR (matching how the type is spelled at src/js/node/http2.ts:1292's comment already refers to it as "net.Socket"). That keeps the const dead as intended and makes the source-lint correct. Alternatively, add type Socket = InstanceType<typeof net.Socket>; next to const TLSSocket. Restoring the const would also work but requires deleting the source-lint assertion at dead-symbols-http2-settings-inspector-stubs.test.ts:42.
Removes 775 lines that nothing references (8 lines of signature, import and comment adjustments added), across node:http2, the HTTP/2 frame parser, the JSC/WebCore bindings, uSockets, four Rust crates, the builtin-name tables and scripts/. No behavior change. A 153-line source lint pins every removed symbol.
Problem
src/js/node/http2.tsbinds every entry ofconstantsas a local (const { ... } = constants;); 178 of the 240 bindings have been unused since the block was added in feat(node:http2) Implement HTTP2 server support #14286 (every call site readsconstants.X).const Socket = net.Socketand the#url/#isServerfields ofServerHttp2Sessionare never read either (tsc --noUnusedLocals, re-checked by grep)._nativeAssertSettings($newRustFunction("h2_frame_parser.rs", "jsAssertSettings")) has been unused since Fix HTTP/2 settings: getPackedSettings, getUnpackedSettings, enableConnectProtocol #28074 gave http2.ts its ownassertSettings(). That madejs_assert_settingsinsrc/runtime/api/bun/h2_frame_parser.rs(139 lines) and its facade re-export insrc/runtime/api.rsunreachable: the js2native codegen was the only thing that named it. The threeBUN_DECLARE_HOST_FUNCTION(BUN__HTTP2_*)lines inZigGlobalObject.cppare declarations of the Zig-era exports for the same helpers and have no definition anywhere.InspectorHTTPServerAgent::{startListening, stopListening, getRequestBody, getResponseBody}are TODO stubs that nothing can dispatch to: the generatedHTTPServerBackendDispatcherHandlerin the pinned WebKit only declaresenable()/disable(), so thesevirtual ... finalmethods override nothing and have no callers.Bun__DNSResolver__new/Bun__DNSResolver__cancel(BunObject.cpp),jsBufferConstructorFunction_isBuffer(JSBuffer.cpp,Buffer.isBufferis a JS builtin),getStringOption(CryptoUtil.h). Plus theIsIDLEnumerationhelper template (IDLTypes.h) and theglobalBuiltinFunctionmacro (ZigGlobalObject.cpp), each with zero uses.us_internal_ssl_sni_userdataandus_internal_ssl_handshake_abortwere added in socket: replace us_socket_context_t with embedded groups + kind dispatch #29932 and never called (us_socket_server_name_userdata, which the first one wrapped, stays).us_cleanup_security_framework(darwin) had no caller; the loader's failure paths are the only thing that ever frees aSecurityFramework, and they still do. TheSecTrustSettingsResulttypedef next to it was unused too.dead_public, intersected over linux-gnu, linux-musl, android, freebsd, darwin and windows-msvc, then re-checked by hand):ExprData::is_e_string,ArrayHashMap::get_adapted, the debug-onlyBehavior::eq, andjs_parser::FunctionKind, whoseStmtvariant was never constructed.validate_function_nameonly ever ran for function expressions, so the parameter is gone and the one call site passes nothing; the error text already said "function expression".Loader,byobRequest,controller,post,resume,started,state,textDecoderandviewinBunBuiltinNames.hhave no$name/@nameuse in any builtin and nonamePublicName()/namePrivateName()use in C++ (each entry costs two identifiers at VM startup). Theirbuiltins.d.tsdeclarations go with them, as do the six$stream*--defines inreplacements.ts(no builtin reads them; their only trace was the.d.ts), the"Loader"entry ofglobalsToPrefix(the only bareLoaderin src/js is inside a block comment), and a duplicated"Buffer"entry.scripts/find-dead-exports.ts(303 lines, Restrict crate-internal items to pub(crate) and remove dead code it exposes #31254) was a textual approximation of the analysistools/hawk/+hawk.tomlnow do (Narrow crate-internal Rust visibility across all targets and delete the code it proves dead #36184); nothing references it.Fix
validate_function_namesignature, one comment insemver/lib.rsthat namedget_adapted(now names the survivingget_index_adapted), and the re-export line inapi.rs.$newRustFunctioncall is what makes the Rust function removable:generate-js2native.tsemits thecrate::api::bun::h2_frame_parser::js_assert_settingsthunk only for calls it finds in src/js, so after this change nothing generated names it either.bundle-functions.tsregeneratesBunBuiltinNames+extras.hfrom the names builtins actually use, so a builtin-name entry that were still needed would be re-added by the build rather than break it; none was.bun bdbuilds;bun run rust:check-allpasses on all ten target triples;cargo fmt --checkand prettier are clean.bun bd testpasses ontest/js/node/http2/node-http2.test.js(357),test/js/node/http2/node-http2-continuation.test.ts,test/js/node/buffer.test.js,test/js/node/crypto/node-crypto.test.js,test/js/node/tls/node-tls-connect.test.ts,test/js/web/streams/streams.test.jsandtest/bundler/bundler_minify.test.ts;test/js/node/tls/node-tls-server.test.tspasses 69/70, the remaining one (SNICallback runs even when the requested servername matches the bind hostname) fails identically with the released binary in this container (it bindslocalhostand connects to 127.0.0.1). The four function-name diagnosticsvalidate_function_nameemits are unchanged between the released binary and this build for both statement and expression positions.bun test test/internal/source-lints/passes (158 tests); the newdead-symbols-http2-settings-inspector-stubs.test.tsfails on main and passes here.BUN__HTTP2_*lines sit right under theBun__NodeUtil__jsParseArgsline Remove dead code from the FFI shim layer, link_interface tables, jssink/jest codegen, and REPL shims #37788 removes, and theInspectorHTTPServerAgenthunks are a few lines above the event dispatchers Remove dead code from node:crypto bindings, JSC/WebCore bindings, uSockets, and llhttp #37454 removes. One item found dead here,JSValkeyClient::close_subscription_ctx, is the renamed form of theSubscriptionCtx::closethat Remove dead code from simdutf, ncrypto, uws shims, and JSC/WebCore bindings #38439 already removes, so it was left to that PR.Background
$rust("file.rs", "name")/$newRustFunction(...)calls in src/js are collected at build time bysrc/codegen/generate-js2native.ts, which emits one thunk per call intogenerated_js2native.rs/GeneratedJS2Native.h. A Rust host function is therefore reachable only while some builtin names it; theapi.rsfacade module exists so the generated thunks have a stable path to call.BunBuiltinNames.his the list of private identifiers (@namein builtin JS,builtinNames.namePrivateName()in C++) thatBunBuiltinNamesinterns when a VM starts.bundle-functions.tsdiffs the names builtins use against this list and writes the difference toBunBuiltinNames+extras.h, which is why removing an entry can only ever remove startup work, never break a builtin.*BackendDispatcherHandlerinterface; a command reaches an agent only through that interface. The HTTPServer domain bun ships in its WebKit definesenableanddisable, so methods an agent declares beyond those are unreachable by construction.tools/hawk/README.md) is the workspace-wide reachability analysis the repo uses forpubitems, since rustc'sdead_codetreats everypubitem of a library crate as a root. Its per-target reports were intersected so that anything live under somecfgon any shipped platform was kept; the repr(C) FFI struct fields and flag/errno table entries it also reports were left alone on purpose, as in Remove dead code from bun_core, bun_css, bun_jsc, and the FFI crates #38703.Found dead but left alone (follow-ups, not in this diff)
bun_sys::ErrorCase::LeakFdOnFailis never constructed (everymake_lib_uv_owned_for_syscallcaller passesCloseOnFail), but removing it means dropping the parameter at 13 call sites, several of them in files open PRs are editing.libusockets.hstill declaresus_udp_socket_receive,us_udp_buffer_set_packet_payload,us_create_udp_packet_bufferandus_udp_socket_bindwith no definitions, andudp.ccarries a commented-outus_udp_packet_buffer_ecn; both spots are adjacent to hunks in Remove dead code from node:crypto bindings, JSC/WebCore bindings, uSockets, and llhttp #37454.NapiEnv::currentFinalizer()became unused in Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 a week ago; left for that work to settle, as Remove dead code from node:crypto bindings, JSC/WebCore bindings, uSockets, and llhttp #37454 did with its siblings.replacements.tsalso emits a$ImportKindLabelToIddefine nothing reads, but it comes out of the same loop as three live tables.ws.jsWebSocket.prototype.setSocket(a throwing stub) andasync_hooksAsyncResource.emitBefore/emitAfterhave no in-tree users but are on exported classes.scripts/packer/build-image.pkr.hclandscripts/trace.shhave no invoker, but they are CI infrastructure rather than code; flagged for whoever owns those.