Enable -Zpolonius=next and remove NLL borrow-checker workarounds - #36951
Enable -Zpolonius=next and remove NLL borrow-checker workarounds#36951robobun wants to merge 15 commits into
Conversation
Requires building with -Zpolonius=next. Deletes 7 unsafe raw-pointer reborrows in bun_which and the open command's editor detection, converts the editor helpers from bool plus out-param to returned Option borrows, and replaces two contains_key-then-get double lookups with single lookups.
Wires the polonius alpha borrow checker into the ninja cargo edge and the generated .cargo/config.toml (including new sections for the windows-msvc triples, which previously had none) so bun bd, CI, plain cargo check, rust:check-all, and rust-analyzer all accept the same code. The pinned toolchain is already nightly. Measured cost: ~2% on a cold cargo check of the workspace.
Rewrites 25 borrow-checker workaround sites to their natural form: single lookups instead of contains_key-then-get, direct early returns of borrows instead of index/len round-trips, plain reborrows instead of raw-pointer reborrows. Several sites (linear_fifo, env_loader, patchPackage, Chunk, fmt) now require the polonius borrow checker enabled in the previous commit; the rest simply lost their scaffolding.
Adds cases for Bun.which PATH-segment iteration (hit in a later segment after misses), path.win32.toNamespacedPath branch coverage (long-path fall-through, device root, UNC conversion, dot-dot resolution), and inspect.table column discovery across heterogeneous rows.
WalkthroughChangesThe PR enables Polonius in Rust builds and simplifies slice, buffer, cache, path, lookup, bundler, and DNS handling. Tests cover table column discovery, PATH traversal, and Windows namespaced paths. Borrow simplification and runtime cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/js/bun/console/bun-inspect-table.test.ts`:
- Around line 21-23: Update the table test data to discover columns in a clearly
non-alphabetical first-seen order, and configure the relevant
inspection/snapshot calls to preserve that discovery order instead of sorting.
Keep the assertions as a regression test that distinguishes insertion order from
lexical sorting, including the affected snapshot expectations.
In `@test/js/bun/util/which.test.ts`:
- Around line 274-290: Replace the external chmod shell invocation in the test
around which("prog_in_third", { PATH }) with chmodSync(join(d,
"third/prog_in_third"), 0o755), retaining the existing !isWindows guard and
ensuring the required built-in filesystem import is available.
🪄 Autofix
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: a35a3cd7-b490-459d-9444-e0eb409b25c3
⛔ Files ignored due to path filters (1)
test/js/bun/console/__snapshots__/bun-inspect-table.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (30)
scripts/build/cargo-config.tsscripts/build/rust.tssrc/bun_core/fmt.rssrc/bundler/Chunk.rssrc/bundler/transpiler.rssrc/collections/array_hash_map.rssrc/collections/linear_fifo.rssrc/dotenv/env_loader.rssrc/exe_format/elf.rssrc/install/PackageManager/WorkspacePackageJSONCache.rssrc/install/PackageManager/patchPackage.rssrc/install/isolated_install.rssrc/install/isolated_install/Hardlinker.rssrc/jsc/ConsoleObject.rssrc/runtime/api/bun/spawn/stdio.rssrc/runtime/bake/production.rssrc/runtime/cli/open.rssrc/runtime/cli/upgrade_command.rssrc/runtime/dns_jsc/dns.rssrc/runtime/node/node_fs.rssrc/runtime/node/path.rssrc/runtime/node/types.rssrc/runtime/socket/Listener.rssrc/runtime/test_runner/snapshot.rssrc/runtime/webcore/Body.rssrc/sql_jsc/postgres/DataCell.rssrc/which/lib.rstest/js/bun/console/bun-inspect-table.test.tstest/js/bun/util/which.test.tstest/js/node/path/to-namespaced-path.test.js
💤 Files with no reviewable changes (2)
- src/runtime/api/bun/spawn/stdio.rs
- src/runtime/webcore/Body.rs
Make the table column-discovery input non-alphabetical so the snapshot distinguishes first-seen order from lexical sorting, add an unsorted variant, and use chmodSync instead of shelling out.
|
Addressed both review comments in 2851dd2: the table column-discovery input now uses a non-alphabetical first-seen order (d, b, a, c, e) with an additional unsorted snapshot so insertion order is distinguishable from lexical sorting, and the which test uses chmodSync instead of spawning chmod. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Status: merged main into the branch (9254173) to clear conflicts with #37052 in src/bun_core/fmt.rs and src/which/lib.rs. Resolution keeps this PR's shape at both sites and uses the highway helpers main now requires (strings::last_index_of_char / index_of_char_usize in format_ip, strings::tokenize in which_win). The one red lane in build 89156 (AsyncLocalStorage-tracking LSAN leak in RSA keygen) was a main break, fixed by #36986, so it should be gone on the rerun. Verified after the merge: cargo check --workspace under -Zpolonius=next is clean (host target, plus bun_core/bun_which for x86_64-pc-windows-msvc since which_win is cfg(windows)), and test/internal/source-lints passes (75/75). Diff against main is unchanged otherwise: 31 files, +243/-312. Waiting on CI and a maintainer. |
There was a problem hiding this comment.
Thanks — both earlier comment nits (the dead grep polonius hint and the stale Windows-msvc docstring) are addressed in 850981a. I didn't find any bugs this pass, but this adopts an alpha borrow checker as a hard workspace dependency and rewrites control flow across 27 files in critical subsystems, so it needs a maintainer sign-off.
Checked: the dns.rs reordering (map value now written after uv_poll_init_socket) — value type is a raw pointer so swap_remove on the failure path is still a no-op drop; isolated_install.rs members borrow of scc_stack — truncate(start) runs after the last use; elf.rs strtab hoist — the mutable write only happens on the return path where strtab is dead.
Extended reasoning...
Overview
Enables -Zpolonius=next in both the ninja cargo edge (CARGO_ENCODED_RUSTFLAGS in rust.ts) and every generated .cargo/config.toml [target.*] section (cargo-config.ts, including new Windows-msvc rustflags-only sections). Then removes ~27 files' worth of NLL problem-case-3 workarounds: seven unsafe raw-pointer reborrow blocks (bun_which, open.rs, Hardlinker.rs), contains-then-get double lookups (snapshot.rs, WorkspacePackageJSONCache.rs), index/len round-trips instead of returned borrows (fmt.rs, path.rs, node_fs.rs, ConsoleObject.rs, linear_fifo.rs, Listener.rs, types.rs, elf.rs, DataCell.rs), a bool-plus-out-param signature reshaped to Option<&[u8]> (open.rs), an owned-Vec return collapsed to a borrowed &ZStr (patchPackage.rs), and dropped clones/Box::from in production.rs, isolated_install.rs, Chunk.rs, transpiler.rs, upgrade_command.rs, stdio.rs, Body.rs, env_loader.rs. Adds tests for the touched paths (console.table column discovery, which() PATH-segment iteration, path.win32.toNamespacedPath branches).
Security risks
None identified. The removed unsafe blocks were all raw-pointer reborrows working around NLL imprecision, not soundness holes; their replacements are straightforward safe borrows accepted by polonius. No auth/crypto/permission code touched.
Level of scrutiny
High. This is a toolchain-level architectural decision — the workspace now hard-requires an alpha borrow checker (linear_fifo.rs and others no longer compile under stock NLL), so every future nightly bump carries the risk of polonius acceptance changing at the margin. Combined with control-flow rewrites across 27 files spanning bundler, package manager, DNS resolver, node:fs/node:path, socket listener, ELF writer, and postgres — each change is small and mechanical in isolation, but the aggregate is well past the threshold for auto-approval.
Other factors
- My two prior inline nits (dead
grep polonius in src/pointer; stale "Windows-msvc targets are omitted" docstring) were both addressed in commit 850981a. - I spot-checked the three reorderings with actual behavioral surface: (1)
dns.rs—*poll_entry.value_ptr = new_pollmoved after theuv_poll_init_socketcheck. On the failure path,swap_remove(&fd)now removes a default-valued slot instead of a slot holdingnew_poll, but the value type is*mut UvDnsPoll(Copy, no Drop), so net state is identical. (2)isolated_install.rs—membersnow borrowsscc_stack[start..]instead of cloning; verifiedscc_stack.truncate(start)(line 1609) runs after the last use ofmembers(line 1605), so no aliasing. (3)elf.rs—strtabhoisted out of the loop; the&mut self.datawrite only occurs on thereturnpath wherestrtabis no longer live. - CodeRabbit's two minor test-quality comments were addressed in 2851dd2.
- Tests were added, but the PR notes the platform-specific ones (which.test.ts on Windows, to-namespaced-path.test.js) defer to CI.
…s-cleanups # Conflicts: # src/bun_core/fmt.rs # src/which/lib.rs
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/dns_jsc/dns.rs (1)
4833-4844: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a regression test for failed poll initialization.
Force
uv_poll_init_socketto fail on Windows. Assert thatpollscontains no entry forfdafteron_dns_socket_statereturns. This protects the delayed-publication invariant.As per coding guidelines, “Every behavioral change must include an automated regression test in the same change.”
🤖 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/runtime/dns_jsc/dns.rs` around lines 4833 - 4844, Add a Windows-specific regression test covering on_dns_socket_state when uv_poll_init_socket fails: force initialization to return an error, invoke the handler for a file descriptor, and assert polls has no entry for that descriptor afterward. Keep the test focused on the delayed-publication invariant and place it with the existing DNS poll tests.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.
Inline comments:
In `@scripts/build/rust.ts`:
- Around line 413-421: Add or extend a hermetic test covering
generateCargoConfig() and cargoBuildInvocation() for Windows MSVC and
non-Windows configurations. Assert Windows target sections contain ["-Z",
"polonius=next"], decode CARGO_ENCODED_RUSTFLAGS using \x1f and assert
-Zpolonius=next, and preserve assertions for existing non-Windows linker flags;
run it with bun bd test <test-file>.
---
Outside diff comments:
In `@src/runtime/dns_jsc/dns.rs`:
- Around line 4833-4844: Add a Windows-specific regression test covering
on_dns_socket_state when uv_poll_init_socket fails: force initialization to
return an error, invoke the handler for a file descriptor, and assert polls has
no entry for that descriptor afterward. Keep the test focused on the
delayed-publication invariant and place it with the existing DNS poll tests.
🪄 Autofix
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: d476e2ae-09b5-4167-8f2d-35932b96e5bc
⛔ Files ignored due to path filters (1)
test/js/bun/console/__snapshots__/bun-inspect-table.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (30)
scripts/build/cargo-config.tsscripts/build/rust.tssrc/bun_core/fmt.rssrc/bundler/Chunk.rssrc/bundler/transpiler.rssrc/collections/array_hash_map.rssrc/collections/linear_fifo.rssrc/dotenv/env_loader.rssrc/exe_format/elf.rssrc/install/PackageManager/WorkspacePackageJSONCache.rssrc/install/PackageManager/patchPackage.rssrc/install/isolated_install.rssrc/install/isolated_install/Hardlinker.rssrc/jsc/ConsoleObject.rssrc/runtime/api/bun/spawn/stdio.rssrc/runtime/bake/production.rssrc/runtime/cli/open.rssrc/runtime/cli/upgrade_command.rssrc/runtime/dns_jsc/dns.rssrc/runtime/node/node_fs.rssrc/runtime/node/path.rssrc/runtime/node/types.rssrc/runtime/socket/Listener.rssrc/runtime/test_runner/snapshot.rssrc/runtime/webcore/Body.rssrc/sql_jsc/postgres/DataCell.rssrc/which/lib.rstest/js/bun/console/bun-inspect-table.test.tstest/js/bun/util/which.test.tstest/js/node/path/to-namespaced-path.test.js
💤 Files with no reviewable changes (2)
- src/runtime/webcore/Body.rs
- src/runtime/api/bun/spawn/stdio.rs
There was a problem hiding this comment.
I reviewed this PR and found no bugs (the two doc-pointer nits from the earlier pass were addressed in 850981a). Because this makes the entire workspace hard-depend on an alpha nightly compiler feature (-Zpolonius=next) and refactors borrow patterns across ~27 subsystems, a maintainer should sign off on the toolchain-policy decision.
What was reviewed:
- Verified the
dns.rsreorder (publishnew_pollafter init succeeds) is behavior-preserving on the failure path — value type is a raw pointer,swap_removestill cleans up the just-inserted slot. - Verified
isolated_install.rsmembersborrow ofscc_stack[start..]ends beforescc_stack.truncate(start)at line 1613. - Checked
elf.rshoistedstrtabborrow: dead before thewrite_u64_le(&mut self.data[..])on the.interpmatch path, and the write region (section header table) does not overlap the string table. - Confirmed the build-config changes keep
.cargo/config.tomlandCARGO_ENCODED_RUSTFLAGSin sync across all targets including the new windows-msvc rustflags-only sections.
Extended reasoning...
Overview
This PR enables -Zpolonius=next (the alpha Polonius borrow checker) in both the ninja cargo edge (scripts/build/rust.ts) and every generated [target.*] section of .cargo/config.toml (scripts/build/cargo-config.ts), then removes the NLL "problem case 3" workarounds it makes unnecessary across 27 source files: raw-pointer reborrows in bun_which and open.rs, contains_key-then-get double lookups in snapshot.rs and WorkspacePackageJSONCache.rs, index/len round-trips in node:path, fmt.rs, linear_fifo.rs, ConsoleObject.rs, elf.rs, and others, plus a bool+out-param → Option<&[u8]> signature change in open.rs. Net: 31 files, +243/−312. Tests are added for Bun.which PATH-segment iteration, path.win32.toNamespacedPath branch coverage, and inspect.table column discovery order.
Security risks
None identified. Seven unsafe raw-pointer reborrow blocks are removed (net safety improvement). No new unsafe, no auth/crypto/permission surface touched. The changes are borrow-shape refactors that Polonius proves sound; each site was spot-checked for behavior preservation.
Level of scrutiny
High. While each individual site is a mechanical de-scaffolding, the aggregate is a workspace-wide toolchain-policy change: after this PR the codebase no longer compiles under the stock NLL borrow checker (linear_fifo.rs and others hard-require the flag). That couples every future rust-toolchain.toml bump to Polonius alpha stability. The PR description acknowledges this ("future nightly bumps may change acceptance at the margin") and the ~2% compile-time cost. This is exactly the kind of infrastructure decision a maintainer should explicitly approve rather than have land via automated review.
Other factors
- All prior review feedback (CodeRabbit's two test nits, my two stale-comment nits) was addressed and resolved.
- The
dns.rschange is the only one with a visible statement reorder (publish-after-init instead of publish-before-init); I traced it and confirmed the map value type is*mut UvDnsPoll(Copy, no Drop), soswap_removeon the failure path behaves identically either way. patchPackage.rschanges the helper return type from ownedVec<u8>to borrowed&ZStrand moves the.to_vec()to the four call sites — behavior-preserving, one fewer allocation on the borrowed path is irrelevant since callers immediately clone.- Test coverage was added for the three user-visible code paths most affected (
whichPATH iteration,toNamespacedPathbranches,inspect.tablecolumn discovery), and the PR description reports passing test suites across the touched areas pluscargo mirionbun_collections. - The build-config half now emits windows-msvc
[target.*]sections for the first time; I verified the docstring and inline comment were updated to match after my earlier nit.
Removing the flag from rust.ts already fails the build (bun_collections no longer compiles under the stock borrow checker), but nothing guarded the generated .cargo/config.toml, which plain cargo check, rust:check-all and rust-analyzer rely on. Covers every target triple, including the rustflags-only windows-msvc sections.
|
Follow-up on the latest review round:
|
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. CodeRabbit's request for regression coverage of both flag paths looks addressed by the new test/internal/rust-polonius-flag.test.ts (cf16871) — that thread can likely be resolved.
Given this is a workspace-wide adoption of the alpha -Zpolonius=next flag plus a 27-file refactor across many subsystems (including removal of seven unsafe reborrow blocks and a write-ordering change in dns.rs), a maintainer sign-off still makes sense here.
What was reviewed: the build-script changes (rust.ts / cargo-config.ts) for flag consistency across ninja and .cargo/config.toml; the unsafe removals in which/lib.rs, open.rs, and Hardlinker.rs for behavior preservation; the dns.rs reorder (map slot now written only after uv_poll_init_socket succeeds — net map state unchanged on both paths); and the isolated_install.rs members borrow-vs-clone change for any intervening scc_stack mutation (none found).
Extended reasoning...
Overview
This PR enables -Zpolonius=next for every rustc invocation (both the ninja cargo edge via CARGO_ENCODED_RUSTFLAGS and every [target.*] section in the generated .cargo/config.toml, including newly-emitted windows-msvc sections) and removes ~27 files' worth of NLL borrow-checker workarounds that the flag makes unnecessary. The workarounds removed span raw-pointer reborrows under unsafe, contains-then-get double lookups, index/len round-trips, and one bool-plus-out-param signature. Four test files gain coverage (which PATH-segment iteration, path.win32.toNamespacedPath branches, console.table column discovery order, and a new hermetic test that pins the flag in both build-config sources).
Security risks
None identified. No auth/crypto/permissions code is touched. The unsafe blocks being removed were raw-pointer reborrows that existed solely to work around NLL problem-case-3; their removal reduces the unsafe surface rather than adding to it. The elf.rs change hoists an immutable strtab slice out of a loop but the write path still returns immediately after — no new aliasing.
Level of scrutiny
High. This is not a mechanical change: it commits the entire workspace to an alpha borrow-checker whose acceptance boundary can shift on future nightly bumps (the PR description acknowledges this). Several of the 27 source edits are pure scaffolding deletion, but others are structural — open.rs changes two function signatures from bool + out-param to Option<…>, patchPackage.rs changes two helpers from returning owned Vec<u8> to borrowed &ZStr (with .to_vec() moved to the four call sites), dns.rs reorders a map write relative to a fallible uv_poll_init_socket call, and isolated_install.rs replaces a defensive .to_vec() copy with a live borrow into scc_stack. Each of these is individually reviewable and I found no correctness issues, but the aggregate scope plus the ~2% compile-time cost and the toolchain-risk tradeoff is a maintainer decision.
Other factors
- My two prior nits (stale docstring and dead grep hint) were fixed in 850981a.
- CodeRabbit's 2026-08-10 request for hermetic test coverage of
generateCargoConfig()/cargoBuildInvocation()is answered bytest/internal/rust-polonius-flag.test.ts(cf16871): it decodesCARGO_ENCODED_RUSTFLAGSon\\x1f, asserts-Zpolonius=nextfor five build configs, parses the generated TOML per-triple, checks windows-msvc sections carry"-Z", "polonius=next"withoutlink-arg=-fuse-ld=lld, and checks non-windows triples keep the lld link-arg. That thread is unresolved in the UI but substantively addressed. - The author explicitly noted they are waiting on a maintainer.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it flips the whole workspace onto an alpha rustc feature (-Zpolonius=next) and reshapes borrow patterns across ~27 source files in many subsystems, a maintainer sign-off on the toolchain decision is still warranted.
What was reviewed:
- Each removed workaround traced for behavior preservation — e.g.
elf.rsstrtab hoist (mutating path returns before the borrow is reused),dns.rspoll-entry write reorder (failure path stillswap_removes the slot),patchPackage.rs&ZStrreturn (all callers immediately.to_vec()). - Build-config changes: flag pushed in both
CARGO_ENCODED_RUSTFLAGSand every generated[target.*]section including new windows-msvc entries; docstring/comment now match. - The
by_path_for_editor/by_fallbacksignature change from bool+out-param toOption<&[u8]>— all three call sites indetect_editorupdated consistently.
Extended reasoning...
Overview
This PR enables -Zpolonius=next (the alpha Polonius borrow checker) for every rustc invocation — both the ninja CARGO_ENCODED_RUSTFLAGS path and the generated .cargo/config.toml per-target sections — and then deletes the NLL "problem case 3" workarounds that flag makes unnecessary across 27 Rust source files. The workarounds removed fall into a few classes: raw-pointer reborrows around loop-carried &mut (which/lib.rs, open.rs, Hardlinker.rs), contains_key-then-get double lookups (WorkspacePackageJSONCache, snapshot.rs), index/len round-trips instead of returned borrows (fmt.rs, path.rs, types.rs, Listener.rs, node_fs.rs, ConsoleObject.rs), and a bool+out-param signature collapsed to Option<&T> (open.rs). Three test files add coverage for the touched code paths (which PATH-segment iteration, console.table column discovery, path.win32.toNamespacedPath branches).
Security risks
None identified. Seven unsafe blocks are deleted (raw-pointer reborrows in which_win, which_for_spawn, and open.rs), which is a net reduction in unsafe surface. No new unsafe is introduced. The changes are refactors of borrow structure, not of validation, parsing, or trust boundaries.
Level of scrutiny
High. This is not a mechanical change: it makes the entire ~200-crate workspace hard-depend on an alpha nightly rustc feature (per the PR description, "future nightly bumps may change acceptance at the margin"), and it touches control flow in memory-sensitive code across the bundler, installer, ELF writer, DNS resolver, and node:path/fs. Each site is individually small and the bug-hunting pass found nothing, but the aggregate is a toolchain policy decision that a maintainer should explicitly accept — the author's own status update says "Waiting on CI and a maintainer."
Other factors
A maintainer (alii) has already engaged on the PR (requested deletion of the build-flag unit test, done in f4bf3ec), so there is active human review in progress. My two prior nit comments (stale grep hint in rust.ts, stale docstring in cargo-config.ts) were addressed in 850981a. All CodeRabbit threads are resolved. The dns.rs reorder was specifically justified by the author (Copy raw pointer, failure path swap_removes either way) and I agree the observable state is identical. Given the scope and the explicit toolchain dependency being introduced, deferring to the maintainer already in the loop is the right call.
|
Updated 8:35 PM PT - Aug 10th, 2026
✅ @robobun, your commit e43da769652d64c5538591870f5f4078b827c589 passed in 🧪 To try this PR locally: bunx bun-pr 36951That installs a local version of the PR into your bun-36951 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it makes -Zpolonius=next (an alpha borrow checker) a hard compile-time dependency for the whole workspace and reshapes control flow across ~27 Rust files, a maintainer sign-off on the toolchain commitment is still worthwhile.
What was reviewed
- Each refactor site checked for behavior preservation vs. the deleted workaround (e.g.
elf.rsstrtab hoist stays valid becauseread_shdronly readsself.data;dns.rsreordered slot write is observationally identical on the failure path sinceswap_removeruns either way). - The seven deleted
unsaferaw-pointer reborrows inwhich/lib.rsandcli/open.rs— replacements are straight&mut *bufreborrows with the same one-live-borrow-per-iteration invariant. cargo-config.ts/rust.ts: flag reaches every rustc invocation (ninja, plain cargo, rust-analyzer, windows-msvc targets); the two prior doc-comment nits from this bot were addressed in 850981a.
Extended reasoning...
Overview
This PR enables -Zpolonius=next in every Rust compilation path (ninja's CARGO_ENCODED_RUSTFLAGS in scripts/build/rust.ts and every [target.*] section of the generated .cargo/config.toml, including new rustflags-only sections for the windows-msvc triples), then deletes the NLL-problem-case-3 workarounds it makes unnecessary across 27 source files in src/. The workspace no longer compiles under the stock borrow checker after this change. Net diff is roughly +243/-312; three new tests cover console.table column discovery, Bun.which PATH-segment iteration, and path.win32.toNamespacedPath branch coverage.
Security risks
None identified. The change removes seven unsafe raw-pointer reborrow blocks (a net safety improvement) and does not touch auth, crypto, TLS, or input-validation paths. The refactors are borrow-shape changes, not logic changes; each site was checked against its predecessor for identical observable behavior.
Level of scrutiny
High — not because any individual site is risky, but because (a) the change spans many unrelated subsystems (bundler, package manager, node:fs/path, DNS poll, ELF writer, sockets, postgres DataCell, test-runner snapshots), so a subtle behavior drift in one wouldn't be caught by the others' tests, and (b) adopting an alpha nightly borrow checker as a hard workspace dependency is a toolchain-policy decision: future rust-toolchain.toml bumps may change what polonius accepts at the margin, and someone needs to own that risk. The PR description acknowledges this and the ~2% cold-check compile-time cost.
Other factors
- My two prior inline nits (stale grep hint in
rust.ts, contradictory docstring incargo-config.ts) were fixed in 850981a and verified in the current diff. - A maintainer (alii) has already engaged (requested deletion of the build-flag unit test, done in f4bf3ec).
- The bug-hunting system found nothing this run. Candidate concerns I ruled out: the
elf.rsstrtab hoist looked like it might alias a mutable borrow inside the loop, butread_shdrtakes&selfand only readsself.data, so the shared borrow is fine and the section-header write happens after the loop returns; thedns.rsreorder leaves an uninitializedvalue_ptrslot only until the very next line writes it, and the failure path'sswap_remove(&fd)clears the slot regardless of whether it was written, so no dangling entry escapes. - CI build #91766 is in flight for the latest merge-from-main; the earlier build's one red lane was a known main break (#36986) unrelated to this PR.
Polonius alpha is the default borrow checker on nightlies from 2026-08-06 on (rust-lang/rust#159343), so the workspace now builds on the pinned toolchain with or without -Zpolonius=next. The flag stays in rust.ts and the generated .cargo/config.toml so a future toolchain bump cannot silently change which borrow checker builds bun if upstream toggles the nightly default while the alpha bakes. The new nightly bundles LLVM 23, which trips the rustc-no-regular-lto-summary workaround's re-check threshold. That fix-up has been unreachable since every LTO platform moved to ThinLTO (rustLtoLinkInputs returned early whenever cfg.lto was set, and crossLangLto implies lto), so take the cleanup path the entry prescribes: delete rust-lto-fix-cli.ts, the rust_lto_fix rule and rustLtoLinkInputs, unwrap its call sites, drop the llvm-tools component, and remove the entry. rust-lld and llvm-objcopy come from the rustc component and the clang toolchain respectively, so nothing else used llvm-tools. The four GitHub workflows that pin RUSTUP_TOOLCHAIN are kept in sync with rust-toolchain.toml.
robobun
changed the title
|
Scope change, per discussion with @alii: this PR now also bumps the toolchain (750fa2d). Polonius alpha is the nightly default since nightly-2026-08-06 (rust-lang/rust#159343), so rather than enabling it on the old pin, the branch moves |
… fix-up" This reverts commit 750fa2d.
robobun
changed the title
|
Reverted the toolchain bump (750fa2d) in 24ac600. It breaks the windows-x64 cross build at link time (build 91851): Cause: the cross-language LTO link on Windows is driven by rust-lld from the Rust toolchain, which the bump moves to LLVM 23, while the prebuilt WebKit LTO bitcode is still produced by the LLVM 21 clang. main at this branch's merge-base (c63dc64, same WebKit) links fine on the same lane with the 2026-07-20 nightly's rust-lld, and the LTO fix-up code the bump removed had an explicit windows early-return, so the skew is the only delta. That is a toolchain-bump problem independent of this PR, so it is better handled in its own PR (cherry-pick 750fa2d to restart from it). The polonius-is-default-now observation from its commit message still holds and is the reason the flag here is low risk; the flag stays for now since the pinned 2026-07-20 nightly still needs it. Branch is back to the shape that was green on every lane, plus the main merge. PR title restored to match. |
There was a problem hiding this comment.
I reviewed this revision and the bug-hunting pass found no issues. Given the scope — a workspace-wide borrow-checker switch plus ~27 rewritten sites across bundler/install/DNS/node-compat and the removal of seven unsafe reborrow blocks — a maintainer sign-off is still warranted.
Checked: elf.rs strtab hoist (shared borrow dead before the &mut self.data write on the .interp path); dns.rs poll-entry write reorder (slot is swap_removed on the failure path either way, so post-state is identical); path.rs to_namespaced_path_windows_t now indexes resolved instead of buf — resolve_windows_t returns &buf[0..len] so bytes match; which_win / open.rs reborrow deletions return immediately on hit so only one &mut buf is ever live.
Extended reasoning...
Overview
This PR adds -Zpolonius=next to every rustc invocation (ninja edge in rust.ts and every [target.*] section in the generated .cargo/config.toml, including new rustflags-only sections for windows-msvc triples) and deletes the NLL workarounds that flag makes unnecessary: seven unsafe raw-pointer reborrow blocks (which/lib.rs, cli/open.rs), double-lookup patterns (snapshot.rs, WorkspacePackageJSONCache.rs), index/len round-trips (fmt.rs, path.rs, linear_fifo.rs, elf.rs, node_fs.rs, Listener.rs, types.rs, DataCell.rs), a bool-plus-out-param signature (open.rs), an owning Vec<u8> return that becomes &ZStr (patchPackage.rs), and assorted dead scaffolding. Tests are added for Bun.which PATH-segment iteration, path.win32.toNamespacedPath branch coverage, and console.table column-discovery order. A toolchain bump to nightly-2026-08-11 was attempted (750fa2d) and reverted (24ac600) after a Windows cross-LTO link failure; the branch is back to enabling the flag on the existing pin.
Security risks
None identified. The change removes unsafe blocks rather than adding them; the deleted reborrows were already sound-by-construction (each returned immediately on hit so only one &mut was live), and their safe replacements are strictly narrower. No auth, crypto, network parsing, or untrusted-input paths are touched in a way that changes validation.
Level of scrutiny
High. This flips the borrow checker for the entire workspace to a nightly alpha that upstream explicitly reserves the right to toggle back off (the PR description itself cites two P-high unsoundness reports filed within days of the nightly default flip). It touches 27 source files across many subsystems, changes build-script behavior for every target, and removes unsafe code whose correctness argument was local and now depends on the new checker's acceptance. Each individual site is a small mechanical simplification, but the aggregate is an infrastructure decision a maintainer should own.
Other factors
- A maintainer (alii) is already engaged (requested and got the
rust-polonius-flag.test.tsdeletion, discussed the toolchain-bump scope). - My two prior inline nits (stale grep hint in
rust.ts, stale docstring incargo-config.ts) were addressed in 850981a. - The PR description states the previous revision (same src changes, old nightly + flag) was green across the full CI matrix; the current head (e43da76) is a CI retrigger after the toolchain-bump revert.
- The
dns.rsreorder and thepatchPackage.rsreturn-type change are the only sites where control flow / ownership visibly moved rather than just collapsing an index round-trip; both were traced and are behaviorally equivalent (the dns slot is aCopyraw pointer swap-removed on failure either way;patchPackagecallers now.to_vec()at the four return sites instead of inside the helper).
| for other in chunks.iter() { | ||
| if let Content::Javascript(js) = &other.content { | ||
| if other.entry_point.is_entry_point() | ||
| && other.entry_point.entry_point_id() == entry_point_id | ||
| { | ||
| let css_chunk_indices = &js.css_chunks[..]; | ||
| if !css_chunk_indices.is_empty() { | ||
| let idx = css_chunk_indices[0] as usize; | ||
| return Some(&mut chunks[idx]); | ||
| } | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This code compiles on stable without Polonius. I also tried to test the refactor on the first version of this code in the repo and it also compiles on stable. Do you know what was the original motivation for this workaround? Did the code look differently when it was introduced (maybe that point is not in git history anymore)?
3 participants
Enables the polonius borrow checker (
-Zpolonius=next) for all Rust compilation and deletes the workarounds it makes unnecessary.Context
Polonius alpha became the default borrow checker on nightly starting with nightly-2026-08-06 (rust-lang/rust#159343, via compiler-team MCP 1015), so bun's next toolchain bump switches borrow checkers regardless. On the currently pinned nightly-2026-07-20 it is opt-in, which is what the flag does here. The flag is worth keeping even after a bump: upstream has said it will flip the default back off if problems turn up while the alpha bakes (two soundness reports of the "accepts code it should reject" kind are open, rust-lang/rust#160669 and #160670), and a toolchain bump should not silently change which borrow checker builds bun.
A bump to nightly-2026-08-11 was tried on this branch (750fa2d) and reverted (24ac600): it moves rust-lld to LLVM 23 while the prebuilt WebKit LTO bitcode is still produced by LLVM 21, and the windows-x64 cross-language LTO link then fails with undefined WTF symbols (build 91851). That is independent of this change and belongs in its own PR.
What polonius buys
It accepts NLL "problem case 3": a borrow returned or escaping on one control-flow path no longer blocks use or mutation of the borrowed place on the other paths. The workspace had accumulated workarounds for exactly this: unsafe raw-pointer reborrows, contains_key-then-get double lookups, index/len round-trips instead of returning a borrow, and a bool-plus-out-param helper signature.
Changes
scripts/build/rust.ts:-Zpolonius=nextin the ninja cargo edge (CARGO_ENCODED_RUSTFLAGS), sobun bdand CI builds use it.scripts/build/cargo-config.ts: the flag in every generated[target.*]rustflags section, including new rustflags-only sections for the windows-msvc triples (previously omitted entirely), so plaincargo check,rust:check-all,cargo miri, clippy and rust-analyzer accept the same code the build does.src: 25 files. Seven unsafe raw-pointer reborrow blocks deleted (bun_which, theopencommand's editor detection), editor helpers returnOption<&[u8]>instead of bool plus out-param, double hash lookups collapsed to single lookups (test_runner snapshots,WorkspacePackageJSONCache), and about twenty more sites lose their index/len/copy scaffolding (node:path, env loader,ConsoleObject,linear_fifo,patchPackage, elf writer,node_fsreaddir, postgresDataCell, and others). Several of these require polonius (bun_collections,bun_dotenvandbun_installfail under stock NLL); the rest turned out to be removable under NLL too and simply lost dead scaffolding.Bun.whichacross PATH segments,path.win32.toNamespacedPathbranches,console.tablecolumn discovery order). No existing expectations changed.Cost and risk
cargo check --workspacecold: 55.7s without the flag, 56.6s with it (8 cores), about 2%.Verification
cargo check --workspaceclean under the flag on x86_64-linux, x86_64-windows-msvc and aarch64-darwin targets; without the flag it fails as expected (linear_fifo.rsE0499 first, thenenv_loader.rs,patchPackage.rs,WorkspacePackageJSONCache.rsas each crate is unblocked).bun bdbuild green; standalone--compilesmoke test (exercises the elf writer change).cargo test -p bun_collections(34 pass, coverslinear_fifo);cargo miri test -p bun_collectionsclean.bun bd test: node/path suite, cli/run/env, web/console, fs readdir-recursive-leak, web/fetch/body, cli/install/bun-patch, cli/install/isolated-install all pass.no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/which.test.ts test/js/node/path/to-namespaced-path.test.js