refactor(rust): eliminate 57 borrowck-workaround allocs and unsafe launders - #35399
refactor(rust): eliminate 57 borrowck-workaround allocs and unsafe launders#35399robobun wants to merge 11 commits into
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 (40)
WalkthroughThis PR removes borrow-checker workarounds across bundler, package installation, parser, runtime, shell, SQL, and test code. It replaces temporary allocations, raw-pointer lifetime shaping, cloned collections, and lazy state with direct borrowing, ownership transfer, scoped guards, and inline collections. ChangesBorrow-checker cleanup
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:32 AM PT - Jul 25th, 2026
❌ @robobun, your commit d6ba924 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35399That installs a local version of the PR into your bun-35399 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This is a behavior-preserving refactor (bucket B of the Status at d6ba924 (synced with main at ae4b17d):
CI: build #79970 had 189 lanes pass with zero test failures (7 expired agents). The retrigger (#80075) hit a fleet-wide build-job timeout (2 build jobs timed out, 169 downstream never ran). The diff itself is green; ready to merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this touches 43 files across bundler/parser/install/JSC/SQL/shell with subtle borrow-lifetime rewrites, so it's worth a human pass.
What was reviewed:
mem::take-then-restore sites checked for early-return leaks — the?atcomputeCrossChunkDependencies.rs:366fires only on OOM into a fn-localchunk_metas, so the unrestoredimportsis unobservable.server_body::load_and_resolve_pluginsnow leavesself.state = Err(instead ofUnqueued) ifto_js_array/create_utf8_for_js?-return before line 1016 — likely intentional but a state-machine change.append_isolated_hashes_for_imported_chunks&mut→&signature change: the external caller ingenerateChunksInParallel.rs:351still passes&mutwhich auto-reborrows.seq.rsreorder (enqueue before storingbuf),repl::handle_enterbuffer take,fold.rsget_or_putindex write,LazyPackageDestinationDirdeletion — all traced, no issues found.
Extended reasoning...
Overview
Bucket-B cleanup of "reshaped for borrowck" markers: 57 sites across 43 Rust files, replacing porting-era workarounds (extra allocations, double hash lookups, unsafe pointer launders) with idiomatic safe Rust. Categories: entry-API collapses, .to_vec()/.clone() → mem::take+restore or split_at_mut, &mut → & where recursion is read-only, scopeguard::guard(payload) instead of sibling raw-pointer copies, and dead-code deletion (LazyPackageDestinationDir).
Security risks
None identified. No parsing of untrusted input changed, no auth/crypto/permissions touched. The unsafe removals reduce attack surface rather than adding it.
Level of scrutiny
High. Each individual site is small and mechanical, but the aggregate spans hot paths in the bundler linker, JS parser fold pass, package installer, VM lifecycle, HTTP/2 frame parser, SQL connection readers, and shell IO. Several rewrites (mem::take+restore, state mem::replace, statement reordering around enqueue) change error-path semantics in ways that only matter under failure — exactly the class the repo review guide calls out ("Every error/abort path actively completes the operation"). The server_body state change and computeCrossChunkDependencies take-without-guard are examples where I convinced myself they're fine, but a maintainer familiar with each subsystem's invariants should confirm.
Other factors
- No new tests (behavior-preserving refactor by design; PR body argues type checker + existing suites are the coverage).
- CI build
#79473was still running at review time. - No prior human review comments.
- The self-review commit (
1e5e583d) already addressed one round of issues (twin sites,&[Chunk]signature). - The
fold.rsget_or_putrewrite leaves an uninitialized value slot live acrossp.new_symbol()/p.module_scope_mut()before writingvalues_mut()[index]— I verified those calls don't touchcommonjs_named_exports, but this is the kind of implicit invariant a maintainer should sanity-check.
|
On the On |
b22ac04 to
0888f68
Compare
![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 `@test/internal/source-lints/borrowck-reshape-markers.test.ts`:
- Line 22: Update the borrowck marker ratchet in the test to match the actual
repository-wide count of 330, or complete the cleanup so the count genuinely
reaches the intended lower threshold. Verify the result using the test’s
source-lint counting logic before finalizing, and keep the assertion in the
borrowck reshape marker test consistent with the current marker total.
🪄 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: a0ca34b2-53c4-4eb7-8d5a-cc0789808f6a
📒 Files selected for processing (45)
src/bundler/LinkerContext.rssrc/bundler/bundle_v2.rssrc/bundler/defines.rssrc/bundler/linker_context/computeChunks.rssrc/bundler/linker_context/computeCrossChunkDependencies.rssrc/css/properties/flex.rssrc/dotenv/env_loader.rssrc/install/PackageInstaller.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/WorkspacePackageJSONCache.rssrc/install/PackageManager/install_with_manager.rssrc/install/PackageManager/patchPackage.rssrc/install/PackageManager/runTasks.rssrc/install/yarn.rssrc/js_parser/fold.rssrc/js_parser/lexer.rssrc/js_parser/lib.rssrc/js_parser/visit/visit_stmt.rssrc/jsc/AsyncModule.rssrc/jsc/ConsoleObject.rssrc/jsc/SavedSourceMap.rssrc/jsc/VirtualMachine.rssrc/paths/resolve_path.rssrc/runtime/api/BunObject.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/filesystem_router.rssrc/runtime/bake/production.rssrc/runtime/cli/publish_command.rssrc/runtime/cli/repl.rssrc/runtime/jsc_hooks.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/shell/IOReader.rssrc/runtime/shell/IOWriter.rssrc/runtime/shell/builtin/seq.rssrc/runtime/shell/interpreter.rssrc/runtime/test_runner/diff/diff_match_patch.rssrc/runtime/test_runner/snapshot.rssrc/runtime/webcore/Blob.rssrc/shell_parser/braces.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/postgres/PostgresSQLConnection.rstest/internal/source-lints/borrowck-reshape-markers.test.tstest/internal/source-lints/dead-code-escape-limits.json
💤 Files with no reviewable changes (4)
- test/internal/source-lints/dead-code-escape-limits.json
- src/jsc/SavedSourceMap.rs
- src/runtime/shell/interpreter.rs
- src/jsc/ConsoleObject.rs
There was a problem hiding this comment.
No new issues found on e004aaa — the two earlier findings (dead Seq::buf field; filesystem_router aarch64 segfault) are both addressed. That said, this is a 45-file refactor rewriting aliasing-sensitive code across the bundler, parser, JSC (VirtualMachine, Blob), package manager, shell, and SQL connection paths, and one earlier revision already segfaulted in CI, so a human pass is warranted.
What was reviewed:
braces.rssplit_at_mut: verifiednew_key > out_keyinvariant holds (counter is monotone,out_keyalways a prior counter value).patchPackage.rs/install_with_manager.rsdropped.to_vec():lockfileis a local disjoint frommanager.lockfile; borrowck proves the rest.computeCrossChunkDependencies.rsmem::take:importsis restored after the loop; nothing inside readschunk_metas[chunk_index].imports.LinkerContext::append_isolated_hashes_for_imported_chunks: checked the two callers — both already pass&*chunks/immutable, so the&mut → &signature change is compatible.
Extended reasoning...
Overview
Bucket B of the reshaped for borrowck cleanup: 57 sites across 45 files, replacing per-iteration allocations, double hash lookups, and raw-pointer lifetime launders with idiomatic Rust (entry API, mem::take, split_at_mut, disjoint field borrows, scopeguard::guard payloads, index+reborrow). Also deletes the now-dead LazyPackageDestinationDir enum from PackageInstaller.rs and adds a ratchet test pinning the marker count at 311.
Security risks
None identified. No auth/crypto/permission surface. The changes are internal borrow-shape rewrites; the closest to security-adjacent is the SQL connection read() refactor (MySQL/Postgres), which reindexes into byte_list after adjusting head — bounds are still checked against remaining().len() before the read.
Level of scrutiny
High. Per REVIEW.md, memory safety is the most-blocked category, and this PR is entirely about rewriting borrow/lifetime shapes in native code — including 20 unsafe launders replaced with safe equivalents, and several sites where the original comment explicitly documented an aliasing hazard (e.g. patchPackage.rs, Blob.rs, jsc_hooks.rs, VirtualMachine.rs). The type checker and existing test suites are the primary coverage, but an earlier revision of this same PR segfaulted on aarch64 (filesystem_router) despite the local suite passing — a concrete demonstration that borrowck acceptance ≠ runtime soundness when UnsafeCell/BackRef/interior mutability are involved.
Other factors
- Both prior findings from this reviewer are addressed and resolved (0888f68).
- The CodeRabbit ratchet-count finding was a symlink double-count, fixed in e004aaa.
- CI on the latest commit (e004aaa, build #79543) is still building per the robobun status comment.
- Several changes lean on "it compiles so borrowck proved it" for sites whose original comment named an aliasing hazard through interior mutability (
JsCell,BackRef, raw*mut). Those are precisely the cases borrowck cannot see, and warrant a human check that the specific hazard the original author documented is genuinely absent — particularlyBlob.rs(store borrowed fromselfacrossheap::into_raw(t)),repl::handle_enter(mem::takeof the line buffer vs. the previous copy-then-clear), and thelexer.rsscan_pragmareborrow.
| let import_refs: Vec<Ref> = chunk_metas[chunk_index].imports.keys().to_vec(); | ||
| for import_ref in import_refs { | ||
| let imports = core::mem::take(&mut chunk_metas[chunk_index].imports); | ||
| for &import_ref in imports.keys() { |
There was a problem hiding this comment.
This is all run on one thread right? I don't remember
There was a problem hiding this comment.
Yes, single-threaded. chunk_metas is a fn-local Vec<ChunkMeta> created at line 24 of compute_cross_chunk_dependencies, filled by the sequential for loop at line 78-80, then handed to this function as &mut [ChunkMeta]; it never leaves the call stack.
The walk step that esbuild runs per-chunk in goroutines is a plain sequential for chunk in chunks.iter_mut() here, so nothing else can observe chunk_metas[chunk_index].imports being empty between the mem::take and the restore at line 386. The body itself only touches chunk_metas[other_chunk_index].exports (guarded other_chunk_index != chunk_index at line 344) and chunks[chunk_index], never chunk_metas[*].imports.
|
@robobun sync! |
…unders Removes 57 'reshaped for borrowck' sites where the Zig->Rust port introduced extra heap allocations, double hash lookups, or unsafe raw-pointer launders to satisfy the borrow checker. Each fix is a local restructure (no signature changes, no new fields) that preserves behavior exactly. Breakdown: - 6 entry-API sites: collapse get()+insert() double-lookup into a single entry()/get_or_put() lookup (fold.rs, snapshot.rs, ConsoleObject.rs; three kept as-is with the NLL/parse-borrow note since the entry API doesn't fit). - 31 allocation sites: drop the extra Vec/Box via index+reborrow (LinkerContext, computeChunks, yarn), mem::take+restore (computeCrossChunkDependencies, repl handle_enter, IOReader, server_body), split_at_mut (braces x3), plain &[u8] where the borrow was already disjoint (filesystem_router, patchPackage, install_with_manager, bake/production x2), or SmallVec where a snapshot must stay (h2_frame_parser, IOWriter). Two sites (flex.rs:879, PackageManagerEnqueue.rs:995) keep the allocation because the underlying buffer reallocates or is emitted twice; their comment now explains that instead of 'reshaped for borrowck'. - 20 raw-pointer launders rewritten to safe Rust: reorder statements (BunObject, publish_command), disjoint field borrows (VirtualMachine:5527, PostgresSQLConnection:2407), scopeguard payload instead of sibling *mut copy (js_bun_spawn_bindings x2, jsc_hooks), usize offset math instead of *mut base-ptr (resolve_path x2), match &mut instead of from_ref().cast_mut() (server/mod), SmallVec copy instead of detach_lifetime (runTasks), and simple comment deletion where the hoist was already idiomatic (interpreter). PackageInstaller.rs: the else-branch destination_dir that was only ever close()'d (a no-op) is deleted, which made LazyPackageDestinationDir dead; removed the enum and the now-unreachable get_dir() error path. 368 -> 311 'reshaped for borrowck' comments remain (-57). 43 files, +247 / -517 lines. Two sites from the original audit were already fixed on main by #35321 (KEventWatcher.rs:130, WindowsWatcher.rs:525).
…ed_hashes - IOReader: on_reader_done_cb now mem::takes s.readers like on_reader_error (both are terminal; dispatch_reader_done creates a fresh IOReader, never re-registers on this one). - fold.rs: apply the same get_or_put collapse to the exports_ref arm at :384 so both commonjs_named_exports sites use one hash lookup. - LinkerContext: append_isolated_hashes_for_imported_chunks only reads chunks, so take &[Chunk]. Both loops become plain iteration with no per-step re-indexing and no unreachable!(). The sole caller auto-reborrows.
Pins the current marker count so it only moves down as the remaining cleanup buckets land. Fails if a new workaround is introduced without lowering the limit.
…dead Seq::buf filesystem_router: restore the dir copy in bust_dir_cache; CI on ubuntu-aarch64 hit a SIGSEGV in the reload()/Bun.build() race test with the borrowed slice. The recursive walk races the bundler thread on the process-global entry cache, so keep this slice owned. Comment updated to explain the race rather than 'reshaped for borrowck'. seq: the buf field became write-only after enqueue() now takes &out directly; delete the field, its Default initializer, and the assignment.
CI's globAllSources() follows the src/cli -> runtime/cli symlink, double- counting 19 markers (330 vs 311). Dedup via realpath like the sibling dead-code-escapes lint does.
e004aaa to
c411f31
Compare
There was a problem hiding this comment.
I didn't find bugs in this pass, but this is a 40-file behavior-preserving refactor spanning the bundler, installer, JSC bindings, server, shell, SQL, and paths — including 20 rewritten unsafe launders and one site that already segfaulted CI mid-review — so it's worth a human look before merge. Jarred's question on computeCrossChunkDependencies.rs:339 was answered but the thread is still open.
What was reviewed:
IOReadermem::take(&mut s.readers)vs the old.clone()— done/error are terminal, laterremove_readerre-entry is a no-op on the emptied list.repl::handle_entermem::takeofline_editor.buffer— every path previously called.clear()afterward, so draining up front is equivalent.Blob.rsswitch fromt.blob.store()toself.store()—t.blob = self.dupe(), same store.patchPackage/install_with_managerdropped.to_vec()—lockfileis a separate local, borrowck proves disjointness with&mut manager.
Extended reasoning...
Overview
This PR removes 49 reshaped for borrowck markers across 40 Rust files by replacing porting-era workarounds with idiomatic forms: entry-API instead of get+insert, mem::take+restore instead of .clone(), index+reborrow instead of .collect(), disjoint field borrows instead of raw-pointer launders, and scopeguard::guard(payload, ..) instead of sibling *mut copies. It also deletes the now-dead LazyPackageDestinationDir enum (85 lines) and adds a ratchet test pinning the marker count at 293.
Security risks
None identified. No new user-input parsing, no auth/crypto/TLS paths, no network-facing validation. The changes are internal borrow reshapes; the closest to a security surface is the fd-close scopeguard in jsc_hooks.rs, which is a straightforward payload-carrying guard replacing a raw-pointer capture.
Level of scrutiny
High. Each site is small, but the aggregate touches nearly every subsystem, and roughly half the sites replace unsafe code whose correctness the borrow checker could not previously verify. The PR already produced one aarch64 segfault (filesystem_router.rs, since reverted) during review, which is exactly the failure mode REVIEW.md warns about for "behavior-preserving" refactors. Several sites rely on non-local invariants (e.g. fold.rs get_or_put writing V::default() while p.new_symbol/module_scope_mut() run; server_body leaving state = Err on ?-return being unobservable because the caller panics/aborts). These were argued in-thread but a maintainer sign-off on the reasoning is appropriate.
Other factors
- Jarred-Sumner left an inline question on the
computeCrossChunkDependenciesmem::takethat was answered by robobun but not resolved. - Latest CI (#79970 on c411f31, retriggered as d6ba924) status not yet reported in-thread.
- Test coverage is via existing suites for each touched area plus the new ratchet lint; the refactor itself has no dedicated behavioral test (which is expected for a no-behavior-change refactor, but shifts confidence onto CI breadth).
- My two prior inline findings (dead
Seq::buffield;filesystem_routersegfault) were both addressed.
3 participants
Bucket B of the
reshaped for borrowckcleanup: 57 sites with simple local fixes (<10 lines each, no signature changes needed except one vestigial&mut, no new fields). Brings the count from 368 to 311.What changed
Entry-API double-lookups (6 sites):
get()+insert()collapsed into a singleentry()/get_or_put()where the map API supports it (fold.rsboth arms,snapshot.rs). Three sites (WorkspacePackageJSONCache,SavedSourceMap,runTasks:544) keep the two-step shape because an entry borrow cannot span the intervening read/parse/directory call; comment updated to say why.Allocation drops (31 sites):
&[Chunk]instead of&mut [Chunk]forappend_isolated_hashes_for_imported_chunks(read-only recursion):LinkerContextboth loops become plain iteration.collect():computeChunks,yarn,PackageManagerEnqueue:111mem::take+ restore instead of.clone()/.to_vec():computeCrossChunkDependencies,repl::handle_enter,IOReaderboth callbacks,server_bodysplit_at_mutinstead of prefix.to_vec():braces.rs×3&[u8]where the borrow was already disjoint:install_with_manager,patchPackage×2,bake/production×2,repl::handle_tab/arrow-keys,bundle_v2:2440SmallVecwhere a snapshot is required but tiny:h2_frame_parser,IOWriterdefines.rs,js_parser/lib.rs,dotenv/env_loader,seq.rs(also deletes the now-deadSeq::buffield),diff_match_patchBackRefinstead of.cloned():visit_stmt.rs(matches the existing pattern invisit/mod.rs)Three sites keep their allocation because it is required for correctness, with the comment rewritten to explain the hazard instead of
reshaped for borrowck:css/properties/flex.rs:879: basis is emitted twice (shorthand + longhand)PackageManagerEnqueue.rs:995:string_bytesmay reallocate underget_or_put_resolved_package_with_find_resultfilesystem_router.rsbust_dir_cache: the recursive walk races the bundler thread on the process-global entry cache (thereload() while Bun.build()test SIGSEGV'd on aarch64 without the copy)Raw-pointer launders rewritten to safe Rust (20 sites):
BunObject,publish_command,VirtualMachine:4313VirtualMachine:5527,PostgresSQLConnection:2407,MySQLConnection:1705,PostgresSQLConnection:1743scopeguard::guard(payload, ..)instead of sibling*mutcopy:js_bun_spawn_bindings×2,jsc_hooksusizeaddress-diff for offset math, derive the final pointer from the live slice:resolve_path×2match &mut reqinstead offrom_ref().cast_mut():server/modSmallVec<[u8; 64]>copy instead ofdetach_lifetimeon a package name:runTasks:347get/putinstead of holding a*mut u32across&mut self:bundle_v2:2135RawSlice:AsyncModuleself.store()instead of a*const [u8]round-trip:Blob.rsStoreStrarena alloc:lexer.rsinterpreter.rs,ConsoleObject.rsDead code: deleting the unused
destination_dirbinding inPackageInstaller.rsmadeLazyPackageDestinationDirdead (the remaining caller only ever constructs::Dir(fd)andget_dir()is infallible for it). The enum and its impl are removed and the single use site readsdestination_dir.fd()directly.Already done on main:
KEventWatcher.rs:130andWindowsWatcher.rs:525were handled by #35321.Test:
test/internal/source-lints/borrowck-reshape-markers.test.tspins the marker count at 311 so it only moves down.Why
Every site was code that paid a runtime cost (an extra heap allocation, a double hash lookup, or an
unsafeblock) purely to placate the borrow checker at porting time. Each prescribed rewrite here is the idiomatic-Rust shape the code would have been written in directly: disjoint borrows the compiler can already prove, or moving a value instead of cloning it. Net: the hot-path per-iteration allocations from the audit are gone, 20unsafelaunders are replaced with safe equivalents, and the marker comments are removed.Verification
cargo check --workspaceandbun run rust:check-all(all 10 targets) cleancargo clippyon every touched crate cleanbun bdbuildsrg "reshaped for borrowck" --type rust | wc -l: 368 → 311brace.test.ts,seq.test.ts,filesystem_router.test.ts,repl.test.ts, node/path,console-table.test.ts,env.test.ts,bun-patch.test.ts, snapshot tests, bundler edgecase/minify/splitting/html/cjs2esm, shell suite[review] gate passed · iteration 9 · 40 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 4 rejected · iteration 9
evidence per changed file