Skip to content

Remove dead code from bun_jsc, bun_core, the bindgenv2 codegen, 15 error types, and the C++ bindings - #40915

Open
robobun wants to merge 9 commits into
mainfrom
robobun/8de2e40b/dead-code-linker-sweep
Open

robobun wants to merge 9 commits into
mainfrom
robobun/8de2e40b/dead-code-linker-sweep

Conversation

@robobun

@robobun robobun commented Aug 29, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • The workspace denies dead_code and unreachable_pub, and the open dead-code PRs hold the cross-crate pub findings. What remains is invisible to both: trait impls nothing calls, helpers whose only callers are dead, and C++ functions that only their own declarations reference.
  • A relink of the debug build with --gc-sections --print-gc-sections lists every function the binary does not reference. Each candidate was then checked against every platform, const use, tests, derives and the open PRs. 73 files, +88 / -1317 (the insertions are the new test and the define() guard).

Fix

  • Rust: the Zig-port src/jsc/bindgen.rs module with its only users, 20 ErrName impls nothing calls through the trait, three Display impls, Appender::append_lower_case, ten inert #[host_fn] shims, and small unused helpers (Notes).
  • Codegen: src/codegen/bindgenv2 still carried the Zig type emitters. The script only writes C++, and its output is byte-identical before and after.
  • C++: unused node:crypto key helpers, the WebCore::JSErrorHandler class, rejectPromiseWithGetterTypeError, toJSNewlyCreated(Ref<Blob>), and two validateInteger instantiations.
  • Verified: bun bd builds. New test/internal/class-definitions.test.ts covers the define() guard and the jest class flags (fails on the base src/codegen). bun bd test on 12 files in the touched areas (1,061 pass; the 6 failures reproduce with a binary linked from the unchanged objects). bun run rust:check-all passes on every target. CI on 1379860: 181 of 182 jobs pass; the red one is url.test.ts on darwin x64, which fails on main too (ICU 76 table). Self-reviewed: 11 concerns raised, 9 addressed (Notes).

Background

  • rustc's dead_code lint runs per crate and treats pub items and trait impls as roots, so a pub fn that only a dead pub fn calls passes the lint.
  • Debug objects are -O0 with one section per function, so a discarded section has no caller at all. A Linux link cannot see platform-gated callers, so every textual reference was traced by hand.
  • ErrName (bun_core::output) names an error kind for the root error printer. JSErrorHandler is WebKit's five-argument onerror listener; Bun's onerror setters all use JSEventListener.
Notes

Removed, Rust:

  • src/jsc/bindgen.rs (file deleted, pub mod bindgen dropped from src/jsc/lib.rs): the Bindgen trait and every adapter (BindgenTrivial, BindgenStrongAny, BindgenNull, BindgenOptional, BindgenString, BindgenArray, ExternTaggedUnion2, ExternUnion2, ExternArrayList). Nothing outside the file names the module; the generated code in src/jsc/generated.rs defines its own ExternArrayList. Three comments in generated.rs that described layouts by these names now describe the layout. Remove dead code from bun_core, bun_jsc, bun_css, react_compiler, and two C++ bindings #40367 removes BindgenTrivial from this file and rewords one more generated.rs comment; whichever PR lands second drops that hunk on rebase.
  • bun_jsc::Strong::adopt (only BindgenStrongAny used it), bun_alloc::realloc_raw (only BindgenArray), bun_core::WTFString with impl ExternalSharedDescriptor for WTFStringImplStruct and the re-exports in bun_core::lib, bun_core::wtf, bun_core::string::wtf, bun_ptr (only BindgenString).
  • impl ErrName for Error in bun_ast, bun_js_parser, bun_js_parser_jsc, bun_js_printer, bun_exe_format, bun_dotenv, bun_shell_parser, bun_io (plus its inherent name()), bun_libarchive (plus inherent name()), bun_options_types, bun_spawn_sys, bun_sourcemap, bun_standalone_graph, bun_crash_handler, bun_brotli, bun_clap, bun_css, bun_router (plus inherent name()), bun_uws_sys (plus inherent name()), and ForManifestError in bun_install. ErrName has three generic consumers (Output::err, JSGlobalObject::throw_error, handle_root_error); none is instantiated with these types on any target (rust:check-all would fail otherwise). Every escape point of each type was traced: callers use ? into a #[from] variant, .is_err(), let _ =, match on variants, or the inherent name().
  • impl Display for bun_md::ParserError with its empty impl Error (consumers match on variants), impl Display for bun_jsc::SystemError (consumers call to_error_instance; the live Display is on bun_sys::SystemError), impl Display for HTMLImportManifest (only EscapedJson is formatted).
  • Appender::append_lower_case (trait method never called through the trait) with the impls in bun_install::FilenameStoreAppender, bun_resolver::FilenameStoreAppender, pm_diff_command::BumpAppender, and the whole impl Appender for &FilenameStore in bun_resolver (every call site passes a FilenameStoreAppender or BumpAppender).
  • MaxHeapScope Deref/DerefMut (the one scope() caller keeps the guard for Drop only), ArrayBitSet::set_intersection and AutoBitSet::set_intersection, Builtins::{from_executable, len, is_empty, modules} (consumers use parse, module, find, dependencies), AbortSignal::detach.
  • #[crate::host_fn(export = "Bun__setSyntheticAllocationLimitForTesting")] and the bare #[bun_jsc::host_fn] on ten functions that dispatch_js2native.rs calls directly (get_active_tasks, js_escape_reg_exp, js_escape_reg_exp_for_package_name_matching, to_utf16_alloc_sentinel, patch_make_diff, patch_apply, patch_parse, translate_nt_status_to_e, translate_uv_error_to_e, sigaction_layout). The attribute only emits a __jsc_host_* shim that nothing references; the functions stay.
  • jest.classes.ts: call: true on the seven noConstructor: true Expect classes. Not a behaviour change: generate-classes.ts only consumes <Name>Class__call in the JS<Name>Constructor class, which noConstructor suppresses, so the flag emitted a thunk and a declaration nothing referenced. The matchers keep their inherent call(), which expect.any(...) et al. invoke directly. The classes: ExpectAny, ExpectAnything, ExpectArrayContaining, ExpectCloseTo, ExpectObjectContaining, ExpectStringContaining, ExpectStringMatching. Re-running generate-classes.ts before and after shows exactly those 7 thunks, 7 declarations and 7 type-only .d.ts interfaces gone. Expect and ExpectTypeOf keep call: true and still wire their __call.

Added: define() in src/codegen/class-definitions.ts now rejects call: true together with noConstructor: true, so the inert combination cannot come back.

Removed, codegen (src/codegen/bindgenv2): Type.bindgenType, Type.zigType, Type.optionalZigType, CodeStyle, every subclass implementation in internal/*.ts, the bindgenOptional() helper, and toZigNamespace with the second duplicate-name check it fed (the first check on type.name stays). The 14 generated files from the 3 .bindv2.ts sources are byte-identical; test/internal/build-codegen-declared-outputs.test.ts passes.

Removed, C++:

  • src/jsc/bindings/node/crypto/CryptoUtil.cpp/.h: keyFromString, passphraseFromBufferSource (only keyFromString called it), the 4-argument parseKeyFormat and 6-argument parseKeyType overloads (live code uses the ThrowScope& overloads).
  • src/jsc/bindings/webcore/JSErrorHandler.cpp/.h (files deleted), the includes in EventEmitter.cpp and EventTarget.cpp, and the setAttributeEventListener<JSErrorHandler> instantiation. JSErrorHandler::create had no caller.
  • rejectPromiseWithGetterTypeError (JSDOMExceptionHandling.cpp/.h) and the CastedThisErrorBehavior::RejectPromise branch in IDLAttribute::get that was its only reference (no attribute instantiates it). get() now static_asserts that it is never instantiated with RejectPromise, so a future user gets a build error instead of the ReturnEarly fallthrough.
  • toJSNewlyCreated(..., Ref<Blob>&&) and the inline RefPtr<Blob> forwarder (blob.cpp/.h); Blob wrappers go through toJS(..., Blob&).
  • NodeValidator.cpp: the validateInteger<size_t> and validateInteger<uint32_t> JSValue-name explicit instantiations (the only JSValue-name caller uses ssize_t).

Tests run: test/js/bun/test/expect.test.js, expect-extend.test.js, test/js/node/crypto/crypto.key-objects.test.ts, crypto-sign-regression.test.ts, test/js/web/workers/worker.test.ts, message-event.test.ts, test/js/web/abort/abort.test.ts, test/js/web/fetch/blob-cow.test.ts, test/js/web/html/blob-array-fast-path.test.ts, test/js/node/events/event-emitter.test.ts, test/js/bun/http/serve.test.ts, test/cli/install/bun-add.test.ts. The 2 worker failures (message flood timing, preload import timeout) and 4 serve failures (IPv6, root port range, #6583, /bun:info loopback) reproduce with the pre-change relinked binary in this container.

Overlap with open PRs: no deletion here repeats one in #39929, #40122, #40172, #40232, #40294, #40367, #40492, #40525, #40557, #40690, #40762, #40824 or #40881. Files shared with them, at other hunks: src/js_parser/error.rs (#40525), src/bun_core/string/immutable.rs (#40525, #40824), src/bundler/HTMLImportManifest.rs (#40762), src/jsc/AbortSignal.rs (#40232, #40824), src/jsc/virtual_machine_exports.rs (#40557), src/jsc/bindgen.rs (#40367), src/jsc/bindings/node/crypto/CryptoUtil.cpp/.h (#40232). JSErrorHandler.cpp is deleted here while #39929 edits one line of it.

Left alone on purpose (linker-dead on Linux, live elsewhere or by design): Bun__Process__hasTitle, getMainThreadScriptExecutionContext, uws_get_loop_with_native, WStr, fmt_path_u16, write_windows_env_block, Dir::copy_file, File::kind, WaitGroup (Windows); Blob__fromMmapWithType and the init_mmap chain, bun_sysconf__SC_CLK_TCK, posix_spawnattr_reset_signals, add_pre_exit_callback, FilePoll/FlagsFormatter Display, Bun__noOrphans_* (macOS/BSD); bun_analytics::*, inflate_embedded* (release only); ScopedLogger::new, hash_const, RawRwLock::new, Semaphore::new, the comptime_string_map! and enumset helpers (const contexts); every thiserror/strum/bitflags derive; btjs/dumpBtjsTrace (lldb helpers); impl Drop for Once<T> and GarbageCollectionController (never dropped today, but correct); the V8 and NAPI API (exported for native modules); PerformanceResourceTiming and friends (web-visible globals).

Self-review: 11 concerns, 9 addressed in 80b894e (delete bindgen.rs outright, the five extra ErrName impls, the ten inert host_fn shims, the define() guard) and 4a96393 (uWS). Not taken: re-slicing the JSErrorHandler, uWS and bindgen.rs hunks into #39929, #40294 and #40367 (those PRs are not mine to edit; the JSErrorHandler.cpp modify/delete against #39929 resolves as "deleted" on rebase), and waiting for #39929 to land first.

Follow-ups that become possible once open PRs land: NameMinifier::default_number_to_minified_name (#40557 removes its only caller), the uWS run() / listen(port, cb) entry points and their uws_app_run / uws_app_listen C wrappers (#40172 removes the Rust App::run / App::listen that declare them; the linker discards all of them today), Stat<BIG>::get_constructor's Bun__JSBigIntStatsObjectConstructor branch (never monomorphized).

Method: clang++ @inputs -Wl,--gc-sections -Wl,--print-gc-sections over the existing debug objects, without -rdynamic, then llvm-cxxfilt on the removed .text.* sections and a diff against llvm-nm of the kept symbols.


[review] gate passed · iteration 4 · 73 files touched

fails on main (without fix)
ASAN without fix: 2 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/class-definitions.test.ts
bun test v1.4.1 (d578a8c70)

test/internal/class-definitions.test.ts:
21 |         noConstructor: true,
22 |         call: true,
23 |         klass: {},
24 |         proto: {},
25 |       }),
26 |     ).toThrow("NoCtor: `call: true` has no effect with `noConstructor: true`");
           ^
error: expect(received).toThrow(expected)

Expected substring: "NoCtor: `call: true` has no effect with `noConstructor: true`"

Received function did not throw
Received value: ClassDefinition {
  name: "NoCtor",
  lang: undefined,
  rustPath: undefined,
  sharedThis: undefined,
  construct: false,
  constructNeedsThis: undefined,
  call: true,
  forBind: undefined,
  prototypeBase: undefined,
  finalize: undefined,
  refCounted: undefined,
  overridesToJS: false,
  klass: {},
  proto: {},
  values: [],
  valuesArray: undefined,
  JSType: undefined,
  noConstructor: true,
  final: undefined,
  estimatedSize: false,
  memoryCost: undefined,
  hasPendingActivity: undefined,
  isEventEmitter: undefined,
  configurab
... (truncated)

release without fix: 2 FAILED
bun test v1.4.1-canary.1 (a33b7ec7c)

test/internal/class-definitions.test.ts:
21 |         noConstructor: true,
22 |         call: true,
23 |         klass: {},
24 |         proto: {},
25 |       }),
26 |     ).toThrow("NoCtor: `call: true` has no effect with `noConstructor: true`");
           ^
error: expect(received).toThrow(expected)

Expected substring: "NoCtor: `call: true` has no effect with `noConstructor: true`"

Received function did not throw
Received value: ClassDefinition {
  name: "NoCtor",
  lang: undefined,
  rustPath: undefined,
  sharedThis: undefined,
  construct: false,
  constructNeedsThis: undefined,
  call: true,
  forBind: undefined,
  prototypeBase: undefined,
  finalize: undefined,
  refCounted: undefined,
  overridesToJS: false,
  klass: {},
  proto: {},
  values: [],
  valuesArray: undefined,
  JSType: undefined,
  noConstructor: true,
  final: undefined,
  estimatedSize: false,
  memoryCost: undefined,
  hasPendingActivity: undefined,
  isEventEmitter: undefined,
  configurable: undefined,
  enumerable: undefined,
  structuredClone: undefined,
  inspectCustom: undefined,
}

      at <anonymous> (/workspace/bun/test/internal/class-defini
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/class-definitions.test.ts
bun test v1.4.1 (d578a8c70)

test/internal/class-definitions.test.ts:
(pass) define() > rejects call: true on a class without a constructor [4.30ms]
(pass) define() > accepts a callable class that has a constructor [3.11ms]
(pass) define() > accepts a non-callable class without a constructor [3.04ms]
(pass) define() > only the jest classes with a constructor are callable [3.19ms]

 4 pass
 0 fail
 4 expect() calls
Ran 4 tests across 1 file. [2.72s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 600ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/72] gen bindgenv2
[2/72] gen generated_host_exports.rs
generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 244 extern-C blocks audited
[3/72] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
  - ResolveMessage (15 fields)
  - BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
  - Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
  - ResourceUsage (8 fields)
  - Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
  - CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
  - FileSystemRouter (5 fields)
  - FrameworkFileSystemRouter (2 fields)
  - MatchedRoute (8 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts
  - Glob (5 fields)
Found 1 classes from /workspace/bun/src/runtime/api/h2.classes.ts
  - H2FrameParser
... (truncated)
diff hotspot
src/ast/error.rs                              |   6 -
 src/brotli/error.rs                           |   6 -
 src/bun_alloc/MaxHeapAllocator.rs             |  17 +-
 src/bun_alloc/lib.rs                          |  20 --
 src/bun_core/external_shared.rs               |  22 --
 src/bun_core/lib.rs                           |   2 +-
 src/bun_core/string/immutable.rs              |   3 +-
 src/bun_core/string/wtf.rs                    |   6 -
 src/bun_core/wtf.rs                           |   2 +-
 src/bundler/HTMLImportManifest.rs             |  16 --
 src/clap/error.rs                             |   6 -
 src/codegen/bindgenv2/internal/any.ts         |  17 +-
 src/codegen/bindgenv2/internal/array.ts       |   8 +-
 src/codegen/bindgenv2/internal/base.ts        |  18 --
 src/codegen/bindgenv2/internal/dictionary.ts  |   7 -
 src/codegen/bindgenv2/internal/enumeration.ts |  15 +-
 src/codegen/bindgenv2/internal/interfaces.ts  |  20 +-
 src/codegen/bindgenv2/internal/optional.ts    |  36 +---
 src/codegen/bindgenv2/internal/primitives.ts  |  50 +----
 src/codegen/bindgenv2/internal/string.ts      |  20 +-
 src/codegen/bindgenv2/internal/union.ts       |  31 +--
 src/codegen/bindgenv2/script.ts               |  22 --
 src/codegen/class-definitions.ts              |   5 +
 src/collections/bit_set.rs                    |  18 --
 src/crash_handler/error.rs                    |   6 -
 src/css/crate_error.rs                        |   6 -
 src/dotenv/error.rs                           |   6 -
 src/exe_format/builtins.rs                    |  19 +-
 src/exe_format/error.rs                       |   6 -
 src/install/NetworkTask.rs                    |  12 +-
 src/io/error.rs                               |  17 --
 src/js_parser/error.rs                        |   6 -
 src/js_parser_jsc/error.rs                    |   6 -
 src/js_printer/error.rs                       |   6 -
 src/jsc/AbortSignal.rs                        |   5 -
 src/jsc/Strong.rs               
... (truncated)

gate history · 3 passed · 1 rejected · iteration 4

evidence per changed file
file                                           reads  edits  tests
src/ast/error.rs                                   0      0      0
src/brotli/error.rs                                0      0      0
src/bun_alloc/MaxHeapAllocator.rs                  0      0      0
src/bun_alloc/lib.rs                               0      0      0
src/bun_core/external_shared.rs                    0      0      0
src/bun_core/lib.rs                                0      0      0
src/bun_core/string/immutable.rs                   0      0      0
src/bun_core/string/wtf.rs                         0      0      0
src/bun_core/wtf.rs                                0      0      0
src/bundler/HTMLImportManifest.rs                  0      0      0
src/clap/error.rs                                  0      0      0
src/codegen/bindgenv2/internal/any.ts              0      0      0
src/codegen/bindgenv2/internal/array.ts            0      0      0
src/codegen/bindgenv2/internal/base.ts             1      0      0
src/codegen/bindgenv2/internal/dictionary.ts       0      0      0
src/codegen/bindgenv2/internal/enumeration.ts      0      0      0
(+ 57 more files)

…ters, unused error impls, dead C++ bindings

A relink of the debug build with --gc-sections --print-gc-sections lists
every function nothing in the final binary references. This removes the
ones that are dead on every platform:

- src/jsc/bindgen.rs: the Zig-port Bindgen adapters (BindgenStrongAny,
  BindgenNull, BindgenOptional, BindgenString, BindgenArray, ExternTaggedUnion2,
  ExternUnion2, ExternArrayList) and their only users Strong::adopt,
  bun_alloc::realloc_raw, and the WTFString alias with its
  ExternalSharedDescriptor impl.
- src/codegen/bindgenv2: the Zig type emitters (bindgenType, zigType,
  optionalZigType, CodeStyle, toZigNamespace). The generator only emits C++.
- Hand-written ErrName impls on 15 crate error types that nothing calls
  through the trait, the unused Display impls on bun_md::ParserError,
  bun_jsc::SystemError and HTMLImportManifest, Appender::append_lower_case
  and its impls, MaxHeapScope Deref impls, BitSet::set_intersection,
  Builtins::{from_executable,len,is_empty,modules}, AbortSignal::detach.
- jest.classes.ts: call: true on the seven noConstructor Expect classes only
  emitted an unused thunk and extern declaration.
- C++: the unused node:crypto key helpers keyFromString,
  passphraseFromBufferSource and the 4/6-argument parseKeyFormat/parseKeyType
  overloads, the WebCore::JSErrorHandler class, rejectPromiseWithGetterTypeError,
  toJSNewlyCreated(Ref<Blob>), two validateInteger instantiations, and the
  uWS run()/listen(port, cb) entry points with their uws_app_run/uws_app_listen
  C wrappers.
@robobun

robobun commented Aug 29, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:49 PM PT - Aug 29th, 2026

❌ @robobun, your commit 1379860 has 1 failures in Build #108448 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 40915

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

bun-40915 --bun

@coderabbitai

coderabbitai Bot commented Aug 29, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 18df7cc3-0726-4f7c-b650-acbb9e26a971

📥 Commits

Reviewing files that changed from the base of the PR and between eeba22a and 5a3d66d.

📒 Files selected for processing (1)
  • test/internal/class-definitions.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.


Walkthrough

The pull request removes obsolete public APIs and binding helpers across Rust, TypeScript, and C++ code. It narrows string appender contracts, removes legacy error-name and bindgen surfaces, updates JSC binding behavior, and marks selected Jest asymmetric matchers as non-callable.

Changes

API cleanup

Layer / File(s) Summary
Bindgen type API removal
src/codegen/bindgenv2/internal/*, src/codegen/bindgenv2/script.ts, src/codegen/class-definitions.ts
Removes CodeStyle, Bindgen and Zig type-generation methods, Zig namespace collision tracking, and invalid call/noConstructor combinations.
JSC binding cleanup
src/jsc/AbortSignal.rs, src/jsc/Strong.rs, src/jsc/SystemError.rs, src/jsc/bindings/*, src/jsc/lib.rs, src/jsc/generated.rs, src/jsc/virtual_machine_exports.rs, src/patch_jsc/testing.rs, src/sys_jsc/error_jsc.rs
Removes unused ownership, formatting, conversion, exception, validation, and host-export surfaces. JSC attribute getter handling now rejects unsupported promise behavior at compile time and returns undefined for non-throwing cast failures.
Core utility contract cleanup
src/bun_alloc/*, src/bun_core/*, src/bundler/HTMLImportManifest.rs, src/collections/bit_set.rs, src/exe_format/*, src/*/error.rs, src/ptr/lib.rs
Removes obsolete allocation, string, manifest, bit-set, builtins, shared-string, and error-name APIs.
Runtime and string helper cleanup
src/install/NetworkTask.rs, src/resolver/*, src/runtime/cli/pm_diff_command.rs, src/runtime/test_runner/*
Removes lowercase appender methods and marks seven asymmetric Jest matchers as non-callable.

Merge Risk: ⚪ Minimal · up to 5a3d6

This cleanup removes unused code without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the primary change: removing dead code across Rust, TypeScript code generation, and C++ bindings.
Description check ✅ Passed The description provides detailed problem, fix, background, verification, test results, and scope information. It uses different headings from the template, but it includes the required content.

Comment @coderabbitai help to get the list of available commands.

construct: false,
noConstructor: true,
call: true,
call: false,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In isolation this change looks unrelated and dangerous, and is actually a behavioural change and not removing dead code? What is this change?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hello @robobun

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hi. Full trace is in #40915 (comment). Short version: for a noConstructor: true class, call: true changes no behaviour. The only consumer of <Name>Class__call in generate-classes.ts is the JS<Name>Constructor class (Base(vm, structure, <Name>Class__call, construct)), and generateConstructorImpl is skipped when noConstructor is set (line 1623). So for these seven classes the flag emitted a Rust thunk and a C++ extern declaration that nothing referenced, which is how the linker flagged them.

Regenerating with both flag values and diffing shows only those 7 thunks, 7 declarations and 7 type-only .d.ts interfaces differ. expect.closeTo(...) still calls ExpectCloseTo::call directly (expect.rs:1300), the matcher objects were never callable, and new expect.any(...) threw before and after. Happy to instead teach the generator to ignore call under noConstructor and keep the flags as they were, if you prefer that shape.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Follow-up in 80b894e: define() in src/codegen/class-definitions.ts now throws on call: true with noConstructor: true, so the generator rejects the inert combination instead of silently emitting an unused thunk. The old jest.classes.ts fails that check; the current one passes and generates byte-identical output.

@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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I grepped for remaining callers/externs of the other deleted C++ symbols — JSErrorHandler, keyFromString/passphraseFromBufferSource, toJSNewlyCreated(Ref<Blob>), rejectPromiseWithGetterTypeError, and the two validateInteger JSValue-name instantiations — and found none; the uWS pair is the only removal that leaves a dangling Rust extern "C" declaration.

Extended reasoning...

The confirmed finding flags uws_app_run/uws_app_listen being deleted from libuwsockets.cpp while src/uws_sys/App.rs still declares and calls them. Since this is a gc-sections-driven sweep, the obvious question is whether the same dangling-reference class applies to any of the other C++ deletions. I checked each removed C++ symbol against the full src/ tree and none of JSErrorHandler, the CryptoUtil helpers, the Blob toJSNewlyCreated overloads, rejectPromiseWithGetterTypeError/CastedThisErrorBehavior::RejectPromise, or the dropped validateInteger explicit instantiations have surviving references, so the uWS case appears to be the only instance of that bug class in the diff. Also confirmed Bun__setSyntheticAllocationLimitForTesting is still reachable via dispatch_js2native.rs after the #[host_fn(export = ...)] attribute was dropped.

Comment thread src/uws_sys/libuwsockets.cpp
@robobun

robobun commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

Good question, and I should have spelled this out in the body. It is not a behaviour change. Here is what call does in src/codegen/generate-classes.ts and why it is inert for these seven classes.

call: true has three effects in the generator:

  1. ZigGeneratedClasses.cpp: extern JSC_DECLARE_HOST_FUNCTION(<Name>Class__call) (line 330).
  2. generated_classes.rs: a #[no_mangle] <Name>Class__call thunk that forwards to <Name>::call (line 1994).
  3. The JS<Name>Constructor class: its InternalFunction base gets <Name>Class__call as the call target, so Name(...) without new constructs an instance (lines 525 to 576).

Effect 3 is the only consumer of the symbol, and it lives in generateConstructorImpl, which runs only when noConstructor is false (line 1623). All seven of these classes have noConstructor: true, so no constructor class exists and nothing ever references <Name>Class__call. That is why the linker discarded the seven thunks (ExpectAnyClass__call, ExpectAnythingClass__call, ...): they have no caller in the final binary.

I regenerated the classes with both flag values and diffed the output. The only differences are the seven extern declarations, the seven Rust thunks, and seven type-only interface <Name>Constructor { (...args: any[]): any } blocks in the .d.ts. No C++ that executes changes.

The Rust ExpectCloseTo::call and friends stay. They are what expect.closeTo(...) et al. invoke directly (src/runtime/test_runner/expect.rs:1290-1315, through AsymmetricMatcherClass::invoke). Expect and ExpectTypeOf keep call: true because they do have constructors and their __call is wired.

Runtime check, same file against the release binary and the debug build with this change:

expect(1.005).toEqual(expect.closeTo(1, 2));
expect({ a: 1 }).toEqual({ a: expect.any(Number) });
const m = expect.closeTo(1, 2);
expect(typeof m).toBe("object");
expect(() => (m as any)()).toThrow();
expect(() => new (expect.any as any)(Number)).toThrow();

Both pass identically. The asymmetric matcher instances were never callable and expect.any was never constructible with new before this change either.

If you would rather keep the flags and have the generator skip the call thunk for noConstructor classes instead, I can do that too. I went with the flag because the generator does the right thing already; the flag was just wrong.

src/uws_sys/App.rs still declares uws_app_run and uws_app_listen. Removing
only the C++ definitions leaves dangling extern declarations.
Comment thread src/bun_alloc/MaxHeapAllocator.rs Outdated
Comment thread src/install/NetworkTask.rs Outdated
@robobun

robobun commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

4a96393 restores the uWS run() / listen(port, cb) entry points and their uws_app_run / uws_app_listen C wrappers. src/uws_sys/App.rs still declares them, and #40172 removes that Rust side, so this PR leaves the pair alone rather than duplicate those deletions. The PR body lists them as a follow-up. Nothing else changed: 60 files, +26 / -1208.

@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.

…, and ten inert host_fn shims

The remaining Bindgen trait and BindgenTrivial adapter had no users either.
The ErrName impls on the bun_brotli, bun_clap, bun_css, bun_router and
bun_uws_sys error types are never called through the trait. A bare
#[bun_jsc::host_fn] on a function that dispatch_js2native.rs calls directly
only emits an unused __jsc_host_* shim.

define() now rejects call: true together with noConstructor: true, since the
constructor class is the only consumer of the call target.
Comment thread src/jsc/generated.rs Outdated
Comment thread src/patch_jsc/testing.rs Outdated

@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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟡 src/jsc/generated.rs — nit: this doc comment still names BindgenOptional(BindgenTrivial<T>).ExternType, but commit 80b894e deletes the whole src/jsc/bindgen.rs module and scrubbed the three other such references in this file (lines 161, 181-182, 447); this one was missed and now points at types that no longer exist. Fix: reword to describe the extern layout directly, matching the updated comment on ExternArrayList a few lines below.

    Extended reasoning...

    The final commit in this PR removes src/jsc/bindgen.rs entirely (the module declaration is dropped from src/jsc/lib.rs:461) and, in the same file src/jsc/generated.rs, rewrites three comments that previously referenced bindgen.ExternArrayList, BindgenOptional(BindgenString).ExternType, and BindgenNull so they no longer name deleted types. Line 132's /// BindgenOptional(BindgenTrivial<T>).ExternType — ... was left untouched. On the base branch those types exist and the reference is accurate; after merging, a reader following the doc comment finds no BindgenOptional or BindgenTrivial anywhere in the tree. No behavioural impact — documentation only — but it is the one instance the sweep in this PR missed while explicitly fixing its siblings.

    Verification: nit — Commit 80b894e deletes src/jsc/bindgen.rs outright (module declaration removed from src/jsc/lib.rs:461; git show HEAD:src/jsc/bindgen.rs → "does not exist"). In the same commit, src/jsc/generated.rs rewrites the three other bindgen-type references: line 161 (bindgen.ExternArrayList(T) → "Extern layout of a C++ vector handed to Rust"), lines 181-182… | nit — The stale…

@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.

if (!thisObject) [[unlikely]] {
if constexpr (shouldThrow == CastedThisErrorBehavior::Throw)
return JSC::throwVMDOMAttributeGetterTypeError(&lexicalGlobalObject, throwScope, JSClass::info(), attributeName);
else if constexpr (shouldThrow == CastedThisErrorBehavior::RejectPromise)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can you replace this with a static_assert false or similar? if we ever add something that uses this it would silently now owrk.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done in eeba22a: get() now opens with static_assert(shouldThrow != CastedThisErrorBehavior::RejectPromise, ...), so any getter instantiated with that behavior fails the build instead of falling through to the ReturnEarly path. Builds clean with bun bd; the abort, message-event and url tests pass.

The RejectPromise branch is gone, so a getter instantiated with that
behavior would fall through to the ReturnEarly path and return undefined.
The static_assert turns that into a build error.

@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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread test/internal/class-definitions.test.ts Outdated

@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.

This pull request has been reviewed several times and this review found new issues. Where they share a root cause, one fix may close them together.

Comment thread src/jsc/lib.rs
The previous commit re-added them by accident.

@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.

alii pushed a commit that referenced this pull request Sep 8, 2026
…llections, and three JS builtins (#42069)

### Problem
- 25 dead-code PRs are already open, so a name grep finds little that is
unclaimed. This run used the compiler instead: demote each `pub` item to
`pub(crate)` and let rustc's `dead_code` lint say what nothing uses.
- What survived that check on all nine CI target triples, and is not
already in an open PR, is the list below.

### Fix
- `bun_runtime::Error`: remove 14 variants that nothing constructs
(`FmtError`, `ERR_TLS_CERT_ALTNAME_INVALID`, `ConnectionClosed`,
`MissingCredentials`, `InvalidMethod`, `InvalidEndpoint`,
`InvalidSessionToken`, `SignError`, `MissingPackageJSON`,
`MissingEntryPoint`, `lcovCoverageError`, `CompilationFailed`,
`JSErrorObject`, `Unsupported`), their `name()` arms, the two match arms
that tested for them (`cli/mod.rs`, `jsc_hooks.rs`), and a
`cfg(not(macos))` block inside a `cfg(macos)` function in
`webview/HostProcess.rs`.
- `bun_jsc`: remove `CrateError::JSErrorObject` (its only producer was
the arm above) and the three never-called `HotReloadTaskView` methods.
The trait stays as the type-erasure marker that `HotReloaderCtx::reload`
takes.
- `bun_collections`: `StringHashMap::values_mut` has no caller in any
crate.
- JS builtins: a 23-line commented-out `fileURLToPath` in `node/url.ts`
(unchanged since 2025-01), the unused `format` / `formatWithOptions`
getters in `internal/repl/node-inspect.js`, and the unused
`reportUncaughtException` export in `internal/shared.ts`.
- Verified: `cargo check --workspace` on all nine triples from
`rust:check-all`, `bun bd`, then `repl.test.ts`,
`readline.node.test.ts`, `hot.test.ts`, `watch.test.ts`, and the
`node/url` tests.

### Background
- The workspace denies `dead_code` and `unreachable_pub`, so rustc
already rejects unused private items. The blind spot is a `pub` item
that is reachable from its crate root but that no other crate imports.
Demoting it to `pub(crate)` puts it back under the lint.
- A demoted inherent method can silently lose to a trait method of the
same name in other crates (`Blob::finalize` against the blanket
`JsFinalize`, `__IsFreeze::IS_FREEZE` against `__NotFreeze`). rustc then
reports the inherent item as dead while behavior changed. Every method
hit was checked for a same-named path in other crates and those were
left alone.

<details><summary>Notes</summary>

- Scope: 87 Rust crates (everything under `src/` except the proc-macro
crates), `src/js`, the C++ `extern "C"` definitions in
`src/jsc/bindings`, `Cargo.toml` dependencies, and orphan `.rs` files
(via cargo dep-info). No unused dependencies and no orphan files were
found.
- Found dead but already removed by an open PR, so not repeated here:
the `bindgen.rs` marker structs and `ExportRenamer` (#40915, #40557),
the `uws_sys`/`mimalloc_sys`/`lsquic_sys` externs (#40172),
`has_termination_request`, `clear_exception`, `from_typed_array`,
`INTERNAL_MODULE_REGISTRY_FLAG`, and the 20 orphaned C++ `extern "C"`
functions (#40232).
- Probably dead, left out of the diff: 119 never-constructed `Feature`
variants in `src/css/prefixes.rs` (the file is generated by
`build-prefixes.js`), the never-constructed
`bake::Mode::ProductionDynamic`, and several struct fields rustc reports
as never read that exist to own an allocation (`JSTranspiler::arena`,
`CurrentBundle::{bv2, heap, ast_alloc_state}`).
- rustc's lint misreports inherent associated types
(`ThreadPool::Worker`, `EntryPoint::Kind`) as unused. They are used and
were kept.
- `bun_core::strings::split_once` (the multi-byte variant) has no
caller, but `clippy.toml` names it as the replacement for
`str::split_once` and `bstr`'s `split_once_str`, so it stays.

</details>

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.

3 participants