Skip to content

Commit 43aaebb

Browse files
committed
Fix double-drop of partially-moved locals
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. A dying local is dropped whole, but its partial-move sub-places are passed to the drop as `except`: the drop walk skips those subtrees (they are resolved at the move destination) and resolves the still-owned siblings, so the fix is both sound and complete. `Env::dropping_assumption` gains the `except` set and `Path::same_place`. The comparison peels `Deref`, because the drop walk inserts a `Deref` for every `own`-box the type elaboration introduces (mut locals, and every tuple field) while the moved-out places come straight from MIR without them; moved-out places never contain a real deref, so peeling cannot conflate distinct places. 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> Claude-Session: https://claude.ai/code/session_01NU1g7aHMxcSEZgeKProDr1
1 parent 981f62d commit 43aaebb

10 files changed

Lines changed: 254 additions & 74 deletions

File tree

src/analyze/basic_block.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use std::borrow::Cow;
2-
use std::collections::{HashMap, HashSet};
2+
use std::collections::HashMap;
33

44
use rustc_hir::def::DefKind;
55
use rustc_index::IndexVec;
@@ -1008,10 +1008,11 @@ impl<'tcx, 'ctx> Analyzer<'tcx, 'ctx> {
10081008
}
10091009
}
10101010

1011-
fn drop_places(&mut self, places: HashSet<mir::Place<'tcx>>) {
1012-
for place in places {
1013-
tracing::info!(?place, "implicitly dropped");
1014-
self.env.drop_place(place);
1011+
fn drop_places(&mut self, set: drop_point::DropSet<'tcx>) {
1012+
let except: Vec<mir::Place<'tcx>> = set.except.into_iter().collect();
1013+
for place in set.drops {
1014+
tracing::info!(?place, ?except, "implicitly dropped");
1015+
self.env.drop_place(place, &except);
10151016
}
10161017
}
10171018

Lines changed: 114 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1,95 +1,134 @@
11
use std::collections::{HashMap, HashSet};
22

33
use rustc_index::bit_set::DenseBitSet;
4-
use rustc_middle::mir::{self, BasicBlock, Body, Local};
4+
use rustc_middle::mir::{self, BasicBlock, Body, Local, Location};
5+
use rustc_middle::ty::TyCtxt;
56
use rustc_mir_dataflow::{impls::MaybeLiveLocals, ResultsCursor};
67

7-
/// Implicit-drop targets. A drop is a `Place`; a whole-local drop is just a
8-
/// place with an empty projection (semantically a bare `Local`).
8+
/// A set of implicit-drop targets: `drops` are places to drop; `except` are
9+
/// moved-out sub-places to skip when a drop walks into them.
10+
#[derive(Debug, Clone, Default)]
11+
pub struct DropSet<'tcx> {
12+
pub drops: HashSet<mir::Place<'tcx>>,
13+
pub except: HashSet<mir::Place<'tcx>>,
14+
}
15+
16+
impl<'tcx> DropSet<'tcx> {
17+
pub fn insert(&mut self, place: mir::Place<'tcx>) {
18+
self.drops.insert(place);
19+
}
20+
21+
fn extend(&mut self, other: &DropSet<'tcx>) {
22+
self.drops.extend(other.drops.iter().copied());
23+
self.except.extend(other.except.iter().copied());
24+
}
25+
}
26+
927
#[derive(Debug, Clone, Default)]
1028
pub struct DropPoints<'tcx> {
11-
pub before_statements: HashSet<mir::Place<'tcx>>,
12-
after_statements: Vec<HashSet<mir::Place<'tcx>>>,
13-
after_terminator: HashMap<BasicBlock, HashSet<mir::Place<'tcx>>>,
14-
/// Drops scheduled after the terminator regardless of the target, in
15-
/// addition to the liveness-derived sets above.
16-
after_terminator_extra: HashSet<mir::Place<'tcx>>,
29+
pub before_statements: DropSet<'tcx>,
30+
after_statements: Vec<DropSet<'tcx>>,
31+
after_terminator: HashMap<BasicBlock, DropSet<'tcx>>,
32+
after_terminator_extra: DropSet<'tcx>,
1733
}
1834

1935
impl DropPoints<'_> {
20-
pub fn builder<'mir, 'tcx>(body: &'mir Body<'tcx>) -> DropPointsBuilder<'mir, 'tcx> {
36+
pub fn builder<'mir, 'tcx>(
37+
tcx: TyCtxt<'tcx>,
38+
body: &'mir Body<'tcx>,
39+
) -> DropPointsBuilder<'mir, 'tcx> {
2140
DropPointsBuilder {
2241
body,
2342
bb_ins_cache: HashMap::new(),
43+
moves: Moves::collect(tcx, body),
2444
}
2545
}
2646
}
2747

2848
impl<'tcx> DropPoints<'tcx> {
29-
pub fn after_statement(&self, statement_index: usize) -> HashSet<mir::Place<'tcx>> {
49+
pub fn after_statement(&self, statement_index: usize) -> DropSet<'tcx> {
3050
self.after_statements[statement_index].clone()
3151
}
3252

3353
pub fn insert_after_terminator(&mut self, place: mir::Place<'tcx>) {
3454
self.after_terminator_extra.insert(place);
3555
}
3656

37-
pub fn after_terminator(&self, target: &BasicBlock) -> HashSet<mir::Place<'tcx>> {
57+
pub fn after_terminator(&self, target: &BasicBlock) -> DropSet<'tcx> {
3858
let mut set = self.after_terminator[target].clone();
39-
set.extend(self.after_statements.last().unwrap().iter().copied());
40-
set.extend(self.after_terminator_extra.iter().copied());
59+
set.extend(self.after_statements.last().unwrap());
60+
set.extend(&self.after_terminator_extra);
4161
set
4262
}
4363
}
4464

45-
#[derive(Debug, Clone)]
65+
#[derive(Clone)]
4666
pub struct DropPointsBuilder<'mir, 'tcx> {
4767
body: &'mir Body<'tcx>,
4868
bb_ins_cache: HashMap<BasicBlock, DenseBitSet<Local>>,
69+
moves: Moves<'tcx>,
4970
}
5071

51-
/// Locals whose ownership is fully transferred away by the statement (or
52-
/// terminator) at `statement_index`. Such a local is left uninitialized, so its
53-
/// drop obligation (including resolving any mutable-borrow prophecies it owns)
54-
/// moves to the destination and it must not be dropped at the move site.
72+
/// The `move`d operands of a body, excluding reference-typed places.
5573
///
56-
/// Only owned (non-reference) operands are reported: `move`d references are
57-
/// turned into reborrows by `ReborrowVisitor`/`RustCallVisitor`, so the source
58-
/// local remains live and must still be dropped.
59-
fn moved_locals<'tcx>(
60-
body: &Body<'tcx>,
61-
bb: BasicBlock,
62-
statement_index: usize,
63-
) -> DenseBitSet<Local> {
64-
struct Visitor<'a, 'tcx> {
65-
body: &'a Body<'tcx>,
66-
locals: DenseBitSet<Local>,
67-
}
68-
impl<'tcx> mir::visit::Visitor<'tcx> for Visitor<'_, 'tcx> {
69-
fn visit_operand(&mut self, operand: &mir::Operand<'tcx>, _location: mir::Location) {
70-
if let mir::Operand::Move(place) = operand {
71-
if place.projection.is_empty() && !self.body.local_decls[place.local].ty.is_ref() {
72-
self.locals.insert(place.local);
74+
/// `move`d references are turned into reborrows by
75+
/// `ReborrowVisitor`/`RustCallVisitor`, so the source still owns its prophecy
76+
/// and must be dropped normally; they are therefore not recorded here.
77+
///
78+
/// A whole-local move leaves the local uninitialized, transferring its entire
79+
/// drop obligation (including resolving any mutable-borrow prophecies it owns)
80+
/// to the destination, so it must not be dropped at the move site — where the
81+
/// local also becomes dead, hence the per-location keying. A partial (projected)
82+
/// field move transfers only a sub-place, but the parent stays live until its
83+
/// remaining parts die later; dropping it wholesale at that point would walk
84+
/// into the moved-out sub-place and resolve its `&mut` prophecy a second time,
85+
/// so a partially-moved local is excluded from dropping entirely.
86+
#[derive(Clone, Default)]
87+
struct Moves<'tcx> {
88+
/// Whole-local moves, keyed by the location performing the move.
89+
whole: HashMap<Location, DenseBitSet<Local>>,
90+
/// Partial field moves, keyed by the parent local.
91+
partial: HashMap<Local, Vec<mir::Place<'tcx>>>,
92+
}
93+
94+
impl<'tcx> Moves<'tcx> {
95+
fn collect(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) -> Moves<'tcx> {
96+
struct Visitor<'a, 'tcx> {
97+
tcx: TyCtxt<'tcx>,
98+
body: &'a Body<'tcx>,
99+
moves: Moves<'tcx>,
100+
}
101+
impl<'tcx> mir::visit::Visitor<'tcx> for Visitor<'_, 'tcx> {
102+
fn visit_operand(&mut self, operand: &mir::Operand<'tcx>, location: Location) {
103+
let mir::Operand::Move(place) = operand else {
104+
return;
105+
};
106+
if place.ty(&self.body.local_decls, self.tcx).ty.is_ref() {
107+
return;
108+
}
109+
if place.projection.is_empty() {
110+
self.moves
111+
.whole
112+
.entry(location)
113+
.or_insert_with(|| DenseBitSet::new_empty(self.body.local_decls.len()))
114+
.insert(place.local);
115+
} else {
116+
let entry = self.moves.partial.entry(place.local).or_default();
117+
if !entry.contains(place) {
118+
entry.push(*place);
119+
}
73120
}
74121
}
75122
}
123+
let mut visitor = Visitor {
124+
tcx,
125+
body,
126+
moves: Moves::default(),
127+
};
128+
use mir::visit::Visitor as _;
129+
visitor.visit_body(body);
130+
visitor.moves
76131
}
77-
let mut visitor = Visitor {
78-
body,
79-
locals: DenseBitSet::new_empty(body.local_decls.len()),
80-
};
81-
let loc = mir::Location {
82-
statement_index,
83-
block: bb,
84-
};
85-
let data = &body.basic_blocks[bb];
86-
use mir::visit::Visitor as _;
87-
if statement_index < data.statements.len() {
88-
visitor.visit_statement(&data.statements[statement_index], loc);
89-
} else if let Some(tmnt) = &data.terminator {
90-
visitor.visit_terminator(tmnt, loc);
91-
}
92-
visitor.locals
93132
}
94133

95134
fn def_local<'tcx>(data: &mir::BasicBlockData<'tcx>, statement_index: usize) -> Option<Local> {
@@ -121,6 +160,21 @@ fn def_local<'tcx>(data: &mir::BasicBlockData<'tcx>, statement_index: usize) ->
121160
}
122161

123162
impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> {
163+
/// Turn a set of locals that become dead into the drop targets. A local with
164+
/// a partial field move is excluded: dropping it wholesale would resolve the
165+
/// moved-out sub-place's `&mut` prophecy a second time (it is resolved at the
166+
/// move destination instead).
167+
fn drop_set(&self, locals: DenseBitSet<Local>) -> DropSet<'tcx> {
168+
let mut set = DropSet::default();
169+
for local in locals.iter() {
170+
set.drops.insert(local.into());
171+
if let Some(moved) = self.moves.partial.get(&local) {
172+
set.except.extend(moved.iter().copied());
173+
}
174+
}
175+
set
176+
}
177+
124178
pub fn build(
125179
&mut self,
126180
results: &mut ResultsCursor<'mir, 'tcx, MaybeLiveLocals>,
@@ -130,7 +184,7 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> {
130184

131185
let mut after_terminator = HashMap::new();
132186
let mut after_statements = Vec::new();
133-
after_statements.resize_with(data.statements.len() + 1, HashSet::new);
187+
after_statements.resize_with(data.statements.len() + 1, DropSet::default);
134188

135189
results.seek_to_block_end(bb);
136190
let live_locals_after_terminator = results.get().clone();
@@ -147,7 +201,7 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> {
147201
t.subtract(&self.bb_ins_cache[&succ_bb]);
148202
t
149203
};
150-
after_terminator.insert(succ_bb, edge_drops.iter().map(Into::into).collect());
204+
after_terminator.insert(succ_bb, self.drop_set(edge_drops));
151205
ins.union(&self.bb_ins_cache[&succ_bb]);
152206
}
153207

@@ -170,8 +224,10 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> {
170224
t.insert(def);
171225
}
172226
t.subtract(&last_live_locals);
173-
t.subtract(&moved_locals(self.body, bb, statement_index));
174-
t.iter().map(Into::into).collect()
227+
if let Some(moved) = self.moves.whole.get(&loc) {
228+
t.subtract(moved);
229+
}
230+
self.drop_set(t)
175231
};
176232
last_live_locals = live_locals;
177233
}
@@ -185,10 +241,10 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> {
185241
"analyzed implicit drop points"
186242
);
187243
DropPoints {
188-
before_statements: HashSet::default(),
244+
before_statements: DropSet::default(),
189245
after_statements,
190246
after_terminator,
191-
after_terminator_extra: HashSet::default(),
247+
after_terminator_extra: DropSet::default(),
192248
}
193249
}
194250
}

src/analyze/local_def.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1065,7 +1065,7 @@ impl<'tcx, 'ctx> Analyzer<'tcx, 'ctx> {
10651065
.iterate_to_fixpoint(self.tcx, &self.body, None)
10661066
.into_results_cursor(&self.body);
10671067

1068-
let mut builder = analyze::basic_block::DropPoints::builder(&self.body);
1068+
let mut builder = analyze::basic_block::DropPoints::builder(self.tcx, &self.body);
10691069
for (bb, _data) in mir::traversal::postorder(&self.body) {
10701070
let span = tracing::info_span!("refine_basic_block", ?bb);
10711071
let _guard = span.enter();

src/refine/env.rs

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1087,6 +1087,26 @@ impl Path {
10871087
fn tuple_proj(self, idx: usize) -> Self {
10881088
Path::TupleProj(Box::new(self), idx)
10891089
}
1090+
1091+
/// Structural place equality for the drop walk's except-skip. `Deref` is
1092+
/// treated as transparent on both sides: the drop walk inserts a `Deref`
1093+
/// for every `own`-box the type elaboration introduces (mut locals, and
1094+
/// every tuple field), whereas the moved-out places this is compared against
1095+
/// come straight from MIR and carry no such derefs. Moved-out places never
1096+
/// contain a real deref (reference moves are excluded upstream), so peeling
1097+
/// derefs cannot conflate distinct owned places here.
1098+
fn same_place(&self, other: &Path) -> bool {
1099+
match (self, other) {
1100+
(Path::Deref(a), _) => a.same_place(other),
1101+
(_, Path::Deref(b)) => self.same_place(b),
1102+
(Path::Local(a), Path::Local(b)) => a == b,
1103+
(Path::TupleProj(a, i), Path::TupleProj(b, j)) => i == j && a.same_place(b),
1104+
(Path::Downcast(a, va, fa), Path::Downcast(b, vb, fb)) => {
1105+
va == vb && fa == fb && a.same_place(b)
1106+
}
1107+
_ => false,
1108+
}
1109+
}
10901110
}
10911111

10921112
impl<T> Env<T>
@@ -1110,18 +1130,21 @@ where
11101130
self.path_type(&place.into())
11111131
}
11121132

1113-
fn dropping_assumption(&mut self, path: &Path) -> Assumption {
1133+
fn dropping_assumption(&mut self, path: &Path, except: &[Path]) -> Assumption {
1134+
if except.iter().any(|e| e.same_place(path)) {
1135+
return Assumption::default();
1136+
}
11141137
let ty = self.path_type(path);
11151138
if ty.ty.is_mut() {
11161139
let mut builder = PlaceTypeBuilder::default();
11171140
let (_, term) = builder.subsume(ty);
11181141
builder.push_formula(term.clone().mut_final().equal_to(term.mut_current()));
11191142
builder.build_assumption()
11201143
} else if ty.ty.is_own() {
1121-
self.dropping_assumption(&path.clone().deref())
1144+
self.dropping_assumption(&path.clone().deref(), except)
11221145
} else if let Some(tty) = ty.ty.as_tuple() {
11231146
(0..tty.elems.len())
1124-
.map(|i| self.dropping_assumption(&path.clone().tuple_proj(i)))
1147+
.map(|i| self.dropping_assumption(&path.clone().tuple_proj(i), except))
11251148
.collect()
11261149
} else if let Some(ety) = ty.ty.as_enum() {
11271150
let enum_def = self.enum_defs.enum_def(&ety.symbol);
@@ -1174,7 +1197,7 @@ where
11741197
let Assumption {
11751198
existentials: assumption_existentials,
11761199
body: assumption_body,
1177-
} = self.dropping_assumption(&Path::PlaceTy(Box::new(field_pty)));
1200+
} = self.dropping_assumption(&Path::PlaceTy(Box::new(field_pty)), except);
11781201
// dropping assumption should not generate any existential
11791202
assert!(assumption_existentials.is_empty());
11801203
formula.push_conj(assumption_body);
@@ -1189,14 +1212,11 @@ where
11891212
}
11901213
}
11911214

1192-
pub fn drop_place(&mut self, place: Place<'_>) {
1193-
let assumption = self.dropping_assumption(&place.into());
1215+
pub fn drop_place(&mut self, place: Place<'_>, except: &[Place<'_>]) {
1216+
let except: Vec<Path> = except.iter().map(|p| Path::from(*p)).collect();
1217+
let assumption = self.dropping_assumption(&place.into(), &except);
11941218
if !assumption.is_top() {
11951219
self.assume(assumption);
11961220
}
11971221
}
1198-
1199-
pub fn drop_local(&mut self, local: Local) {
1200-
self.drop_place(local.into());
1201-
}
12021222
}

tests/ui/fail/partial_move_drop.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
//@error-in-other-file: Unsat
2+
//@compile-flags: -C debug-assertions=off
3+
4+
// Regression test for #121: a local that is partially moved must not have a
5+
// dropping assumption emitted for the moved-out portion. Here `(&mut a,)` is
6+
// moved out of `s` into `b`; dropping `s` wholesale used to resolve the
7+
// moved-out `&mut a` prophecy a second time, making the constraints
8+
// contradictory so that this false assertion was wrongly accepted.
9+
fn main() {
10+
let mut a = 1_i64;
11+
let s = ((&mut a,),);
12+
let b = s.0; // partial move: `(&mut a,)` moves out of `s`
13+
*b.0 = 2;
14+
assert!(a == 1); // false at runtime: a == 2
15+
}

0 commit comments

Comments
 (0)