Conversation
The C++ shims for NoOrphansTracker, the CoreGraphics and WIC image backends, and the io_darwin waker defined empty stubs on the platforms they do not support. Every Rust caller of those symbols is behind a target_os cfg, so nothing references the stubs on any target. Also remove two trait impls that nothing calls: OptionsEnvArg for Box<ZStr> and From<Box<[u8]>> for cache::Contents.
|
Status: ready for review.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughThe pull request removes unsupported boxed conversion implementations and non-native platform fallback stubs. It also updates ChangesRust API cleanup
Platform stub cleanup
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The cleanup does not leave an inspected supported target or internal conversion path without its required implementation. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — pure dead-code removal; every deleted symbol and impl is unreferenced on the platforms that lose it.
What was reviewed:
- Traced each removed C++ stub to its Rust callers:
Bun__noOrphans_*are used only inside#[cfg(target_os = "macos")]items insrc/spawn/process.rs(wait_mac_kqueue, gateduse/defer!) andsrc/io/ParentDeathWatchdog.rs;io_darwin_*only in the macOS-gatedKEventWakerinsrc/io/lib.rs;bun_coregraphics_*only inbackend_coregraphics.rs(mounted undercfg(target_os = "macos"));bun_wic_propbag_write_*only inbackend_wic.rs(#![cfg(windows)]). The un-gated declarations insrc/spawn_sys/lib.rsare declarations only, so no link reference is emitted. - No C++ header, export list, or build script names these symbols; the shims compile as standalone TUs (
scripts/build/unified.tsnoUnify), and an empty TU on non-target platforms matches the existingroot_certs_darwin.cpp/root_certs_windows.cpppattern. - The three
append_options_envcallers use&'static ZStrorBunString, and everyContents::fromsite passesVec<u8>orCow<[u8]>, so the deletedBox<ZStr>andFrom<Box<[u8]>>impls had no users (a leftover would fail to compile).
<details>
<summary>Extended reasoning...</summary>
Overview
The PR deletes 68 lines across six files: the #else fallback stubs in four platform-specific C++ shims (src/io/io_darwin.cpp, src/jsc/bindings/NoOrphansTracker.cpp, src/jsc/bindings/image_coregraphics_shim.cpp, src/jsc/bindings/image_wic_shim.cpp), the impl OptionsEnvArg for Box<ZStr> in src/bun_core/util.rs (with a doc-comment update naming the two remaining impls), and impl From<Box<[u8]>> for cache::Contents in src/resolver/lib.rs. No behavior is added or changed on any platform where the code was live.
Security risks
None. The change only removes never-called no-op stubs and two unused trait impls; it does not touch input validation, auth, crypto, or any user-reachable path. Removing the unsafe fat-pointer cast in the Box<ZStr> impl reduces unsafe surface rather than adding it.
Level of scrutiny
The one real risk with this kind of change is link-time symbol resolution on the platforms that lose the definitions, since Rust extern "C" declarations compile cleanly whether or not a definition exists. I grepped the full tree (excluding vendor/build/test) for every removed symbol and confirmed each is referenced only from code gated on the platform that still defines it: Bun__noOrphans_* from wait_mac_kqueue (lines 3463-3703 of process.rs, #[cfg(target_os = "macos")]), a macOS-gated use, defer!, and if block, plus a macOS-gated call and extern block in ParentDeathWatchdog.rs; io_darwin_* from the macOS-gated extern block and impl KEventWaker; bun_coregraphics_* from backend_coregraphics.rs, whose mod is #[cfg(target_os = "macos")] in image/mod.rs; bun_wic_propbag_write_* from backend_wic.rs under #![cfg(windows)]. The un-gated pub safe fn Bun__noOrphans_* declarations in src/spawn_sys/lib.rs emit no undefined-symbol reference without a call site. No C++ header, .def, export list, or build script names any of them. The shims are in the noUnify list so each compiles as its own TU; an empty TU on other platforms already exists in-tree (root_certs_darwin.cpp, root_certs_windows.cpp), and the build flags do not enable -pedantic/-Wempty-translation-unit.
For the Rust impls, all three append_options_env callers (cli/mod.rs with Vec<&'static ZStr>, node_process.rs with BunString, and the internal &'static ZStr path in util.rs) avoid Box<ZStr>, and the Contents::from sites (transpiler.rs with Vec<u8> from decode_data, lib.rs with Cow/Vec) avoid Box<[u8]>. Any missed user would be a compile error, so the author's reported rust:check-all pass plus CI covers the residual risk.
Other factors
The change is small, mechanical, and self-contained; the changed paths are not covered by .github/CODEOWNERS; no tests are needed for deletion of code that nothing calls (and REVIEW.md explicitly says not to add tests that dead code stays dead). The bug-hunting pass ran dry after ruling out the four link-time candidates, which matches my own reading.
</details>
|
Both automated reviews are in. Neither asks for a change, so the branch stays at 75eccbd.
|
Problem
Bun__noOrphans_*(NoOrphansTracker.cpp),bun_coregraphics_*(image_coregraphics_shim.cpp),bun_wic_propbag_write_*(image_wic_shim.cpp) andio_darwin_*(io_darwin.cpp). The comments say the linker needs them. It does not: every Rust caller is behind#[cfg(target_os = "macos")]or#![cfg(windows)], so no other target references the symbols.OptionsEnvArg for Box<ZStr>(src/bun_core/util.rs) andFrom<Box<[u8]>> for cache::Contents(src/resolver/lib.rs). Thedead_codelint of rustc does not report trait impls.Fix
rgfinds each symbol only at its definition and at the cfg-gated call sites. The Linux debug link does not use--gc-sections, so a reference that is left fails the link. The link passes.bun bd,bun run rust:check-all(12 targets),cargo check --workspace --release, andbun bd testontest/cli/env/bun-options.test.ts,test/js/bun/image/image.test.ts,test/cli/run/no-orphans.test.tsand twotest/js/bun/resolve/files.Background
#if OS(DARWIN),#if defined(__APPLE__)or#if defined(_WIN32). The#elsebranch held empty definitions of the sameextern "C"names.extern "C"declaration in Rust makes a link-time reference only where code calls it. A call site under#[cfg(...)]does not exist on the other targets.--gc-sections --print-gc-sections. That link lists every function that nothing references.Notes
Removed symbols:
src/jsc/bindings/NoOrphansTracker.cpp(non-Darwin stubs):Bun__noOrphans_begin,Bun__noOrphans_releaseKq,Bun__noOrphans_onFork,Bun__noOrphans_onExit,Bun__noOrphans_killTracked. Callers:src/spawn/process.rsandsrc/io/ParentDeathWatchdog.rs, all under#[cfg(target_os = "macos")].src/jsc/bindings/image_coregraphics_shim.cpp(non-Apple stubs):bun_coregraphics_decode,bun_coregraphics_encode,bun_coregraphics_scale,bun_coregraphics_rotate90,bun_coregraphics_reflect,bun_coregraphics_clipboard,bun_coregraphics_clipboard_change_count. Only caller:src/runtime/image/backend_coregraphics.rs, mounted with#[cfg(target_os = "macos")]insrc/runtime/image/mod.rs.src/jsc/bindings/image_wic_shim.cpp(non-Windows stubs):bun_wic_propbag_write_f32,bun_wic_propbag_write_u8. Only caller:src/runtime/image/backend_wic.rs, which starts with#![cfg(windows)].src/io/io_darwin.cpp(non-Apple stubs):io_darwin_create_machport,io_darwin_schedule_wakeup. Only caller: the macOSKEventWakerinsrc/io/lib.rs, declared and used under#[cfg(target_os = "macos")].src/bun_core/util.rs:impl OptionsEnvArg for Box<ZStr>. The three callers ofappend_options_envuse&'static ZStrorbun_core::String. The trait doc comment now names the two impls that exist.src/resolver/lib.rs:impl From<Box<[u8]>> for Contents.From<Vec<u8>>stays, it has callers.Verification details:
test/internal/source-lints/CLAUDE.mdasks not to add tests for dead symbols.test/cli/run/no-orphans.test.ts("a non-Bun grandchild spawned with uid/gid is reaped") is not from this change. It fails the same way with a release build of main in the same container, and the Linux code path does not touch the removed macOS stubs.cargo check --workspace --all-targetsstops in thebun_bundlerunit tests on main already (src/bundler/options.rs:2328,Some(0)whereOption<ContentHash>is expected). This PR does not touch that crate.#elseblock ofNoOrphansTracker.cpp. If it lands first, that hunk needs a small rebase.Scope of this sweep and what came back clean, so the next run can skip it:
Bun__napi_get_versioninnapi.cppis the renamednapi_internal_get_versionthat Remove dead code from the JSC bindings, headers.h, the console builtin, and bun_jsc #40232 removes.--gc-sectionsrelink lists 427 unreferenced functions in Bun's own C and C++. 247 are already removed by open PRs. Most of the rest are called from Rust code that is compiled on another platform only, or are macro-generated (*_getterinZigGlobalObject.cpp), or belong to the bindgen integer converter matrix thatsrc/codegen/bindgen.tscan emit.dead_codeandunreachable_pubare deny-level, so a flip of everypubinbun_runtimetopub(crate)on Linux, Windows and macOS reports zero dead functions. It reports about 20 never-read fields and never-built variants, nearly all wire-protocol or FFI tables. A strip of every#[allow(dead_code)]escape on the same three targets reports nothing that is dead on all of them.Display for FilePoll(withFlagsFormatter) andDefault for store::Byteslook dead on Linux but macOS and Windows use them, so they stay. Therust:check-allrun caught both.#if 0, nocfg(any()), no orphan.rsfile, no header that nothing includes, nomacro_rules!without an invocation, no unused#define, no Rustexterndeclaration without a caller (two exist, both in open PRs), no unusedBunBuiltinNames.hentry, no unused$intrinsicdeclaration outside open PRs. oxlintno-unused-varsand an unused-export scan oversrc/js,src/codegen,scriptsandpackagesonly find items that open PRs already remove.cargo::unused_dependencieslint reports five dependency edges that are unused on all 12 targets. Three are removed by Remove dead code from bun-uws, bun-usockets, the require.extensions FFI, and unused Cargo dependencies #40294.bun_zstdinbun_resolveris used undercfg(bun_codegen_embed), andbstrinbun_wyhashis used by a unit test, so both stay.