Remove dead code from the FFI shim layer, link_interface tables, jssink/jest codegen, and REPL shims - #37788
Remove dead code from the FFI shim layer, link_interface tables, jssink/jest codegen, and REPL shims#37788robobun wants to merge 3 commits into
Conversation
…codegen surfaces Delete the JSC__*/ZigString__*/WebCore__* C-ABI shims in bindings.cpp that no Rust code calls (and their headers.h declarations), declarations in headers.h for functions that no longer exist, link_interface! methods nothing dispatches through (JsEventLoop, ErrnoNames, TranspilerCacheImpl), Rust exports with no C++ caller (console no-op hooks, Bun__tickWhilePaused, Bun__internal_drainTimers, Bun__getVerboseFetchValue, Bun__Timer__getNextID, WTFStringImpl ref/deref, BlobArrayBuffer_deallocator, WebSocket writeBlob, BigInt f64/u64 ordering), a few unreferenced helpers on the global object and NapiEnv, the per-sink __getter that generate-jssink.ts emitted for every sink, the unused call: true on the asymmetric matcher classes, and two REPL shim exports nothing reads. Bun.deepEquals relied on the deleted JSC__JSValue__deepEquals shim as the implicit instantiation point of Bun__deepEquals<false, false, false, false>; it is now instantiated explicitly next to the existing one.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (44)
Comment |
|
Updated 6:05 AM PT - Aug 12th, 2026
❌ @robobun, your commit 24c6dd5 has some failures in 🧪 To try this PR locally: bunx bun-pr 37788That installs a local version of the PR into your bun-37788 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two inline nits, I spot-checked that the removed symbols are actually unreferenced across src/: the WebSocket writeBlob exports, the Bun__ConsoleObject__{profile,record,screenshot} hooks, the seven <Sink>() constructor accessors, performanceObject()/navigatorObject()/hasNapiFinalizers(), the JsEventLoop::{pipe_read_buffer,stdout,stderr} dispatch arms, and the REPL shim exports (completion.js only destructures ALL_PROPERTIES/SKIP_SYMBOLS and only calls BuiltinModule.getSchemeOnlyModuleNames()). Also confirmed the removed StringJsc::to_range_error_instance is distinct from the still-live ZigStringJsc method of the same name, and the explicit Bun__deepEquals<false,false,false,false> instantiation is present.
Extended reasoning...
This is a large dead-code removal across 44 files in the FFI shim layer, so I'm not approving on my own, but the spot-checks above all came back clean and the two inline findings are cosmetic.
…-inspect header comment
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Review follow-ups, both pushed:
Net diff is now +315 / -1211; the source lint still fails on |
There was a problem hiding this comment.
I reviewed this PR again after 522b44c / 24c6dd5 and didn't find any bugs — both earlier nits (the stale node-inspect.js header comment and the leftover <wtf/CrossThreadTask.h> include) are addressed, and the comment-cop items are resolved. Given the scope (~1,200 lines removed across 44 files in the core FFI shim layer — bindings.cpp, headers.h, ZigGlobalObject.{cpp,h}, plus link_interface! tables and two codegen surfaces), a human pass is still worthwhile.
What was reviewed:
- The one non-deletion change: the explicit
Bun__deepEquals<false,false,false,false>instantiation replaces the one the removedJSC__JSValue__deepEqualsbody used to provide — matches the callers inBunObject.cpp. jest.classes.tscall: trueremovals: confirmedgenerate-classes.tsonly wirescallinto a<Class>Class__callthunk on the constructor path, whichnoConstructor: truesuppresses;expect.any()etc. reachExpectAny::callviaexpect.rs.node-shims.jsONLY_ENUMERABLE/SKIP_STRINGS— checkedcompletion.jsonly destructuresALL_PROPERTIES/SKIP_SYMBOLS.timer_allimport removal inTimer.rs—timer_all_mutis still imported and used.
Extended reasoning...
Overview
This PR removes ~1,200 lines of dead code from the Rust↔C++ FFI shim layer (bindings.cpp, headers.h, headers-handwritten.h, helpers.h, BunString.cpp, JSBigIntBinding.cpp, ZigGlobalObject.{cpp,h}, napi.h, ScriptExecutionContext.h), the bun_dispatch::link_interface! tables in bun_core/bun_event_loop/bun_ast and their impl arms, several #[no_mangle] Rust exports (ConsoleObject noop hooks, Bun__tickWhilePaused, Bun__Timer__getNextID, BlobArrayBuffer_deallocator, WebSocket write_blob, etc.), two codegen surfaces (generate-jssink.ts per-sink __getter, jest.classes.ts call: true on 7 noConstructor matchers), and REPL shim exports. The additions are: one explicit template instantiation in bindings.cpp, comment/attribute adjustments, a #[cfg(test)] gate on SystemErrno::MAX for Windows, and a new source-lint test that pins every removed symbol.
The candidates were found by relinking with --gc-sections --print-gc-sections, then filtered against textual references across all source trees (so platform-gated and debugger-only code was left alone) and cross-checked at the symbol level against open PRs. The PR description lists what was intentionally left (URL::from_js, remaining simdutf__*, JSC__VM__notifyNeedDebuggerBreak, etc.) with the specific in-flight PR each conflicts with.
Security risks
None. This is purely subtractive against code the linker already discards; no new input handling, no auth/crypto/permission surfaces touched.
Level of scrutiny
High — not because the individual deletions are risky (each removed function was already dead in the shipped binary), but because of the breadth: 44 files across the core Rust/C++ binding boundary, including bindings.cpp (−609 lines), headers.h (−88 lines), and ZigGlobalObject.{cpp,h}. The one subtle correctness dependency is the explicit Bun__deepEquals<false,false,false,false> instantiation, which the author added after the first build broke — that's now in place next to the existing <true,false,false,true> instantiation. The link_interface! removals require knowing that no dispatch site names those methods, which is easy to get wrong for macro-generated tables; the source-lint test guards against re-addition but not against a missed live caller. A maintainer spot-checking the --print-gc-sections methodology and the open-PR coordination list would be valuable.
Other factors
- My two earlier inline nits (stale
node-inspect.jscomment, leftoverCrossThreadTask.hinclude) were addressed in 522b44c; the comment-cop flags were addressed in 24c6dd5 with per-thread replies. - Verification listed in the PR description covers full
bun bd,rust:check-allon all 10 targets,cargo test -p bun_errno, and the relevant test files for each touched subsystem. - The new source-lint test is well-scoped (reads source files, per-symbol regex checks, all 7 tests fail on
mainper the evidence block). - The
jest.classes.tschange touches the test-runner codegen input; I verified ingenerate-classes.tsthatcall: trueundernoConstructor: trueonly feeds the constructor's call slot, which is never emitted.
|
CI status for 24c6dd5 (build 93013): 193 of 194 jobs passed; the only red job is one of the two The expiry is a capacity problem on the |
2 participants
Problem
link_interface!tables, a few#[no_mangle]Rust exports, two codegen surfaces, and the REPL shims.headers.hand the C++ sources declare functions that no longer exist anywhere, such as the 13Reader__*__fastpath/FFI__ptr__fastpathDOMJIT entry points.Fix
generate-jssink.tsstops emitting a per-sink getter that was installed for no sink, and the 7noConstructorjest matcher classes dropcall: true.--gc-sections --print-gc-sections) and textually unreferenced acrosssrc/,packages/,scripts/and regenerated codegen. Anything referenced only from another platform, a debugger entry point, or an open PR was left alone (listed under "Left alone on purpose" in the original).Bun.deepEquals(a, b)was getting itsBun__deepEquals<false, false, false, false>instantiation from a deleted shim body, so it gets an explicit one (the first build caught this); the WindowsSystemErrno::MAXbecomes#[cfg(test)]because its last non-test reader went away (rust:check-allon the Windows targets caught this).mainand pass here. The full debug build,rust:check-allon all 10 targets, and the runtime tests over the touched areas pass. Two tests timed out locally on a heavily loaded box; both are in paths this diff only removes unreferenced exports from, so CI is expected to pass them.Background
extern "C"shims inbindings.cpp, declared for Rust inheaders.h(the[[ZIG_EXPORT]]ones also get generatedcppbindwrappers); C++ calls Rust through#[no_mangle]exports. Neither compiler can tell when the other side stopped calling something; only the linker can.bun_dispatch::link_interface!: declares a table of methods a lower-level crate can call on a higher-level crate it cannot depend on directly; alink_impl_*!block in the higher crate supplies the arms. Removing a method means removing the table entry and its arm together.--gc-sections: with each function in its own section, the linker drops every section nothing references, and--print-gc-sectionslists what it dropped. That list is what "not in the shipped binary" means above.generate-jssink.tsemits the C++ wrapper class for each of the 8 sink types (ArrayBufferSinkand friends);jest.classes.tsis the input to the class-binding generator, wherecall: trueemits a<Class>Class__callthunk for the class's constructor function.template ...;line or the link fails.[review] gate passed · iteration 0 · 44 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file
Original description
Removes 1,208 lines that nothing references (55 lines added in src, all comment/attribute adjustments plus one explicit template instantiation; the rest of the additions are the source lint), across the Rust/C++ FFI shim layer, the
bun_dispatch::link_interface!tables, a handful of#[no_mangle]Rust exports, two codegen surfaces, and the REPL shims. No behavior change: every removed function was discarded by the linker, i.e. the shipped binary never contained it.How the candidates were found
The debug binary was relinked with
-Wl,--gc-sections -Wl,--print-gc-sections; the discarded.text.*sections from bun's own objects (C++ andlibbun_rust.a) gave the set of functions nothing references at link time. That set was then filtered bysrc/,packages/,scripts/and the regeneratedbuild/debug/codegen/output, so anything still referenced from code that only compiles on another platform (Bun__Process__hasTitle,archive_entry_set_pathname_utf8,bun_sysconf__SC_CLK_TCK, the kqueue no-orphans hooks, ...) or reached only from a debugger entry point (dumpBtjsTraceand everything under it) was left alone;StringJsc::to_range_error_instance->JSC__createRangeError,FetchHeaders::from->WebCore__FetchHeaders__createValue,Bun__tickWhilePaused->EventLoop::tick_while_paused);JSC__VM__notifyNeedDebuggerBreak, which Start the inspector at runtime on SIGUSR1 / process._debugProcess #37336 adds a caller for).Additionally every declaration in
headers.hand everyextern "C"declaration in the C++ sources was checked against the symbols the binary actually defines, which found the declarations for functions that no longer exist anywhere.Removed
bindings.cpp/headers.h(about 680 lines)68 C-ABI shims that no Rust code declares or calls any more (the Rust
JSValue/JSPromise/VMtypes implement these inline or never needed them), together with theirheaders.hdeclarations and thecppbindwrappers generated for the[[ZIG_EXPORT]]ones:JSC__JSValue__{isCell, isNull, isUndefined, isUndefinedOrNull, isNumber, isObject, isInt32, isInt32AsAnyInt, isError, isGetterSetter, isCustomGetterSetter, eqlCell, deepEquals, jsNumberFromChar, jsNumberFromU16, jsNumberFromInt32, jsNumberFromInt64, jsNumberFromUint64, jsTDZValue, jsType, createInternalPromise, createRangeError, createTypeError, fastGetDirect_, getPropertyValue, putRecord, symbolKeyFor}JSC__JSInternalPromise__{create, isHandled, reject, rejectAsHandled, rejectAsHandledException, rejectedPromise, resolve, result, setHandled, status}(the type is aliased toJSPromise; onlyresolvedPromiseis used),JSC__JSPromise__{asValue, isHandled, resolveOnNextTick, rejectOnNextTickWithHandled}JSC__JSObject__{getArrayLength, getDirect, putDirect},JSC__JSCell__{getObject, toObject},JSC__JSString__toObject,JSC__JSFunction__optimizeSoon,JSC__JSGlobalObject__{getCachedObject, putCachedObject},JSC__JSMap__has,JSC__JSModuleLoader__evaluate,JSC__createRangeErrorJSC__VM__{clearExecutionTimeLimit, setExecutionTimeLimit, deleteAllCode, isEntered, isJITEnabled, notifyNeedShellTimeoutCheck, notifyNeedWatchdogCheck, performOpportunisticallyScheduledTasks}ZigString__{to16BitValue, toAtomicValue, toExternalValueWithCallback},WebCore__DOMURL__{href_, pathname_},WebCore__FetchHeaders__createValue(+FetchHeaders::fromin Rust),Bun__CallFrame__isFromBunMain, andZig::toJSStringinhelpers.h, which only the twocreate*Errorshims usedReader__*__fastpath/FFI__ptr__fastpathDOMJIT entry points (only the slow paths exist),JSC__JSGlobalObject__createSyntheticModule_,JSC__JSValue__{then, createStringArray, hasOwnProperty, toString},JSC__VM__create,ZigException__fromException,Zig__GlobalObject__{fetch, promiseRejectionTracker}, a stray_fromJSBun.deepEquals(a, b)was getting itsBun__deepEquals<false, false, false, false>instantiation from the deletedJSC__JSValue__deepEqualsbody, so it now has an explicit instantiation next to the existing one for<true, false, false, true>(this is what the first build of this branch caught).link_interface!methods nothing dispatches throughJsEventLoop::{file_polls, put_file_poll, pipe_read_buffer, stdout, stderr}(bun_event_loop) and theirJscarms inbun_jsc::event_loop, plusEventLoop::pipe_read_buffer, which only the arm called. The livepipe_read_bufferisbun_io'sEventLoopCtx, a different interface.ErrnoNames::{max_dense, win32_name}(bun_core) and theirSysarms,win32_errno_nameandsystem_errno_max_denseinbun_errno; the WindowsSystemErrno::MAXthat onlymax_denseread becomes#[cfg(test)](the exhaustive round-trip test still iterates it, and the Win32 mapping test now callsinit_win32_errordirectly).TranspilerCacheImpl::is_disabled(bun_ast) and itsJscarm;RuntimeTranspilerStorecalls the inherentis_disabled()directly.Rust exports with no C++ caller
Bun__ConsoleObject__{profile, profileEnd, record, recordEnd, screenshot}and theconsole_noop_hooks!macro;ConsoleObject.cpponly forwardstimeStamp, which is now a plain functionBun__tickWhilePaused+EventLoop::tick_while_paused(+ two declarations inBunDebugger.cpp;runWhilePausedblocks on a condition variable now),Bun__internal_drainTimers,Bun__getVerboseFetchValue(declared inJSEnvironmentVariableMap.cpp, never called),Bun__Timer__getNextIDBun__WTFStringImpl__{ref, deref}on both sides (the Rust comment already said "Rust no longer calls these"; ref/deref are inlined inbun_alloc, onlydestroycrosses FFI),BlobArrayBuffer_deallocatorinblob/Store.rs(Blob.rshas the liveblob_store_array_buffer_deallocator),WebSocket::write_bloband its two__writeBlobexports (WebSocket.cppsends blobs throughwriteBinaryData),Bun__linux_trace_closeJSC__JSBigInt__{orderDouble, orderUint64}and theBigIntOrderableimpls forf64/u64; onlyi64comparisons are performedexport = "Bun__NodeUtil__jsParseArgs"/"PostgresSQLConnection__createInstance"symbol names (both functions are reached through Rust tables; the C++BUN_DECLARE_HOST_FUNCTIONfor the former is gone too)C++ helpers
ZigGlobalObject:JSDOMFileConstructor_getter/_setter,functionLazyNavigatorGetter+navigatorObject(),GlobalObject_getPerformanceObject+performanceObject()(navigator/performanceare installed from the LUT directly),hasNapiFinalizers()+NapiEnv::hasFinalizers(),NapiEnv::isVMTerminating(),ScriptExecutionContext::postCrossThreadTask, theNodeVM*ModulePrototype()accessors<Sink>()constructor accessors whose only reader was the generated getter below (ArrayBufferSink()stays;BunObject.cppuses it)Codegen
generate-jssink.tsno longer emitsfunction<Sink>__getter, which was declared and defined for all 8 sinks and installed for none of themjest.classes.ts:call: trueon the 7noConstructorasymmetric matcher classes only produced a<Class>Class__callthunk that the (non-existent) constructor would have used;expect.any()etc. reachExpectAny::callthroughexpect.rsdirectlyBuilt-in JS
internal/repl/node-inspect.js: theformat/formatWithOptionsgetters;internal/repl/node-shims.js:BuiltinModule.{exists, canBeRequiredByUsers, canBeRequiredWithoutScheme}and twoconstantsentries (completion.js is the only consumer and destructures neither)test/internal/source-lints/dead-symbols-ffi-shims-dispatch.test.tspins every removed symbol; all 7 tests fail onmainand pass here (each regex was also checked individually to match onmain).Verification
bun bd(full debug build) passes;bun run rust:check-allpasses on all 10 target triples (the first run failed on the two Windows targets becauseSystemErrno::MAXbecame unused there, which is the#[cfg(test)]change above);cargo test -p bun_errno; rustfmt, clang-format, prettier and oxlint are clean.bun bd testpasses ontest/js/bun/test/expect.test.js,test/js/bun/bun-object/deep-equals.test.ts,test/js/bun/globals.test.js,test/js/web/web-globals.test.js,test/cli/run/transpiler-cache.test.ts,test/js/bun/repl/repl.test.ts,test/js/web/websocket/{websocket-blob,websocket-client}.test.ts,test/js/bun/util/{arraybuffersink,filesink}.test.ts,test/js/node/readline/readline.node.test.ts,test/js/node/fs/fs-stats-constructor.test.ts,test/js/web/timers/setImmediate.test.js,test/js/bun/console/console-write.test.ts, andtest/internal/source-lints/. Two tests timed out locally (setInterval doesn't leak memoryat 30s and the parseArgs100 mixed several timesstress test at 5s); the container was running at a load average of ~55 on 16 cores and the release binary runs both in well under a second, and neither path is touched functionally here (the timer change removes two unreferenced exports, the parseArgs change removes an unused symbol name), so I expect CI to disagree with my machine rather than with the diff.return 0;), and none adds a reference to them.Left alone on purpose
JSC__VM__{hasTerminationRequest, isTerminationException}+VM::has_termination_requestandJSC__VM__notifyNeedDebuggerBreakare unreferenced but sit in the middle of One termination signal: Err(Thrown) always means an exception is pending; the loop reads the gate, host code never does #37275 / get a caller in Start the inspector at runtime on SIGUSR1 / process._debugProcess #37336.URL::from_js/URL__fromJShas no callers, but Make jsc::URL a re-export of bun_url with a UrlJsc extension trait #37359 rewritesURL.rs;jsFunctionCreateFunctionThatMasqueradesAsUndefinedis unreferenced but adjacent to node:path: rewrite on JSString storage in Rust, match Node.js v26 exactly, ~3x faster #37305's hunks, andjsFunctionNotImplementedgets a caller in Clear pending exceptions from lazy property getters during property enumeration #37213.simdutf__*wrappers (the*_with_errors,convert_valid_*andutf32_length_*family) anduws_{app_listen, app_run, h3_req_get_parameter, loop_defer, res_clear_corked_socket, ws_iterate_topics}are interleaved with the hunks of Remove dead code from libuv_sys, cares_sys, simdutf FFI, test_runner, and C++ bindings #37332 / Remove dead code from uws_sys, webcore bindings, crash_handler, and scripts #37181 in the same extern blocks; they can go once those land.Bun__JSBigIntStatsObjectConstructor(only reached from theBIG = trueinstantiation ofStat::get_constructor, which nothing calls),Bun__allocUint8ArrayForCopy(theUint8Arrayarm ofArrayBuffer::alloc) andBun__WebSocketClientTLS__initWithTunnelare dead instantiations of otherwise live generic/macro code and need a small refactor rather than a deletion.__setDestroyCallback/__createObject/__assignToStreamand the per-class__fromJS/__dangerouslySetPtr/*CachedValuethunks that happen to be unused for some sinks/classes are uniform codegen output; the<Class>_getterexpansions inZigGlobalObject.cppstill need the macro split Remove dead code from node:crypto bindings, JSC/WebCore bindings, uSockets, and llhttp #37454 mentioned.