Remove dead code from the WebCore wrapper headers, the node:crypto bindings, NodeVMModule, and the build scripts - #43914
Conversation
…ndings, NodeVMModule, and the build scripts - Remove the `using DOMWrapped` alias from 14 wrapper headers. Only JSDOMIterator reads `DOMWrapped`, and only for the four wrappers that have an iterator. Those four take the alias from JSDOMWrapper. - Remove `NodeVMModule::Type`, which nothing names. - Remove four locals that are declared and never read (CryptoPrimes.cpp, JSCipherPrototype.cpp, BunJSCModule.h). - Remove `ToolSpec.versionArg`, which no tool spec sets. - Remove the `sys/Error.rs` entry of `rustIdentifierPaths` and the `internalRegistry` field the module registry scanner returns. Nothing reads either of them. - Remove entries in .gitattributes, .prettierignore and oxlint.json that name paths that do not exist, and one duplicate ignore pattern.
|
Status: ready for review.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (24)
💤 Files with no reviewable changes (22)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughThe pull request removes repository configuration entries, an alternate build-tool version argument, codegen return and identifier entries, C++ binding declarations, and unused local variables. ChangesRepository tooling
Build tool version checks
Code generation interfaces
JSC declarations and implementation cleanup
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The cleanup does not establish a material behavior or configured-check regression in the inspected paths. The PR is mergeable with normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Note on the Biome warning for |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding (which is pre-existing, not introduced here), I checked the deletions themselves: the only reader of DOMWrapped is JSDOMIterator.h, and none of the 14 wrappers losing the alias is instantiated as an iterator Wrapper, so they fall back to the identical base alias in JSDOMWrapper.h:76; no builtin calls $rust("sys/Error.rs", ...); the removed .gitattributes/oxlint.json entries name paths that do not exist in the tree (no scripts/build/deps/*.snapshot, examples/, ZigGeneratedCode.h, or bun-simdutf.h).
Extended reasoning...
Deletion-only sweep (+2/-39) across 14 WebCore wrapper headers, node:crypto locals, NodeVMModule, BunJSCModule.h, two codegen scripts, tools.ts, and three config files; it touches no security-sensitive surface. The one confirmed finding is a pre-existing lifetime bug adjacent to the removed pathOut local, not something this PR introduces. Grepping confirmed each removed C++ alias falls back to the base-class alias and the removed codegen/config entries have no consumers.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/jsc/modules/BunJSCModule.h— pre-existing: a script that callsstartSamplingProfiler(dir)frombun:jscmakes the at-exit profile report read freed memory. BunJSCModule.h:476 buildspathCStringas a local CString, and BunJSCModule.h:484 stores its raw buffer pointer intoOptions::samplingProfilerPath(), which JSC'sreportDataToOptionFiledereferences from the atexit hook long after the block ended. Fix: give the path storage that outlives the function, for example a function-static or intentionally leaked CString (orfastStrDup) whosedata()is whatOptions::samplingProfilerPath()keeps. This hunk only removed the unusedpathOutnext to it; the dangling store was already there.Why this was flagged
User JS calls
startSamplingProfiler("/some/dir")frombun:jsc(registered at src/jsc/modules/BunJSCModule.h:989). At BunJSCModule.h:476auto pathCString = toCString(String(path));creates a WTF::CString whose buffer is refcounted and owned by that local. BunJSCModule.h:484 doesOptions::samplingProfilerPath() = pathCString.span().data();and BunJSCModule.h:485 callssamplingProfiler.registerForReportAtExit(). When theif (!path.isEmpty())block ends at BunJSCModule.h:486 the CString is destroyed and its buffer freed, but the global option still holds the pointer. At process exit JSC's atexit handler runsSamplingProfiler::reportDataToOptionFile(), which readsOptions::samplingProfilerPath()to build the output file name, so it reads freed heap memory: under ASan this is a heap-use-after-free abort, otherwise the profile is written to a garbage or wrong path or the process crashes at exit. The base branch has the same defect; this PR removes only the unusedStringPrintStream pathOut;from the same block, so it is pre-existing. No safeguard exists: nothing else keeps the…Verification: pre-existing (the diff touches this exact block: it deletes the unused
StringPrintStream pathOut;that sat at old line 476, but the dangling-pointer store is unchanged from the base). Trigger: any script that callsstartSamplingProfiler("<dir>")frombun:jsc(exported at /home/claude/bun/src/jsc/modules/BunJSCModule.h:989, typed in packages/bun-types/jsc.d.ts:766, exercised by…
The open fix for the startSamplingProfiler crash already removes this line. Leave it to that change.
|
On the review finding about
|
There was a problem hiding this comment.
LGTM — pure dead-code removal with every deleted name confirmed to have no remaining reader.
What was reviewed:
DOMWrapped: the only consumer isJSDOMIterator.h, instantiated solely for JSCookieMap/JSURLSearchParams/JSDOMFormData/JSFetchHeaders, none of which are touched; the baseJSDOMWrapper<T>alias still exists for the 14 edited wrappers.versionArg,internalRegistryreturn field,NodeVMModule::Type, and the four C++ locals: repo-wide grep finds no reference outside the deleted lines;sys/Error.rsstill exists but no$rust()call names it, and a stale key would fail loudly at codegen anyway.- Config removals (.gitattributes, .prettierignore, oxlint.json override globs): every dropped path is absent from the tree; the oxlint duplicate ignore entry was an exact repeat.
Extended reasoning...
The net diff is +2/-38 across 23 files: type-alias and enum removals in WebCore wrapper headers and NodeVMModule.h, unused locals in two node:crypto .cpp files, an unused ToolSpec field and codegen map entry in build scripts, and stale entries in three config files. The crypto files are touched only to drop never-read locals, so no security-sensitive logic changes; no exception-check or ownership paths are altered. None of the changed files fall under .github/CODEOWNERS. The second commit reverted the BunJSCModule.h hunk after the earlier review, and the remaining diff verified clean on independent greps, which decided approve over defer.
|
Updated 12:15 PM PT - Sep 24th, 2026
✅ @robobun, your commit 3cc1293b5f610d1aaf8aff651ea4a27e7d4a8af2 passed in 🧪 To try this PR locally: bunx bun-pr 43914That installs a local version of the PR into your bun-43914 --bun |
2 participants
Behaviour change: none
Problem
Fix
using DOMWrappedfrom 14 wrapper headers (JSAbortSignal.h,JSBroadcastChannel.h,JSCloseEvent.h,JSCustomEvent.h,JSErrorEvent.h,JSMessageEvent.h,JSMessagePort.h,JSPerformance.h,JSPerformanceMark.h,JSPerformanceMeasure.h,JSPerformanceResourceTiming.h,JSWebSocket.h,JSWorker.h,JSWebView.h).NodeVMModule::Typeand three locals that nothing reads:contents(CryptoPrimes.cpp),dataStringandencodingString(JSCipherPrototype.cpp).ToolSpec.versionArg(scripts/build/tools.ts), thesys/Error.rskey ofrustIdentifierPaths(generate-js2native.ts), and theinternalRegistryfield thatcreateInternalModuleRegistryreturns. Remove eight entries of.gitattributes,.prettierignoreandoxlint.jsonthat name paths that do not exist, and one duplicate pattern.rgfinds no other use of each name insrc/,packages/,scripts/,test/andbuild/debug/codegen/.bun bdpasses. With the debug build:abort.test.ts,broadcast-channel.test.ts,websocket-client.test.ts,perf_hooks.test.ts,vm.test.ts,test-crypto-prime.js,test/internal/source-lints/.Background
DOMWrappednames the C++ class that a JS wrapper class wraps.JSDOMWrapper<T>defines it for each wrapper. OnlyJSDOMIterator.hreads it, forJSFetchHeaders,JSDOMFormData,JSURLSearchParamsandJSCookieMap. None of those four declares its own alias.ToolSpecdescribes a tool that the build looks for on the PATH.versionArgwas for a tool that prints its version withversionand not--version. No tool spec sets it.Downsides
rg, the debug build, and the suites above.Notes
How this sweep searched
any_dispatch!,comptime_string_map!statics, struct field shorthand, the codegen namer#ref), no unreferenced item was left outside the open PRs.--gc-sections --print-gc-sections. The debug build has no inlining, so a dropped function section has no caller on linux-x64. The relink dropped 337 C-named functions and 166 plain C++ functions from Bun's own objects. The open PRs already delete them, except the items under "Kept" below.src/js/: every builtin export has aCodeGeneratorreference, every internal module has a loader, and no module-local binding is unused..rsfiles outside every module tree, headers that nothing includes,.cppfiles outside the build, unused Cargo dependencies (cargoreports 9 on linux. Open PRs already remove 4. The other 5 have a use on another target or in tests): nothing to remove.Kept, with the reason
WEBCORE_GENERATED_CONSTRUCTOR_GETTER(ZigGlobalObject.cpp) defines a<Name>_getterfor 51 classes. Only 15 have a user. To remove the other 36 needs a second macro, which adds lines.PerformanceResourceTiming,ResourceTiming,NetworkLoadMetrics,ResourceLoadTiming: nothing creates an instance, butPerformanceResourceTimingis a global constructor, so the class is API surface.bun_core::strings::split_once: no caller, butclippy.tomlnames it as the replacement forstr::split_once.StringPrintStream pathOut(BunJSCModule.h): an unread local, but Write the sampling profiler report at exit from Bun, not JSC's atexit hook #41137 already removes that line in its rewrite ofstartSamplingProfiler.jsFunctionAppendOnLoadPluginNodeand three sibling host functions,Process_defaultSetter,Bun__napi_get_version: no caller, already removed by Remove dead code from the JSC bindings, headers.h, the console builtin, and bun_jsc #40232.completions/bun-cli.jsonandmisctools/generate-cli-completions.ts: nothing reads the JSON, but feature PRs keep it up to date by hand.scripts/lldb-inline-tool.cpp,scripts/lldb-inline.sh,scripts/github-metrics.ts,scripts/gamble.ts: nothing references them, but they are manual developer tools.HiveBitSet::_FITS(src/collections/hive_array.rs): a static assertion, so it stays. It has a defect outside the scope of this PR: it is an associated const of a generic impl and nothing references it, so rustc never evaluates it. A reference such aslet () = Self::_FITS;ininit_empty()makes it run.ncrypto.cpp, and theCMAKE_*exports inflake.nixandshell.nix(not testable here).no test proof · iteration 0 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check