Skip to content

Remove dead code from the class and sink codegen, the streams bindings, and bun_runtime - #42266

Open
robobun wants to merge 3 commits into
mainfrom
robobun/0a756acf/dead-code-sweep
Open

robobun wants to merge 3 commits into
mainfrom
robobun/0a756acf/dead-code-sweep

Conversation

@robobun

@robobun robobun commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • src/codegen/generate-classes.ts emits a js<Class>Constructor custom getter for each of the 67 constructible generated classes. Nothing installs it on a prototype and nothing references it.
  • src/codegen/generate-jssink.ts has the same pattern: function<Sink>__getter for the 7 sinks, with no reference.
  • A few handwritten items have no users left: createStreamsBoundHandler (last caller removed in Bun.serve direct streams: flush on controller.close(), end the response once, call cancel() only on a client abort #41894), the empty POSIX signalHandler(int), two builtin names, two write-only struct fields, and some re-exports.

Fix

  • Remove both getter templates. ZigGeneratedClasses.cpp shrinks by 871 lines, JSSink.cpp and JSSink.h by 63.
  • Remove the six sink constructor accessors in ZigGlobalObject.h that only the sink getter called, and the other items listed in Notes. Each one has zero references in src/, packages/, the codegen scripts and the generated output.
  • Verified: bun bd builds and links. bun run rust:check-all is 12 of 12 ok. arraybuffersink.test.ts, streams.test.js, cpu-prof.test.ts and test/internal/source-lints/ pass. X.prototype.constructor === X holds for the generated classes, before and after.
  • No test file: the change only deletes unreferenced code, so no behavior changes, and test/internal/source-lints/CLAUDE.md asks for no tests for dead symbols.

Background

  • generate-classes.ts turns each *.classes.ts definition into a C++ wrapper class, a prototype class and a constructor class. The constructor property of a prototype comes from the constructor class, not from this getter.
  • generate-jssink.ts does the same for the JSSink stream sink classes.
  • No open dead-code PR removes any of these lines. Remove dead code from bun_core, the JSC bindings, the debugger, and 24 other crates #41385 edits lines next to the sink accessors in ZigGlobalObject.h, so one of the two needs a rebase after the other merges.
Notes

Removed symbols:

  • js${typeName}Constructor getter template (generate-classes.ts). A relink of the debug binary with --gc-sections --print-gc-sections drops all 67 instances.
  • function${name}__getter template (generate-jssink.ts). The same relink drops all 7.
  • Zig::GlobalObject::FileSink(), HTTPResponseSink(), HTTPSResponseSink(), NetworkSink(), FetchRequestBodySink(), HTMLRewriterSink(). ArrayBufferSink() stays, BunObject.cpp and BunStreamConsumers.cpp call it. The *Structure() accessors stay, JSSink.cpp calls them.
  • Bun::WebStreams::createStreamsBoundHandler (WebStreamsMisc.cpp, WebStreamsInternals.h).
  • Bun::signalHandler(int) in BunProcess.cpp. The body was empty on POSIX. The only reference, &signalHandler, is inside the Windows branch and takes the uv_signal_t* overload, which stays.
  • onClose and onDrain in BunBuiltinNames.h. No $onClose or $onDrain in src/js, no onClosePublicName, onClosePrivateName, onDrainPublicName or onDrainPrivateName in C++. bundle-functions.ts adds a private name on its own when a builtin starts to use one.
  • CPUProfilerConfig::interval. run_command.rs passes the interval to set_sampling_interval directly.
  • MultiPartUpload::global_this. Both constructors wrote it, nothing read it.
  • Re-exports with no user on Linux, Windows or macOS: ten names in runtime/image/mod.rs, mode_from_js and dirent::Dirent in runtime/node.rs.

Test counts: streams.test.js 549, arraybuffersink.test.ts 14, cpu-prof.test.ts 12, test/internal/source-lints/ 172.

Method: one pass that marks every pub item pub(crate) per crate and reads the dead_code output, then a second pass that tags each hit #[deprecated] and collects the use sites from cargo check --workspace on 9 targets. For C++, the --print-gc-sections list from a relink, then rg for each name.

Found but left alone:

…s, and bun_runtime

- generate-classes.ts: stop emitting the js<Class>Constructor custom getter.
  Nothing installs it on a prototype or references it. It was generated for
  all 67 constructible classes.
- generate-jssink.ts: stop emitting function<Sink>__getter for the 7 sinks.
  Nothing references it.
- ZigGlobalObject.h: remove the six sink constructor accessors that only the
  sink getter called. ArrayBufferSink() stays, BunObject.cpp and
  BunStreamConsumers.cpp call it.
- WebStreamsMisc.cpp: remove createStreamsBoundHandler. Its last caller
  went away in #41894.
- BunProcess.cpp: remove the empty POSIX signalHandler(int). Only the
  Windows overload is referenced.
- BunBuiltinNames.h: remove onClose and onDrain.
- CPUProfilerConfig::interval and MultiPartUpload::global_this: written at
  construction, never read.
- runtime/image/mod.rs, runtime/node.rs: trim re-exports that nothing names.
@robobun

robobun commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:34 PM PT - Sep 10th, 2026

✅ @robobun, your commit 552d3876fb16173cf496407d7faa856b11bb04fe passed in Build #114110! 🎉


🧪   To try this PR locally:

bunx bun-pr 42266

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

bun-42266 --bun

Comment thread src/runtime/webcore/s3/client.rs Resolved
@coderabbitai

coderabbitai Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: a56d78f8-847b-4add-adb8-8d828b8090a1

📥 Commits

Reviewing files that changed from the base of the PR and between f4f2a7c and 552d387.

📒 Files selected for processing (2)
  • src/runtime/webcore/s3/client.rs
  • src/runtime/webcore/s3/multipart.rs

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


Walkthrough

The changes remove obsolete generated getters and stream helpers, narrow runtime public exports, remove profiler configuration state, specialize signal handling for Windows, and remove global_this from multipart upload state.

Changes

Runtime binding and API cleanup

Layer / File(s) Summary
Generated constructor and sink cleanup
src/codegen/generate-classes.ts, src/codegen/generate-jssink.ts
Generated bindings no longer emit custom constructor or sink getter declarations and implementations.
Stream binding helper removal
src/jsc/bindings/ZigGlobalObject.h, src/jsc/bindings/webcore/streams/*
Sink constructor accessors and the shared createStreamsBoundHandler declaration and implementation were removed.
Public API and profiler surface cleanup
src/js/builtins/BunBuiltinNames.h, src/jsc/BunCPUProfiler.rs, src/runtime/cli/run_command.rs, src/runtime/image/mod.rs, src/runtime/node.rs
Unused builtin names, profiler interval state, and selected public re-exports were removed.
Windows signal handler specialization
src/jsc/bindings/BunProcess.cpp
signalHandler is now defined only for Windows and uses the libuv callback signature.
Multipart upload lifetime state removal
src/runtime/webcore/s3/client.rs, src/runtime/webcore/s3/multipart.rs
Multipart upload construction no longer stores or initializes global_this; the lifetime comment uses the direct borrow guarantee.

Suggested reviewers: dylan-conway, jarred-sumner, alii

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 552d3

The cleanup preserves the S3 upload lifetime guarantees and introduces no identified runtime or data-integrity risk.

🚥 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 from class and sink code generation, stream bindings, and bun_runtime.
Description check ✅ Passed The description explains the problem, lists the fixes, and provides detailed build, check, and test verification. It covers the template requirements despite using additional headings instead of the e…

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

@robobun

robobun commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

Status

  • Scope: dead code that no other open dead-code PR removes. I compared each deletion with the diffs of the 26 open sweep PRs, file by file.
  • How I found it: a per-crate pub to pub(crate) pass to read rustc's dead_code output, a #[deprecated] tag pass to collect use sites from cargo check --workspace on 9 targets, and the --print-gc-sections list from a relink of the debug binary for C++.
  • How I verified it: rg for each name across src/, packages/, src/codegen/ and build/debug/codegen/, then bun bd, bun run rust:check-all (12 of 12 ok), arraybuffersink.test.ts, streams.test.js, cpu-prof.test.ts and test/internal/source-lints/.
  • Review: the one review thread (a long comment in s3/client.rs) is fixed in 552d387 and resolved. CodeRabbit has no actionable comments.

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

LGTM — mechanical dead-code sweep, all removals check out.

What was reviewed:

  • Grepped for every removed symbol: the six sink constructor accessors, createStreamsBoundHandler, onClose/onDrain private names, and the removed Rust re-exports have zero remaining references in src/.
  • &signalHandler at BunProcess.cpp:1652 is inside the #else of #if !OS(WINDOWS), so the POSIX overload was genuinely dead.
  • Confirmed global_static in upload_stream is still consumed by S3UploadStreamWrapper.global (line 957), so the updated JSC_BORROW comment is accurate; writable_stream no longer needs one.
  • js${typeName}Constructor is not referenced by any HashTableValue emitter in generate-classes.ts — the prototype's constructor comes from LazyClassStructure/createConstructor.
Extended reasoning...

Overview

This PR is a targeted dead-code sweep across 13 files (+4/−79). It removes two unreferenced codegen templates (js${typeName}Constructor custom getter in generate-classes.ts, function${name}__getter in generate-jssink.ts), the six ZigGlobalObject sink-constructor accessors that only those getters called, an empty POSIX signalHandler(int) overload, the unused createStreamsBoundHandler helper, two builtin-name macros, and a handful of unread Rust struct fields and re-exports (CPUProfilerConfig::interval, MultiPartUpload::global_this, image/node module re-exports). No behavior is added; every hunk is a deletion or a struct-init-site adjustment for a deleted field.

Security risks

None. Nothing here touches auth, crypto, input parsing, or any user-reachable validation path. Deleting an unread GlobalRef field on MultiPartUpload cannot affect GC rooting because GlobalRef is a borrow of the process-lifetime global object, not a JS heap root, and the S3 wrapper still holds its own global field where needed.

Level of scrutiny

Low-to-moderate. The change is mechanical and the PR description documents a rigorous verification method (--gc-sections --print-gc-sections relink for C++, per-crate pub(crate) + #[deprecated] sweep across 9 Rust targets, bun bd link, rust:check-all 12/12). I independently grepped the tree for every removed symbol and confirmed zero remaining call sites; spot-checked that the sole &signalHandler reference is Windows-gated; confirmed the removed js<Type>Constructor getter is not wired into any prototype HashTableValue (constructor linkage goes through LazyClassStructure::setConstructor instead); and verified mode_from_js callers in node_fs.rs go through types::mode_from_js directly, not the removed top-level re-export.

Other factors

No CODEOWNERS entries cover the changed paths. The two bot inline threads at client.rs:902 were addressed by commit 552d387, which corrected the JSC_BORROW comment to name the actual storage site (S3UploadStreamWrapper) after the MultiPartUpload field was dropped — that fix is correct. REVIEW.md explicitly requires deleting dead code and naming the deletions in the description, which this PR does thoroughly. The change is small, self-contained, and compiles/links on all targets per the description; there is nothing here a human reviewer would need to weigh in on.

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