Fix unsoundness from closure capturing &mut in a struct/tuple field (#41)#92
Open
coord-e wants to merge 1 commit into
Open
Fix unsoundness from closure capturing &mut in a struct/tuple field (#41)#92coord-e wants to merge 1 commit into
coord-e wants to merge 1 commit into
Conversation
365e389 to
115a8e0
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a soundness bug in Thrust’s implicit drop-point analysis where moved (owned) locals could still be treated as dropped at the move site, producing contradictory environments when closures capturing &mut are stored in tuple/struct fields.
Changes:
- Add tracking of locals whose ownership is transferred via
moveand exclude them from implicit drop points (for non-reference types). - Add UI regression tests covering both the accepted (“pass”) and rejected (“fail”) variants of the closure-in-field mutation scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/analyze/basic_block/drop_point.rs |
Excludes moved (owned) locals from implicit drop sets to avoid premature drops at move sites. |
tests/ui/pass/closure_field_mut.rs |
Regression test ensuring closure mutation through a tuple field is tracked and accepted when asserted correctly. |
tests/ui/fail/closure_field_mut.rs |
Regression test ensuring incorrect post-call assertions are rejected (avoids vacuous acceptance via inconsistent env). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #41. A closure capturing `&mut` stored in a tuple/struct field made thrust derive a contradictory environment and unsoundly accept programs that can panic. The drop-point analysis treats a local's last use as its drop site. For a value moved into an aggregate (e.g. a closure moved into a tuple), this dropped the moved-out temporary at the move site, prematurely resolving the captured reference's prophecy (final == current) to its construction value. The later call mutating through the closure then contradicted that, making the environment inconsistent and any following assertion vacuously "safe". Owned (non-reference) locals whose ownership is transferred away by a move are now excluded from the drop set: their drop obligation belongs to the destination. Moved references are left untouched since ReborrowVisitor/ RustCallVisitor turn them into reborrows, so the source remains live. https://claude.ai/code/session_01HHar2z2xTNwffns5SF7wRe
acdbb24 to
afadb7a
Compare
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 #41. A closure capturing
&mutstored in a tuple/struct field made thrust derive a contradictory environment and unsoundly accept programs that can panic (the issue'sassert!(false)was proven unreachable).Root cause
The drop-point analysis (
src/analyze/basic_block/drop_point.rs) treats a local's last use as its drop site, based purely on liveness. When a value is moved into an aggregate (e.g. a closure moved into a tuple via_2 = (move _3, 0)), the moved-out temporary_3was still dropped at the move site. Dropping a value containing a&mutassertsfinal == currenton the captured reference, which prematurely resolves its prophecy to the construction-time value (0). The subsequent(s.0)()call mutates through the closure (final = 1), contradicting the pinned0. An inconsistent environment makes any following assertion vacuously "safe" — the unsoundness.Fix
Exclude owned (non-reference) locals whose ownership is transferred away by a
movefrom the drop set — their drop obligation belongs to the destination. Moved references are deliberately left untouched, becauseReborrowVisitor/RustCallVisitorrewrite them into reborrows, so the source local stays live and must still be dropped. (Restricting to non-reference types is essential: suppressing all moved-local drops regresses cases likeclosure_mut_0, where the original MIRmoves a&mutthat thrust reborrows.)https://claude.ai/code/session_01HHar2z2xTNwffns5SF7wRe
Generated by Claude Code