Capture partial reborrows of &mut aggregate fields in mut_locals#127
Merged
Conversation
Reborrowing a `&mut`-typed field out of an aggregate (tuple/struct) parameter panicked with "deref unbound var" in `Env::locate_place`. Such an aggregate parameter is an immutable local whose type is a tuple, so it took the `immut_bind` path and was stored without flow bindings. The reborrow elaboration then failed to resolve the field projection to the inner `&mut`. Detect mutable references reachable through aggregate and pointer projections via a new `Type::contains_mut` and route such parameters through the flow-decomposing `mut_bind` path, matching how a top-level `&mut` parameter is already handled. Fixes #125 https://claude.ai/code/session_01MQbByXbDZ8FxkhRra4nPfN
Follow the repo convention of pairing each ui test in pass/ and fail/. Drive the test from main so verification has a concrete assertion to check, and add the fail counterpart asserting the negated condition.
42b206f to
bc72f58
Compare
#125 panicked ("deref unbound var") when reborrowing a &mut field out of an aggregate parameter. Root cause: the partial reborrow of `w.0` was never detected, so the base local was neither box-elaborated nor flow- decomposed, and resolving the field projection during reborrow failed. A &mut field read out of an aggregate appears as `Operand::Copy(_1.0)` (the pointer value is copied), but `mut_locals`' operand rule only matched `Operand::Move`, so the base local `_1` was never marked. ReborrowVisitor reborrows any &mut-typed operand place regardless of Copy/Move, so the two were inconsistent. Match Copy as well, narrowing the type test from `is_mutable_ptr()` to a &mut reference: `is_mutable_ptr()` also covers `*mut` raw pointers, which are Copy and are never reborrowed, so widening to Copy without the reference restriction would over-mark every raw-pointer copy. This makes the type-based `contains_mut` workaround unnecessary; revert it and remove the helper. Keep the pass/fail regression tests. Fixes #125 https://claude.ai/code/session_01MQbByXbDZ8FxkhRra4nPfN
&mut aggregate fields in mut_locals
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a panic in the MIR analysis pipeline when reborrowing a &mut-typed field out of an aggregate (tuple/struct) parameter by ensuring the base local is recognized as (partially) mutably borrowed in mut_locals.
Changes:
- Extend
mut_localsoperand handling to treat bothOperand::MoveandOperand::Copyof&mutreferences as indicating a mutable borrow. - Narrow the mutability detection to
&mutreferences (excluding*mutraw pointers) to avoid over-marking locals. - Add UI pass/fail regression tests covering reborrowing a
&mutfield from an aggregate parameter.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/ui/pass/reborrow_mut_field_of_aggregate_param.rs | Adds a passing regression case ensuring aggregate-field &mut reborrow no longer panics and behaves as expected. |
| tests/ui/fail/reborrow_mut_field_of_aggregate_param.rs | Adds a failing regression case ensuring the analysis reports Unsat (rather than panicking) for an inconsistent postcondition. |
| src/analyze/local_def.rs | Updates mut_locals to detect &mut reborrows through Operand::Copy as well as Operand::Move, aligned with ReborrowVisitor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #125.
Analyzing a function that takes an aggregate parameter (tuple/struct) and reborrows a
&mut-typed field out of it panicked withderef unbound varinEnv::locate_place:Root cause
The base local of the aggregate (
_1) was never recognized as being (partially) mutably borrowed, so it was neither box-elaborated nor flow-decomposed, and resolving the_1.0projection during reborrow elaboration failed.reassign_local_mutabilitiesderives local mutability frommut_locals, whose operand rule only matchedOperand::Move:But a
&mutfield read out of an aggregate lowers toOperand::Copy, so_1was never marked. This is despiteReborrowVisitor::visit_operandreborrowing any&mut-typed operand place (operand.place(), Copy or Move) — the two passes were inconsistent.Why a
&mutread isOperand::CopyOperand::CopyvsOperand::Moveis not about whether the type isCopy— it records whether the source place stays valid after the read. Per the rustc doc comment onOperand::Copy: "Before drop elaboration, the type of the place must beCopy. After drop elaboration there is no such requirement." The MIR thrust consumes (optimized_mir) is post-drop-elaboration, so a non-Copy&mutis legally read viacopy. rustc emitscopywhenever the source is dead/reborrowed, which is essentially every&mutread:So
Move-only matched a case that almost never occurs for&mut. (Top-level&mutlocals still worked becausebind_localhas a separaterty.ty.is_mut()type-check; a tuple isn'tis_mut(), so the aggregate field fell through both.)Fix
Match
Copyas well, and narrow the type test fromis_mutable_ptr()to a&mutreference:The narrowing is required:
is_mutable_ptr()also matches*mut Traw pointers, which genuinely areCopyand are read bycopyconstantly, but are never reborrowed. The oldMove-only gate excluded them for free (a*mutisCopy, so it never appeared asMove); addingCopywithout restricting to references would over-mark every raw-pointer copy. This now aligns exactly with whatReborrowVisitorreborrows.This makes the type-based
contains_mutworkaround from earlier revisions unnecessary; it was reverted and removed.Tests
Added pass/fail UI regression tests (
reborrow_mut_field_of_aggregate_param). Full suite green (234 passed, 0 failed) with z3 4.13.0 and the COAR docker image, matching CI.References
Operanddoc comment (Copy/Move semantics): https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/enum.Operand.htmlMove): What about: use-after-move and (maybe) use-after-drop rust-lang/unsafe-code-guidelines#188🤖 Generated with Claude Code