镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Don't re-drop partially-moved sub-places (fix #121, #122) - #155

Draft
coord-e wants to merge 3 commits into
mainfrom
claude/pr-154-mergeable-vt1l0f
Draft

coord-e wants to merge 3 commits into
mainfrom
claude/pr-154-mergeable-vt1l0f

Conversation

@coord-e

@coord-e coord-e commented Jun 28, 2026 •

Copy link
Copy Markdown
Owner

Fixes #121 and #122. Supersedes #124.

Dropping an aggregate after moving out a field can resolve that field's mutable-borrow prophecy twice. Contradictory resolutions can make false assertions verify. The drop must resolve only the parts still owned at that point on that execution path: a field moved in one branch remains owned in the other, and assigning a value back restores its drop obligation.

Implementation

  • Track partial-move ownership after statements and on control-flow edges. Moving an owned place transfers its drop obligation; assigning to that place, a containing field, or the whole local restores ownership. Call destinations become initialized on the successful return edge. Reference moves retain the existing reborrow behavior.
  • Specialize analysis MIR blocks when incoming ownership states differ, so each drop has a path-specific set of exclusions. Reuse existing block versions for loops and discard ownership history for locals no longer needed downstream. Projected assignments retain the state of untouched fields. Preserve loop-invariant associations across block specialization.
  • Keep DropSet { drops, except }: drop targets are whole locals, and exclusions are moved-out MIR places. Include a parent local in the drop candidates after a direct field assignment. Whole-local move suppression remains keyed by the move's location.
  • Use the existing MIR-place elaboration for exclusions, preserving real Box dereferences and inserting only the synthetic ownership dereferences. Match those paths against the refined type's drop traversal, including enum downcasts, to skip moved-out subtrees while resolving still-owned siblings.

Validation

  • All 29 partial-move UI tests pass with thrust-fptprove-wrapper. They cover moves into locals and calls, siblings, conditional moves, field/parent/local reinitialization, enum payloads, loops with invariants, call-return initialization, sequential conditionals, and moves through Box values and boxed fields. Pass/fail pairs check both successful verification and rejection of false assertions.
  • With thrust-fptprove-wrapper and a 180-second default solver limit, 409 UI tests pass; the two known timeout cases are excluded. The pass and fail variants of slice_split_first_mut_loop.rs previously timed out even with a 180-second limit. Their generated SMT constraints remain byte-for-byte identical before the ownership-tracking changes, after those changes, and after the Box fix. These two cases remain unverified; the entire 411-test suite is not reported as green.
  • Unit tests (3), Cargo integration tests (2), and doc tests (2) pass. cargo fmt --all -- --check, cargo clippy -- -D warnings, and git diff --check pass.
  • Three reviewers reviewed the entire branch diff and rechecked the fixes for loop-invariant handling, unnecessary block multiplication, enum exclusions, and Box dereferences. No additional findings remained in their final reviews.

Existing limitation

Writing through a field after reinitialization, for example let t = s.0; s.0 = t; *s.0.0 = 10;, can still panic with deref unbound var. This reproduces both before and after these changes and is separate from restoring the drop obligation.

@coord-e
coord-e force-pushed the claude/pr-154-mergeable-vt1l0f branch 5 times, most recently from 12b6c5f to 43aaebb Compare July 2, 2026 04:54
@coord-e
coord-e force-pushed the claude/pr-154-mergeable-vt1l0f branch 2 times, most recently from e5b8321 to 4c88826 Compare July 8, 2026 10:55
@coord-e coord-e changed the title Track drop points by MIR Place, splitting partially-moved locals (fix #121, #122) Don't re-drop partially-moved sub-places (fix #121, #122) Jul 8, 2026
@coord-e
coord-e force-pushed the claude/pr-154-mergeable-vt1l0f branch 2 times, most recently from cd98f92 to a87b28e Compare August 9, 2026 02:34
@coord-e
coord-e requested a balanced review from Copilot August 9, 2026 02:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes prophecy double-resolution when partially moved locals are dropped.

Changes:

  • Tracks moved sub-places as drop exceptions.
  • Skips excepted paths during drop-formula generation.
  • Adds pass/fail regression coverage for local, call, and sibling cases.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/analyze/basic_block/drop_point.rs Collects moves and builds exception-aware drop sets.
src/analyze/basic_block.rs Applies drop sets during block analysis.
src/analyze/local_def.rs Propagates lifetime-aware drop-point types.
src/refine/env.rs Skips moved paths while generating drop formulas.
tests/ui/pass/partial_move_drop.rs Tests a valid local partial move.
tests/ui/fail/partial_move_drop.rs Rejects the unsound local variant.
tests/ui/pass/partial_move_field_call.rs Tests a valid field move into a call.
tests/ui/fail/partial_move_field_call.rs Rejects the unsound call variant.
tests/ui/pass/partial_move_sibling.rs Tests correct sibling prophecy resolution.
tests/ui/fail/partial_move_sibling.rs Guards against double-resolution with a sibling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/analyze/basic_block/drop_point.rs Outdated
Comment thread src/refine/env.rs Outdated
Comment thread tests/ui/fail/partial_move_field_call.rs Outdated
Comment thread src/refine/env.rs Outdated
claude and others added 2 commits September 28, 2026 18:47
A local with a partial field move (e.g. `move (_2.0)`) was still dropped
wholesale, so dropping it walked into the moved-out sub-place and resolved
the `&mut` prophecy it owns a second time. The two resolutions contradict,
making the clause body unsatisfiable, after which any assertion -- including
false ones -- "verifies" (#121, #122).

`Moves::collect` gathers all non-reference `move`d operands in one body
traversal: whole-local moves (keyed by location, where the local also dies)
and, keyed by parent local, the partial field moves. `DropSet { drops, except }`
carries, per drop point, the whole locals to drop plus the moved-out sub-places
to skip. A dying local is dropped whole, but its partial-move sub-places are
passed to the drop as `except`: the drop walk resolves the still-owned siblings
and skips the moved-out subtrees (resolved at the move destination), so the fix
is both sound and complete. Drop targets stay whole locals; the closure
environment restored by `RustCallVisitor` reaches its drop through a
projection-less temporary.

`Env::dropping_formula_for_term` threads the drop-walk `path` and `except`
alongside the type/term walk and returns early on any subtree matching an
excepted path. The moved-out places are elaborated to the walk's form via
`Env::elaborated_path`, which inserts the `own`-box `Deref`s the type
elaboration introduces (mut/reborrowed locals, and every tuple field) so the
comparison lines up exactly -- without this a partially-moved local that gets
box-elaborated (e.g. when a sibling is mutated through) would not be skipped,
and the false assertion would verify vacuously.

Adds regression tests: the two original shapes (#121, #122), plus a
partially-moved local with a still-owned sibling (completeness) and a soundness
guard where the moved part is used and the sibling is reborrowed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A3zd4nCdz5atbgNuqXEJTc
@coord-e
coord-e force-pushed the claude/pr-154-mergeable-vt1l0f branch from a87b28e to 45a0927 Compare September 28, 2026 10:51

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unsoundness: partially-moved locals are still implicitly dropped, resolving prophecies of moved-out &mut borrows

3 participants