Auto merge of #87998 - nneonneo:master, r=oli-obk

Avoid spurious "previous iteration of loop" errors

Only follow backwards edges during `get_moved_indexes` if the move path is definitely initialized at loop entry. Otherwise, the error occurred prior to the loop, so we ignore the backwards edges to avoid generating misleading "value moved here, in previous iteration of loop" errors.

This patch also slightly improves the analysis of inits, including `NonPanicPathOnly` initializations (which are ignored by `drop_flag_effects::for_location_inits`). This is required for the definite initialization analysis, but may also help find certain skipped reinits in rare cases.

Patch passes all non-ignored src/test/ui testcases.

Fixes #72649.

Author: bors

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -10,8 +10,7 @@ use rustc_middle::mir::{
     ProjectionElem, Rvalue, Statement, StatementKind, Terminator, TerminatorKind, VarBindingForm,
 };
 use rustc_middle::ty::{self, suggest_constraining_type_param, Ty};
-use rustc_mir_dataflow::drop_flag_effects;
-use rustc_mir_dataflow::move_paths::{MoveOutIndex, MovePathIndex};
+use rustc_mir_dataflow::move_paths::{InitKind, MoveOutIndex, MovePathIndex};
 use rustc_span::source_map::DesugaringKind;
 use rustc_span::symbol::sym;
 use rustc_span::{BytePos, MultiSpan, Span, DUMMY_SP};
@@ -1531,25 +1530,45 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             }
         }
 
+        let mut mpis = vec![mpi];
+        let move_paths = &self.move_data.move_paths;
+        mpis.extend(move_paths[mpi].parents(move_paths).map(|(mpi, _)| mpi));
+
         let mut stack = Vec::new();
-        stack.extend(predecessor_locations(self.body, location).map(|predecessor| {
-            let is_back_edge = location.dominates(predecessor, &self.dominators);
-            (predecessor, is_back_edge)
-        }));
+        let mut back_edge_stack = Vec::new();
+
+        predecessor_locations(self.body, location).for_each(|predecessor| {
+            if location.dominates(predecessor, &self.dominators) {
+                back_edge_stack.push(predecessor)
+            } else {
+                stack.push(predecessor);
+            }
+        });
+
+        let mut reached_start = false;
+
+        /* Check if the mpi is initialized as an argument */
+        let mut is_argument = false;
+        for arg in self.body.args_iter() {
+            let path = self.move_data.rev_lookup.find_local(arg);
+            if mpis.contains(&path) {
+                is_argument = true;
+            }
+        }
 
         let mut visited = FxHashSet::default();
         let mut move_locations = FxHashSet::default();
         let mut reinits = vec![];
         let mut result = vec![];
 
-        'dfs: while let Some((location, is_back_edge)) = stack.pop() {
+        let mut dfs_iter = |result: &mut Vec<MoveSite>, location: Location, is_back_edge: bool| {
             debug!(
                 "report_use_of_moved_or_uninitialized: (current_location={:?}, back_edge={})",
                 location, is_back_edge
             );
 
             if !visited.insert(location) {
-                continue;
+                return true;
             }
 
             // check for moves
@@ -1568,10 +1587,6 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 // worry about the other case: that is, if there is a move of a.b.c, it is already
                 // marked as a move of a.b and a as well, so we will generate the correct errors
                 // there.
-                let mut mpis = vec![mpi];
-                let move_paths = &self.move_data.move_paths;
-                mpis.extend(move_paths[mpi].parents(move_paths).map(|(mpi, _)| mpi));
-
                 for moi in &self.move_data.loc_map[location] {
                     debug!("report_use_of_moved_or_uninitialized: moi={:?}", moi);
                     let path = self.move_data.moves[*moi].path;
@@ -1599,33 +1614,70 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                         // Because we stop the DFS here, we only highlight `let c = a`,
                         // and not `let b = a`. We will of course also report an error at
                         // `let c = a` which highlights `let b = a` as the move.
-                        continue 'dfs;
+                        return true;
                     }
                 }
             }
 
             // check for inits
             let mut any_match = false;
-            drop_flag_effects::for_location_inits(
-                self.infcx.tcx,
-                &self.body,
-                self.move_data,
-                location,
-                |m| {
-                    if m == mpi {
-                        any_match = true;
+            for ii in &self.move_data.init_loc_map[location] {
+                let init = self.move_data.inits[*ii];
+                match init.kind {
+                    InitKind::Deep | InitKind::NonPanicPathOnly => {
+                        if mpis.contains(&init.path) {
+                            any_match = true;
+                        }
                     }
-                },
-            );
+                    InitKind::Shallow => {
+                        if mpi == init.path {
+                            any_match = true;
+                        }
+                    }
+                }
+            }
             if any_match {
                 reinits.push(location);
-                continue 'dfs;
+                return true;
             }
+            return false;
+        };
 
-            stack.extend(predecessor_locations(self.body, location).map(|predecessor| {
-                let back_edge = location.dominates(predecessor, &self.dominators);
-                (predecessor, is_back_edge || back_edge)
-            }));
+        while let Some(location) = stack.pop() {
+            if dfs_iter(&mut result, location, false) {
+                continue;
+            }
+
+            let mut has_predecessor = false;
+            predecessor_locations(self.body, location).for_each(|predecessor| {
+                if location.dominates(predecessor, &self.dominators) {
+                    back_edge_stack.push(predecessor)
+                } else {
+                    stack.push(predecessor);
+                }
+                has_predecessor = true;
+            });
+
+            if !has_predecessor {
+                reached_start = true;
+            }
+        }
+        if (is_argument || !reached_start) && result.is_empty() {
+            /* Process back edges (moves in future loop iterations) only if
+               the move path is definitely initialized upon loop entry,
+               to avoid spurious "in previous iteration" errors.
+               During DFS, if there's a path from the error back to the start
+               of the function with no intervening init or move, then the
+               move path may be uninitialized at loop entry.
+            */
+            while let Some(location) = back_edge_stack.pop() {
+                if dfs_iter(&mut result, location, true) {
+                    continue;
+                }
+
+                predecessor_locations(self.body, location)
+                    .for_each(|predecessor| back_edge_stack.push(predecessor));
+            }
         }
 
         // Check if we can reach these reinits from a move location.

src/test/ui/moves/issue-72649-uninit-in-loop.rs

@@ -0,0 +1,74 @@
+// Regression test for issue #72649
+// Tests that we don't emit spurious
+// 'value moved in previous iteration of loop' message
+
+struct NonCopy;
+
+fn good() {
+    loop {
+        let value = NonCopy{};
+        let _used = value;
+    }
+}
+
+fn moved_here_1() {
+    loop {
+        let value = NonCopy{};
+        //~^ NOTE move occurs because `value` has type `NonCopy`, which does not implement the `Copy` trait
+        let _used = value;
+        //~^ NOTE value moved here
+        let _used2 = value; //~ ERROR use of moved value: `value`
+        //~^ NOTE value used here after move
+    }
+}
+
+fn moved_here_2() {
+    let value = NonCopy{};
+    //~^ NOTE move occurs because `value` has type `NonCopy`, which does not implement the `Copy` trait
+    loop {
+        let _used = value;
+        //~^ NOTE value moved here
+        loop {
+            let _used2 = value; //~ ERROR use of moved value: `value`
+            //~^ NOTE value used here after move
+        }
+    }
+}
+
+fn moved_loop_1() {
+    let value = NonCopy{};
+    //~^ NOTE move occurs because `value` has type `NonCopy`, which does not implement the `Copy` trait
+    loop {
+        let _used = value; //~ ERROR use of moved value: `value`
+        //~^ NOTE value moved here, in previous iteration of loop
+    }
+}
+
+fn moved_loop_2() {
+    let mut value = NonCopy{};
+    //~^ NOTE move occurs because `value` has type `NonCopy`, which does not implement the `Copy` trait
+    let _used = value;
+    value = NonCopy{};
+    loop {
+        let _used2 = value; //~ ERROR use of moved value: `value`
+        //~^ NOTE value moved here, in previous iteration of loop
+    }
+}
+
+fn uninit_1() {
+    loop {
+        let value: NonCopy;
+        let _used = value; //~ ERROR use of possibly-uninitialized variable: `value`
+        //~^ NOTE use of possibly-uninitialized `value`
+    }
+}
+
+fn uninit_2() {
+    let mut value: NonCopy;
+    loop {
+        let _used = value; //~ ERROR use of possibly-uninitialized variable: `value`
+        //~^ NOTE use of possibly-uninitialized `value`
+    }
+}
+
+fn main() {}

src/test/ui/moves/issue-72649-uninit-in-loop.stderr

@@ -0,0 +1,58 @@
+error[E0382]: use of moved value: `value`
+  --> $DIR/issue-72649-uninit-in-loop.rs:20:22
+   |
+LL |         let value = NonCopy{};
+   |             ----- move occurs because `value` has type `NonCopy`, which does not implement the `Copy` trait
+LL |
+LL |         let _used = value;
+   |                     ----- value moved here
+LL |
+LL |         let _used2 = value;
+   |                      ^^^^^ value used here after move
+
+error[E0382]: use of moved value: `value`
+  --> $DIR/issue-72649-uninit-in-loop.rs:32:26
+   |
+LL |     let value = NonCopy{};
+   |         ----- move occurs because `value` has type `NonCopy`, which does not implement the `Copy` trait
+...
+LL |         let _used = value;
+   |                     ----- value moved here
+...
+LL |             let _used2 = value;
+   |                          ^^^^^ value used here after move
+
+error[E0382]: use of moved value: `value`
+  --> $DIR/issue-72649-uninit-in-loop.rs:42:21
+   |
+LL |     let value = NonCopy{};
+   |         ----- move occurs because `value` has type `NonCopy`, which does not implement the `Copy` trait
+...
+LL |         let _used = value;
+   |                     ^^^^^ value moved here, in previous iteration of loop
+
+error[E0382]: use of moved value: `value`
+  --> $DIR/issue-72649-uninit-in-loop.rs:53:22
+   |
+LL |     let mut value = NonCopy{};
+   |         --------- move occurs because `value` has type `NonCopy`, which does not implement the `Copy` trait
+...
+LL |         let _used2 = value;
+   |                      ^^^^^ value moved here, in previous iteration of loop
+
+error[E0381]: use of possibly-uninitialized variable: `value`
+  --> $DIR/issue-72649-uninit-in-loop.rs:61:21
+   |
+LL |         let _used = value;
+   |                     ^^^^^ use of possibly-uninitialized `value`
+
+error[E0381]: use of possibly-uninitialized variable: `value`
+  --> $DIR/issue-72649-uninit-in-loop.rs:69:21
+   |
+LL |         let _used = value;
+   |                     ^^^^^ use of possibly-uninitialized `value`
+
+error: aborting due to 6 previous errors
+
+Some errors have detailed explanations: E0381, E0382.
+For more information about an error, try `rustc --explain E0381`.