Remove dead code from C++ bindings (ErrorCode, JSX509CertificateConstructor, JSDOMWrapperCache) and bun_core - #36500
Conversation
C++ bindings:
- JSX509CertificateConstructor.{h,cpp}: empty namespace blocks only; the
real JSX509CertificateConstructor class is defined inline in
JSX509Certificate.cpp
- BunHeapProfiler.h: only included by its own .cpp; generateHeapProfile
is now file-static
- Bun::ERR::{CRYPTO_TIMING_SAFE_EQUAL_LENGTH, KEY_GENERATION_JOB_FAILED,
CLOSED_MESSAGE_PORT, CRYPTO_INVALID_KEYTYPE (no-message overload)}:
throw wrappers with zero C++ callers (the corresponding ErrorCode enum
values remain live and are reached from Rust/JS)
- JSDOMWrapperCache.h deprecatedGetDOMStructure / getOrCreateWrapper:
forward-declared templates with zero instantiations
- {ModuleLoader,JSNextTickQueue,JSMockFunction}.h createWithInitialValues:
declared, never defined, never called
- AsymmetricKeyValue.h AsymmetricKeyValue(EVP_PKEY*, bool): declared,
never defined, never called
- JSBufferList getBufferList: zero callers; ZigGlobalObject::JSBufferList()
is used directly
Rust:
- bun_core::container_of_const: zero callers (the *mut-out container_of
covers every call site); re-export in bun_ptr dropped
- bun_core::literal! macro: zero invocations; callers use byte literals
or w!() directly
- http::ssl_config From<Arc<SSLConfig>> for SharedPtr: Arc<SSLConfig> is
never constructed outside ssl_config.rs; both construction sites use
SharedPtr::new or the tuple constructor
Verified: rg -w across src/ and build/debug/codegen/ shows each removed
symbol is referenced only at its own definition. bun bd passes;
bun run rust:check-all passes all 10 target triples.
|
Warning Review limit reached
Next review available in: 18 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 (1)
WalkthroughThis change removes unused public Rust and C++ APIs, deletes obsolete binding files, limits heap-profile generation to one source file, and adds source-lint checks that prevent removed symbols from returning. ChangesDead API cleanup
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The re-export speaks for itself; dropping the comment also keeps the diff minimal now that literal! is gone from the list.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Same usage pattern: only called by extern "C" Bun__generateHeapSnapshotV8 in the same translation unit, never declared in a header.
|
Updated 1:05 AM PT - Jul 31st, 2026
@Jarred-Sumner, your commit 9dec341 is building: |
There was a problem hiding this comment.
LGTM — pure dead-symbol removals, all verified as uncalled/undefined; the earlier generateHeapSnapshotV8 nit was addressed in 75589a0. The remaining JSDOMWrapperCache.h note is a non-blocking completeness observation about pre-existing WebKit-inherited declarations and can go in a follow-up.
Spot-checked: container_of_const/literal! have zero hits in src/; the removed Bun::ERR:: wrappers have no C++ callers (the ErrorCode::ERR_* enum entries in ErrorCode.ts remain, as stated); the codegen ${typeName}__createWithInitialValues extern-C symbol is unrelated to the removed static createWithInitialValues method declarations; no remaining includes of the deleted headers.
Extended reasoning...
Overview
Dead-code sweep across 19 files: deletes three empty/self-referencing files (JSX509CertificateConstructor.{h,cpp}, BunHeapProfiler.h), removes four uncalled Bun::ERR:: throw-wrapper overloads from ErrorCode.{h,cpp}, drops three declared-never-defined createWithInitialValues static factories, two never-instantiated templates from JSDOMWrapperCache.h, one declared-never-defined constructor from AsymmetricKeyValue.h, the uncalled getBufferList free function, and three dead Rust items (container_of_const, literal!, From<Arc<SSLConfig>>). Adds a source-lint regression test that fails if any of these reappear. Net -127 lines in src/.
Security risks
None. Every change is a deletion of an uncalled/undefined symbol or a linkage narrowing (static) of a TU-local function. No control-flow, validation, or crypto logic is touched — the ErrorCode::ERR_* enum values and their Rust/JS callers remain intact; only the unused C++ convenience wrappers are removed.
Level of scrutiny
Low-to-moderate. This is the fourth PR in a series of similar dead-code sweeps (#35437, #36178, #36390). The risk surface is "is each symbol actually dead?", which the author verified via rg -w across src/, src/codegen/, and build/debug/codegen/, plus a full bun bd build and rust:check-all across all 10 target triples. I independently spot-checked the key claims: no remaining references to container_of_const, literal!, getBufferList, or the deleted headers; the remaining ERR_* hits are the enum entries (intentionally kept); and the codegen __createWithInitialValues extern-C symbol is a distinct name from the removed static class methods. Declared-never-defined items (the three createWithInitialValues, AsymmetricKeyValue(EVP_PKEY*, bool), getOrCreateWrapper) would have link-errored on any use, so their removal is provably safe.
Other factors
Two prior review rounds already happened: the comment-cop bot's note on the string/mod.rs explanatory comment was addressed (9629caf), and my generateHeapSnapshotV8 linkage-consistency nit was addressed (75589a0). The one open inline comment on JSDOMWrapperCache.h:43 is explicitly non-blocking — it points out two more declared-never-defined siblings in WebKit-inherited code that the sweep could optionally cover, but leaving them does not affect correctness. The new source-lint test follows the established test/internal/source-lints/ pattern and was verified to fail on the pre-change tree.
…ed wrap overload The getDOMPrototype forward declaration took JSC::JSGlobalObject* but the definition takes JSDOMGlobalObject&, making them distinct overloads (the forward-declared one was never defined or called). Corrected the declaration to match. The wrap(JSGlobalObject*, DOMWrapperWorld&, DOMClass&) overload was declared but never defined; only the (JSGlobalObject*, JSDOMGlobalObject*, DOMClass&) overload is defined and used.
There was a problem hiding this comment.
LGTM — pure dead-code removal, all prior nits addressed.
What was reviewed:
- Verified each removed C++ symbol (
ERR::wrappers,createWithInitialValues,getBufferList,deprecatedGetDOMStructure/getOrCreateWrapper,AsymmetricKeyValue(EVP_PKEY*, bool)) has zero remaining references insrc/or codegen; theErrorCode::ERR_*enum values stay live via Rust/JS callers. - Confirmed the codegen
${typeName}__createWithInitialValuespattern is unrelated to the removed static class-method declarations. - Rust removals:
container_of_constandbun_core::literal!have no call sites (path_literal!/os_path_literal!are separate macros inbun_paths);Arc<SSLConfig>is not named outsidessl_config.rs. staticlinkage additions and thegetDOMPrototypefwd-decl fix from the follow-up commits are consistency-only, no behavior change.
Extended reasoning...
Overview
This PR is a dead-code sweep across src/jsc/bindings/*.{h,cpp} and a handful of bun_core/bun_http/bun_ptr Rust helpers, net -127 lines. Every removal falls into one of: (a) declared-but-never-defined (would link-error if invoked — createWithInitialValues ×3, AsymmetricKeyValue(EVP_PKEY*, bool), getOrCreateWrapper, the mismatched wrap overload); (b) defined-but-zero-callers (four Bun::ERR:: throw wrappers, getBufferList, deprecatedGetDOMStructure, container_of_const, literal!, From<Arc<SSLConfig>>); or (c) empty/self-only-included files (JSX509CertificateConstructor.{h,cpp}, BunHeapProfiler.h). The two non-deletion changes — adding static to generateHeapProfile/generateHeapSnapshotV8 and fixing the getDOMPrototype forward-declaration signature — are linkage/declaration-only with no runtime effect.
Security risks
None. The removed ERR:: throw wrappers (CRYPTO_TIMING_SAFE_EQUAL_LENGTH, KEY_GENERATION_JOB_FAILED, CLOSED_MESSAGE_PORT, no-arg CRYPTO_INVALID_KEYTYPE) were unreferenced C++ convenience helpers; the underlying ErrorCode::ERR_* enum values remain and are still thrown from Rust (node_crypto_binding.rs) and JS ($ERR_CLOSED_MESSAGE_PORT), so no error path is weakened. The SSLConfig change removes an unused From impl only — no TLS logic touched.
Level of scrutiny
Low-to-moderate. Dead-code removal is inherently low-risk (deletions of uncallable/undefined symbols cannot change observable behavior), and the PR description documents per-symbol rg -w verification plus bun bd and rust:check-all across all 10 targets. I independently spot-checked each removal class against the current tree and confirmed zero remaining references outside the definitions themselves. The one place I looked harder was createWithInitialValues, since generate-classes.ts contains that substring — but it emits ${typeName}__createWithInitialValues free functions for .classes.ts-driven types, which is an entirely separate mechanism from the three hand-written JSInternalFieldObjectImpl subclasses whose never-defined static factory declarations are removed here.
Other factors
Both of my prior inline nits (make generateHeapSnapshotV8 file-static for consistency; fix the getDOMPrototype fwd-decl signature and drop the undefined wrap(..., DOMWrapperWorld&, ...) overload) were addressed in 75589a0 and a6f58e6 respectively, and the comment-cop feedback on string/mod.rs was resolved in 9629caf. A source-lint test (test/internal/source-lints/dead-symbols-errorcode-x509-buncore.test.ts) guards every removed symbol against reintroduction and was verified to fail on revert. No outstanding review threads.
There was a problem hiding this comment.
LGTM — pure dead-code removal with zero callers verified for every deleted symbol; both earlier nits (file-static for generateHeapSnapshotV8, mismatched getDOMPrototype/wrap fwd-decls) are addressed.
What was reviewed:
- Grepped each removed C++ wrapper (
CRYPTO_TIMING_SAFE_EQUAL_LENGTH,KEY_GENERATION_JOB_FAILED,CLOSED_MESSAGE_PORT, 2-argCRYPTO_INVALID_KEYTYPE,getBufferList,createWithInitialValues,deprecatedGetDOMStructure,getOrCreateWrapper) and Rust helper (container_of_const,literal!,From<Arc<SSLConfig>>) acrosssrc/— no callers; theErrorCode::ERR_*enum values and 3-argCRYPTO_INVALID_KEYTYPEoverload remain live. - Confirmed
JSX509CertificateConstructoris defined inline inJSX509Certificate.cpp:82and the deleted.h/.cppwere empty shells; no build-system references. - The one
createWithInitialValueshit ingenerate-classes.tsproduces a distinct mangled${typeName}__createWithInitialValuessymbol for codegen'd classes — unrelated to the three hand-written declarations removed here.
Extended reasoning...
Overview
Dead-code sweep across src/jsc/bindings/*.{h,cpp} and three bun_core/bun_http helpers. Deletes two whole files (JSX509CertificateConstructor.{h,cpp} — empty namespace shells; BunHeapProfiler.h — single-TU forward decl), four unused Bun::ERR:: throw wrappers, several declared-never-defined static factories/constructors (createWithInitialValues × 3, AsymmetricKeyValue(EVP_PKEY*, bool)), two never-instantiated template forward decls in JSDOMWrapperCache.h, the getBufferList free function, and three Rust items (container_of_const, the literal! macro, From<Arc<SSLConfig>> for SharedPtr). Adds a source-lint test guarding reintroduction. Net -127 lines.
Security risks
None. No runtime code paths change — every removed symbol had zero callers (spot-verified with repo-wide grep). The ErrorCode::ERR_* enum values behind the removed throw wrappers remain in ErrorCode.ts and are still reached from Rust (node_crypto_binding.rs) and JS ($ERR_CLOSED_MESSAGE_PORT), so error behavior is unchanged. TLS/crypto surface is untouched: the deleted AsymmetricKeyValue(EVP_PKEY*, bool) constructor was declared but never defined (would have link-errored if called); the SSLConfig change removes only an unused From impl.
Level of scrutiny
Low. This is a mechanical removal of provably-unreachable declarations and definitions. The C++ removals are either (a) declared-never-defined symbols that would fail to link if invoked, (b) defined-but-never-called convenience wrappers whose underlying enum values remain, or (c) empty files. The Rust removals are pub items with zero references outside their own definition. bun bd and bun run rust:check-all (all 10 targets) pass per the description. The JSDOMWrapperCache.h fwd-decl signature correction (a6f58e6) is a no-op behaviorally — it aligns the declaration with the existing definition rather than introducing a new overload.
Other factors
Both prior 🟡 review nits from earlier runs of this reviewer were addressed (75589a0, a6f58e6) and the threads are resolved. The comment-cop bot's note on string/mod.rs was also addressed (9629caf). The added test/internal/source-lints/ test is a static file-content check with no runtime dependency, matching the existing pattern in that directory. The one remaining createWithInitialValues grep hit in generate-classes.ts generates a mangled ${typeName}__createWithInitialValues extern-C symbol for .classes.ts-driven classes and is unrelated to the three hand-written JSInternalFieldObjectImpl subclasses touched here.
There was a problem hiding this comment.
LGTM — mechanical dead-code removal with each deletion independently verified.
What was reviewed:
- Spot-checked removed ErrorCode wrappers (
CRYPTO_TIMING_SAFE_EQUAL_LENGTH,KEY_GENERATION_JOB_FAILED,CLOSED_MESSAGE_PORT, no-argCRYPTO_INVALID_KEYTYPE) — enum values remain live viaErrorCode.ts/Rust/JS; only the unused C++ throw wrappers are gone. The 3-argCRYPTO_INVALID_KEYTYPEoverload used byJSECDHPrototype.cppis kept. - Confirmed
container_of_const,literal!,getBufferList,getOrCreateWrapper,deprecatedGetDOMStructure,Arc<SSLConfig>have zero remaining references; deleted headers are not listed in any build file. - Both earlier nits (file-
staticforgenerateHeapSnapshotV8;getDOMPrototype/wrapforward-decl mismatch) were addressed in follow-up commits.
Extended reasoning...
Overview
This PR is a pure dead-code sweep across src/jsc/bindings/*.{h,cpp} and a handful of bun_core/bun_http/bun_ptr items, netting -127 lines. It deletes two whole files that contained only includes and empty namespaces (JSX509CertificateConstructor.{h,cpp}) or a single self-referential declaration (BunHeapProfiler.h), removes four unused Bun::ERR:: convenience throw wrappers whose underlying ErrorCode enum values remain reachable from Rust/JS, drops several declared-never-defined C++ static factories and template overloads, and removes three uncalled Rust items (container_of_const, the literal! macro, and a From<Arc<SSLConfig>> impl).
Security risks
None. This is deletion-only; no new code paths, no changed behavior. The removed ERR:: wrappers were never invoked, and their error codes remain routed through the centralized ErrorCode machinery from Rust (node_crypto_binding.rs) and JS ($ERR_CLOSED_MESSAGE_PORT). The SSLConfig change removes an unused From conversion — no TLS logic touched.
Level of scrutiny
Low-medium. Dead-code removal is inherently low-risk — the compiler/linker would catch any missed reference. I independently verified the PR's zero-caller claims for each removed symbol via grep across src/ (including codegen inputs and build glob patterns) and confirmed the deleted file names appear nowhere in CMake or elsewhere. The one createWithInitialValues hit in generate-classes.ts is an unrelated ${typeName}__createWithInitialValues extern-C pattern, not the removed static class methods.
Other factors
Two prior inline nits from my earlier review passes were both addressed (75589a0 made generateHeapSnapshotV8 file-static to match its sibling; a6f58e6 fixed the getDOMPrototype forward-decl parameter type and dropped the undefined wrap(..., DOMWrapperWorld&, ...) overload). The comment-cop bot's flag on string/mod.rs was resolved in 9629caf. The guard test was later dropped in 9dec341 — reasonable, since these are one-time deletions and the compiler already enforces they stay gone. PR description states bun bd and bun run rust:check-all (all 10 targets) pass.
…clippy lints - src/jsc/bindings/BunHeapProfiler.h: restored (deleted by #36500 on main, still needed for $newCppFunction in src/js/node/v8.ts -> GeneratedJS2Native.h) - src/jsc/web_worker.rs: parent_ref binding was removed by the &self-only refactor merge; use the surrounding unsafe { (*parent).field } pattern - clippy: question_mark/manual_contains in ResolveMessage.rs, redundant_slicing in package_json.rs, unnecessary_lazy_evaluations in BunHeapProfiler.rs
- restore src/jsc/bindings/BunHeapProfiler.h (deleted by #36500 on main, still needed by $newCppFunction sites in src/js/node/v8.ts) - web_worker.rs: parent_ref was removed by #36571; deref parent directly - path.rs: re-expose resolve_{posix,windows}_t as pub(crate) for permission.rs - clap: re-expose Diagnostic fields as pub for Arguments.rs node-compat errors - permission.rs: switch std::sync::RwLock -> bun_threading::RwLock, std::env::var -> bun_core::env_var::NODE_OPTIONS (disallowed_types/methods) - run_command.rs: exec_check pub -> pub(crate) (unreachable_pub) - BunHeapProfiler.rs: then(|| ..) -> then_some (unnecessary_lazy_evaluations) - Timer.rs: move SAFETY comment onto the unsafe block it documents
… fallout - Restore src/jsc/bindings/BunHeapProfiler.h (deleted by #36500 on main, but still needed by GeneratedJS2Native.h for the $newCppFunction calls added on this branch). - VirtualMachine.rs / jsc_hooks.rs: resolve_maybe_needs_trailing_slash now takes ResolveMode, not is_esm/is_user_require_resolve; derive the two bools from `mode` for Bun__runModuleResolveHooks. Use `&raw const` for the pointer args. - web_worker.rs: parent_ref binding was lost in the merge; read heap_profiler_config via `(*parent)` like the neighbouring fields. - permission.rs: switch to bun_threading::RwLock (no poisoning) and bun_core::env_var::NODE_OPTIONS per clippy disallowed-types/methods. - clap Diagnostic fields pub (read by bun_runtime::cli::Arguments). - path.rs resolve_posix_t pub(crate) (called from permission.rs). - Minor clippy: then_some, contains(), SAFETY comment placement, unreachable_pub on exec_check.
…c/bun_runtime - src/jsc/bindings/BunHeapProfiler.h: restored (deleted by #36500 on main, still needed for $newCppFunction in src/js/node/v8.ts -> GeneratedJS2Native.h) - src/jsc/web_worker.rs: parent_ref binding was removed by the &self-only refactor merge; use the surrounding unsafe { (*parent).field } pattern - src/runtime/node/types.rs: define BUFFER_EXPECTED_TYPES imported by node_fs.rs - src/clap/lib.rs: Diagnostic fields pub (read by bun_runtime::cli::Arguments) - src/runtime/node/path.rs: resolve_{posix,windows}_t pub(crate) for permission.rs - src/runtime/permission.rs: std::sync::RwLock -> bun_threading::RwLock, std::env::var -> bun_core::env_var::NODE_OPTIONS, manual_contains lint - clippy: unreachable_pub (run_command.rs), undocumented_unsafe_blocks (Timer.rs), unnecessary_lazy_evaluations (BunHeapProfiler.rs)
… fallout
- src/jsc/bindings/BunHeapProfiler.h: restore header required by
GeneratedJS2Native.h for $newCppFunction("BunHeapProfiler.cpp", ...)
calls in src/js/node/v8.ts (deleted by #36500, still referenced here)
- src/jsc/web_worker.rs: parent_ref -> unsafe (*parent) deref (removed
by #36571); move SAFETY comment to satisfy undocumented_unsafe_blocks
- src/bun_core/output.rs: configure_thread_no_js pub(crate) -> pub so
the WatchReloadGrace thread in bun_jsc can call it
- src/jsc/VirtualMachine.rs: bun_watcher_ptr pub(crate) -> pub for
run_command.rs --watch-path registration
- src/clap/lib.rs: Diagnostic.{arg,short,long} pub(crate) -> pub for
Arguments.rs node-compat error reporting
- src/runtime/node/path.rs: resolve_{posix,windows}_t -> pub(crate) for
permission.rs
- src/runtime/cli/run_command.rs: exec_check pub -> pub(crate)
(unreachable_pub)
- src/jsc/BunHeapProfiler.rs: then(||) -> then_some (clippy)
- src/jsc/hot_reloader.rs: allow disallowed from_utf8_lossy for argv
display (node-parity lossy decode)
2 participants
Dead-code sweep of top-level
src/jsc/bindings/*.{h,cpp}plus a fewbun_core/bun_httphelpers. Net -127 lines insrc/.Removed (whole files)
src/jsc/bindings/JSX509CertificateConstructor.{h,cpp}: contained only includes and an emptynamespace Bun { }block. The realJSX509CertificateConstructorclass is defined inline inJSX509Certificate.cpp:82. The header was included only by its own.cpp.src/jsc/bindings/BunHeapProfiler.h: declaredBun::generateHeapProfile, which was only included by and called from its own.cpp(theextern "C" Bun__generateHeapProfilecaller is in the same TU). The function is now file-static.Removed (C++ symbols)
Bun::ERR::CRYPTO_TIMING_SAFE_EQUAL_LENGTH,KEY_GENERATION_JOB_FAILED,CLOSED_MESSAGE_PORT,CRYPTO_INVALID_KEYTYPE(no-message overload) inErrorCode.{h,cpp}: convenience throw wrappers with zero C++ callers. TheErrorCode::ERR_*enum values remain live and are still reached from Rust (node_crypto_binding.rs) and JS ($ERR_CLOSED_MESSAGE_PORTinworker_threads.ts). The 3-argCRYPTO_INVALID_KEYTYPE(..., ASCIILiteral)overload stays (used byJSECDHPrototype.cpp).WebCore::deprecatedGetDOMStructure<T>andgetOrCreateWrapper<T>inJSDOMWrapperCache.h: forward-declared templates with zero instantiations (getOrCreateWrapperwas never even defined).deprecatedGlobalObjectForPrototypestays (called byJSDOMExceptionHandling.cpp).createWithInitialValuesstatic factory declarations inModuleLoader.h,JSNextTickQueue.h,JSMockFunction.h: declared, never defined, never called (would link-error if invoked).AsymmetricKeyValue(EVP_PKEY*, bool)constructor inAsymmetricKeyValue.h: declared, never defined; only theAsymmetricKeyValue(CryptoKey&)overload exists in the.cpp.WebCore::getBufferListinJSBufferList.{h,cpp}: zero callers;ZigGlobalObject::JSBufferList()is used directly.Removed (Rust)
bun_core::container_of_const(and itsbun_ptrre-export): the*const-out variant has zero callers; every intrusive-field recovery site uses the*mut-outcontainer_of.bun_core::literal!macro: zero invocations anywhere; callers use byte literals orw!()directly.impl From<Arc<SSLConfig>> for http::ssl_config::SharedPtr:Arc<SSLConfig>is never named outsidessl_config.rsitself; both construction sites useSharedPtr::newor the tuple constructor.Verification
For each removed symbol,
rg -wacrosssrc/,src/codegen/, andbuild/debug/codegen/shows references only at the definition.bun bdpasses;bun run rust:check-allpasses all 10 target triples. X509 tests andcrypto.timingSafeEqualerror path pass.test/internal/source-lints/dead-symbols-errorcode-x509-buncore.test.tsguards against reintroduction: withsrc/reverted, all 3 checks fail (each removed symbol is detected as present); with this change, all 3 pass.Areas scanned clean (no dead pub items found)
src/io,src/install(12 largest files +PackageManager/*),src/http(exceptssl_configabove),src/uws,src/runtime/api,src/runtime/bake,src/bun_core(except the two items above),src/collections,src/dotenv,src/which,src/js/node/*(22 files), every#[allow(dead_code)]in 16 files (all justified cfg/macro/test escapes).