From 981f62d04a489fa6cb097a533978ecbba9f870f7 Mon Sep 17 00:00:00 2001 From: Hiromi Ogawa Date: Sun, 28 Jun 2026 22:06:02 +0900 Subject: [PATCH 1/2] Track drop points by MIR place Replace the Local-only drop-point representation with MIR `Place` so implicit drops are tracked at the right granularity. Drop points are stored as `HashSet` (a whole-local drop is just a place with an empty projection), and the analyzer drops them through a single `drop_places` helper. --- src/analyze/basic_block.rs | 55 ++++++-------- src/analyze/basic_block/drop_point.rs | 76 +++++++------------- src/analyze/basic_block/visitor/rust_call.rs | 16 ++--- src/analyze/local_def.rs | 4 +- src/refine/env.rs | 8 ++- 5 files changed, 63 insertions(+), 96 deletions(-) diff --git a/src/analyze/basic_block.rs b/src/analyze/basic_block.rs index 4ed04196..0032c4a6 100644 --- a/src/analyze/basic_block.rs +++ b/src/analyze/basic_block.rs @@ -1,5 +1,5 @@ use std::borrow::Cow; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use rustc_hir::def::DefKind; use rustc_index::IndexVec; @@ -136,7 +136,7 @@ pub struct Analyzer<'tcx, 'ctx> { tcx: TyCtxt<'tcx>, local_def_id: LocalDefId, - drop_points: DropPoints, + drop_points: DropPoints<'tcx>, basic_block: BasicBlock, body: Cow<'tcx, Body<'tcx>>, @@ -1008,14 +1008,17 @@ impl<'tcx, 'ctx> Analyzer<'tcx, 'ctx> { } } - fn drop_local(&mut self, local: Local) { - self.env.drop_local(local); + fn drop_places(&mut self, places: HashSet>) { + for place in places { + tracing::info!(?place, "implicitly dropped"); + self.env.drop_place(place); + } } - /// Schedules `local` to be implicitly dropped after this block's terminator, + /// Schedules `place` to be implicitly dropped after this block's terminator, /// in addition to the liveness-derived drop points. - fn drop_after_terminator(&mut self, local: Local) { - self.drop_points.insert_after_terminator(local); + fn drop_after_terminator(&mut self, place: mir::Place<'tcx>) { + self.drop_points.insert_after_terminator(place); } fn add_prophecy_var(&mut self, statement_index: usize, ty: mir_ty::Ty<'tcx>) { @@ -1152,10 +1155,8 @@ impl<'tcx, 'ctx> Analyzer<'tcx, 'ctx> { } fn analyze_statements(&mut self) { - for local in self.drop_points.before_statements.clone() { - tracing::info!(?local, "implicitly dropped before statements"); - self.drop_local(local); - } + let before_statements = self.drop_points.before_statements.clone(); + self.drop_places(before_statements); let statements = self.body.basic_blocks[self.basic_block].statements.clone(); for (stmt_idx, mut stmt) in statements.iter().cloned().enumerate() { if stmt_idx == statements.len() - 1 && self.terminator_is_drop_call().is_some() { @@ -1174,10 +1175,8 @@ impl<'tcx, 'ctx> Analyzer<'tcx, 'ctx> { | StatementKind::StorageDead(_) => {} _ => unimplemented!("stmt={:?}", stmt.kind), } - for local in self.drop_points.after_statement(stmt_idx).iter() { - tracing::info!(?local, ?stmt_idx, "implicitly dropped after statement"); - self.drop_local(local); - } + let after_statement = self.drop_points.after_statement(stmt_idx); + self.drop_places(after_statement); } } @@ -1257,27 +1256,21 @@ impl<'tcx, 'ctx> Analyzer<'tcx, 'ctx> { targets.clone(), outer_fn_param_vars, |a, target| { - for local in a.drop_points.after_terminator(&target) { - tracing::info!(?local, ?target, "implicitly dropped for target"); - a.drop_local(local); - } + let set = a.drop_points.after_terminator(&target); + a.drop_places(set); }, ); } TerminatorKind::Call { target, .. } => { if let Some(target) = target { - for local in self.drop_points.after_terminator(target) { - tracing::info!(?local, "implicitly dropped after call"); - self.drop_local(local); - } + let set = self.drop_points.after_terminator(target); + self.drop_places(set); self.type_goto(*target, outer_fn_param_vars); } } TerminatorKind::Drop { target, .. } => { - for local in self.drop_points.after_terminator(target) { - tracing::info!(?local, "dropped"); - self.drop_local(local); - } + let set = self.drop_points.after_terminator(target); + self.drop_places(set); self.type_goto(*target, outer_fn_param_vars); } TerminatorKind::Assert { @@ -1286,10 +1279,8 @@ impl<'tcx, 'ctx> Analyzer<'tcx, 'ctx> { target, .. } => { - for local in self.drop_points.after_terminator(target) { - tracing::info!(?local, "dropped"); - self.drop_local(local); - } + let set = self.drop_points.after_terminator(target); + self.drop_places(set); self.type_operand( cond.clone(), &rty::RefinedType::refined_with_term( @@ -1470,7 +1461,7 @@ impl<'tcx, 'ctx> Analyzer<'tcx, 'ctx> { } } - pub fn drop_points(&mut self, drop_points: DropPoints) -> &mut Self { + pub fn drop_points(&mut self, drop_points: DropPoints<'tcx>) -> &mut Self { self.drop_points = drop_points; self } diff --git a/src/analyze/basic_block/drop_point.rs b/src/analyze/basic_block/drop_point.rs index e660e3c3..e0d79d3e 100644 --- a/src/analyze/basic_block/drop_point.rs +++ b/src/analyze/basic_block/drop_point.rs @@ -1,66 +1,44 @@ -use std::collections::{BTreeSet, HashMap}; +use std::collections::{HashMap, HashSet}; use rustc_index::bit_set::DenseBitSet; use rustc_middle::mir::{self, BasicBlock, Body, Local}; use rustc_mir_dataflow::{impls::MaybeLiveLocals, ResultsCursor}; +/// Implicit-drop targets. A drop is a `Place`; a whole-local drop is just a +/// place with an empty projection (semantically a bare `Local`). #[derive(Debug, Clone, Default)] -pub struct DropPoints { - // TODO: ad-hoc - pub before_statements: Vec, - after_statements: Vec>, - after_terminator: HashMap>, - /// Locals dropped after the terminator regardless of the target, in - /// addition to the liveness-derived sets above. A set, since the same local - /// must not be dropped twice; ordered by index to keep drops deterministic. - after_terminator_extra: BTreeSet, +pub struct DropPoints<'tcx> { + pub before_statements: HashSet>, + after_statements: Vec>>, + after_terminator: HashMap>>, + /// Drops scheduled after the terminator regardless of the target, in + /// addition to the liveness-derived sets above. + after_terminator_extra: HashSet>, } -impl DropPoints { +impl DropPoints<'_> { pub fn builder<'mir, 'tcx>(body: &'mir Body<'tcx>) -> DropPointsBuilder<'mir, 'tcx> { DropPointsBuilder { body, bb_ins_cache: HashMap::new(), } } +} - pub fn position(&self, local: Local) -> Option { - self.after_statements - .iter() - .position(|s| s.contains(local)) - .or_else(|| { - self.is_after_terminator(local) - .then_some(self.after_statements.len()) - }) - } - - fn is_after_terminator(&self, local: Local) -> bool { - self.after_terminator.values().any(|s| s.contains(local)) - || self.after_terminator_extra.contains(&local) - } - - pub fn remove_after_statement(&mut self, statement_index: usize, local: Local) -> bool { - self.after_statements[statement_index].remove(local) - } - - pub fn insert_after_statement(&mut self, statement_index: usize, local: Local) -> bool { - self.after_statements[statement_index].insert(local) - } - - pub fn after_statement(&self, statement_index: usize) -> DenseBitSet { +impl<'tcx> DropPoints<'tcx> { + pub fn after_statement(&self, statement_index: usize) -> HashSet> { self.after_statements[statement_index].clone() } - pub fn insert_after_terminator(&mut self, local: Local) { - self.after_terminator_extra.insert(local); + pub fn insert_after_terminator(&mut self, place: mir::Place<'tcx>) { + self.after_terminator_extra.insert(place); } - pub fn after_terminator(&self, target: &BasicBlock) -> Vec { - let mut t = self.after_terminator[target].clone(); - t.union(self.after_statements.last().unwrap()); - t.iter() - .chain(self.after_terminator_extra.iter().copied()) - .collect() + pub fn after_terminator(&self, target: &BasicBlock) -> HashSet> { + let mut set = self.after_terminator[target].clone(); + set.extend(self.after_statements.last().unwrap().iter().copied()); + set.extend(self.after_terminator_extra.iter().copied()); + set } } @@ -147,12 +125,12 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> { &mut self, results: &mut ResultsCursor<'mir, 'tcx, MaybeLiveLocals>, bb: BasicBlock, - ) -> DropPoints { + ) -> DropPoints<'tcx> { let data = &self.body.basic_blocks[bb]; let mut after_terminator = HashMap::new(); let mut after_statements = Vec::new(); - after_statements.resize_with(data.statements.len() + 1, || DenseBitSet::new_empty(0)); + after_statements.resize_with(data.statements.len() + 1, HashSet::new); results.seek_to_block_end(bb); let live_locals_after_terminator = results.get().clone(); @@ -169,7 +147,7 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> { t.subtract(&self.bb_ins_cache[&succ_bb]); t }; - after_terminator.insert(succ_bb, edge_drops); + after_terminator.insert(succ_bb, edge_drops.iter().map(Into::into).collect()); ins.union(&self.bb_ins_cache[&succ_bb]); } @@ -193,7 +171,7 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> { } t.subtract(&last_live_locals); t.subtract(&moved_locals(self.body, bb, statement_index)); - t + t.iter().map(Into::into).collect() }; last_live_locals = live_locals; } @@ -207,10 +185,10 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> { "analyzed implicit drop points" ); DropPoints { - before_statements: Default::default(), + before_statements: HashSet::default(), after_statements, after_terminator, - after_terminator_extra: Default::default(), + after_terminator_extra: HashSet::default(), } } } diff --git a/src/analyze/basic_block/visitor/rust_call.rs b/src/analyze/basic_block/visitor/rust_call.rs index b46c0467..1de02ea5 100644 --- a/src/analyze/basic_block/visitor/rust_call.rs +++ b/src/analyze/basic_block/visitor/rust_call.rs @@ -122,17 +122,11 @@ impl<'a, 'tcx, 'ctx> mir::visit::MutVisitor<'tcx> for RustCallVisitor<'a, 'tcx, // FnOnce::call_once consumes the closure, but the resolved function // only borrows it: drop the borrow and the environment after the // call to resolve the prophecies of the captured mutable borrows. - self.analyzer.drop_after_terminator(borrowed_closure_local); - // The original MIR moves the closure into the call, so `moved_locals` - // dropped its drop obligation, expecting the callee to consume it; we - // must restore it. `moved_locals` only steals whole-local moves, so we - // only restore those: with a projection the obligation was never stolen - // and the normal drop machinery still handles it (re-adding it would - // double-drop). In practice a non-`Copy` closure (the only kind reaching - // this case) is always moved through a projection-less temporary. - if arg_closure_place.projection.is_empty() { - self.analyzer.drop_after_terminator(arg_closure_place.local); - } + self.analyzer + .drop_after_terminator(borrowed_closure_local.into()); + // The original MIR moves the closure into the call, so restore a + // drop obligation for the moved closure environment at the precise place. + self.analyzer.drop_after_terminator(arg_closure_place); tracing::debug!("applied mut-borrow for closure argument"); } } diff --git a/src/analyze/local_def.rs b/src/analyze/local_def.rs index 72616477..79fa0d73 100644 --- a/src/analyze/local_def.rs +++ b/src/analyze/local_def.rs @@ -49,7 +49,7 @@ pub struct Analyzer<'tcx, 'ctx> { body: Body<'tcx>, /// to substitute HIR types during translation in [`crate::analyze::annot_fn`] generic_args: mir_ty::GenericArgsRef<'tcx>, - drop_points: HashMap, + drop_points: HashMap>, type_builder: TypeBuilder<'tcx>, } @@ -1100,7 +1100,7 @@ impl<'tcx, 'ctx> Analyzer<'tcx, 'ctx> { .get_mut(&bb) .unwrap() .before_statements - .push(a); + .insert(a.into()); } } // function return type is basic block return type diff --git a/src/refine/env.rs b/src/refine/env.rs index ba494acb..b700a9eb 100644 --- a/src/refine/env.rs +++ b/src/refine/env.rs @@ -1189,10 +1189,14 @@ where } } - pub fn drop_local(&mut self, local: Local) { - let assumption = self.dropping_assumption(&Path::Local(local)); + pub fn drop_place(&mut self, place: Place<'_>) { + let assumption = self.dropping_assumption(&place.into()); if !assumption.is_top() { self.assume(assumption); } } + + pub fn drop_local(&mut self, local: Local) { + self.drop_place(local.into()); + } } From 43aaebbe34601fca55de40415cad6cda4f81eeb6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 2 Jul 2026 01:33:15 +0000 Subject: [PATCH 2/2] 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 Claude-Session: https://claude.ai/code/session_01NU1g7aHMxcSEZgeKProDr1 --- src/analyze/basic_block.rs | 11 +- src/analyze/basic_block/drop_point.rs | 172 +++++++++++++++-------- src/analyze/local_def.rs | 2 +- src/refine/env.rs | 40 ++++-- tests/ui/fail/partial_move_drop.rs | 15 ++ tests/ui/fail/partial_move_field_call.rs | 22 +++ tests/ui/fail/partial_move_sibling.rs | 17 +++ tests/ui/pass/partial_move_drop.rs | 13 ++ tests/ui/pass/partial_move_field_call.rs | 19 +++ tests/ui/pass/partial_move_sibling.rs | 17 +++ 10 files changed, 254 insertions(+), 74 deletions(-) create mode 100644 tests/ui/fail/partial_move_drop.rs create mode 100644 tests/ui/fail/partial_move_field_call.rs create mode 100644 tests/ui/fail/partial_move_sibling.rs create mode 100644 tests/ui/pass/partial_move_drop.rs create mode 100644 tests/ui/pass/partial_move_field_call.rs create mode 100644 tests/ui/pass/partial_move_sibling.rs diff --git a/src/analyze/basic_block.rs b/src/analyze/basic_block.rs index 0032c4a6..e21eedc7 100644 --- a/src/analyze/basic_block.rs +++ b/src/analyze/basic_block.rs @@ -1,5 +1,5 @@ use std::borrow::Cow; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use rustc_hir::def::DefKind; use rustc_index::IndexVec; @@ -1008,10 +1008,11 @@ impl<'tcx, 'ctx> Analyzer<'tcx, 'ctx> { } } - fn drop_places(&mut self, places: HashSet>) { - for place in places { - tracing::info!(?place, "implicitly dropped"); - self.env.drop_place(place); + fn drop_places(&mut self, set: drop_point::DropSet<'tcx>) { + let except: Vec> = set.except.into_iter().collect(); + for place in set.drops { + tracing::info!(?place, ?except, "implicitly dropped"); + self.env.drop_place(place, &except); } } diff --git a/src/analyze/basic_block/drop_point.rs b/src/analyze/basic_block/drop_point.rs index e0d79d3e..d17933f5 100644 --- a/src/analyze/basic_block/drop_point.rs +++ b/src/analyze/basic_block/drop_point.rs @@ -1,32 +1,52 @@ use std::collections::{HashMap, HashSet}; use rustc_index::bit_set::DenseBitSet; -use rustc_middle::mir::{self, BasicBlock, Body, Local}; +use rustc_middle::mir::{self, BasicBlock, Body, Local, Location}; +use rustc_middle::ty::TyCtxt; use rustc_mir_dataflow::{impls::MaybeLiveLocals, ResultsCursor}; -/// Implicit-drop targets. A drop is a `Place`; a whole-local drop is just a -/// place with an empty projection (semantically a bare `Local`). +/// A set of implicit-drop targets: `drops` are places to drop; `except` are +/// moved-out sub-places to skip when a drop walks into them. +#[derive(Debug, Clone, Default)] +pub struct DropSet<'tcx> { + pub drops: HashSet>, + pub except: HashSet>, +} + +impl<'tcx> DropSet<'tcx> { + pub fn insert(&mut self, place: mir::Place<'tcx>) { + self.drops.insert(place); + } + + fn extend(&mut self, other: &DropSet<'tcx>) { + self.drops.extend(other.drops.iter().copied()); + self.except.extend(other.except.iter().copied()); + } +} + #[derive(Debug, Clone, Default)] pub struct DropPoints<'tcx> { - pub before_statements: HashSet>, - after_statements: Vec>>, - after_terminator: HashMap>>, - /// Drops scheduled after the terminator regardless of the target, in - /// addition to the liveness-derived sets above. - after_terminator_extra: HashSet>, + pub before_statements: DropSet<'tcx>, + after_statements: Vec>, + after_terminator: HashMap>, + after_terminator_extra: DropSet<'tcx>, } impl DropPoints<'_> { - pub fn builder<'mir, 'tcx>(body: &'mir Body<'tcx>) -> DropPointsBuilder<'mir, 'tcx> { + pub fn builder<'mir, 'tcx>( + tcx: TyCtxt<'tcx>, + body: &'mir Body<'tcx>, + ) -> DropPointsBuilder<'mir, 'tcx> { DropPointsBuilder { body, bb_ins_cache: HashMap::new(), + moves: Moves::collect(tcx, body), } } } impl<'tcx> DropPoints<'tcx> { - pub fn after_statement(&self, statement_index: usize) -> HashSet> { + pub fn after_statement(&self, statement_index: usize) -> DropSet<'tcx> { self.after_statements[statement_index].clone() } @@ -34,62 +54,81 @@ impl<'tcx> DropPoints<'tcx> { self.after_terminator_extra.insert(place); } - pub fn after_terminator(&self, target: &BasicBlock) -> HashSet> { + pub fn after_terminator(&self, target: &BasicBlock) -> DropSet<'tcx> { let mut set = self.after_terminator[target].clone(); - set.extend(self.after_statements.last().unwrap().iter().copied()); - set.extend(self.after_terminator_extra.iter().copied()); + set.extend(self.after_statements.last().unwrap()); + set.extend(&self.after_terminator_extra); set } } -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct DropPointsBuilder<'mir, 'tcx> { body: &'mir Body<'tcx>, bb_ins_cache: HashMap>, + moves: Moves<'tcx>, } -/// Locals whose ownership is fully transferred away by the statement (or -/// terminator) at `statement_index`. Such a local is left uninitialized, so its -/// drop obligation (including resolving any mutable-borrow prophecies it owns) -/// moves to the destination and it must not be dropped at the move site. +/// The `move`d operands of a body, excluding reference-typed places. /// -/// Only owned (non-reference) operands are reported: `move`d references are -/// turned into reborrows by `ReborrowVisitor`/`RustCallVisitor`, so the source -/// local remains live and must still be dropped. -fn moved_locals<'tcx>( - body: &Body<'tcx>, - bb: BasicBlock, - statement_index: usize, -) -> DenseBitSet { - struct Visitor<'a, 'tcx> { - body: &'a Body<'tcx>, - locals: DenseBitSet, - } - impl<'tcx> mir::visit::Visitor<'tcx> for Visitor<'_, 'tcx> { - fn visit_operand(&mut self, operand: &mir::Operand<'tcx>, _location: mir::Location) { - if let mir::Operand::Move(place) = operand { - if place.projection.is_empty() && !self.body.local_decls[place.local].ty.is_ref() { - self.locals.insert(place.local); +/// `move`d references are turned into reborrows by +/// `ReborrowVisitor`/`RustCallVisitor`, so the source still owns its prophecy +/// and must be dropped normally; they are therefore not recorded here. +/// +/// A whole-local move leaves the local uninitialized, transferring its entire +/// drop obligation (including resolving any mutable-borrow prophecies it owns) +/// to the destination, so it must not be dropped at the move site — where the +/// local also becomes dead, hence the per-location keying. A partial (projected) +/// field move transfers only a sub-place, but the parent stays live until its +/// remaining parts die later; dropping it wholesale at that point would walk +/// into the moved-out sub-place and resolve its `&mut` prophecy a second time, +/// so a partially-moved local is excluded from dropping entirely. +#[derive(Clone, Default)] +struct Moves<'tcx> { + /// Whole-local moves, keyed by the location performing the move. + whole: HashMap>, + /// Partial field moves, keyed by the parent local. + partial: HashMap>>, +} + +impl<'tcx> Moves<'tcx> { + fn collect(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) -> Moves<'tcx> { + struct Visitor<'a, 'tcx> { + tcx: TyCtxt<'tcx>, + body: &'a Body<'tcx>, + moves: Moves<'tcx>, + } + impl<'tcx> mir::visit::Visitor<'tcx> for Visitor<'_, 'tcx> { + fn visit_operand(&mut self, operand: &mir::Operand<'tcx>, location: Location) { + let mir::Operand::Move(place) = operand else { + return; + }; + if place.ty(&self.body.local_decls, self.tcx).ty.is_ref() { + return; + } + if place.projection.is_empty() { + self.moves + .whole + .entry(location) + .or_insert_with(|| DenseBitSet::new_empty(self.body.local_decls.len())) + .insert(place.local); + } else { + let entry = self.moves.partial.entry(place.local).or_default(); + if !entry.contains(place) { + entry.push(*place); + } } } } + let mut visitor = Visitor { + tcx, + body, + moves: Moves::default(), + }; + use mir::visit::Visitor as _; + visitor.visit_body(body); + visitor.moves } - let mut visitor = Visitor { - body, - locals: DenseBitSet::new_empty(body.local_decls.len()), - }; - let loc = mir::Location { - statement_index, - block: bb, - }; - let data = &body.basic_blocks[bb]; - use mir::visit::Visitor as _; - if statement_index < data.statements.len() { - visitor.visit_statement(&data.statements[statement_index], loc); - } else if let Some(tmnt) = &data.terminator { - visitor.visit_terminator(tmnt, loc); - } - visitor.locals } fn def_local<'tcx>(data: &mir::BasicBlockData<'tcx>, statement_index: usize) -> Option { @@ -121,6 +160,21 @@ fn def_local<'tcx>(data: &mir::BasicBlockData<'tcx>, statement_index: usize) -> } impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> { + /// Turn a set of locals that become dead into the drop targets. A local with + /// a partial field move is excluded: dropping it wholesale would resolve the + /// moved-out sub-place's `&mut` prophecy a second time (it is resolved at the + /// move destination instead). + fn drop_set(&self, locals: DenseBitSet) -> DropSet<'tcx> { + let mut set = DropSet::default(); + for local in locals.iter() { + set.drops.insert(local.into()); + if let Some(moved) = self.moves.partial.get(&local) { + set.except.extend(moved.iter().copied()); + } + } + set + } + pub fn build( &mut self, results: &mut ResultsCursor<'mir, 'tcx, MaybeLiveLocals>, @@ -130,7 +184,7 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> { let mut after_terminator = HashMap::new(); let mut after_statements = Vec::new(); - after_statements.resize_with(data.statements.len() + 1, HashSet::new); + after_statements.resize_with(data.statements.len() + 1, DropSet::default); results.seek_to_block_end(bb); let live_locals_after_terminator = results.get().clone(); @@ -147,7 +201,7 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> { t.subtract(&self.bb_ins_cache[&succ_bb]); t }; - after_terminator.insert(succ_bb, edge_drops.iter().map(Into::into).collect()); + after_terminator.insert(succ_bb, self.drop_set(edge_drops)); ins.union(&self.bb_ins_cache[&succ_bb]); } @@ -170,8 +224,10 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> { t.insert(def); } t.subtract(&last_live_locals); - t.subtract(&moved_locals(self.body, bb, statement_index)); - t.iter().map(Into::into).collect() + if let Some(moved) = self.moves.whole.get(&loc) { + t.subtract(moved); + } + self.drop_set(t) }; last_live_locals = live_locals; } @@ -185,10 +241,10 @@ impl<'mir, 'tcx> DropPointsBuilder<'mir, 'tcx> { "analyzed implicit drop points" ); DropPoints { - before_statements: HashSet::default(), + before_statements: DropSet::default(), after_statements, after_terminator, - after_terminator_extra: HashSet::default(), + after_terminator_extra: DropSet::default(), } } } diff --git a/src/analyze/local_def.rs b/src/analyze/local_def.rs index 79fa0d73..2d489755 100644 --- a/src/analyze/local_def.rs +++ b/src/analyze/local_def.rs @@ -1065,7 +1065,7 @@ impl<'tcx, 'ctx> Analyzer<'tcx, 'ctx> { .iterate_to_fixpoint(self.tcx, &self.body, None) .into_results_cursor(&self.body); - let mut builder = analyze::basic_block::DropPoints::builder(&self.body); + let mut builder = analyze::basic_block::DropPoints::builder(self.tcx, &self.body); for (bb, _data) in mir::traversal::postorder(&self.body) { let span = tracing::info_span!("refine_basic_block", ?bb); let _guard = span.enter(); diff --git a/src/refine/env.rs b/src/refine/env.rs index b700a9eb..d36aaa57 100644 --- a/src/refine/env.rs +++ b/src/refine/env.rs @@ -1087,6 +1087,26 @@ impl Path { fn tuple_proj(self, idx: usize) -> Self { Path::TupleProj(Box::new(self), idx) } + + /// Structural place equality for the drop walk's except-skip. `Deref` is + /// treated as transparent on both sides: the drop walk inserts a `Deref` + /// for every `own`-box the type elaboration introduces (mut locals, and + /// every tuple field), whereas the moved-out places this is compared against + /// come straight from MIR and carry no such derefs. Moved-out places never + /// contain a real deref (reference moves are excluded upstream), so peeling + /// derefs cannot conflate distinct owned places here. + fn same_place(&self, other: &Path) -> bool { + match (self, other) { + (Path::Deref(a), _) => a.same_place(other), + (_, Path::Deref(b)) => self.same_place(b), + (Path::Local(a), Path::Local(b)) => a == b, + (Path::TupleProj(a, i), Path::TupleProj(b, j)) => i == j && a.same_place(b), + (Path::Downcast(a, va, fa), Path::Downcast(b, vb, fb)) => { + va == vb && fa == fb && a.same_place(b) + } + _ => false, + } + } } impl Env @@ -1110,7 +1130,10 @@ where self.path_type(&place.into()) } - fn dropping_assumption(&mut self, path: &Path) -> Assumption { + fn dropping_assumption(&mut self, path: &Path, except: &[Path]) -> Assumption { + if except.iter().any(|e| e.same_place(path)) { + return Assumption::default(); + } let ty = self.path_type(path); if ty.ty.is_mut() { let mut builder = PlaceTypeBuilder::default(); @@ -1118,10 +1141,10 @@ where builder.push_formula(term.clone().mut_final().equal_to(term.mut_current())); builder.build_assumption() } else if ty.ty.is_own() { - self.dropping_assumption(&path.clone().deref()) + self.dropping_assumption(&path.clone().deref(), except) } else if let Some(tty) = ty.ty.as_tuple() { (0..tty.elems.len()) - .map(|i| self.dropping_assumption(&path.clone().tuple_proj(i))) + .map(|i| self.dropping_assumption(&path.clone().tuple_proj(i), except)) .collect() } else if let Some(ety) = ty.ty.as_enum() { let enum_def = self.enum_defs.enum_def(&ety.symbol); @@ -1174,7 +1197,7 @@ where let Assumption { existentials: assumption_existentials, body: assumption_body, - } = self.dropping_assumption(&Path::PlaceTy(Box::new(field_pty))); + } = self.dropping_assumption(&Path::PlaceTy(Box::new(field_pty)), except); // dropping assumption should not generate any existential assert!(assumption_existentials.is_empty()); formula.push_conj(assumption_body); @@ -1189,14 +1212,11 @@ where } } - pub fn drop_place(&mut self, place: Place<'_>) { - let assumption = self.dropping_assumption(&place.into()); + pub fn drop_place(&mut self, place: Place<'_>, except: &[Place<'_>]) { + let except: Vec = except.iter().map(|p| Path::from(*p)).collect(); + let assumption = self.dropping_assumption(&place.into(), &except); if !assumption.is_top() { self.assume(assumption); } } - - pub fn drop_local(&mut self, local: Local) { - self.drop_place(local.into()); - } } diff --git a/tests/ui/fail/partial_move_drop.rs b/tests/ui/fail/partial_move_drop.rs new file mode 100644 index 00000000..6b24945f --- /dev/null +++ b/tests/ui/fail/partial_move_drop.rs @@ -0,0 +1,15 @@ +//@error-in-other-file: Unsat +//@compile-flags: -C debug-assertions=off + +// Regression test for #121: a local that is partially moved must not have a +// dropping assumption emitted for the moved-out portion. Here `(&mut a,)` is +// moved out of `s` into `b`; dropping `s` wholesale used to resolve the +// moved-out `&mut a` prophecy a second time, making the constraints +// contradictory so that this false assertion was wrongly accepted. +fn main() { + let mut a = 1_i64; + let s = ((&mut a,),); + let b = s.0; // partial move: `(&mut a,)` moves out of `s` + *b.0 = 2; + assert!(a == 1); // false at runtime: a == 2 +} diff --git a/tests/ui/fail/partial_move_field_call.rs b/tests/ui/fail/partial_move_field_call.rs new file mode 100644 index 00000000..fedb5e0b --- /dev/null +++ b/tests/ui/fail/partial_move_field_call.rs @@ -0,0 +1,22 @@ +//@error-in-other-file: Unsat +//@compile-flags: -C debug-assertions=off + +// Regression test for #122: a `&mut`-bearing field moved out of an aggregate +// into a call must not be re-dropped when the parent is dropped wholesale. +// `w.0` (an owned `(&mut i32,)`) is moved into `bump`; dropping `w` afterwards +// used to resolve the moved-out `&mut` prophecy a second time, so this false +// assertion was wrongly accepted. +fn bump(p: (&mut i64,)) { + *p.0 += 1; +} + +fn apply(w: ((&mut i64,),)) { + bump(w.0); // moves owned field `(&mut i64,)` out of `w` into the call +} + +fn main() { + let mut x = 1_i64; + let w = ((&mut x,),); + apply(w); + assert!(x == 999); // false at runtime: x == 2 +} diff --git a/tests/ui/fail/partial_move_sibling.rs b/tests/ui/fail/partial_move_sibling.rs new file mode 100644 index 00000000..7e2fba3f --- /dev/null +++ b/tests/ui/fail/partial_move_sibling.rs @@ -0,0 +1,17 @@ +//@error-in-other-file: Unsat +//@compile-flags: -C debug-assertions=off + +// Soundness guard for the case where the moved-out part is used *and* the +// still-owned sibling is reborrowed: dropping `s` while excepting the moved-out +// `s.0` must not resolve `s.0`'s prophecy a second time (it is resolved via +// `x`). Otherwise the constraints go contradictory and this false assertion +// would verify vacuously. +fn main() { + let mut a = 1_i64; + let mut b = 2_i64; + let s = ((&mut a,), (&mut b,)); + let x = s.0; + *x.0 = 10; + *(s.1).0 = 20; + assert!(a == 999); // false at runtime: a == 10 +} diff --git a/tests/ui/pass/partial_move_drop.rs b/tests/ui/pass/partial_move_drop.rs new file mode 100644 index 00000000..23c080fa --- /dev/null +++ b/tests/ui/pass/partial_move_drop.rs @@ -0,0 +1,13 @@ +//@check-pass +//@compile-flags: -C debug-assertions=off + +// Companion to the #121 regression test: the moved-out `&mut a` prophecy is +// still resolved exactly once (through `b`), so the true post-condition holds. +// This guards against over-suppressing the drop. +fn main() { + let mut a = 1_i64; + let s = ((&mut a,),); + let b = s.0; // partial move: `(&mut a,)` moves out of `s` + *b.0 = 2; + assert!(a == 2); +} diff --git a/tests/ui/pass/partial_move_field_call.rs b/tests/ui/pass/partial_move_field_call.rs new file mode 100644 index 00000000..0ba87b09 --- /dev/null +++ b/tests/ui/pass/partial_move_field_call.rs @@ -0,0 +1,19 @@ +//@check-pass +//@compile-flags: -C debug-assertions=off + +// Companion to the #122 regression test: the moved-out `&mut` prophecy is +// resolved exactly once (inside `bump`), so the true post-condition holds. +fn bump(p: (&mut i64,)) { + *p.0 += 1; +} + +fn apply(w: ((&mut i64,),)) { + bump(w.0); // moves owned field `(&mut i64,)` out of `w` into the call +} + +fn main() { + let mut x = 1_i64; + let w = ((&mut x,),); + apply(w); + assert!(x == 2); +} diff --git a/tests/ui/pass/partial_move_sibling.rs b/tests/ui/pass/partial_move_sibling.rs new file mode 100644 index 00000000..a9a80ac3 --- /dev/null +++ b/tests/ui/pass/partial_move_sibling.rs @@ -0,0 +1,17 @@ +//@check-pass +//@compile-flags: -C debug-assertions=off + +// A partially-moved local with a still-owned sibling. `s.0` moves into `x` (its +// `&mut a` resolved through `x`); `s.1` stays owned by `s` and is mutated in +// place. Dropping `s` skips the moved-out `s.0` but must still resolve the +// sibling `s.1`'s `&mut b` prophecy exactly once, so both post-conditions hold. +fn main() { + let mut a = 1_i64; + let mut b = 2_i64; + let s = ((&mut a,), (&mut b,)); + let x = s.0; // partial move: s.0 moves out, s.1 still owned + *x.0 = 10; + *(s.1).0 = 20; + assert!(a == 10); + assert!(b == 20); +}