Remove dead code from bun_core, bun_css, bun_jsc, and the FFI crates - #38703
Remove dead code from bun_core, bun_css, bun_jsc, and the FFI crates#38703robobun wants to merge 1 commit into
Conversation
Cross-crate reachability analysis over the Rust workspace (intersected across linux-gnu, linux-musl, android, freebsd, darwin and windows-msvc, test targets included) found these pub items unreachable from any shipped root. Deletes the inherent eql/to_css/parse forwarders in bun_css that the trait impls replaced (including the CalcValue::eql hook that only they called), leftover helpers in bun_core/bun_alloc/bun_ast/bun_ptr/bun_jsc, and unused wrappers, aliases and constants in the sys crates. Verified with cargo check --workspace on all ten CI triples (plus the bun_debug/bun_asan cfgs and --all-targets on linux) and a full debug build. A source lint in test/internal/source-lints keeps the symbols from coming back.
|
Warning Review limit reached
Next review available in: 2 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 (49)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Given the breadth — ~67 public items removed across 49 files in foundational crates (bun_core, bun_jsc, bun_sys, bun_css, and eight FFI *_sys crates), including a required trait method on CalcValue — a human pass would still be worthwhile.
What was reviewed:
- Spot-checked the higher-risk removals: the shim's
kernel32::WaitForSingleObjectre-export (shim has its own local extern inbun_shim_impl.rsmod k32),UTIME_OMIT(only comment/libuv-header references remain),bun_uws_sys::Opcode::Close(bun_httpdefines its ownOpcodeenum), andposix::read(linux_syscall::read_rawstill used bylinux::read). - Confirmed the
CalcValue::eqltrait-method removal is closed: all six impls dropped it, and structural equality goes through the survivingPartialEq for Calc<V>/MathFunction<V>. - The new source-lint follows the existing
test/internal/source-lints/dead-symbols-*.test.tspattern.
Extended reasoning...
Overview
This PR removes ~631 lines of dead Rust across 49 files in bun_core, bun_alloc, bun_ast, bun_ptr, bun_css, bun_jsc, bun_sys, and eight FFI *_sys crates (boringssl, cares, tcc, uws, windows, zlib), plus adds a 139-line source-lint test guarding against reintroduction. Every removed item was identified via cross-target (6 OS triples) hawk dead_public analysis, deduplicated against ~11 open dead-code PRs, and re-verified with rg over src/, src/codegen/, and regenerated build/debug/codegen/. Verification included cargo check --workspace on all 10 CI triples plus --cfg bun_debug --cfg bun_asan and --cfg bun_codegen_embed --all-targets, a full bun bd build, and targeted test runs.
Security risks
Two BoringSSL constants (X509_V_OK, SSL_SESS_CACHE_CLIENT) and GeneralNames::is_empty are removed. These are unused constant declarations / trivial helpers — removing them cannot weaken any security check (no callers exist). No auth/crypto/permission logic is modified.
Level of scrutiny
This warrants human review despite being pure deletions. The individual removals are mechanical, but the aggregate touches foundational infrastructure that every other crate depends on: a required trait method (CalcValue::eql) is dropped from a public trait, cfg-gated re-exports are pruned across platform boundaries (Windows shim, UTIME_OMIT, WindowsLoop non-Windows alias), and ~67 items across 8 FFI-sys crates are removed. A single false-positive in the reachability analysis (e.g., a macro-generated call site, a codegen path not exercised in the regenerated output, or an in-flight PR not in the deduplication list) would break the build for downstream work. The methodology is exemplary and my spot-checks all confirm correctness, but 49 files across the lowest tiers of the crate graph is beyond the "simple and self-contained" bar for auto-approval.
Other factors
- The new test file follows the established
test/internal/source-lints/dead-symbols-*.test.tspattern (three siblings already exist), so it is not novel test infrastructure. - The PR description's "deliberately left alone" section demonstrates careful judgment on hawk false positives (const-generic positions, cfg-gated callers, clippy.toml references, in-flight PR #36225), which raises confidence.
- Comment-only edits in
rules/mod.rs,css_derive/lib.rs,selectors/parser.rs,sys/windows/mod.rs, etc. correctly trim references to removed items without adding new prose. - No prior reviews or comments to address; CodeRabbit was rate-limited and did not review.
|
Updated 4:45 PM PT - Aug 14th, 2026
❌ @robobun, your commit c426ed6 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 38703That installs a local version of the PR into your bun-38703 --bun |
2 participants
Net -631 lines of Rust source (49 files), plus a 139-line source lint that keeps the symbols from coming back.
Method: ran the repo's hawk setup (
tools/hawk/,dead_publiconly) once per target for linux-gnu, linux-musl, android, freebsd, darwin and windows-msvc, and kept only the items reported unreachable on every target (test targets included). Items whose file is touched by one of the currently open dead-code PRs were dropped if that PR removes the same symbol (checked against each PR's diff), so nothing here duplicates an open PR. Every remaining item was then re-checked by hand withrgoversrc/,src/codegen/and freshly regeneratedbuild/debug/codegen/before deletion; that pass caught a few hawk false positives, listed at the bottom.bun_css (-203)
The inherent
eql/to_css/parseforwarders that predate theCssEql/ToCss/Parsetrait impls; every caller already goes through the traits.Calc::eql,MathFunction::eqland the privateeql_calc_listthey shared (values/calc.rs)CalcValue::eqltrait hook and its six impls: its only caller wasCalc::eql. That in turn made the UFCS-only inherentTime::eqlandAngle::eqldead (Angle::eqlispub(crate), so rustc flagged it once the trait hook was gone); both removedToken::eql,Num::eql,Dimension::eql(css_parser.rs) andgenerics::implement_eql, which only they calledgenerics::parse<T: Parse>free functionDefaultAtRule::to_css(whole impl block)GenericSelectorList::to_css,GenericSelector::to_css,GenericComponent::to_css,Combinator::to_css:#[deprecated]tombstones that unconditionallyunreachable!()TokenList::parse_with_options,CssStringFns::parse(and thecss::CssResult as Resultimport only it used)bun_jsc (-175)
BuiltinName::getand the privateBUILTIN_NAME_MAPcomptime map that only it read (lib.rs)MarkedArrayBuffer::to_js(every holder goes throughto_node_buffer/destroy)TopExceptionScope::new,ExceptionValidationScope::new(all construction goes throughinit/init_guard*)JSGlobalObject::to_js,JSGlobalObject::ref_,JSGlobalObject::ctx(the latter two were already marked for deletion in a comment)AbortReason::to_js(whole impl block, plus theCommonAbortReasonExtimport only it used),JSCell::to_js,JSValue::cast,ErrorBuilder::new(values are built with a struct literal),TagPayload::get,task::new,JsPtr::on_js_thread,Completion::off_thread,UUID::ZERObun_core, bun_alloc, bun_ast, bun_ptr (-134)
RawSlice::from_raw, the bufferconcatin lib.rs (callers usestrings::concat),SliceWithUnderlyingString::from_utf8,BoundedArrayAligned::get,ExternalShared::as_ptr,ExternalSharedOptional::get,Unaligned::new,Mutex::get_mut,RwLock::get_mut,GenericIndexOptional::{is_some, is_none, get},Timespec::new, and thewrite::Resultalias (bun_io defines its own)impl AllocError { name }(wrapping error types hardcode the string), the top-levelusable_size(thedefault_alloc::usable_sizeeveryone calls stays),BSSList::init(bss_singleton!usesinit_at)symbol::Map::initBackRef::shared,DetachablePtr::as_ptrsys crates (-119)
UTIME_OMITand itsc::re-export, the rawposix::readwrapper,DynLib::handleNewSocketHandler::groupandAnySocket::group, theSocketTcp/SocketTlstype aliases and their re-exports (the same-namedAnySocketvariants are what everything uses),AnyResponse::init,SocketGroup::is_empty,PosixLoop::wake,PosixLoop::run,Opcode::Close(theOpcode::Closein bun_http is a different type), and the non-WindowsWindowsLoopstub alias (its only users are#[cfg(windows)])WaitForSingleObjectResult wrapper and its raw extern (bun_sys and the install shim each declare their own; the shim's dead re-export of it is dropped too),NTSTATUS::rawGeneralNames::is_empty,X509_V_OK,SSL_SESS_CACHE_CLIENTimpl AddrInfo_hints { is_empty }z_alloc_fn/z_free_fnlegacy aliases and their re-export,ByteSymbolCallbackalias and its re-exportComment lines that named a removed item were trimmed; no comments were added.
Verification
cargo check --workspaceon all ten CI triples (bun run rust:check-all), plus--cfg bun_debug --cfg bun_asanand--cfg bun_codegen_embed --all-targetson linux: clean, so nodead_code/unused_importsfallout is left behindcargo fmt --checkclean; fullbun bdbuildsbun bd testontest/js/bun/css/{css,color}.test.ts,test/bundler/css/css-modules.test.ts,test/js/web/abort/abort.test.ts,test/js/bun/util/inspect.test.js,test/js/node/zlib/deflate-streaming.test.ts,test/js/bun/net/localhost-loopback-contract.test.ts: all passtest/internal/source-lints/dead-symbols-cross-crate-sweep.test.ts: every one of its 67 entries fires against the pre-PR sources and none fire afterFound dead but deliberately left alone
CssModuleReference::eql: dead on main, but css: implement handle_composes to build CssModuleReference entries #36225 (handle_composes) starts calling it, so it staysStoredTrace::from: only caller is under#[cfg(bun_debug)](hawk ran without that cfg); livehost_fn::host_fn_this_value:generate-classes.tscan still emit calls to itstrings::{split_once, rsplit_once, rsplit_once_char}: no callers, butclippy.tomlpoints people at them as the sanctioned replacements for the std versions; removing them means rewording those entriesconst _:assertion positions as dead (Kind::ABS/REL,PROPERTY_BITSET_BITS,CALLBACK_COUNT,DeferredRequest::MAX_PREALLOCATED,native_promise_context::Tag::COUNT,MAX_CID_LEN,ARES_EDESTRUCTIONas an enum discriminant); all live, all kept. Itsmodulefindings are also not "module unused" (ffi::ffi,udp::raw,bake::jscare all used), so none were acted onO::{NOATIME, DSYNC, SYMLINK, NOFOLLOW_ANY},posix::R_OK,posix::POLL_OUT,TCSA/RlimitResourcevariants) were left for table completeness, matching the overrides already inhawk.tomlpubfields and ~25 never-constructed variants are reported on every target as well; those need per-field judgment (repr(C) layouts, liveness anchors, generated accessors) and are left for a separate pass