refactor(rust): remove resolved borrowck-reshape comments (bucket A, 256 sites) - #35360
refactor(rust): remove resolved borrowck-reshape comments (bucket A, 256 sites)#35360robobun wants to merge 8 commits into
Conversation
…256 sites) Removes 'reshaped for borrowck' comments at sites where the code is already in its optimal safe-Rust form and the comment is now dead weight. Bucket A1 (236 sites): idiomatic zero-cost patterns (scalar-copy, reborrow, split-borrow, take-replace, index-loop, clone-small, buffer-stash, own-transfer). Comment deleted; code unchanged. Bucket A2 (20 sites): raw *mut Self dispatch that is required for correctness (FFI callback provenance or reentrant JS that mutates/frees self). The 'reshaped for borrowck' note is replaced with a SAFETY comment stating the provenance/reentrancy invariant. One A1 site (INotifyWatcher) was already removed on main by #35321, so 256 of the 257 audited sites are touched here. Marker count drops 368 -> 112; the remaining 112 are buckets B/C (actual code fixes) tracked separately.
|
Updated 2:24 AM PT - Jul 24th, 2026
✅ @robobun, your commit a04a18595ea503a62b5765c36c39d22bd9c5d889 passed in 🧪 To try this PR locally: bunx bun-pr 35360That installs a local version of the PR into your bun-35360 --bun |
|
Status: all review threads resolved across 9 rounds. Diff is strictly comment-only (every changed line is
HEAD: a04a185. Follow-ups during review:
|
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
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 (8)
WalkthroughThis PR updates borrow scopes, raw-pointer safety documentation, callback handling, and selected edge-case control flow across bundling, parsing, installation, runtime, WebCore, SQL, downloads, and watcher code. ChangesBorrow-safety and runtime flow
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: 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/js_parser_jsc/Macro.rs`:
- Around line 222-224: Update MacroContext::call so the re-entrant
run_with_api_lock closure does not capture or retain the macro_ reference across
the JS call. Re-acquire the macro entry from self.macros inside the closure
after any re-entrant mutations, or use stable owned storage, ensuring no pointer
or reference can become invalid after get_or_put reallocates the map.
🪄 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: 08ecf721-9fca-4005-aec7-f2b5960902d4
📒 Files selected for processing (127)
src/bundler/Chunk.rssrc/bundler/LinkerContext.rssrc/bundler/ParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/bundle_v2.rssrc/bundler/entry_points.rssrc/bundler/linker_context/computeChunks.rssrc/bundler/linker_context/generateCodeForLazyExport.rssrc/bundler/options.rssrc/bundler/transpiler.rssrc/collections/linear_fifo.rssrc/css/css_parser.rssrc/css/properties/animation.rssrc/dotenv/env_loader.rssrc/exe_format/elf.rssrc/exe_format/macho.rssrc/glob/GlobWalker.rssrc/http/HeaderBuilder.rssrc/http/h2_client/PendingConnect.rssrc/http/h3_client/AltSvc.rssrc/http/lib.rssrc/http_jsc/websocket_client/WebSocketProxyTunnel.rssrc/install/PackageInstaller.rssrc/install/PackageManager/PackageManagerDirectories.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/PackageManagerLifecycle.rssrc/install/PackageManager/PackageManagerResolution.rssrc/install/PackageManager/install_with_manager.rssrc/install/PackageManager/patchPackage.rssrc/install/PackageManager/runTasks.rssrc/install/PackageManager/security_scanner.rssrc/install/PackageManager/updatePackageJSONAndInstall.rssrc/install/bin.rssrc/install/isolated_install/Installer.rssrc/install/lockfile/Tree.rssrc/install/lockfile/bun.lockb.rssrc/install/npm.rssrc/install/yarn.rssrc/io/PipeWriter.rssrc/js_parser/lexer.rssrc/js_parser/lower/lower_esm_exports_hmr.rssrc/js_parser/p.rssrc/js_parser/parse/parse_prefix.rssrc/js_parser/visit/mod.rssrc/js_parser/visit/visit_binary.rssrc/js_parser/visit/visit_stmt.rssrc/js_parser_jsc/Macro.rssrc/js_printer/lib.rssrc/jsc/AsyncModule.rssrc/jsc/ConsoleObject.rssrc/jsc/SavedSourceMap.rssrc/jsc/VirtualMachine.rssrc/jsc/event_loop.rssrc/jsc/ipc.rssrc/libarchive/lib.rssrc/md/helpers.rssrc/md/inlines.rssrc/parsers/yaml.rssrc/patch/lib.rssrc/paths/resolve_path.rssrc/resolver/lib.rssrc/resolver/resolver.rssrc/router/lib.rssrc/runtime/api/JSBundler.rssrc/runtime/api/MarkdownObject.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/bun/spawn/stdio.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/FrameworkRouter.rssrc/runtime/bake/dev_server/mod.rssrc/runtime/bake/production.rssrc/runtime/cli/audit_command.rssrc/runtime/cli/open.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/pm_view_command.rssrc/runtime/cli/repl.rssrc/runtime/cli/run_command.rssrc/runtime/cli/test/ChangedFilesFilter.rssrc/runtime/cli/test/Scanner.rssrc/runtime/cli/test/parallel/aggregate.rssrc/runtime/jsc_hooks.rssrc/runtime/node/types.rssrc/runtime/server/RequestContext.rssrc/runtime/server/mod.rssrc/runtime/shell/IOReader.rssrc/runtime/shell/IOWriter.rssrc/runtime/shell/builtin/mkdir.rssrc/runtime/shell/interpreter.rssrc/runtime/socket/Listener.rssrc/runtime/test_runner/Execution.rssrc/runtime/test_runner/Order.rssrc/runtime/test_runner/ScopeFunctions.rssrc/runtime/test_runner/bun_test.rssrc/runtime/test_runner/diff/diff_match_patch.rssrc/runtime/test_runner/diff/printDiff.rssrc/runtime/test_runner/expect.rssrc/runtime/test_runner/expect/toMatchSnapshot.rssrc/runtime/test_runner/expect/toThrowErrorMatchingInlineSnapshot.rssrc/runtime/test_runner/pretty_format.rssrc/runtime/timer/timer_object_internals.rssrc/runtime/valkey_jsc/js_valkey_functions.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/Response.rssrc/runtime/webcore/blob/copy_file.rssrc/runtime/webcore/blob/read_file.rssrc/runtime/webcore/blob/write_file.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/download_stream.rssrc/runtime/webcore/s3/simple_request.rssrc/semver/Version.rssrc/semver/lib.rssrc/shell_parser/parse.rssrc/sourcemap/InternalSourceMap.rssrc/sourcemap_jsc/JSSourceMap.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/MySQLRequestQueue.rssrc/sql_jsc/mysql/protocol/DecodeBinaryValue.rssrc/sql_jsc/postgres/DataCell.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/shared/SQLDataCell.rssrc/standalone_graph/StandaloneModuleGraph.rssrc/watcher/INotifyWatcher.rssrc/watcher/Watcher.rssrc/watcher/WindowsWatcher.rs
💤 Files with no reviewable changes (116)
- src/md/inlines.rs
- src/runtime/shell/builtin/mkdir.rs
- src/sourcemap_jsc/JSSourceMap.rs
- src/runtime/test_runner/expect/toMatchSnapshot.rs
- src/bundler/linker_context/computeChunks.rs
- src/bundler/Chunk.rs
- src/runtime/test_runner/expect/toThrowErrorMatchingInlineSnapshot.rs
- src/runtime/cli/pm_view_command.rs
- src/runtime/cli/test/ChangedFilesFilter.rs
- src/runtime/api/JSBundler.rs
- src/bundler/LinkerContext.rs
- src/runtime/shell/IOWriter.rs
- src/md/helpers.rs
- src/http/HeaderBuilder.rs
- src/install/PackageManager/PackageManagerResolution.rs
- src/sql_jsc/shared/SQLDataCell.rs
- src/runtime/cli/test/parallel/aggregate.rs
- src/js_parser/parse/parse_prefix.rs
- src/runtime/webcore/Response.rs
- src/install/isolated_install/Installer.rs
- src/http/h3_client/AltSvc.rs
- src/runtime/test_runner/diff/printDiff.rs
- src/runtime/cli/audit_command.rs
- src/bundler/linker_context/generateCodeForLazyExport.rs
- src/runtime/bake/FrameworkRouter.rs
- src/resolver/lib.rs
- src/runtime/test_runner/Execution.rs
- src/patch/lib.rs
- src/dotenv/env_loader.rs
- src/sql_jsc/mysql/MySQLRequestQueue.rs
- src/runtime/api/bun/spawn/stdio.rs
- src/jsc/ConsoleObject.rs
- src/sql_jsc/postgres/PostgresSQLConnection.rs
- src/runtime/valkey_jsc/js_valkey_functions.rs
- src/sql_jsc/mysql/MySQLConnection.rs
- src/runtime/test_runner/pretty_format.rs
- src/parsers/yaml.rs
- src/http/h2_client/PendingConnect.rs
- src/js_parser/visit/mod.rs
- src/sql_jsc/mysql/protocol/DecodeBinaryValue.rs
- src/css/properties/animation.rs
- src/runtime/webcore/fetch/FetchTasklet.rs
- src/runtime/test_runner/bun_test.rs
- src/exe_format/macho.rs
- src/js_parser/visit/visit_stmt.rs
- src/semver/Version.rs
- src/runtime/shell/IOReader.rs
- src/runtime/timer/timer_object_internals.rs
- src/runtime/cli/run_command.rs
- src/exe_format/elf.rs
- src/install/PackageManager/updatePackageJSONAndInstall.rs
- src/http_jsc/websocket_client/WebSocketProxyTunnel.rs
- src/runtime/cli/open.rs
- src/css/css_parser.rs
- src/jsc/AsyncModule.rs
- src/runtime/cli/pack_command.rs
- src/runtime/node/types.rs
- src/shell_parser/parse.rs
- src/bundler/options.rs
- src/runtime/bake/production.rs
- src/sourcemap/InternalSourceMap.rs
- src/js_parser/p.rs
- src/runtime/server/mod.rs
- src/watcher/Watcher.rs
- src/install/PackageManager/PackageManagerEnqueue.rs
- src/js_parser/visit/visit_binary.rs
- src/runtime/cli/repl.rs
- src/js_parser/lexer.rs
- src/jsc/SavedSourceMap.rs
- src/watcher/INotifyWatcher.rs
- src/runtime/webcore/Blob.rs
- src/sql_jsc/postgres/DataCell.rs
- src/install/bin.rs
- src/install/PackageManager/install_with_manager.rs
- src/semver/lib.rs
- src/install/PackageManager/security_scanner.rs
- src/watcher/WindowsWatcher.rs
- src/bundler/transpiler.rs
- src/runtime/api/bun/js_bun_spawn_bindings.rs
- src/install/PackageManager/PackageManagerDirectories.rs
- src/runtime/webcore/FileSink.rs
- src/router/lib.rs
- src/runtime/bake/dev_server/mod.rs
- src/libarchive/lib.rs
- src/install/PackageManager/patchPackage.rs
- src/install/PackageInstaller.rs
- src/runtime/socket/Listener.rs
- src/runtime/test_runner/Order.rs
- src/install/npm.rs
- src/js_parser/lower/lower_esm_exports_hmr.rs
- src/paths/resolve_path.rs
- src/glob/GlobWalker.rs
- src/collections/linear_fifo.rs
- src/runtime/test_runner/expect.rs
- src/runtime/webcore/s3/simple_request.rs
- src/runtime/test_runner/diff/diff_match_patch.rs
- src/install/PackageManager/runTasks.rs
- src/runtime/webcore/s3/download_stream.rs
- src/bundler/ParseTask.rs
- src/standalone_graph/StandaloneModuleGraph.rs
- src/jsc/ipc.rs
- src/install/yarn.rs
- src/install/lockfile/Tree.rs
- src/jsc/event_loop.rs
- src/runtime/shell/interpreter.rs
- src/runtime/cli/test/Scanner.rs
- src/http/lib.rs
- src/runtime/bake/DevServer.rs
- src/runtime/test_runner/ScopeFunctions.rs
- src/resolver/resolver.rs
- src/runtime/api/MarkdownObject.rs
- src/runtime/webcore/blob/copy_file.rs
- src/bundler/entry_points.rs
- src/runtime/webcore/Body.rs
- src/js_printer/lib.rs
- src/install/lockfile/bun.lockb.rs
The previous wording asserted that re-entry mutates self.macros, which contradicts the inner SAFETY on macro_. The bump pointer is provably stable (Option<Arena> inline in a heap-boxed MacroContext); restrict the comment to that. The macro_ pointer stability question is pre-existing in the code and tracked separately.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…drop 3 orphaned tails ParseTask.rs, IOReader.rs, IOWriter.rs: the deleted text documented invariants borrowck cannot enforce (state() is &self -> &mut State; StoreStr erases the contents lifetime). Restore the invariant portion as a plain comment without the 'reshaped for borrowck' framing. http/lib.rs, parsers/yaml.rs, bake/production.rs: the block-extent heuristic stopped one line early at an uppercase-start sentence that was semantically part of the same note, leaving a dangling tail. Delete those too.
The A2 replacement stacked a new SAFETY on top of the pre-existing one-line SAFETY for the same unsafe block. Merge them.
There was a problem hiding this comment.
All four rounds of feedback have been addressed (6bcc8c2, b346969, 8f63345) and this pass found nothing new. I'm still deferring rather than approving: 127 files is a lot of surface, and the 20 A2 sites introduce new // SAFETY: prose making specific provenance/reentrancy claims on unsafe blocks — worth a maintainer skim from someone with Zig→Rust port context, given the earlier passes turned up ~8 misbucketed/orphaned sites.
What was reviewed:
- Verified the four restored-invariant sites (ParseTask.rs StoreStr write-back, IOReader/IOWriter
state()re-derive) now keep the load-bearing text minus the "reshaped for borrowck" prefix. - Verified the three orphaned tail fragments (http/lib.rs, yaml.rs, production.rs) are gone and the bundle_v2.rs doubled SAFETY is merged.
- Spot-checked A2 SAFETY rewrites in h2_frame_parser.rs, PipeWriter.rs, jsc_hooks.rs, RequestContext.rs, read_file.rs/write_file.rs, VirtualMachine.rs — claims look consistent with surrounding code.
- Confirmed the diff is comment-only (no statement/expression changes).
Extended reasoning...
Overview
This PR is bucket A of the reshaped for borrowck comment sweep: 256 sites across 127 Rust files where the Zig→Rust port left breadcrumb comments explaining borrow-checker-driven restructuring. A1 (236 sites) deletes the comment outright because the resulting code is idiomatic Rust; A2 (20 sites) replaces the breadcrumb with a // SAFETY: comment because the raw-pointer pattern is a correctness requirement (FFI callback context, reentrant JS), not a workaround. Net: +59/−557, comment-only, bun bd and rust:check-all pass per the description.
Security risks
None. No code paths change; no user-facing behavior, parsing, auth, or crypto is touched. The only risk class is documentation correctness: an inaccurate SAFETY comment could mislead a future refactor into introducing UB, and a deleted invariant comment could let a future edit remove a load-bearing write-back. Both were the subject of earlier review rounds and the flagged instances were fixed.
Level of scrutiny
Medium. The mechanical A1 deletions are low-risk (borrowck would reject any refactor that violates them). The A2 rewrites deserve real scrutiny because they now assert specific memory-safety invariants (write provenance, dispatch_depth deferral, reentrancy) on unsafe blocks — REVIEW.md treats SAFETY prose as reviewed word-for-word. My earlier passes caught one A2 site (Macro.rs) whose new claim contradicted the adjacent SAFETY and was factually wrong about self.bump; that was tightened in b346969. I spot-checked the remaining A2 sites this round and they read correctly, but I have not exhaustively re-derived all 20.
Other factors
- Two prior claude[bot] review rounds surfaced 8 issues (4 misbucketed invariant comments, 3 orphaned tail fragments, 1 doubled SAFETY header, 1 contradictory SAFETY claim). All were addressed in follow-up commits and I verified each fix in the current diff.
- The author also ran a heuristic sweep for the orphaned-tail and doubled-SAFETY patterns across the whole diff after the fixes.
- No CODEOWNERS gating that I'm aware of; no outstanding human reviewer comments.
- The ~3% initial misbucket rate on a 256-site automated sweep is the main reason I'd like a maintainer with port-history context to skim the A2 set before merge, rather than shadow-approving on volume.
PackageManagerLifecycle.rs tick_lifecycle_scripts: ctx is forwarded opaquely through tick_once and never dereferenced by any reachable task callback (Js arm discards it; every Mini-arm AnyTaskWithExtraContext callback takes _: *mut ()). Drop the SAFETY header (there is no unsafe here) and state that fact instead of the aliasing hazard copied from the audit data. Macro.rs: same-box re-entry into MacroContext::call is prevented, not tolerated. MacroModeGuard sets target=BunMacro for the closure's duration, which sets is_macro_runtime on the transpiler path, and both visitor call sites are gated by !is_macro_runtime. Bun.Transpiler / bundler workers allocate a fresh MacroContext box. State that as the invariant.
|
This diff is strictly comment-only: every changed line is a There is no behavior to regression-test; any test would pass identically with and without the src/ diff. |
…eManagerLifecycle) jsc_hooks.rs auto_tick / auto_tick_active: the new SAFETY sat directly on top of the pre-existing per-line SAFETY; the reentrancy rationale is already in the fn-level # Safety doc. Drop the new lines. VirtualMachine.rs ref_counted_string_with_was_new: clear_ref_string discards ctx with _; nothing dereferences self_ctx, so provenance is irrelevant and there is no unsafe here. Drop the note (A1). PackageManagerLifecycle.rs sleep(): the 'see tick_lifecycle_scripts' cross-ref now points at a comment that says no callback derefs ctx, which reads as contradicted by sleep()'s own is_done callback. Drop the cross-ref prefix; sleep()'s remaining text is self-contained.
There was a problem hiding this comment.
The two open threads from my previous pass (PackageManagerLifecycle.rs sleep() cross-reference and VirtualMachine.rs:3778 SAFETY header) were both addressed by 9f71d47 — the diff now drops the "see tick_lifecycle_scripts —" prefix and deletes the A2 note at ref_counted_string_with_was_new outright. Only the two new nits above remain.
Extended reasoning...
9f71d47's commit message names all three sites (jsc_hooks, VirtualMachine, PackageManagerLifecycle), but the author's inline reply only marked the jsc_hooks thread resolved, so the other two still show as open. Verified against the current diff: sleep() at PackageManagerLifecycle.rs:244 now reads "is_done callback reborrows..." without the stale cross-reference, and VirtualMachine.rs:3778 is a clean two-line deletion with no SAFETY header on the safe ptr::from_mut line. Noting this so a human reviewer doesn't count four open items when only the two new inline nits (both comment-only, no runtime effect) are outstanding.
…esize invariant at 3 Slice<T> sites jsc_hooks.rs load_preloads: the new SAFETY is followed by a blank line and guards no unsafe block; the fn-level # Safety doc and per-line SAFETYs already cover every deref. Same treatment as auto_tick/auto_tick_active in 9f71d47. LinkerContext.rs:560, bundle_v2.rs:1900, bundle_v2.rs:2059: Slice<T> is a value-type raw-pointer snapshot (slice() takes &self), so a mid-loop append would compile and dangle. Keep the no-resize invariant as a one-liner; the cross-reference at bundle_v2.rs:5113 now points at restored text.
…om A2 SAFETY
The second clause ('the hoist before &mut this.io_poll is the correct
ordering') is borrowck narration: ptr::from_mut(this) after &mut this.io_poll
fails to compile (E0499), so the ordering is not a soundness invariant the
reader needs to uphold. Keep only the provenance clause, matching
PipeWriter.rs:508/759, and make the two files consistent.
There was a problem hiding this comment.
No new findings after a04a185; all nine threads from the five earlier review rounds are addressed and resolved. Deferring to a human for the final sign-off given the breadth (127 files) and that the A2 sites rewrite memory-safety SAFETY text.
What was reviewed:
- A1 deletions spot-checked for dropped invariants beyond borrowck narration — the
Slice<T>slab-no-resize andstate()re-derive cases restored in 6bcc8c2/7a590b5 were the only ones with a caller-side soundness clause; remaining.slice()sites (input_files,Watcher.rs,Tree.rs) had no such clause to lose. - A2 SAFETY rewrites checked for false invariants and doubled/floating headers —
VirtualMachine.rs,jsc_hooks.rs×3,Macro.rs,PackageManagerLifecycle.rs,read_file/write_fileall fixed; remaining sites (PipeWriter×2,bundle_v2×2,RequestContext,h2_frame_parser×5) each guard a realunsafederef and state a provenance/reentrancy fact I could verify against the callback. - Inbound cross-references into rewritten sites —
sleep()→tick_lifecycle_scriptsandbundle_v2.rs:5113→find_reachable_fileswere the two; both now land on restored/self-contained text.
Extended reasoning...
Overview
Comment-only sweep across 127 Rust files removing 236 reshaped for borrowck breadcrumbs (bucket A1) and rewording ~20 raw-*mut Self sites into proper // SAFETY: provenance/reentrancy notes (bucket A2). Net +59 −557; bun bd and rust:check-all pass; no executable line changed.
Security risks
None. Comment-only; no code path, validation, or unsafe block is added, removed, or reordered.
Level of scrutiny
Higher than a typical comment PR because (a) REVIEW.md treats SAFETY comments and caller-side invariant notes as reviewed word-for-word, and (b) A1 deletions can silently drop a soundness invariant that borrowck cannot enforce (the Slice<T> raw-pointer-snapshot and state() interior-mut re-derive cases). Five earlier review rounds each surfaced a new instance of exactly that class (doubled SAFETY headers → 8f63345; orphaned tail fragments and dropped state() invariants → 6bcc8c2; false A2 claims at VirtualMachine/Macro/PackageManagerLifecycle → 0f6cba7; missed doubled headers in jsc_hooks → 9f71d47; floating SAFETY + dropped slab-no-resize invariants → 7a590b5; residual A1 narration inside A2 SAFETY at read_file/write_file → a04a185). Each was promptly fixed and the thread resolved.
Other factors
The current bug-hunt pass found nothing new. I re-checked the two systemic risks the earlier rounds converged on: (1) remaining MultiArrayList::slice() call sites in the diff whose deleted comment might have carried a "does not resize" clause — none did beyond the three already restored; (2) remaining A2 SAFETY additions — each now sits on a real reentrancy/FFI-callback pointer whose consumer I traced (e.g. h2_frame_parser's five stream_ptr derefs are guarded by dispatch_depth deferring frees, matching the new comment). Given the breadth and that convergence took five rounds, a human skim of the final diff — particularly the ~15 surviving A2 rewrites — seems worth the small cost before merge.
2 participants
Part of the
reshaped for borrowckcomment sweep (audit data onfarm/2a32eccb/borrowck-audit). This PR handles bucket A: the 257 sites where the code is already in optimal safe-Rust form and only the comment needs to change.What changed
A1 (236 sites): idiomatic zero-heap-cost patterns (scalar-copy, reborrow, split-borrow, take-replace, index-loop, clone-small, buffer-stash, own-transfer). The comment is deleted; code is unchanged.
A2 (20 sites): raw
*mut Selfdispatch that is required for correctness: either the pointer is stored as an FFI callback context that may later freeself, or the body runs a reentrant JS callback that mutatesself. A&self-derived pointer here would carrySharedReadOnlyprovenance and make the later write/dealloc UB (seesrc/CLAUDE.md"Pointer provenance at FFI boundaries"). Thereshaped for borrowcknote is replaced with a// SAFETY:comment stating the provenance/reentrancy invariant so theunsafeis self-documenting.One A1 site (
src/watcher/INotifyWatcher.rs:529) was already removed on main by #35321, so this PR touches 256 of the 257 audited sites.Why
These comments were breadcrumbs from the Zig→Rust port marking spots where code was restructured to satisfy the borrow checker. For the A1 sites the resulting code is just ordinary idiomatic Rust; the comment adds nothing a Rust reader doesn't already see. For the A2 sites the comment was actively misleading: it suggested the raw pointer was a borrow-checker workaround, when it is in fact a correctness requirement that must not be "fixed" into a safe borrow.
Verification
Net:
127 files changed, 59 insertions(+), 557 deletions(-). No code semantics change.