Conversation
…classes, bun_http, and two orphaned data files
|
Warning Review limit reached
On-demand reviews are free for the next 6 days. After that, they cost $0.25 per reviewed file. Or wait 2 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (29)
Comment |
|
Status: ready for review. How each deletion was checked:
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a broad sweep removing ~500 lines of code across 29 files in many subsystems (socket host fns, stream sources, JSC C++ bindings, crypto includes) and touches a CODEOWNER-gated .d.ts file, a human look would still be worthwhile.
What was reviewed:
- Grepped
src/,packages/, andbench/for every removed identifier (ConversionResult,$endon Socket,isClosedon stream sources,endBuffered/end_buffered,FALLBACK_HTML_VERSION,starts_with_generic,DISABLE_COMPRESSION_IN_HTTP_CLIENT,_fixture.txt,protocol.json) — no remaining callers; theConversionResulthit inJSX25519Params.cppis just a local variable name, andbuiltins.d.ts:223's global$endis the unrelated JSC private name. - Checked
END_AFTER_FLUSHstill has other setters inwrite_or_end, so removingend_buffered's set point doesn't strand the flag. - Checked
convert<>inJSDOMConvertBase.hreturnsConverter<T>::ReturnType(notConversionResult<T>), so deleting the header and its transitional-shim conversion operators doesn't break any call site. - Checked
ByteStreamretains its ownparent_const()accessor; only the now-callerlessByteBlobLoader::parent()was dropped.
Extended reasoning...
Overview
This PR is a dead-code sweep removing ~5,900 lines (of which ~5,400 are two orphaned data files: bench/snippets/_fixture.txt and packages/bun-debug-adapter-protocol/src/protocol/protocol.json) and ~500 lines of actual code across 29 files. On the Rust side it removes an always-false feature flag and collapses the resulting Accept-Encoding const, drops the is_closed field and getter from NewSource stream sources plus their two write sites, removes the endBuffered ($end) socket host fn and collapses write_or_end_buffered<IS_END> into a non-generic write_buffered_impl, and deletes several unused re-exports and a stale ENABLE_SHADCN_UI gate. On the C++ side it deletes JSDOMConvertResult.h, the four-include-only dh-primes.h (replaced by a direct <openssl/mem.h> include in ncrypto.cpp), an unused AsyncContextFrame::create overload, a 2-arg GlobalScope constructor, redundant visitChildren template declarations already covered by DECLARE_VISIT_CHILDREN, and an always-skipped <openssl/engine.h> include. Tooling-side deletions cover an LLDB pretty-printer for a Zig-era type and a build-options constant with no reader.
Security risks
The changes brush against crypto code (ncrypto.cpp, dh-primes.h, NodeConstantsModule.h) but only at the include level — no logic, no key handling, no validation is touched. The dh-primes.h header contained nothing but four #include <openssl/...> lines and a license block; three of those headers are already pulled in by ncrypto.h, and the fourth (<openssl/mem.h>) is now included directly. The OPENSSL_NO_ENGINE block removal is safe because BoringSSL always defines that macro. No auth, permission, or input-validation paths are affected. I see no security risk in this diff.
Level of scrutiny
Medium-high. REVIEW.md is explicit that dead-code deletion is required scope and encouraged, but also that "before deleting odd-looking code, git-blame why it was written — it is usually load-bearing" and that public items escape lints so callers must be grepped manually. I grepped every removed public identifier and confirmed zero remaining references. The one nuanced case — the socket write_or_end_buffered<IS_END> collapse — preserves behavior for the surviving write_buffered caller (it always instantiated <false>), and Flags::END_AFTER_FLUSH still has its other setters in write_or_end. The ConversionResult<T> deletion is safe because convert<> in JSDOMConvertBase.h returns Converter<T>::ReturnType directly, not the wrapper type; the only grep hit is a local variable name.
Other factors
The PR description reports bun bd and bun run rust:check-all (12 targets) pass along with a substantial suite of socket, stream, fetch, crypto, and source-lint tests. However, .github/CODEOWNERS assigns *.d.ts to a specific owner, and this PR modifies src/js/builtins.d.ts (a one-line removal). Combined with the breadth — 29 files across sockets, streams, JSC bindings, HTTP client, crypto includes, build scripts, LLDB tooling, and two packages — the guidelines' "do not approve when the change is large or touches CODEOWNER-gated paths" applies, so I'm deferring rather than approving despite finding no defects.
|
The review found no defect, so there is nothing to change. Two notes for the human look it recommends:
The |
|
Updated 4:29 PM PT - Sep 14th, 2026
✅ @robobun, your commit 61f3f5216014752c3bc290196cbd67d7c8e5c0c6 passed in 🧪 To try this PR locally: bunx bun-pr 42756That installs a local version of the PR into your bun-42756 --bun |
Problem
src/jsc/bindings/webcore/JSDOMConvertResult.h(268 lines) definesConversionResult<T>, and nothing uses it.src/jsc/bindings/dh-primes.hholds only four#includelines.TCPSocket/TLSSocketendBufferedis installed only under the private name$end, and no built-in JS calls.$end(. TheisClosedgetter of the internal stream source handles has no reader, soNewSource::is_closedis write-only.bench/snippets/_fixture.txt(its reader went in Rewrite Bun.escapeHTML using Highway SIMD #31483) and theprotocol.jsonsnapshot inpackages/bun-debug-adapter-protocol.Fix
src/,packages/,scripts/,test/andbuild/debug/codegen/outside its own definition. User code cannot spell the private names$endand$bunNativePtr.write_or_end_buffered::<IS_END>had one instantiation left and becomeswrite_buffered_impl.ByteBlobLoader::parent()lost its only caller and goes too.bun bdandbun run rust:check-all(12 targets) pass. Socket,node:net, streams, fetch, crypto and source-lint test files run with the debug build (list in the Notes).Background
*.classes.tsfile declares the JS members of a native class.privateSymbol: "end"installs the member only under the private name$end, which only built-in JS insrc/jscan spell.src/bun_core/feature_flags.rsholds compile-timeboolconstants.DISABLE_COMPRESSION_IN_HTTP_CLIENTisfalse, so theidentityarm ofACCEPT_ENCODING_HEADERis never selected.Notes
Removed, one line each:
src/jsc/bindings/webcore/JSDOMConvertResult.h(whole file):ConversionResult<T>,ConversionResultException,Detail::ConversionResultStorage.rg -wfor the three names hits only the header. Its only includer wasJSDOMConvertBase.h.src/jsc/bindings/dh-primes.h(whole file): a license comment and four#include <openssl/...>lines since Remove dead code from uws_sys, webcore bindings, crash_handler, and scripts #37181 and Remove dead code from install, event_loop, dns, mysql protocol, C++ bindings, and JS internals #36970 removed the tables.ncrypto.cppwas the only includer. It now includes<openssl/mem.h>directly, andncrypto.halready includes the other three headers.TCPSocket/TLSSocketendBuffered($end) insockets.classes.ts,end_bufferedinsocket_body.rs,$end(): voidinbuiltins.d.ts. No.$end(insrc/js.endPrivateNameappears only in the generatedputDirect. Remove dead code from the node:http2 frame parser, the shell interpreter, NodeHTTPResponse, and three JS binding objects #42682 listed this member as found and not deleted, because of theIS_ENDfollow-through that this change does.Blob/File/BytesInternalReadableStreamSource.isClosedinstreams.classes.ts,get_is_closed_from_js, the fieldNewSource::is_closed, its initializer, and its two stores (ByteBlobLoader::to_any_blob,ByteStream::to_any_blob). C++ reads the closed flag from thecloserarray (nativeCloserFlag,BunStreamSource.cpp), not from this getter.ByteBlobLoader::parent()had the store as its only caller. The Remove dead code from bun_bundler, bun_install, bun_runtime, bun_parsers, and the JSC private host functions #41088 hunks for both files still apply.FeatureFlags::DISABLE_COMPRESSION_IN_HTTP_CLIENT(constfalse) and, insrc/http/lib.rs,ACCEPT_ENCODING_NO_COMPRESSION,ACCEPT_ENCODING_HEADER_NO_COMPRESSIONand the two intermediate constants.ACCEPT_ENCODING_HEADERkeeps its valuegzip, deflate, br, zstd.ENABLE_SHADCN_UI(consttrue, under a stale "Disabled until Tailwind v4" comment) and itselsearm inSourceFileProjectGenerator.rs.starts_with_genericandends_with_genericinsrc/bun_core/string/immutable.rs.has_prefix_tandhas_suffix_tstay.FALLBACK_HTML_VERSIONinscripts/build/buildOptionsRs.ts. Nothing reads the generated constant.AsyncContextFrame::create(JSGlobalObject*, JSValue, JSValue). The only call site uses the 4-argument overload.GlobalScope(VM&, Structure*)constructor inBunGlobalScope.h. Both subclasses pass a method table.Zig::untagVoid(const char16_t*)inhelpers.h. All four callers pass aconst unsigned char*.template<typename Visitor> static void visitChildren(JSCell*, Visitor&);inJSX509Certificate.handJSNodePerformanceHooksHistogram.h. The prototype has no definition.DECLARE_VISIT_CHILDRENon the next line declares the real overloads.#ifndef OPENSSL_NO_ENGINE/#include <openssl/engine.h>inNodeConstantsModule.h. BoringSSL'sopensslconf.hdefinesOPENSSL_NO_ENGINEand<openssl/ssl.h>is included above, so the block is always skipped. TheENGINE_METHOD_*constants are numeric literals.bun_BabyList_SynthProvider,bun_BabyList_SummaryProviderand their registration inmisctools/lldb/bun_pretty_printer.py. The regex matches the Zig type namebaby_list.BabyList(...). NoBabyListtype exists now (src/collections/vec_ext.rs:3).bench/snippets/_fixture.txt(1610 lines, 131 KB of HTML). Its only reader,bench/snippets/escapeHTML.js, was deleted in Rewrite Bun.escapeHTML using Highway SIMD #31483.escapeHTML.mjsdoes not read it.packages/bun-debug-adapter-protocol/src/protocol/protocol.json(3779 lines) and the line inscripts/generate-protocol.tsthat wrote it. The script downloads the upstream DAP schema on every run and only the generatedindex.d.tsis imported. Last update: Update debugger things #8180. Remove dead code from the inspector protocol package, bun_jsc, bun_install, and the Windows named pipe modules #40557 deletes the equally unread v8protocol.jsonofbun-inspector-protocol. The package's.gitattributesgoes too: both patterns start withprotocol/, the files are insrc/protocol/, so they match nothing.Tests run with the debug build:
test/js/bun/net/socket.test.ts,test/js/node/net/node-net.test.ts,test/js/web/streams/streams.test.js(563 pass),test/js/web/fetch/body-stream.test.ts(9086 pass),test/js/web/fetch/blob.test.ts,test/js/bun/util/zstd.test.ts(checks theAccept-Encodingvalue),test/js/node/crypto/node-crypto.test.js,test/js/node/crypto/x509.test.ts,test/js/node/perf_hooks/perf_hooks.test.ts,test/js/node/async_hooks/AsyncLocalStorage.test.ts,test/js/bun/http/bun-server.test.ts,test/cli/create/create-jsx.test.ts, andtest/internal/source-lints/{dead-code-escapes,host-export-callers,primordials-exports}.test.ts. The failures are the same with the unmodified release build in the same container: connects to127.0.0.1and IPv6 listens are refused there,create-jsxneeds the network, and oneAsyncLocalStoragetest passes the 5 s limit under ASAN.How the candidates were found:
*.classes.tsmember againstsrc/js.cargo hawk check --only dead-publicfor linux x64, darwin arm64 and windows x64. 823 findings: 668 FFI layout fields, 90 entries of OS constant tables, 23 enum variants of external code tables. Of the 19 functions and methods,strings::split_onceis unused but sits next to a hunk of Remove dead code from bun_jsc, bun_runtime, bun_core, and the JSC bindings #40824. The rest are debug-only or diagnostic, named by the class generator templates, or already deleted by an open pull request.clang -fsyntax-onlyover the 165 Bun C++ translation units with-Wunused-function -Wunused-member-function -Wunused-template -Wunused-const-variable. 11 warnings: all are platform-gated, part of an intentional overload set, or already deleted by an open pull request.Found, not deleted here:
NodeHTTPResponseref,ended,onwritable: no reader insrc/js, buttest/js/node/http/node-http-uaf.test.tssetshandle.onwritablethroughObject.getOwnPropertySymbols, and node:http: finish a response when its body leaves, not when end() buffers it #41822 starts to use it.connected,unref, the MySQLref(with thepoll_ref_hostfns!macro), and theonconnect/oncloseaccessors have no JS reader. Thequeriesgetter initializes its array lazily, so the removal needs more care than a deletion.myersDiffraw-list export ofinternal/assert/myers_diffandOutput::Listinnode_assert.rs: nothing consumes it, but assert: render Myers diffs natively #39924 kept it on purpose.src/codegen/generate-classes.ts(about 175 lines, plus theDOMJIT:entries in three*.classes.tsfiles):class-definitions.tssetsDOMJIT = undefinedon every entry, so the generator never sees one. It looks like a deliberate kill switch. It needs a maintainer decision.Bun__signpost_emitandgenerated_perf_trace_events.h: no caller on any platform, butsrc/bun_core/util.rsdocuments them as plumbing that is not wired yet.ncrypto.cpp(about 75 lines): the file keeps the#ifstructure of nodejs/ncrypto on purpose.JSMessagePort::visitAdditionalChildrenInGCThread: a no-op becauseMessagePort::locallyEntangledPort()always returns null. GC code in four files, two of them in Remove dead code from the WebCore bindings and 20 Rust crates #40525.src/js_parser/benches/string_map_vs_hashmap.rs(800 lines) and itscriterion/regexdev-dependencies: it compiles, and nothing runs it.src/runtime/cli/init/react-*/bun.lock(430 lines): not embedded withinclude_bytes!, but kept in sync with the templates by hand.cpp-only,rust-only,link-only,rust-and-link(about 525 lines inscripts/build): ci: build C++, Rust and link in one build-bun step instead of build-cpp + build-bun #37733 said they can go if the one-step layout holds up. Three tests name them.ERR_REDIS_INVALID_DATABASEinErrorCode.ts: no user, but Remove dead code from bun_css, bun_react_compiler, bun_jsc, and smaller crates #40690 and Remove dead code from bun_core, the JSC bindings, the debugger, and 24 other crates #41385 delete the lines next to it.packages/bun-error/img/*(four images that nothing references) and their glob inscripts/glob-sources.ts: Remove dead code from the JSC bindings, headers.h, the console builtin, and bun_jsc #40232 edits the line next to the glob.scripts/lldb-inline-tool.cpp,scripts/lldb-inline.sh,scripts/github-metrics.ts,scripts/gamble.ts,scripts/trace.sh,scripts/debug-coredump.ts..gitignore,.prettierignore,.dockerignoreand.gitattributesfor paths that no longer exist.CastedThisErrorBehavior::ReturnEarlyinJSDOMCastThisValue.h: no user, but theelsearms inJSDOMAttribute.hthat implement it stay reachable for the other values, so the enumerator stays with them.bun_core::strings::split_once(the multi-byte delimiter form): no caller, but it sits next to thersplit_oncehunk of Remove dead code from bun_jsc, bun_runtime, bun_core, and the JSC bindings #40824.Self-reviewed: two adversarial passes (Rust/JS and C++) over 18 items, each asked to find a missed reference, a platform-gated use, or a behavior change. No concern survived. Three candidates were dropped on the way: a
JSAbortControllerdeclaration that the build showed to be live,NodeHTTPResponse.onwritable(see above), andReturnEarly(see above).