Skip to content

Remove dead code from the node:http2 frame parser, the shell interpreter, NodeHTTPResponse, and three JS binding objects - #42682

Merged
Jarred-Sumner merged 3 commits into
mainfrom
robobun/78fe12e3/dead-code-sweep
Sep 15, 2026
Merged

Jarred-Sumner merged 3 commits into
mainfrom
robobun/78fe12e3/dead-code-sweep

Conversation

@robobun

@robobun robobun commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • Some native members exist only for built-in JS to call, and no built-in JS calls them. The largest is H2FrameParser.setStreamPriority (src/runtime/api/bun/h2_frame_parser.rs). Http2Stream#priority() is a no-op since RFC 9113.
  • Three binding objects (node:vm, node:crypto, bun:sql) carry properties that their only JS consumer never reads.
  • packages/bun-usockets keeps a commented-out bsd_udp_packet_buffer_ecn from 2024-04. Nothing reads completions/spec.yaml, a 2021 appspec file.

Fix

  • Delete each item: 15 files, 535 lines removed. The Notes list every symbol.
  • Correct because every removed name has zero references in src/, packages/, scripts/, test/ and build/debug/codegen/ outside its own definition. No removed member is documented or typed API. User code reaches the h2 parser handle only through the undocumented Symbol.for("::bunhttp2native::") key.
  • The -D dead-code lint then reported three more items in h2_frame_parser.rs, and two Stream fields became write-only. They are removed too.
  • Verified: bun bd and bun run rust:check-all (12 targets) pass. The http2, shell, vm, crypto, node:http, sql, udp and streams test files pass (list in the Notes).

Background

Notes

Removed, one line each:

  • H2FrameParser.setStreamPriority / set_stream_priority (109 lines). No .setStreamPriority( in src/js or test/.
  • H2FrameParser.isStreamAborted / is_stream_aborted. Same check.
  • H2FrameParser.hasNativeRead / has_native_read. Same check.
  • FrameType::HTTP_FRAME_PRIORITY, ErrorCode::PROTOCOL_ERROR, SignalRef::is_aborted. Reported by the dead-code lint once the three functions above were gone. Both enums already list only the wire values that the file uses.
  • Stream::stream_dependency, Stream::exclusive. Their only reads were in set_stream_priority. The declarations, the initializers and the two stores in request() go. The locals that request() writes to the wire stay, and so does Stream::weight (read by getStreamState).
  • ShellInterpreter.isRunning / is_running and .started / get_started. src/js/builtins/shell.ts only calls interp.run().
  • Interpreter::started. The field was only read by get_started. The two stores and the AtomicBool import go with it.
  • NodeHTTPResponse.dumpRequestBody / dump_request_body. Added in node:http improvements #17093, never called from JS at any commit.
  • createNodeVMBinding: kUnlinked, kLinking, kEvaluating, kSourceText, kSynthetic. src/js/node/vm.ts destructures kLinked, kEvaluated, kErrored and never touches the binding object again.
  • createNodeCryptoBinding: SecretKeyObject, PublicKeyObject, PrivateKeyObject. Not destructured in src/js/node/crypto.ts. The classes stay, only the unread properties go.
  • bun_sql_jsc::mysql::create_binding: MySQLConnection. bun_sql_jsc::postgres::create_binding: PostgresSQLConnection. src/js/internal/sql/{mysql,postgres}.ts destructure createConnection, createQuery, init only.
  • packages/bun-usockets: the commented-out bsd_udp_packet_buffer_ecn (bsd.c), its commented-out wrapper us_udp_packet_buffer_ecn (udp.c), and the two commented-out declarations (libusockets.h, internal/networking/bsd.h). git blame: 589f941, 2024-04-26. The libusockets.h lines are context lines of a hunk in Remove dead code from bun-uws, bun-usockets, the require.extensions FFI, and unused Cargo dependencies #40294.
  • completions/spec.yaml. No hit for spec.yaml or appspec in the repo. It still lists the removed bun dev subcommand. The shell completions are hand-maintained and embedded with include_bytes!.

Tests run with the debug build: test/js/node/http2/ (7 files, node-http2.test.js 387 pass, h2-conformance.test.ts 70 pass), the four test-http2-*priority* Node tests, test/js/bun/shell/ (4 files), test/js/node/vm/vm.test.ts, crypto.key-objects.test.ts, five test/js/node/http/ files, five test/js/sql/ files, udp_socket.test.ts, streams.test.js.

How the candidates were found:

  • A repo-wide identifier index (definitions with zero other mentions, with and without comments).
  • A relink of the debug binary with --gc-sections --print-gc-sections. Almost every real hit from that pass is already deleted by one of the open pull requests. The rest were inlined functions, const fns used only at compile time, and Windows or macOS paths.
  • A per-class comparison of every *.classes.ts member against src/js, packages/bun-types and test/. The generated thunk for a proto entry is #[no_mangle] and #[allow(dead_code)], so neither rustc nor the linker can flag these members.
  • Checks for .rs files outside every mod tree, headers that nothing includes, C/C++ files outside the build, Cargo features that nothing enables, and long commented-out blocks. All clean apart from the usockets block.

Found, not deleted here:

  • TCPSocket/TLSSocket endBuffered ($end): no JS caller, but its removal leaves write_or_end_buffered::<IS_END> with one instantiation. That is a refactor, not a deletion.
  • *InternalReadableStreamSource.isClosed getter: no reader, but its removal leaves NewSource::is_closed write-only, and the stores are in files that Remove dead code from bun_bundler, bun_install, bun_runtime, bun_parsers, and the JSC private host functions #41088 touches.
  • NodeHTTPResponse.onwritable: no reader in src/js today, but node:http: finish a response when its body leaves, not when end() buffers it #41822 starts to use it.
  • The HEADERS+PRIORITY emitter and the native SignalRef abort path in H2FrameParser::request(). http2.ts warns with DEP0194 for the priority options and handles options.signal itself. Whether these native paths still run needs a separate look.
  • The IPV6_RECVTCLASS / IP_RECVTOS setsockopt calls in packages/bun-usockets/src/bsd.c ("used for getting the ECN"). Nothing reads the ECN now, but their removal changes socket options, so it is not a pure deletion.
  • macro(mockedFunction) and macro(writer) in src/js/builtins/BunBuiltinNames.h (the builtin-name entries, not the live mockedFunction string in BunCommonStrings.h): no mockedFunctionPrivateName or writerPrivateName user, but two open pull requests edit adjacent lines.
  • scripts/packer/build-image.pkr.hcl, scripts/lldb-inline.sh, scripts/lldb-inline-tool.cpp, scripts/github-metrics.ts, scripts/gamble.ts: nothing references them, but they are standalone tools that a person can run by hand.

Self-reviewed: 12 concerns raised, 10 addressed (the two Stream fields, the header comment lines, and the body corrections above). Rejected: a split into three pull requests, because every deletion is verified the same way and a split triples the CI and review rounds. Deferred: a caller lint for *.classes.ts proto entries, as a follow-up that does not gate this change.

…ter, NodeHTTPResponse, three JS binding objects, and bun-usockets

- H2FrameParser.setStreamPriority, isStreamAborted, hasNativeRead: no caller in src/js (Http2Stream#priority() is a no-op). The dead-code lint then flagged FrameType::HTTP_FRAME_PRIORITY, ErrorCode::PROTOCOL_ERROR and SignalRef::is_aborted.
- ShellInterpreter.isRunning and .started, plus the write-only Interpreter::started field.
- NodeHTTPResponse.dumpRequestBody: never called since it was added.
- vm binding: kUnlinked, kLinking, kEvaluating, kSourceText, kSynthetic are never read by node:vm.
- crypto binding: SecretKeyObject, PublicKeyObject, PrivateKeyObject are never read by node:crypto.
- SQL bindings: the MySQLConnection and PostgresSQLConnection constructors are never read by bun:sql.
- bun-usockets: the commented-out bsd_udp_packet_buffer_ecn and its wrapper (2024-04).
- completions/spec.yaml: a 2021 appspec file that nothing reads.
@robobun

robobun commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:48 AM PT - Sep 14th, 2026

✅ @robobun, your commit 3cc94c1f39db2cf8754f2c02c75443890c22e77a passed in Build #115533! 🎉


🧪   To try this PR locally:

bunx bun-pr 42682

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

bun-42682 --bun

@robobun

robobun commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: open, waiting for CI on 815c02e.

How each deletion was verified: the removed name has no reference outside its own definition in src/, packages/, scripts/, test/ and build/debug/codegen/. bun bd passes. bun run rust:check-all passes on all 12 targets. The test files that cover the touched areas pass (list in the PR body).

The second commit comes from a self-review: it removes two Stream fields that only the deleted set_stream_priority read, and the commented-out ECN declarations in the two usockets headers.

@coderabbitai

coderabbitai Bot commented Sep 14, 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: 78e226bf-4253-4bec-91f9-903052aa3cf0

📥 Commits

Reviewing files that changed from the base of the PR and between 09ba3b8 and 815c02e.

📒 Files selected for processing (3)
  • packages/bun-usockets/src/internal/networking/bsd.h
  • packages/bun-usockets/src/libusockets.h
  • src/runtime/api/bun/h2_frame_parser.rs
💤 Files with no reviewable changes (3)
  • packages/bun-usockets/src/libusockets.h
  • packages/bun-usockets/src/internal/networking/bsd.h
  • src/runtime/api/bun/h2_frame_parser.rs

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


Walkthrough

Changes

The pull request removes the Bun CLI completion specification, obsolete usockets code, selected binding exports and methods, HTTP/2 support, shell started-state tracking, HTTP response body dumping, and direct SQL constructor properties.

Runtime and interface cleanup

Layer / File(s) Summary
CLI completion specification removal
completions/spec.yaml
Deletes the complete Bun CLI completion specification and its command definitions.
Binding export reductions
src/jsc/bindings/NodeVM.cpp, src/jsc/bindings/node/crypto/node_crypto_binding.cpp, src/sql_jsc/mysql.rs, src/sql_jsc/postgres.rs
Removes selected Node VM status exports, crypto constructors, and direct SQL constructor properties.
HTTP/2 protocol support removal
src/runtime/api/bun/h2_frame_parser.rs, src/runtime/api/h2.classes.ts
Removes priority handling, stream abort inspection, native-read inspection, and related protocol entries.
Shell interpreter state removal
src/runtime/shell/interpreter.rs, src/runtime/api/Shell.classes.ts
Removes the started state and the isRunning and started methods.
HTTP response method removal
src/runtime/server/NodeHTTPResponse.rs, src/runtime/server/server.classes.ts
Removes request-body dumping implementation and its prototype method.
usockets commented-code removal
packages/bun-usockets/src/bsd.c, packages/bun-usockets/src/udp.c, packages/bun-usockets/src/internal/networking/bsd.h, packages/bun-usockets/src/libusockets.h
Deletes commented-out UDP ECN helpers and declarations.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 815c0

The changes remove obsolete interfaces and dead code without any supported active risk requiring action before merge.

🚥 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 describes the primary change: removal of unused code from the HTTP/2 parser, shell interpreter, NodeHTTPResponse, and JavaScript bindings. It does not mention usockets or completions…
Description check ✅ Passed The description explains the problem, lists the deleted items, documents the rationale, and provides detailed verification commands and test results. It uses different headings from the template, but …

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

@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 — pure deletions of unreferenced native bindings and stale files, no logic changes.

What was reviewed:

  • Grepped src/js/, src/, and test/ for every removed name (setStreamPriority, isStreamAborted, hasNativeRead, dumpRequestBody, isRunning/started on the shell interpreter, the five k* VM constants, the three *KeyObject binding properties, MySQLConnection/PostgresSQLConnection binding properties) — no remaining consumers; the SQL hits are $ZigGeneratedClasses type references, not reads of the removed binding property.
  • Confirmed src/js/node/crypto.ts and src/js/node/vm.ts destructure only the surviving properties, and both bindings are $cpp/$rust internal objects, not process.binding()-reachable.
  • Checked that enter_stream_dispatch, FrameType, and ErrorCode in the H2 parser still have live users after the removals; the PROTOCOL_ERROR hits left are comments only.
  • completions/spec.yaml has no references anywhere in the repo; the usockets deletions are commented-out code only.
Extended reasoning...

Overview

This PR deletes ~525 lines of dead code across 13 files with a single functional line added (an import narrowing). It removes: three unused H2FrameParser prototype methods and the enum variants/helper they alone consumed; two unused ShellInterpreter prototype methods plus the backing started: AtomicBool field and its two stores; the unused NodeHTTPResponse.dumpRequestBody method; five unread constants from the node:vm binding object; three unread constructor properties from the node:crypto binding object; two unread constructor properties from the bun:sql MySQL/Postgres binding objects; a stale completions/spec.yaml appspec file; and two commented-out ECN helpers in bun-usockets. Each .classes.ts entry is removed in lockstep with its Rust implementation.

Security risks

None identified. The crypto change removes three unread properties from an internal $cpp(...) binding object that is destructured once in src/js/node/crypto.ts and never otherwise reachable — the KeyObject classes themselves and their structures on ZigGlobalObject are untouched. The H2 parser deletions remove client-side priority-frame sending (deprecated by RFC 9113) and two unused query methods; inbound frame parsing and error handling are unchanged. No validation, TLS, auth, or bounds-checking code is weakened.

Level of scrutiny

Medium — the diff spans several subsystems, but every hunk is a pure deletion of a definition plus its codegen declaration, with no control-flow edits to surviving code. Per REVIEW.md's dead-code guidance ("public items escape dead-code lints — grep for callers manually"), I grepped each removed identifier across src/, src/js/, and test/ and found zero remaining consumers. The PR description's claim that every removed member sits behind a #private field, a local, or a symbol-keyed internal object (i.e., not user-reachable API) matches what I read in the consumers. The isRunning hits in src/js/node/test.ts are an unrelated class method, and the SQL MySQLConnection/PostgresSQLConnection hits are codegen type annotations, not reads of the removed binding property.

Other factors

No CODEOWNERS rule covers any changed path. The bug-hunting pass ran to dry_streak with zero findings and zero ruled-out candidates. enter_stream_dispatch and the FrameType/ErrorCode types retain multiple live users after the removals, so no new dead code is introduced. The one non-deletion line narrows an std::sync::atomic import after AtomicBool became unused. The PR description is unusually thorough and every spot-checked claim (destructuring sites, git-blame provenance of the usockets block, spec.yaml being unreferenced) held up.

… the leftover ECN declarations

- Stream::stream_dependency and Stream::exclusive were only read in the deleted set_stream_priority. The declarations, the initializers and the two stores in request() go. The locals that request() writes to the wire stay.
- The commented-out declarations of bsd_udp_packet_buffer_ecn and us_udp_packet_buffer_ecn in the two usockets headers go with the commented-out bodies.

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

@Jarred-Sumner
Jarred-Sumner merged commit a5fe51e into main Sep 15, 2026
11 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the robobun/78fe12e3/dead-code-sweep branch September 15, 2026 02:50
usrbinkat pushed a commit to usrbinkat/bun that referenced this pull request Sep 15, 2026
…ter, NodeHTTPResponse, and three JS binding objects (oven-sh#42682)

### Problem
- Some native members exist only for built-in JS to call, and no
built-in JS calls them. The largest is `H2FrameParser.setStreamPriority`
(`src/runtime/api/bun/h2_frame_parser.rs`). `Http2Stream#priority()` is
a no-op since RFC 9113.
- Three binding objects (`node:vm`, `node:crypto`, `bun:sql`) carry
properties that their only JS consumer never reads.
- `packages/bun-usockets` keeps a commented-out
`bsd_udp_packet_buffer_ecn` from 2024-04. Nothing reads
`completions/spec.yaml`, a 2021 appspec file.

### Fix
- Delete each item: 15 files, 535 lines removed. The Notes list every
symbol.
- Correct because every removed name has zero references in `src/`,
`packages/`, `scripts/`, `test/` and `build/debug/codegen/` outside its
own definition. No removed member is documented or typed API. User code
reaches the h2 parser handle only through the undocumented
`Symbol.for("::bunhttp2native::")` key.
- The `-D dead-code` lint then reported three more items in
`h2_frame_parser.rs`, and two `Stream` fields became write-only. They
are removed too.
- Verified: `bun bd` and `bun run rust:check-all` (12 targets) pass. The
http2, shell, vm, crypto, `node:http`, sql, udp and streams test files
pass (list in the Notes).

### Background
- A `*.classes.ts` file declares the methods of a native class. The
generator emits the C++ wrapper and the Rust glue from it. A `proto`
entry that no JS reads is dead, and so is its Rust function.
- A binding object is a plain object that native code fills and one
built-in module destructures once. A property that the module does not
destructure is never read.
- 28 other dead-code pull requests are open. This one deletes nothing
that they delete. oven-sh#40240 and oven-sh#37588 edit the body of
`set_stream_priority`. Each conflict resolves by taking the deletion.

<details><summary>Notes</summary>

Removed, one line each:

- `H2FrameParser.setStreamPriority` / `set_stream_priority` (109 lines).
No `.setStreamPriority(` in `src/js` or `test/`.
- `H2FrameParser.isStreamAborted` / `is_stream_aborted`. Same check.
- `H2FrameParser.hasNativeRead` / `has_native_read`. Same check.
- `FrameType::HTTP_FRAME_PRIORITY`, `ErrorCode::PROTOCOL_ERROR`,
`SignalRef::is_aborted`. Reported by the dead-code lint once the three
functions above were gone. Both enums already list only the wire values
that the file uses.
- `Stream::stream_dependency`, `Stream::exclusive`. Their only reads
were in `set_stream_priority`. The declarations, the initializers and
the two stores in `request()` go. The locals that `request()` writes to
the wire stay, and so does `Stream::weight` (read by `getStreamState`).
- `ShellInterpreter.isRunning` / `is_running` and `.started` /
`get_started`. `src/js/builtins/shell.ts` only calls `interp.run()`.
- `Interpreter::started`. The field was only read by `get_started`. The
two stores and the `AtomicBool` import go with it.
- `NodeHTTPResponse.dumpRequestBody` / `dump_request_body`. Added in
oven-sh#17093, never called from JS at any commit.
- `createNodeVMBinding`: `kUnlinked`, `kLinking`, `kEvaluating`,
`kSourceText`, `kSynthetic`. `src/js/node/vm.ts` destructures `kLinked`,
`kEvaluated`, `kErrored` and never touches the binding object again.
- `createNodeCryptoBinding`: `SecretKeyObject`, `PublicKeyObject`,
`PrivateKeyObject`. Not destructured in `src/js/node/crypto.ts`. The
classes stay, only the unread properties go.
- `bun_sql_jsc::mysql::create_binding`: `MySQLConnection`.
`bun_sql_jsc::postgres::create_binding`: `PostgresSQLConnection`.
`src/js/internal/sql/{mysql,postgres}.ts` destructure
`createConnection`, `createQuery`, `init` only.
- `packages/bun-usockets`: the commented-out `bsd_udp_packet_buffer_ecn`
(`bsd.c`), its commented-out wrapper `us_udp_packet_buffer_ecn`
(`udp.c`), and the two commented-out declarations (`libusockets.h`,
`internal/networking/bsd.h`). `git blame`: 589f941, 2024-04-26. The
`libusockets.h` lines are context lines of a hunk in oven-sh#40294.
- `completions/spec.yaml`. No hit for `spec.yaml` or `appspec` in the
repo. It still lists the removed `bun dev` subcommand. The shell
completions are hand-maintained and embedded with `include_bytes!`.

Tests run with the debug build: `test/js/node/http2/` (7 files,
`node-http2.test.js` 387 pass, `h2-conformance.test.ts` 70 pass), the
four `test-http2-*priority*` Node tests, `test/js/bun/shell/` (4 files),
`test/js/node/vm/vm.test.ts`, `crypto.key-objects.test.ts`, five
`test/js/node/http/` files, five `test/js/sql/` files,
`udp_socket.test.ts`, `streams.test.js`.

How the candidates were found:

- A repo-wide identifier index (definitions with zero other mentions,
with and without comments).
- A relink of the debug binary with `--gc-sections --print-gc-sections`.
Almost every real hit from that pass is already deleted by one of the
open pull requests. The rest were inlined functions, `const fn`s used
only at compile time, and Windows or macOS paths.
- A per-class comparison of every `*.classes.ts` member against
`src/js`, `packages/bun-types` and `test/`. The generated thunk for a
`proto` entry is `#[no_mangle]` and `#[allow(dead_code)]`, so neither
rustc nor the linker can flag these members.
- Checks for `.rs` files outside every `mod` tree, headers that nothing
includes, C/C++ files outside the build, Cargo features that nothing
enables, and long commented-out blocks. All clean apart from the
usockets block.

Found, not deleted here:

- `TCPSocket`/`TLSSocket` `endBuffered` (`$end`): no JS caller, but its
removal leaves `write_or_end_buffered::<IS_END>` with one instantiation.
That is a refactor, not a deletion.
- `*InternalReadableStreamSource.isClosed` getter: no reader, but its
removal leaves `NewSource::is_closed` write-only, and the stores are in
files that oven-sh#41088 touches.
- `NodeHTTPResponse.onwritable`: no reader in `src/js` today, but oven-sh#41822
starts to use it.
- The HEADERS+PRIORITY emitter and the native `SignalRef` abort path in
`H2FrameParser::request()`. `http2.ts` warns with DEP0194 for the
priority options and handles `options.signal` itself. Whether these
native paths still run needs a separate look.
- The `IPV6_RECVTCLASS` / `IP_RECVTOS` `setsockopt` calls in
`packages/bun-usockets/src/bsd.c` ("used for getting the ECN"). Nothing
reads the ECN now, but their removal changes socket options, so it is
not a pure deletion.
- `macro(mockedFunction)` and `macro(writer)` in
`src/js/builtins/BunBuiltinNames.h` (the builtin-name entries, not the
live `mockedFunction` string in `BunCommonStrings.h`): no
`mockedFunctionPrivateName` or `writerPrivateName` user, but two open
pull requests edit adjacent lines.
- `scripts/packer/build-image.pkr.hcl`, `scripts/lldb-inline.sh`,
`scripts/lldb-inline-tool.cpp`, `scripts/github-metrics.ts`,
`scripts/gamble.ts`: nothing references them, but they are standalone
tools that a person can run by hand.

Self-reviewed: 12 concerns raised, 10 addressed (the two `Stream`
fields, the header comment lines, and the body corrections above).
Rejected: a split into three pull requests, because every deletion is
verified the same way and a split triples the CI and review rounds.
Deferred: a caller lint for `*.classes.ts` `proto` entries, as a
follow-up that does not gate this change.

</details>
robobun added a commit that referenced this pull request Oct 6, 2026
Conflicts: src/runtime/api/bun/h2_frame_parser.rs

main removed is_stream_aborted (#42682) and moved the stream's native
AbortSignal listener to AbortHandle (#43933). This branch removes that
listener, so SignalRef, attach_signal and abort_stream_for_signal are
deleted with it.
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