Skip to content

Remove dead code from JSDOMWrapper.h and ncrypto.cpp: two preprocessor blocks that never compile - #48067

Open
robobun wants to merge 1 commit into
mainfrom
robobun/a2dbe2bd/dead-code-sweep
Open

robobun wants to merge 1 commit into
mainfrom
robobun/a2dbe2bd/dead-code-sweep

Conversation

@robobun

@robobun robobun commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • JSDOMWrapper.h has an #ifndef RENAMED_JSDOM_GLOBAL_OBJECT block that no translation unit can compile. The header first includes ZigGlobalObject.h, which defines the macro and the same alias. The block also holds the closing brace of namespace WebCore: when it is active, clang stops with extraneous closing brace ('}').
  • ncrypto.cpp defines EVP_PKEY_CTX_set_dsa_paramgen_q_bits for OpenSSL older than 1.1.1e. BoringSSL reports 1.1.1g and declares that function.

Fix

  • Delete both blocks (5 and 11 lines). ZigGlobalObject.h loses the 3 guard lines around its alias: no other file names the macro.
  • Correct because the compiler sees the same text. The preprocessed output of ncrypto.cpp and of 40 units that read JSDOMWrapper.h (654 MB) differs only in the __LINE__ of assertions.
  • Verified: bun bd, then 457 tests in 6 files (node-crypto, web-globals, globals, abort, message-channel, broadcast-channel).

Behaviour change: none

Background

  • JSDOMGlobalObject is the WebCore name for the global object. In Bun it is an alias of Zig::GlobalObject. The macro is a guard from 2022 that let either header declare the alias first.
  • ncrypto is the OpenSSL wrapper of node:crypto. Bun links BoringSSL on every platform.
  • Considered a larger cut: 101 more dead lines in bun_libuv_sys and bun_uws_sys. Remove libuv on Windows #47776 deletes that code, so they stay out.

Downsides

  • None found for users. The macro has no hit in generated code, the code generators or the WebKit headers.
  • A failed assertion in one of the three files reports a lower line number.
  • Rename Zig-prefixed identifiers to Bun/Rust equivalents #30747 gains one conflicted file. It already conflicts with main in 1,612 files.
Notes

Evidence

  • JSDOMWrapper.h, probe: the header with the block as the preprocessor leaves it when the macro is not defined, compiled with the flags of BunProcess.cpp. Clang reports 6 errors: 'NodeConstants' is not a class, namespace, or enumeration (2), unknown type name 'JSDOMGlobalObject' (2), one mismatch of ScriptExecutionContext * types, and extraneous closing brace ('}').
  • Every translation unit of the Linux debug build was preprocessed with clang -E: 1,140 hand-written sources, each one alone, and 157 generated ones. None has an active line in JSDOMWrapper.h:33-37 or in ncrypto.cpp:25-34.
  • 551 of the 1,297 units read JSDOMWrapper.h. The 40 compared ones are spread over them (every 13th). With the integers masked on the lines that name one of the three files, the text before and after is byte-identical. Without the mask, Undici.cpp differs in one line: WTFReportAssertionFailure(".../JSDOMWrapper.h", 64, ...) becomes 59.
  • git grep RENAMED_JSDOM_GLOBAL_OBJECT on main finds the two headers only. git blame dates both blocks to March 2022.
  • vendor/boringssl/include/openssl/base.h defines OPENSSL_VERSION_NUMBER 0x1010107f, and evp.h declares EVP_PKEY_CTX_set_dsa_paramgen_q_bits. The fallback tests < 0x1010105fL.
  • The run of 2026-09-26 verified the 8 header lines and held them for a later run (notes of Remove dead code: 17 log scopes nothing logs to and four fields nothing reads, in 10 crates #44050).
  • clang-format reports nothing. bun test test/internal/source-lints/: 194 pass. The three test-crypto-keygen*dsa* tests of the Node suite skip themselves with BoringSSL, as before.

Open pull requests

  • 6,109 were open at 03:44 UTC on 2026-11-07. 112 change one of the two headers and 13 change ncrypto.cpp: 124 in all.
  • Trial merges (git merge-tree) of the 124:
  • No open pull request deletes these lines. The diffs of the 88 open dead-code pull requests were compared.

How this run looked for dead code

  • Rust. cargo hawk, the cross-crate analyzer that hawk.toml is written for (upstream main, built on the repository's nightly), on x86_64-unknown-linux-gnu, x86_64-pc-windows-msvc and aarch64-apple-darwin. Each run covers the product graph and --workspace --all-targets. An item is dead when every target that compiles it reports hawk::dead_public. Result: 842 items.
  • C and C++. Each #if branch with no active line in any of the 1,297 preprocessed units was listed with its condition: 2,441 branches. 1,098 depend on the platform or the build flavour, and most of the rest are in third-party sources such as the SQLite amalgamation. Outside third-party code 442 lines remain. Apart from the two blocks here, each is a branch for another build (no AOT, malloc debugging, NAPI_VERBOSE), generated code, or a line that an open pull request deletes or edits.
  • Also: ENABLE(), USE() and HAVE() conditions on a macro that nothing defines (4), #define names that nothing reads (13), and commented-out code of three lines or more (55 blocks).

Found dead in this run, and left alone


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

JSDOMWrapper.h: the #ifndef RENAMED_JSDOM_GLOBAL_OBJECT block. The
header includes ZigGlobalObject.h, which defines the macro and the
alias, before the block. The block also holds the closing brace of
namespace WebCore, so a translation unit where it is active does not
compile. ZigGlobalObject.h keeps the alias and loses the guard that
only this block made necessary.

ncrypto.cpp: the EVP_PKEY_CTX_set_dsa_paramgen_q_bits fallback for
OpenSSL older than 1.1.1e. BoringSSL reports 1.1.1g and declares the
function.
@robobun

robobun commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 10:42 PM PT - Nov 6th, 2026

❌ @robobun, your commit 4b7c970 has 38 failures in Build #138153 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 48067

That installs a local version of the PR into your bun-48067 executable, so you can run:

bun-48067 --bun

@robobun

robobun commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review.

This pull request has no repro. It deletes 19 lines in 3 files that the preprocessor never keeps: the #ifndef RENAMED_JSDOM_GLOBAL_OBJECT block of JSDOMWrapper.h with its guard in ZigGlobalObject.h, and the EVP_PKEY_CTX_set_dsa_paramgen_q_bits fallback of ncrypto.cpp for OpenSSL older than 1.1.1e.

Checks that were run: bun bd, 457 tests in 6 files, a comparison of the preprocessed text before and after (654 MB, the same apart from the __LINE__ of assertions), clang-format, the source lints, and trial merges with the 124 open pull requests that change one of the three files. The description has the details.

Self-review: not run as a separate pass. The diff is 19 deleted lines with no runtime effect.

CI: build 138153 finished. All 14 build-bun jobs passed (Linux, macOS, Windows, FreeBSD and Android), so the three files compile on every platform. The build is red only on tests that are red without this diff: 27 files under test/aot/ and test/bundler/compile-aot-size.test.ts. Each one also fails on the final builds of the last merged pull requests, and this diff changes no code that runs.

Pull request: #48067

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@coderabbitai

coderabbitai Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The changes make the WebCore::JSDOMGlobalObject alias unconditional in ZigGlobalObject.h and remove its guarded declaration from JSDOMWrapper.h. They also remove a pre-1.1.1e OpenSSL fallback macro from ncrypto.cpp.

Changes

JSDOM global object alias

Layer / File(s) Summary
JSDOM global object alias declaration
src/jsc/bindings/JSDOMWrapper.h, src/jsc/bindings/ZigGlobalObject.h
The guarded alias is removed from JSDOMWrapper.h. ZigGlobalObject.h now declares the alias unconditionally.

OpenSSL compatibility fallback

Layer / File(s) Summary
Remove OpenSSL fallback definition
src/jsc/bindings/ncrypto.cpp
The pre-1.1.1e fallback mapping for EVP_PKEY_CTX_set_dsa_paramgen_q_bits to EVP_PKEY_CTX_ctrl is removed.

Suggested reviewers: dylan-conway

Merge Risk: Minimal · up to 4b7c9

The cleanup has no demonstrated behavior change in the configured build. No actionable merge-blocking issue remains; normal build checks should complete before merging.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the two dead-code removals in JSDOMWrapper.h and ncrypto.cpp, which match the changeset. It is long at 96 characters, and the colon clause repeats detail, but it still conveys the prim…
Description check Passed The description follows the template's two sections. 'What does this PR do?' is covered by the Problem and Fix sections, and 'How did you verify your code works?' is covered by the test run, clang-for…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the two dead-code removals in JSDOMWrapper.h and ncrypto.cpp, which match the changeset. It is long at 96 characters, and the colon clause repeats detail, but it still conveys the primary change clearly.

Full details: Description check

Explanation

The description follows the template's two sections. 'What does this PR do?' is covered by the Problem and Fix sections, and 'How did you verify your code works?' is covered by the test run, clang-format, and source-lint results. It is long and includes extensive evidence in collapsed notes, but the required information is present and complete.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 7ca0042 and 4b7c970.

Files selected for processing: 3

  • src/jsc/bindings/JSDOMWrapper.h
  • src/jsc/bindings/ZigGlobalObject.h
  • src/jsc/bindings/ncrypto.cpp

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants