Skip to content

Remove dead code from libuv_sys, cares_sys, simdutf FFI, test_runner, and C++ bindings - #37332

Open
robobun wants to merge 5 commits into
mainfrom
claude/farm/95390671/dead-code-ffi-sys-test-runner-cpp
Open

Remove dead code from libuv_sys, cares_sys, simdutf FFI, test_runner, and C++ bindings#37332
robobun wants to merge 5 commits into
mainfrom
claude/farm/95390671/dead-code-ffi-sys-test-runner-cpp

Conversation

@robobun

@robobun robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Scheduled dead-code sweep. Areas this run: src/libuv_sys, src/cares_sys, src/brotli_sys, src/simdutf_sys (both sides of the FFI), src/runtime/test_runner, a few bun_runtime leftovers, and C++ bindings outside the files the other open dead-code PRs touch. Net: 43 files, +218 (the new lint) / -1723.

Removed

bun_libuv_sys (Windows-only crate, -619)

  • 169 extern "C" declarations that no Rust code references: the tcp/udp/tty/poll/prepare/check/fs_poll/threading/dl/os-info/metrics surface of uv.h (uv_tcp_*, uv_udp_*, uv_tty_get_winsize, uv_prepare_*, uv_check_*, uv_fs_poll_*, uv_thread_*, uv_key_*, uv_once, uv_os_get_passwd, uv_os_environ, uv_queue_work, uv_getnameinfo, uv_random, uv_async_init, uv_dlopen, ...). The C symbols themselves stay exported for napi addons via symbols.def/symbols.dyn/linker*.lds and the force-link list in napi_body.rs; only bun's own unused Rust declarations go.
  • Structs and callback aliases only those declarations used: uv_getnameinfo_t, uv_random_t, uv_timespec64_t, uv_timeval64_t, uv_dir_t, uv_dirent_t, uv_env_item_t, uv_passwd_t, uv_group_t, uv_metrics_t, uv_key_t, uv_once_t, uv_thread_options_t, uv_lib_t, uv_getnameinfo_cb, uv_random_cb, uv_fs_poll_cb, uv_thread_cb, plus the uv_sem_t/uv_errno_t/uv_handle_s/uv_loop_s/uv_run_mode/uv_pipe_t/uv_fs_s/struct_uv_req_s/struct_uv_stream_s/uv_dirent_type_t/FILE/uv_thread_t/uv_loop_option/uv_membership/uv_tty_vtermstate_t/uv_clock_id aliases.
  • Wrappers nobody called: the UvReq marker trait and its 10 impls, Loop::dump_active_handles, Pipe::set_pending_instances_count, Pipe::as_stream_ptr, uv_async_t::init, uv_stat_t::birthtime.
  • Alias tables nothing read: StdioFlags (the three StdioFlags::INHERIT_FD uses in bun_spawn now spell UV_INHERIT_FD like the rest of that function), UV_FS_O_* (duplicates of the O module), UV_PRIORITY_*, UV_CLOCK_*, UV_LOOP_BLOCK_SIGNAL/UV_METRICS_IDLE_TIME, the udp/tty-mode/membership/copyfile flag groups, UV_MAXHOSTNAMESIZE, UV_IF_NAMESIZE, MAX_PIPENAME_LEN.

bun_cares_sys (-118): ares_library_init, ares_version, ares_init, ares_set_socket_functions, ares_send, ares_search, ares_gethostbyname, ares_getsock, ares_timeout, ares_create_query, ares_expand_name, ares_expand_string, ares_parse_uri_reply, ares_free_string, the ares_socket_functions and struct_ares_uri_reply structs, the ares_ssize_t/struct_timeval aliases, two commented-out ares_fds/ares_process declarations, and the Windows timeval/iovec definitions in lib.rs that existed only for them.

bun_brotli_sys: BrotliDecoder::{is_finished, get_error_code, version} and the BrotliDecoderIsFinished/BrotliDecoderVersion declarations (callers use BrotliDecoderGetErrorCode directly).

simdutf FFI (-230): 22 unused declarations in simdutf.rs and 30 wrappers in bun-simdutf.cpp (the 22 plus 8 that had already lost their Rust declaration), and their stubs in the parser bench shim. The lint asserts the two files now declare exactly the same set of simdutf__* names.

bun_runtime

  • test_runner::expect::JSValueTestExt: 17 forwarder methods (to_fmt, jest_deep_equals, values, keys, to_u32, ...) that bun_jsc::JSValue inherent methods of the same name shadow, so no call site ever resolved to them; rustc reports them unused once the trait is scoped to the crate. The four methods that do add behavior stay.
  • Never-read fields: CustomMatcherParamsFormatter::global_this (and its now-unused lifetime), SuccessfulReturnsFormatter::global_this, FetchOptions::global_this.
  • S3ErrorJsc::to_js (every caller uses the s3_error_to_js free fn).

C++ bindings (roughly -590)

  • Events_functionGetEventListeners / ListenerCount / Once / On in JSEventEmitter.cpp (declared in the header, never installed anywhere) and jsEventEmitterCast + JSEventEmitterWrapper, whose only callers they were. jsEventEmitterCastFast is untouched.
  • jsFunctionDebugNoop, jsFunctionSyncBuiltinExports (NodeModuleModule.cpp; the live export is jsFunctionSyncBuiltinESMExports).
  • jsCookieStaticFunctionSerialize (never attached to the constructor; only parse/from are) plus its helpers toCookieWrapped and Cookie::serialize(VM&, span<Ref<Cookie>>). The prototype serialize() is untouched.
  • Nine JSC_DECLARE_CUSTOM_GETTER(jsBakeResponsePrototypeGet*) declarations with no definition.
  • UTF8Encoding(), createMockResultStructure (the LazyProperty initializer inlines the same code), ActiveDOMObject::queueTaskToDispatchEvent and the queueTaskToDispatchEventInternal / isAllowedToRunScript / PendingActivity::object helpers that became unreferenced with it, StreamQueue::setTotalSize, WorkerMessagingProxy::{askedToTerminate, loaderContextIdentifier, workerThread} getters, 19 m_subspaceFor* / m_clientSubspaceFor* slots no class allocates from, the commonStringInitializer typedef, the BUN_FOREACH_CJS_NATIVE_MODULE macro, a DocumentLoadTiming forward declaration.
  • The five cross-realm (transferable stream) stubs in CrossRealmTransform.cpp / WebStreamsInternals.h, which only threw "not implemented" and had no callers, plus CrossRealmMessageType. WebStreamsInternals.h described these signatures as frozen placeholders from the streams port; nothing reaches them, so they are deleted here, but this is the one group in the PR that is a judgment call rather than a leftover, so it is easy to drop if you would rather keep the placeholders.
  • EventInterfaces.h / EventTargetInterfaces.h: checked-in copies of WebCore's generated enums listing every event and event-target interface in WebKit. Trimmed to the 5 + 9 entries bun's Event / EventTarget subclasses report; survivors keep their numeric values (Event::m_eventInterface is a 7-bit field, and EventFactory.cpp / EventTargetFactory.cpp both have default: arms).

Verification

  • Each symbol: rg across src/, scripts/, test/, packages/ and the regenerated build/debug/codegen/ output (plus substring searches for token-pasted names and vendor/WebKit/Source for anything extern "C").
  • Rust candidates came from scoping same-crate-only pub items to pub(crate) and intersecting rustc's dead_code output across all 8 target families, so platform-specific items (e.g. EmptyCopyFileState on darwin, Listener::NamedPipe on Windows) were excluded automatically; bun_libuv_sys was evaluated on the Windows targets since the module is cfg(windows).
  • cargo check --workspace on all 10 CI triples, cargo check --workspace --tests, cargo fmt --check, clippy on the touched crates, full bun bd, and bun bd test over events, cookies, node:module, TextEncoder/TextDecoder, expect.extend, toHaveReturnedWith, streams, fetch, zlib/brotli, mock, and test/internal/source-lints/ (the dns file's 30 network-dependent tests fail in the sandbox with ENOTFOUND both before and after; its 92 offline tests pass).
  • test/internal/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts pins 67 of the removed symbols (every entry was checked to match on main and not on this branch).

Notes for whoever merges

  • Propagate exception from _events getter in native EventEmitter #32268 adds RETURN_IF_EXCEPTION lines inside the Events_function* bodies deleted here (its actual fix is in jsEventEmitterCastFast, which this PR does not touch); whichever lands second needs a trivial rebase that drops those hunks.
  • Found dead but deliberately left alone: VM::has_termination_request, JSPromise::settle_task, job.rs on_js_thread / off_thread (all added two days ago in Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075, presumably for follow-ups); JSC__VM__hasTerminationRequest goes with them. Single unconstructed variants / unread payloads inside otherwise-live ABI or ported enums (SSRKind::Regular, DeclarationContext::Keyframes, StmtListWhich::AllStmts, OptionsData::Saved(usize), AsyncState::Done(ExitCode)) and the remaining partially-used libuv constant tables were also left as-is. rustc's "field is never read" hits on multi_array_columns! schema structs (JSMeta, BuilderEntry, LineOffsetTable, ServerComponentBoundary, the isolated-install Entry, BundledAst::tla_check) and on the type-punned SerializedSourceMapLoaded are live data, not dead code.

[review] gate passed · iteration 0 · 43 files touched

fails on main (without fix)
ASAN without fix: 6 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/internal/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts
bun test v1.4.0 (3f65303a0)

test/internal/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts:
59 |       ["src/libuv_sys/libuv.rs", /\bUV_FS_O_APPEND\b/],
60 |       ["src/libuv_sys/libuv.rs", /\bUV_PRIORITY_LOW\b/],
61 |       ["src/libuv_sys/libuv.rs", /\bUV_CLOCK_MONOTONIC\b/],
62 |       ["src/spawn/process.rs", /\bStdioFlags\b/],
63 |     ]),
64 |   ).toEqual([]);
         ^
error: expect(received).toEqual(expected)

- []
+ [
+   "src/libuv_sys/libuv.rs: \bfn uv_tcp_connect\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_udp_recv_start\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_tty_get_winsize\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_prepare_init\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_fs_poll_start\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_thread_create\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_os_get_passwd\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_queue_work\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_async_init\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_dlopen\b",
... (truncated)

release without fix: 6 FAILED
bun test v1.4.0-canary.1 (8db13c363)

test/internal/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts:
59 |       ["src/libuv_sys/libuv.rs", /\bUV_FS_O_APPEND\b/],
60 |       ["src/libuv_sys/libuv.rs", /\bUV_PRIORITY_LOW\b/],
61 |       ["src/libuv_sys/libuv.rs", /\bUV_CLOCK_MONOTONIC\b/],
62 |       ["src/spawn/process.rs", /\bStdioFlags\b/],
63 |     ]),
64 |   ).toEqual([]);
         ^
error: expect(received).toEqual(expected)

- []
+ [
+   "src/libuv_sys/libuv.rs: \bfn uv_tcp_connect\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_udp_recv_start\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_tty_get_winsize\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_prepare_init\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_fs_poll_start\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_thread_create\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_os_get_passwd\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_queue_work\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_async_init\b",
+   "src/libuv_sys/libuv.rs: \bfn uv_dlopen\b",
+   "src/libuv_sys/libuv.rs: \bstruct uv_passwd_t\b",
+   "src/libuv_sys/libuv.rs: \bstruct uv_getnameinfo_t\b",
+   "src/libuv_sys/libuv.rs: \bstruct uv_thread_options_t\b",
+   "src/libuv_sys/l
... (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/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts
bun test v1.4.0 (3f65303a0)

test/internal/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts:
(pass) dead libuv FFI declarations (windows-only crate) do not reappear [64.93ms]
(pass) dead c-ares and brotli FFI declarations do not reappear [19.79ms]
(pass) dead simdutf wrappers stay removed on both sides of the FFI [35.67ms]
(pass) dead bun_runtime helpers do not reappear [14.26ms]
(pass) dead C++ bindings do not reappear [28.17ms]
(pass) WebCore event interface enums stay trimmed to the interfaces bun implements [9.48ms]

 6 pass
 0 fail
 11 expect() calls
Ran 6 tests across 1 file. [2.71s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 883ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/126] gen NodeModuleModule.lut.h
Generating /workspace/bun/build/release/codegen/NodeModuleModule.lut.h from /workspace/bun/src/jsc/modules/NodeModuleModule.cpp
[2/126] gen generated_host_exports.rs
generated_host_exports.rs: 93 exports (host=3, lazy=10, generic=80, rust=0); 239 extern-C blocks audited
[3/126] gen cpp.rs (cppbind)
[3/126] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_simdutf_sys v0.0.0 (/workspace/bun/src/simdutf_sys)
�[1m�[92m   Compiling�[0m bun_libuv_sys v0.0.0 (/workspace/bun/src/libuv_sys)
�[1m�[92m   Compiling�[0m bun_brotli_sys v0.0.0 (/workspace/bun/src/brotli_sys)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/works
... (truncated)
diff hotspot
src/brotli_sys/brotli_c.rs                         |  14 -
 src/cares_sys/c_ares.rs                            | 110 +---
 src/cares_sys/lib.rs                               |  16 +-
 src/jsc/bindings/BunHttp2CommonStrings.h           |   2 -
 src/jsc/bindings/Cookie.cpp                        |  19 -
 src/jsc/bindings/Cookie.h                          |   2 -
 src/jsc/bindings/DOMFormData.h                     |   1 -
 src/jsc/bindings/JSBakeResponse.cpp                |  10 -
 src/jsc/bindings/JSMockFunction.cpp                |  23 -
 src/jsc/bindings/TextEncoding.cpp                  |   8 -
 src/jsc/bindings/TextEncoding.h                    |   2 -
 src/jsc/bindings/webcore/ActiveDOMObject.cpp       |  21 -
 src/jsc/bindings/webcore/ActiveDOMObject.h         |  13 -
 src/jsc/bindings/webcore/DOMClientIsoSubspaces.h   |  10 -
 src/jsc/bindings/webcore/DOMIsoSubspaces.h         |  11 -
 src/jsc/bindings/webcore/EventInterfaces.h         | 127 ----
 src/jsc/bindings/webcore/EventTargetInterfaces.h   | 104 ----
 src/jsc/bindings/webcore/JSCookie.cpp              |  36 --
 src/jsc/bindings/webcore/JSEventEmitter.cpp        |  91 ---
 src/jsc/bindings/webcore/JSEventEmitter.h          |   5 -
 src/jsc/bindings/webcore/JSEventEmitterCustom.cpp  |   9 -
 src/jsc/bindings/webcore/JSEventEmitterCustom.h    |  21 -
 src/jsc/bindings/webcore/Performance.h             |   1 -
 src/jsc/bindings/webcore/WorkerMessagingProxy.h    |   3 -
 .../webcore/streams/CrossRealmTransform.cpp        |  48 +-
 .../streams/JSWritableStreamDefaultController.cpp  |   2 +-
 src/jsc/bindings/webcore/streams/StreamQueue.h     |   1 -
 src/jsc/bindings/webcore/streams/StreamsForward.h  |   7 -
 .../bindings/webcore/streams/WebStreamsInternals.h |  15 -
 src/jsc/modules/NativeModuleList.h                 |   3 -
 src/jsc/modules/NodeModuleModule.cpp               |  16 -
 src/libuv_sys/libuv.rs                             | 641 +--------------------
 src/parsers/benches/support/s
... (truncated)

gate history · 4 passed · 0 rejected · iteration 0

evidence per changed file
file                                              reads  edits  tests
src/brotli_sys/brotli_c.rs                            1      2      0
src/cares_sys/c_ares.rs                               5     10      0
src/cares_sys/lib.rs                                  1      2      0
src/jsc/bindings/BunHttp2CommonStrings.h              0      0      0
src/jsc/bindings/Cookie.cpp                           0      0      0
src/jsc/bindings/Cookie.h                             0      0      0
src/jsc/bindings/DOMFormData.h                        0      0      0
src/jsc/bindings/JSBakeResponse.cpp                   0      0      0
src/jsc/bindings/JSMockFunction.cpp                   0      0      0
src/jsc/bindings/TextEncoding.cpp                     0      0      0
src/jsc/bindings/TextEncoding.h                       0      0      0
src/jsc/bindings/webcore/ActiveDOMObject.cpp          0      0      0
src/jsc/bindings/webcore/ActiveDOMObject.h            0      0      0
src/jsc/bindings/webcore/DOMClientIsoSubspaces.h      0      0      0
src/jsc/bindings/webcore/DOMIsoSubspaces.h            0      0      0
src/jsc/bindings/webcore/EventInterfaces.h            1      0      0
(+ 27 more files)

… and C++ bindings

Rust:
- bun_libuv_sys (Windows-only): 169 extern declarations no Rust code
  referenced (tcp/udp/tty/poll/prepare/check/fs_poll/threading/os-info
  surface), the structs and callback aliases only they used, the UvReq
  marker trait, Loop/Pipe/async helper methods nobody called, and the
  StdioFlags / UV_FS_O_* / priority / clock alias tables. bun_spawn now
  uses UV_INHERIT_FD directly like the rest of that file.
- bun_cares_sys: ares_init/ares_send/ares_search/ares_gethostbyname/
  ares_getsock/ares_timeout/ares_create_query/ares_expand_*/
  ares_parse_uri_reply/ares_free_string/ares_library_init/ares_version
  declarations, ares_socket_functions, struct_ares_uri_reply, and the
  Windows timeval/iovec definitions that existed only for them.
- bun_brotli_sys: BrotliDecoder::{is_finished,get_error_code,version}
  and the BrotliDecoderIsFinished/BrotliDecoderVersion declarations.
- bun_simdutf_sys: 22 unused extern declarations plus the 30 C++
  wrappers in bun-simdutf.cpp that no longer had a caller or a
  declaration (and their stubs in the parser bench shim).
- test_runner: JSValueTestExt forwarders that bun_jsc inherent methods
  of the same name shadow, so no call could resolve to them; never-read
  global_this fields on two formatter structs; FetchOptions.global_this;
  S3ErrorJsc::to_js.

C++:
- Events_function{GetEventListeners,ListenerCount,Once,On} host
  functions and the jsEventEmitterCast/JSEventEmitterWrapper helpers
  only they used; jsFunctionDebugNoop and jsFunctionSyncBuiltinExports in
  NodeModuleModule.cpp; jsCookieStaticFunctionSerialize and the
  Cookie::serialize(span) overload behind it; nine getter declarations in
  JSBakeResponse.cpp with no definition.
- UTF8Encoding(), createMockResultStructure, ActiveDOMObject::
  queueTaskToDispatchEvent (+ queueTaskToDispatchEventInternal,
  isAllowedToRunScript, PendingActivity::object), StreamQueue::
  setTotalSize, WorkerMessagingProxy getters, unused DOMIsoSubspaces /
  DOMClientIsoSubspaces slots, the commonStringInitializer typedef and
  the BUN_FOREACH_CJS_NATIVE_MODULE macro.
- The five cross-realm transform stubs (declared in WebStreamsInternals.h,
  each only threw "not implemented") and CrossRealmMessageType.
- EventInterfaces.h / EventTargetInterfaces.h trimmed to the interfaces
  bun actually reports; surviving enumerators keep their values.

Verified with rg across src/, scripts/, test/ and regenerated codegen
output, cargo check --workspace on all 10 CI targets, cargo check
--workspace --tests, and a full bun bd build. The new source lint pins
the removed symbols.
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The pull request removes unused FFI declarations, WebCore and JSC binding APIs, runtime fields, SIMDUTF wrappers, and Web Streams transfer stubs. It updates Windows process spawning and adds source-lint checks for removed symbols.

Changes

External-library FFI reduction

Layer / File(s) Summary
Brotli and c-ares declarations
src/brotli_sys/brotli_c.rs, src/cares_sys/*
Unused decoder, initialization, socket, query, timeout, and parsing bindings were removed. Active error and channel-processing APIs remain.

Windows libuv surface and spawn wiring

Layer / File(s) Summary
Windows libuv bindings
src/libuv_sys/libuv.rs, src/spawn/process.rs
Unused libuv types, constants, callbacks, and functions were removed. uv_guess_handle_raw supports the checked wrapper, and process spawning uses UV_INHERIT_FD.

WebCore and JSC binding cleanup

Layer / File(s) Summary
Binding APIs and interfaces
src/jsc/bindings/*, src/jsc/bindings/webcore/*, src/jsc/modules/*
Unused cookie, encoding, event, worker, DOM subspace, module, and host-function APIs were removed. Event interface enums retain only selected values.

Web Streams API reduction

Layer / File(s) Summary
Cross-realm transfer and queue APIs
src/jsc/bindings/webcore/streams/*
Unimplemented cross-realm transfer stubs and protocol declarations were removed. StreamQueue retains adjustTotalSize instead of setTotalSize.

Runtime and SIMDUTF cleanup

Layer / File(s) Summary
Runtime helpers and SIMDUTF wrappers
src/runtime/*, src/simdutf_sys/*, src/parsers/benches/support/simdutf_shim.cpp
Unused formatter state, fetch state, S3 conversion, SIMDUTF wrappers, and benchmark shims were removed.
Dead-symbol regression checks
test/internal/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts
Source-lint tests detect removed symbols, compare SIMDUTF wrapper parity, and verify retained enum values.

Possibly related PRs

Suggested reviewers: jarred-sumner, alii

🚥 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 main change: removing dead code across the listed FFI, runtime, and C++ binding areas.
Description check ✅ Passed The description explains the changes and provides detailed verification results, including the template's required purpose and testing information.

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

@robobun

robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Status (head 3f65303): ready for a maintainer.

  • Both automated reviews are done: the two CodeRabbit findings were incorrect and have been withdrawn; the four claude[bot] nits (bench shim orphans, libuv section headers, ActiveDOMObject includes, orphaned Win32 re-exports) are fixed in 8db13c3 and 3f65303, and its re-review of this head found nothing further. No open threads.
  • The only items that need a human call are the ones in the description: deleting the CrossRealmTransform placeholder stubs (easy to drop from the PR on request; nothing else depends on those hunks), and the rebase interaction with Propagate exception from _events getter in native EventEmitter #32268.
  • Verification: per-symbol rg, cargo check --workspace on all 10 CI triples, cargo check --workspace --tests, fmt, clippy on the touched crates, full bun bd, the test files listed in the description, and the new source lint (fails on main, passes here). The first CI run on the src/ content was green on every build lane including both Windows ones; build 91829 for this head is in progress.

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

I reviewed this PR and didn't find any bugs. Given the breadth (43 files, ~1700 lines across libuv_sys/cares_sys/simdutf/test_runner/C++ bindings) and the one deletion the description flags as a judgment call (the CrossRealmTransform "frozen placeholder" stubs in WebStreamsInternals.h), a human look would still be worthwhile before this lands.

What was reviewed:

  • Spot-checked that removed JSValueTestExt forwarders (to_fmt, jest_deep_equals, …) are shadowed by bun_jsc::JSValue inherents; toHaveReturnedWith.rs still resolves.
  • Confirmed SuccessfulReturnsFormatter's 'g lifetime is still bound via Formatter<'g> after the field removal, and FetchOptions::global_this is distinct from the FetchTasklet::global_this that is actually read.
  • Verified none of the removed m_subspaceFor* / m_clientSubspaceFor* slots or jsEventEmitterCast (non-Fast) are referenced anywhere in src/.
  • Checked the new source-lint's simdutf Rust↔C++ symbol-set assertion covers the mod base64 declarations and that its \bjsEventEmitterCast\b regex won't false-positive on jsEventEmitterCastFast.
Extended reasoning...

Overview

This is a scheduled dead-code sweep removing ~1700 lines across 43 files: unused extern "C" declarations and structs in the Windows-only bun_libuv_sys crate (~619 lines), bun_cares_sys, bun_brotli_sys, both sides of the simdutf FFI, unused JSValueTestExt trait forwarders in the test runner, never-read struct fields (CustomMatcherParamsFormatter::global_this, SuccessfulReturnsFormatter::global_this, FetchOptions::global_this), and a large set of C++ bindings (uninstalled Events_function* host functions, jsCookieStaticFunctionSerialize, ActiveDOMObject::queueTaskToDispatchEvent, unused iso-subspace slots, the CrossRealmTransform stubs, and trimming the WebCore EventInterfaces/EventTargetInterfaces enums to the entries Bun actually reports). A new source-lint test pins 67 of the removed symbols and asserts the simdutf Rust and C++ declaration sets stay in sync.

Security risks

None. This is pure deletion of unreferenced declarations, wrappers, and struct fields; no data flow, validation, or auth path changes. The only added code is a source-tree lint that reads files with readFileSync and matches regexes.

Level of scrutiny

Moderate-to-high. Each individual deletion is mechanical and the PR description documents an unusually thorough verification (rg across src/scripts/test/packages + regenerated codegen output, cargo check on all 10 CI triples including the Windows-only libuv crate, full bun bd, targeted test runs). I spot-checked several load-bearing claims and they held: to_fmt is a JSValue inherent so the trait forwarder was truly shadowed; the 'g lifetime on SuccessfulReturnsFormatter is still bound after the field removal; the removed FetchOptions::global_this is a different field from the heavily-used FetchTasklet::global_this; none of the removed iso-subspace slots or jsEventEmitterCast appear anywhere in src/; c_ushort is still used in c_ares.rs after c_long was dropped from the import.

However, the sheer breadth (Windows-only FFI that only Windows CI exercises, WebCore-ported enums, streams internals) and the fact that one deletion is explicitly flagged by the author as a judgment call rather than a provable leftover put this outside the "simple/mechanical" bar for auto-approval.

Other factors

  • The author explicitly calls out the CrossRealmTransform stub deletion as "the one group in the PR that is a judgment call rather than a leftover, so it is easy to drop if you would rather keep the placeholders" — that's a maintainer decision, not something an automated review should sign off on.
  • The description notes a known merge interaction with #32268 (which adds RETURN_IF_EXCEPTION inside the Events_function* bodies deleted here).
  • CI is described as "waiting"; the Windows lanes are the only place the ~619-line bun_libuv_sys deletion is compile-checked.
  • No prior human review on the timeline; CodeRabbit was rate-limited.

@robobun

robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the pass. On the two points worth tracking for a human reviewer:

  • The Windows compile coverage for the bun_libuv_sys deletion has now landed in CI: windows-x64-build-bun, windows-aarch64-build-bun and both build-cpp lanes are green in build 91615 (that crate's module is cfg(windows), so those lanes plus the local cargo check --target *-pc-windows-msvc runs are the checks that exercise it). Windows test lanes are still running.
  • The CrossRealmTransform stubs are the only deletion here that is a call rather than a leftover. If a maintainer would rather keep the placeholders, say so and I will drop the hunks in CrossRealmTransform.cpp, WebStreamsInternals.h, StreamsForward.h and the one comment in JSWritableStreamDefaultController.cpp; everything else in the PR is independent of them.

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 7:20 PM PT - Aug 10th, 2026

@robobun, your commit 3f65303a0ed52e176edc5107d6273d0d7bf44c14 passed in Build #91829! 🎉


🧪   To try this PR locally:

bunx bun-pr 37332

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

bun-37332 --bun

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/simdutf_sys/simdutf.rs (1)

36-40: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Protect the complete deleted SIMDUTF surface.

The source-lint deny-list in test/internal/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts does not include every symbol removed here. For example, it omits simdutf__convert_valid_utf8_to_utf16be and simdutf__convert_utf8_to_utf32.

If Rust and C++ reintroduce both declarations, the parity assertion passes and the lint does not detect the regression. Add every removed symbol to the deny-list, or generate the list from one manifest.

The shown source-lint expression and the removed declarations provide the supporting evidence.

Also applies to: 61-61, 71-71, 91-91, 101-101, 111-111, 121-121, 129-130

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/simdutf_sys/simdutf.rs` around lines 36 - 40, Expand the deny-list in
dead-symbols-ffi-sys-test-runner-cpp.test.ts to include every removed SIMDUTF
FFI symbol, including simdutf__convert_valid_utf8_to_utf16be and
simdutf__convert_utf8_to_utf32, so reintroduced Rust and C++ declarations are
detected. Prefer deriving the list from a shared manifest if the existing test
structure supports it, and preserve the parity assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/internal/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts`:
- Line 78: Update the dead-symbols test entry to match the actual `struct
ares_uri_reply` declaration, replacing the underscore-form pattern with a
pattern that accounts for the whitespace between `struct` and `ares_uri_reply`.
- Line 139: Update the dead-symbol test entry for
jsCookieStaticFunctionSerialize to reference src/jsc/bindings/Cookie.cpp instead
of JSCookie.cpp, ensuring the check reads the file containing the removed
declaration.

---

Outside diff comments:
In `@src/simdutf_sys/simdutf.rs`:
- Around line 36-40: Expand the deny-list in
dead-symbols-ffi-sys-test-runner-cpp.test.ts to include every removed SIMDUTF
FFI symbol, including simdutf__convert_valid_utf8_to_utf16be and
simdutf__convert_utf8_to_utf32, so reintroduced Rust and C++ declarations are
detected. Prefer deriving the list from a shared manifest if the existing test
structure supports it, and preserve the parity assertion.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 319eecd7-6ff3-4bf2-8257-cac7796f545f

📥 Commits

Reviewing files that changed from the base of the PR and between c63dc64 and 4e00184.

📒 Files selected for processing (43)
  • src/brotli_sys/brotli_c.rs
  • src/cares_sys/c_ares.rs
  • src/cares_sys/lib.rs
  • src/jsc/bindings/BunHttp2CommonStrings.h
  • src/jsc/bindings/Cookie.cpp
  • src/jsc/bindings/Cookie.h
  • src/jsc/bindings/DOMFormData.h
  • src/jsc/bindings/JSBakeResponse.cpp
  • src/jsc/bindings/JSMockFunction.cpp
  • src/jsc/bindings/TextEncoding.cpp
  • src/jsc/bindings/TextEncoding.h
  • src/jsc/bindings/webcore/ActiveDOMObject.cpp
  • src/jsc/bindings/webcore/ActiveDOMObject.h
  • src/jsc/bindings/webcore/DOMClientIsoSubspaces.h
  • src/jsc/bindings/webcore/DOMIsoSubspaces.h
  • src/jsc/bindings/webcore/EventInterfaces.h
  • src/jsc/bindings/webcore/EventTargetInterfaces.h
  • src/jsc/bindings/webcore/JSCookie.cpp
  • src/jsc/bindings/webcore/JSEventEmitter.cpp
  • src/jsc/bindings/webcore/JSEventEmitter.h
  • src/jsc/bindings/webcore/JSEventEmitterCustom.cpp
  • src/jsc/bindings/webcore/JSEventEmitterCustom.h
  • src/jsc/bindings/webcore/Performance.h
  • src/jsc/bindings/webcore/WorkerMessagingProxy.h
  • src/jsc/bindings/webcore/streams/CrossRealmTransform.cpp
  • src/jsc/bindings/webcore/streams/JSWritableStreamDefaultController.cpp
  • src/jsc/bindings/webcore/streams/StreamQueue.h
  • src/jsc/bindings/webcore/streams/StreamsForward.h
  • src/jsc/bindings/webcore/streams/WebStreamsInternals.h
  • src/jsc/modules/NativeModuleList.h
  • src/jsc/modules/NodeModuleModule.cpp
  • src/libuv_sys/libuv.rs
  • src/parsers/benches/support/simdutf_shim.cpp
  • src/runtime/test_runner/expect.rs
  • src/runtime/test_runner/expect/toHaveReturnedWith.rs
  • src/runtime/test_runner/mod.rs
  • src/runtime/webcore/fetch.rs
  • src/runtime/webcore/fetch/FetchTasklet.rs
  • src/runtime/webcore/s3/error_jsc.rs
  • src/simdutf_sys/bun-simdutf.cpp
  • src/simdutf_sys/simdutf.rs
  • src/spawn/process.rs
  • test/internal/source-lints/dead-symbols-ffi-sys-test-runner-cpp.test.ts
💤 Files with no reviewable changes (33)
  • src/jsc/bindings/TextEncoding.h
  • src/jsc/bindings/webcore/streams/StreamQueue.h
  • src/jsc/bindings/Cookie.cpp
  • src/jsc/bindings/DOMFormData.h
  • src/jsc/modules/NativeModuleList.h
  • src/jsc/bindings/JSBakeResponse.cpp
  • src/jsc/bindings/webcore/JSEventEmitterCustom.cpp
  • src/jsc/bindings/webcore/streams/StreamsForward.h
  • src/jsc/bindings/JSMockFunction.cpp
  • src/runtime/test_runner/expect/toHaveReturnedWith.rs
  • src/jsc/bindings/TextEncoding.cpp
  • src/jsc/bindings/webcore/WorkerMessagingProxy.h
  • src/jsc/bindings/webcore/JSEventEmitterCustom.h
  • src/runtime/webcore/fetch.rs
  • src/jsc/bindings/webcore/JSCookie.cpp
  • src/parsers/benches/support/simdutf_shim.cpp
  • src/jsc/bindings/Cookie.h
  • src/runtime/webcore/s3/error_jsc.rs
  • src/jsc/bindings/webcore/Performance.h
  • src/jsc/bindings/webcore/ActiveDOMObject.cpp
  • src/jsc/bindings/webcore/streams/WebStreamsInternals.h
  • src/jsc/bindings/BunHttp2CommonStrings.h
  • src/jsc/bindings/webcore/ActiveDOMObject.h
  • src/jsc/bindings/webcore/JSEventEmitter.cpp
  • src/jsc/bindings/webcore/EventTargetInterfaces.h
  • src/jsc/bindings/webcore/EventInterfaces.h
  • src/jsc/modules/NodeModuleModule.cpp
  • src/simdutf_sys/bun-simdutf.cpp
  • src/jsc/bindings/webcore/DOMIsoSubspaces.h
  • src/brotli_sys/brotli_c.rs
  • src/runtime/test_runner/mod.rs
  • src/jsc/bindings/webcore/DOMClientIsoSubspaces.h
  • src/jsc/bindings/webcore/JSEventEmitter.h

The parity assertion alone would pass if a removed wrapper came back on
both sides of the FFI at once, so list all 30 removed names instead of a
sample.
@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Review sweep after the CodeRabbit pass:

  • Pushed b9ffb6a: the simdutf lint now pins all 30 removed wrapper names instead of a sample, so a wrapper coming back on both sides of the FFI at once no longer slips past the parity assertion. Checked that every name matches main and none match this branch, and that none of the 32 surviving wrappers match the pattern. Test-only change; src/ is unchanged since the first build, which was green on every build lane including both Windows ones.
  • The two inline findings (struct_ares_uri_reply pattern, JSCookie.cpp path) were not bugs: the first is the literal Rust item name, the second is the file the removed host function lived in. Both threads are answered and resolved, and CodeRabbit has since withdrawn them.

No open threads; waiting on CI for the new head.

Comment thread src/parsers/benches/support/simdutf_shim.cpp Outdated
Comment thread src/libuv_sys/libuv.rs Outdated
Comment thread src/jsc/bindings/webcore/ActiveDOMObject.cpp
- simdutf bench shim: drop the three latin1 wrappers that never had a Rust
  declaration, and make the lint assert the shim only defines symbols
  simdutf.rs declares.
- libuv.rs: remove the now-empty "errors" and "udp" section headers and
  fold the three declarations left under "ip" into the misc group.
- ActiveDOMObject.cpp: drop the Event.h/EventTarget.h includes that only
  the removed queueTaskToDispatchEventInternal used.
@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 8db13c3 for the three review nits: the bench shim's three latin1 wrappers with no Rust declaration are removed (and the lint now checks the shim only defines symbols simdutf.rs declares), the empty errors / udp section headers in libuv.rs are gone with the three stragglers under ip folded into misc, and ActiveDOMObject.cpp no longer includes Event.h / EventTarget.h. Net for the PR against main is now 43 files, -1748 / +261 (the additions are almost entirely the new lint). Rebuilt locally; all review threads are resolved. Waiting on CI.

Comment thread src/libuv_sys/libuv.rs
… uv_lib_t and uv_dir_t

The size assertion for WIN32_FIND_DATAW only guarded its embedding in
uv_dir_t; the canonical one lives next to the type in bun_windows_sys.
@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 3f65303 for the last review nit: HMODULE and WIN32_FIND_DATAW are dropped from the bun_windows_sys re-export list in libuv.rs (they only existed for the removed uv_lib_t / uv_dir_t), along with the libuv-side WIN32_FIND_DATAW size assertion; the canonical one next to the type in bun_windows_sys stays. cargo check --workspace is clean on both Windows triples, so nothing consumed the re-exports. PR is now 43 files, -1751 / +263 against main, with no open review threads. Waiting on CI for this head.

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

I reviewed this PR again after 3f65303 and the bug-hunting pass found no issues; all four earlier nits (simdutf shim orphans, libuv section headers, ActiveDOMObject includes, HMODULE/WIN32_FIND_DATAW re-exports) are addressed and every thread is resolved. Given the breadth of the sweep across FFI crates and C++ bindings, the author-flagged judgment call on removing the CrossRealmTransform placeholder stubs, and the noted rebase interaction with #32268, a human sign-off is still worthwhile.

What was reviewed

  • Spot-checked removed FFI symbols (uv_*, ares_*, simdutf__*) against src/ and codegen for callers; the simdutf Rust↔C++ parity assertion in the new lint holds.
  • Confirmed EventInterfaces.h / EventTargetInterfaces.h survivors keep their numeric values and both factories have default: arms.
  • Verified jsCookieStaticFunctionSerialize / Events_function* were never installed on any HashTableValue and jsFunctionSyncBuiltinExports is distinct from the live jsFunctionSyncBuiltinESMExports.
  • Checked the StdioFlagsUV_INHERIT_FD rewrite in src/spawn/process.rs is value-preserving and covers all three call sites.
Extended reasoning...

Overview

Scheduled dead-code sweep removing ~1750 lines across 43 files: unused libuv/c-ares/brotli/simdutf FFI declarations (Rust extern blocks plus their C++ wrapper counterparts), shadowed JSValueTestExt forwarder methods and never-read struct fields in bun_runtime, and a batch of C++ bindings that were declared/defined but never installed (JSEventEmitter Events_function*, jsCookieStaticFunctionSerialize, jsFunctionDebugNoop/SyncBuiltinExports, createMockResultStructure, UTF8Encoding, ActiveDOMObject event-dispatch helpers, unused iso-subspace slots, the CrossRealmTransform throw-only stubs, and trimming the WebCore EventInterfaces/EventTargetInterfaces enums to the entries bun actually reports). One functional edit outside pure deletion: three StdioFlags::INHERIT_FD uses in src/spawn/process.rs become UV_INHERIT_FD (same value). A new source-lint test pins ~67 removed symbols and asserts simdutf Rust/C++/shim parity.

Security risks

None identified. Everything removed was uncallable (no Rust caller, not installed on any prototype/constructor, or throw-only stubs with no reachable path). No auth, crypto, permission, or input-validation surfaces are touched. The only value-bearing change (StdioFlags::INHERIT_FDUV_INHERIT_FD) is a rename between two constants both defined as 4.

Level of scrutiny

High, and it has received it: the PR went through two CodeRabbit findings (both withdrawn as incorrect), three earlier inline nits from this reviewer plus one follow-up (all fixed in 8db13c3 / 3f65303), and the author documented per-symbol rg verification across src/, scripts/, test/, packages/, and regenerated build/debug/codegen/, with cargo check --workspace on all 10 CI triples and a full bun bd. That said, this is a wide sweep across C++ JSC bindings and Windows-only FFI, and the author explicitly flagged the CrossRealmTransform stub removal as a judgment call ("easy to drop if you would rather keep the placeholders") — that is a maintainer decision, not something an automated review should make.

Other factors

The PR notes a rebase interaction with #32268 (whichever lands second drops the Events_function* hunks). The EventInterfaces enum trimming preserves numeric values for survivors and both factories have default: arms, so I did not find a hazard there, but it and the streams-placeholder removal are the two spots where a maintainer's confirmation is most useful. Deferring rather than approving on those grounds; no code changes are requested.

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