Remove dead code across foundation crates#32000
Conversation
|
Updated 10:11 PM PT - Jun 23rd, 2026
❌ @robobun, your commit 42995f5 has 2 failures in
The baseline build contains instructions not available on Static scan violations
|
|
@robobun adopt |
WalkthroughThis PR consolidates shared output tag rewriting logic, optimizes string escaping through const-generic encoding dispatch, abstracts event loop storage behind a trait for cross-platform code reuse, and removes approximately 30 unused public APIs across collections and utilities while refactoring internal data structures including semver strings, URL FFI handles, and sourcemap generation. ChangesShared Output Tag Rewriting and String Escaping Optimization
Event Loop Storage Abstraction and Cross-Platform Unification
Systematic API Surface Reduction
Data Structure Refactoring
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…urcemap, threading, url
alii
force-pushed
the
claude/split/foundations
branch
from
f0cc8a8 to
58f7624
Compare
|
Adopted. Full diff audited as behavior-preserving; the one external consumer of the changed Merged main at a830e9d with two manual resolutions (carried Bot review state: claude[bot] found no behavioral issues and recommends a human pass given the refactor density on the string escaper and PollStore; its two style nits are fixed in 7ad9fc9 and 42995f5. CodeRabbit findings declined (tag-grammar withdrawn; CI on build 64431 (head 42995f5): 284 jobs passed, 2 failed, both unrelated to this diff:
Ready for a maintainer to retry those two jobs or merge on the strength of the 284 green. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/bun_output_tags/lib.rs`:
- Around line 125-143: The parser currently consumes raw '>' and drops a
trailing backslash at EOF; update the match arms handling b'\\' and b'>' so only
recognized tag/escape sequences are consumed: in the b'>' arm, push the literal
b'>' into out (instead of swallowing it) and advance i; in the b'\\' arm, if i+1
>= fmt.len() (trailing backslash) push the b'\\' into out and stop/advance
appropriately, otherwise keep the existing escape handling for recognized '<' or
'>' and for unknown escapes push both the backslash and the following byte
unchanged. Locate these edits around the match on fmt[i] in lib.rs where fmt,
out, and i are used.
🪄 Autofix (Beta)
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: e0b240f7-9211-43b1-aa09-cbf027083864
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
src/bun_core/output.rssrc/bun_core/string/mod.rssrc/bun_output_tags/lib.rssrc/clap_macros/lib.rssrc/collections/StaticHashMap.rssrc/collections/array_hash_map.rssrc/collections/array_list.rssrc/collections/bit_set.rssrc/collections/lib.rssrc/collections/multi_array_list.rssrc/collections/pool.rssrc/ini/lib.rssrc/io/PipeWriter.rssrc/io/posix_event_loop.rssrc/io/windows_event_loop.rssrc/paths/lib.rssrc/paths/resolve_path.rssrc/router/Cargo.tomlsrc/router/lib.rssrc/semver/Version.rssrc/semver/lib.rssrc/sourcemap/Chunk.rssrc/sourcemap/lib.rssrc/threading/WaitGroup.rssrc/threading/channel.rssrc/threading/guarded.rssrc/threading/work_pool.rssrc/url/Cargo.tomlsrc/url/lib.rs
💤 Files with no reviewable changes (16)
- src/collections/StaticHashMap.rs
- src/semver/Version.rs
- src/collections/lib.rs
- src/threading/guarded.rs
- src/paths/resolve_path.rs
- src/router/Cargo.toml
- src/threading/work_pool.rs
- src/threading/WaitGroup.rs
- src/sourcemap/lib.rs
- src/collections/array_hash_map.rs
- src/sourcemap/Chunk.rs
- src/collections/array_list.rs
- src/threading/channel.rs
- src/collections/pool.rs
- src/paths/lib.rs
- src/collections/bit_set.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/bun_core/output.rs`:
- Around line 765-768: The comments near the ENABLE_LOGS and SCOPED_FILE_WRITER
code exceed the 3-line maximum as per coding guidelines. Reduce both comments
(one at lines 765-768 and another at lines 2607-2610) to 3 lines or fewer by
condensing the explanatory text while preserving the key information about why
ENABLE_LOGS gates readers and why initialization occurs under debug_assertions
to prevent SCOPED_FILE_WRITER from being zeroed.
🪄 Autofix (Beta)
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: e50342ba-4ac9-4dd0-b493-c2930eb3c4db
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
src/bun_core/output.rssrc/collections/array_hash_map.rssrc/ini/lib.rssrc/io/PipeWriter.rssrc/io/posix_event_loop.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 `@src/bun_core/output.rs`:
- Around line 765-768: The comments near the ENABLE_LOGS and SCOPED_FILE_WRITER
code exceed the 3-line maximum as per coding guidelines. Reduce both comments
(one at lines 765-768 and another at lines 2607-2610) to 3 lines or fewer by
condensing the explanatory text while preserving the key information about why
ENABLE_LOGS gates readers and why initialization occurs under debug_assertions
to prevent SCOPED_FILE_WRITER from being zeroed.
🪄 Autofix (Beta)
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: e50342ba-4ac9-4dd0-b493-c2930eb3c4db
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
src/bun_core/output.rssrc/collections/array_hash_map.rssrc/ini/lib.rssrc/io/PipeWriter.rssrc/io/posix_event_loop.rs
🛑 Comments failed to post (1)
src/bun_core/output.rs (1)
765-768: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Trim the new comments to the 3-line maximum.
Both new explanatory comments span four lines.
Proposed comment compaction
- // `ENABLE_LOGS` gates every reader, but initialize under - // `debug_assertions` too so `SCOPED_FILE_WRITER` is never observed - // zeroed if a `#[cfg(debug_assertions)]` path reaches it in - // release-asan/release-assertions. Cheap (one static write). + // `ENABLE_LOGS` gates readers, but initialize under `debug_assertions` + // too so `SCOPED_FILE_WRITER` is not zeroed if a debug-assertions path + // reaches it in release-asan/release-assertions. Cheap (one static write).- // All callers are already gated on `Environment::ENABLE_LOGS`; this is a - // Debug-build self-check (release-asan/release-assertions enable - // `debug_assertions` with `ENABLE_LOGS == false`, so keying on - // `debug_assertions` would turn it into a guaranteed abort there). + // All callers are gated on `Environment::ENABLE_LOGS`; this is a + // debug-build self-check, not a `debug_assertions` check, because + // release-asan/release-assertions run with `ENABLE_LOGS == false`.As per coding guidelines, "Keep code comments to 3 lines max."
Also applies to: 2607-2610
🤖 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/bun_core/output.rs` around lines 765 - 768, The comments near the ENABLE_LOGS and SCOPED_FILE_WRITER code exceed the 3-line maximum as per coding guidelines. Reduce both comments (one at lines 765-768 and another at lines 2607-2610) to 3 lines or fewer by condensing the explanatory text while preserving the key information about why ENABLE_LOGS gates readers and why initialization occurs under debug_assertions to prevent SCOPED_FILE_WRITER from being zeroed.Source: Coding guidelines
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No behavioral issues found, but this touches a lot of load-bearing surface area for a dead-code pass — the const-generic string-escaper rewrite, the genericized PollStore over unsafe raw-pointer code shared across POSIX/Windows, and the URL FFI handle/signature change — so it's worth a human pass before the stacked chain lands on top of it.
Extended reasoning...
Overview
This PR spans 30 files across the foundation crates (collections, threading, paths, semver, sourcemap, ini, clap_macros, bun_core, io, router, url) and nets roughly −1.5k lines. The bulk is genuinely mechanical: deleting unused public methods (put_context, add_many_at, complement/xor_with/difference_with, sort_span, schedule_batch, try_write_item, raw_mutex, add_unsynchronized, etc.), removing orphaned #[cfg(test)] fixture scaffolding from router, and dropping the corresponding bun_js_parser dev-dependency. Those parts are low-risk.
However, several hunks are non-trivial refactors rather than deletions:
bun_core/string/mod.rs:write_pre_quoted_stringis restructured around a const-genericEncodingparameter. The UTF-16 path switches fromcast_slice::<u8, u16>to manual two-byte little-endian reads via acode_unit_at!macro, the$lookahead is rewritten to usecode_unit_at!(i + clamped_width), andquote_for_jsongains a heuristic 12.5% pre-grow. These are claimed behavior-preserving but change indexing structure on a hot transpiler path.io/posix_event_loop.rs+windows_event_loop.rs: the per-platformStoreis replaced by a genericPollStore<P: PollSlot>with a newunsafe trait PollSlotand twomacro_rules!(impl_file_poll_flag_methods!,impl_poll_slot!). This consolidates ~100 lines of duplicated unsafe raw-pointer / Stacked-Borrows-sensitive code into one definition that both platforms instantiate.semver/lib.rs: three open-coded length-match ladders collapse intocan_inline+inline_byteshelpers.url/lib.rs: the opaqueURLhandle is redefined viabun_opaque::opaque_ffi!(now!Send/!Sync),from_stringchanges from&Stringto ownedString, anddestroy/file_url_from_string/path_from_file_urlhelpers are added.bun_output_tags/lib.rs:pretty_fmt_runtimeis moved verbatim frombun_core/output.rs(now#![no_std]+alloc), with the capacity heuristic dropped from* 4to* 2and a new rationale comment.
Security risks
None apparent. No auth, crypto, permissions, or untrusted-input parsing surfaces are touched. The closest thing is the URL FFI handle change, but that's a type-level wrapper around an existing C++ pointer with no new attack surface.
Level of scrutiny
Medium-high. The pure deletions and the pretty_fmt_runtime move are low-risk and well-covered by existing tests (parity tests in output.rs, install/bundler suites). But the string-escaper and PollStore refactors sit on hot/critical paths (JS printer output correctness; event-loop deferred-free under Stacked Borrows on both POSIX and Windows lanes), and this PR is explicitly the base of a stacked chain — regressions here propagate. The PR description states a per-file behavioral-equivalence audit was done and full CI is green (build 61383/61692), which is reassuring, but the refactor density exceeds what I'd auto-approve.
Other factors
- The only finding from the bug hunt is a cosmetic comment-accuracy nit ("5-byte" → "4-byte"); no functional issues surfaced.
- CodeRabbit's one substantive finding (
>/ trailing-\\handling inpretty_fmt_runtime) was a misread of the tag grammar and was withdrawn; that thread is resolved. - robobun has adopted the PR and confirmed CI is effectively green modulo two expired darwin shards unrelated to the diff.
- No human reviewer has weighed in yet despite the PR being open ~16 days; given it gates a stack, a maintainer sign-off seems appropriate.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun_core/string/mod.rs (1)
2462-2465: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTrim these new comments to the three-line cap.
Both doc blocks exceed the repository’s comment-length rule; they can be compressed without losing the invariant/perf rationale.
Suggested wording
- /// `strings::Encoding` stand-in that derives `ConstParamTy` so it can be - /// used as a const-generic parameter (`const ENCODING: Encoding`). The - /// variant set is identical; convert at the boundary if a - /// `strings::Encoding` is ever needed. + /// `strings::Encoding` stand-in deriving `ConstParamTy` for const-generic use. + /// Variants mirror `strings::Encoding`; convert at the boundary when a + /// runtime encoding is needed. @@ - /// `quote_char` / `ascii_only` / `json` are runtime args: the branches on - /// them are cheap and well-predicted, and collapsing the monomorphizations - /// keeps the hot transpile pages dense. `ENCODING` stays `const` — it - /// changes the code-unit indexing structure of the loop, so a per-encoding - /// copy is genuinely different code. + /// Keep formatting flags runtime-dispatched to collapse monomorphizations; + /// `ENCODING` stays const because it changes the loop's code-unit indexing + /// structure, so each encoding is genuinely different code.As per coding guidelines, “Keep code comments to 3 lines max.”
Also applies to: 2500-2504
🤖 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/bun_core/string/mod.rs` around lines 2462 - 2465, The doc comments at the Encoding type definition exceed the repository's 3-line maximum limit. Trim the doc comment starting around line 2462 that describes the strings::Encoding stand-in and its const-generic parameter capability down to 3 lines or fewer, retaining the key information that it derives ConstParamTy for const-generic usage and that the variant set is identical. Also apply the same compression to the similar doc comment block around line 2500-2504 to meet the coding guidelines.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/bun_core/string/mod.rs`:
- Around line 2462-2465: The doc comments at the Encoding type definition exceed
the repository's 3-line maximum limit. Trim the doc comment starting around line
2462 that describes the strings::Encoding stand-in and its const-generic
parameter capability down to 3 lines or fewer, retaining the key information
that it derives ConstParamTy for const-generic usage and that the variant set is
identical. Also apply the same compression to the similar doc comment block
around line 2500-2504 to meet the coding guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d4457bd8-6c9e-48a3-9a7b-4a1a10bdb0c7
📒 Files selected for processing (2)
src/bun_core/string/mod.rssrc/clap_macros/lib.rs
There was a problem hiding this comment.
Thanks for addressing both nits — I didn't find any behavioral issues, but the const-generic rework of write_pre_quoted_string_inner (hot-path escaper) and the unsafe PollStore<P> genericization across POSIX/Windows are dense enough that a human pass is worth it before this lands as the base of the stacked chain.
Extended reasoning...
Overview
30 files, net −1.5k lines across foundation crates. Roughly three buckets:
- Pure deletions of uncalled public items (collections
ArrayList/BitSet/MultiArrayList/StaticHashMap/SmallList/pool, threadingWaitGroup/Channel/GuardedBy/WorkPool, paths, sourcemap, semversort_gt, router#[cfg(test)]fixture scaffolding + dev-dep). - Dedup moves:
pretty_fmt_runtimehoisted intobun_output_tagsand re-exported frombun_core::output/ called fromclap_macros;PipeWriterSyncFile blocking loop extracted towrite_sync_file; semver inline-string init collapsed throughcan_inline/inline_bytes; inipnpm_matcher_from_exprreplaced byPnpmMatcher::from_expr. - Non-trivial refactors:
write_pre_quoted_stringsplit into a runtime-encoding adapter +#[inline(never)]const-genericwrite_pre_quoted_string_inner<W, const ENCODING>(changes UTF-16 indexing to manualfrom_le_bytes, fuses the twowrite_allcalls in the ASCII/UTF-8 fast path, reroutes$lookahead throughcode_unit_at!, adds agrow_if_neededheuristic inquote_for_json);Store→ genericPollStore<P: PollSlot>with a newPollSlotunsafe trait +impl_poll_slot!/impl_file_poll_flag_methods!macros, WindowsStoredeleted and aliased toposix::PollStore<FilePoll>;whatwg::URLswitched toopaque_ffi!andfrom_stringnow takesStringby value.
Security risks
None identified. No auth/crypto/permissions surface; the URL FFI change is a handle-type swap (opaque_ffi!) plus an owned-vs-borrowed parameter change whose only external caller goes through from_utf8 (per the adoption comment). The unsafe code touched (PollStore intrusive list, impl_poll_slot!) preserves the existing Stacked-Borrows discipline (raw-pointer field ops only, no &mut P materialized while &mut PollStore is live) and adds a const { assert!(!needs_drop::<P>()) } guard.
Level of scrutiny
High, for two specific hunks:
src/bun_core/string/mod.rs—write_pre_quoted_string_inneris the escaper behind the JS printer andquote_for_json; the diff changes loop structure (fusedwrite_all, manual little-endian UTF-16 unit reads replacingcast_slice,clamped_width-based$lookahead) in ways that are equivalence-by-inspection rather than mechanical. A wrong byte here surfaces as snapshot diffs across the bundler/transpiler suite, but it's hot enough that a human should confirm the equivalence reasoning.src/io/{posix,windows}_event_loop.rs— the WindowsStoreis now the POSIXPollStore<FilePoll>instantiation. The merge resolution carriesPollMemoryPressureinto the macro so Windowsis_registered()now also tests it (previously did not); the PR body calls this intentional and harmless since nothing sets that flag on Windows, but it is a behavior delta on a platform I can't exercise here.
The rest (deletions, pretty_fmt_runtime move, PipeWriter extraction, semver inline helper) is mechanical and low-risk.
Other factors
- Both of my prior inline nits (4-byte vs 5-byte capacity comment; CLAUDE.md 3-line comment cap) were addressed in 7ad9fc9 and 42995f5 and are resolved.
- CodeRabbit's one substantive flag was withdrawn after the tag-grammar was clarified; no open reviewer threads remain.
- CI is running on the current head; the PR body reports green full-workspace
cargo check(linux + windows io crate) post-merge. - This PR is the base of a stacked chain (#32022 etc.), which raises the cost of a latent regression slipping through.
2 participants
What this does
Small dead-code deletions and helper dedups across the foundation crates: collections (unused StaticHashMap/array_list/bit_set items), threading, paths, semver, sourcemap, ini, clap_macros, bun_core output/string, io (PipeWriter/event-loop macro extraction), router (deletes orphaned cfg(test) fixture scaffolding and its dev-dependency), and url (re-exports
bun_opaquehandle types; Cargo.lock updated accordingly). Net −1.5k lines.This is the base of a small stacked chain (install-cli → jsc-runtime → webcore will be based on it).
Split from #31912 (whole-repo simplification pass; closing that PR in favor of module-scoped splits). This PR only moves and removes code — zero intended behavior change. Verified there by a per-file behavioral-equivalence audit and full CI (green on build 61383); verified here by a standalone full-workspace compile check.
Why no new test files
This diff removes unused code and dedups identical implementations, so there is no behavior change for a new test to pin down. Regression coverage is the existing suite, which already exercises every touched path: bundler/transpiler snapshots for the string printer, the io and spawn suites for FilePoll/PipeWriter (Windows lanes cover
windows_event_loop), the install suites for semver string interning, andtest/cli/install/public-hoist-pattern.test.tsfor the ini matcher path.multi_array_listkeeps its in-crate#[test]coverage.Merge resolution (main at a830e9d)
Two hand-resolved points:
src/io/posix_event_loop.rs: Add process.on('memoryPressure') event #32594 addedFlags::PollMemoryPressuretois_registered(), which this PR moves intoimpl_file_poll_flag_methods!(). Carried the new flag into the macro body (so the Windows expansion picks it up too; harmless there: the FilePollFlags::MemoryPressureregistration inmemory_pressure.rsis#[cfg(not(windows))]-gated, Windows uses auv_async_tbackend, so the flag is never set on a Windows FilePoll, same as the pre-existingPollMachportclause. Cross-checkedcargo check -p bun_io --target x86_64-pc-windows-msvc).src/collections/array_hash_map.rs: restoredget_index_mut, which gained a caller insrc/react_compiler/collections.rsvia React Compiler integration #32504 after this PR was authored. The other two deletions in that file (ensure_total_capacity_context,StringHashMap::put_owned) remain dead and are still removed.Swept every other public item this PR deletes against current
origin/main; none gained callers. Fullcargo check --workspaceis green after the merge.