Auto merge of #85686 - ptrojahn:loop_reinitialize, r=estebank

Add help on reinitialization between move and access

Fixes #83760

Author: bors

compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs

@@ -12,7 +12,7 @@ use rustc_middle::mir::{
 use rustc_middle::ty::{self, suggest_constraining_type_param, Ty};
 use rustc_span::source_map::DesugaringKind;
 use rustc_span::symbol::sym;
-use rustc_span::{Span, DUMMY_SP};
+use rustc_span::{MultiSpan, Span, DUMMY_SP};
 use rustc_trait_selection::infer::InferCtxtExt;
 
 use crate::dataflow::drop_flag_effects;
@@ -66,7 +66,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             self.move_spans(moved_place, location).or_else(|| self.borrow_spans(span, location));
         let span = use_spans.args_or_use();
 
-        let move_site_vec = self.get_moved_indexes(location, mpi);
+        let (move_site_vec, maybe_reinitialized_locations) = self.get_moved_indexes(location, mpi);
         debug!(
             "report_use_of_moved_or_uninitialized: move_site_vec={:?} use_spans={:?}",
             move_site_vec, use_spans
@@ -139,6 +139,32 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 self.describe_place_with_options(moved_place, IncludingDowncast(true)),
             );
 
+            let reinit_spans = maybe_reinitialized_locations
+                .iter()
+                .take(3)
+                .map(|loc| {
+                    self.move_spans(self.move_data.move_paths[mpi].place.as_ref(), *loc)
+                        .args_or_use()
+                })
+                .collect::<Vec<Span>>();
+            let reinits = maybe_reinitialized_locations.len();
+            if reinits == 1 {
+                err.span_label(reinit_spans[0], "this reinitialization might get skipped");
+            } else if reinits > 1 {
+                err.span_note(
+                    MultiSpan::from_spans(reinit_spans),
+                    &if reinits <= 3 {
+                        format!("these {} reinitializations might get skipped", reinits)
+                    } else {
+                        format!(
+                            "these 3 reinitializations and {} other{} might get skipped",
+                            reinits - 3,
+                            if reinits == 4 { "" } else { "s" }
+                        )
+                    },
+                );
+            }
+
             self.add_moved_or_invoked_closure_note(location, used_place, &mut err);
 
             let mut is_loop_move = false;
@@ -219,7 +245,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                                     ),
                                 );
                             }
-                            if is_option_or_result {
+                            if is_option_or_result && maybe_reinitialized_locations.is_empty() {
                                 err.span_suggestion_verbose(
                                     fn_call_span.shrink_to_lo(),
                                     "consider calling `.as_ref()` to borrow the type's contents",
@@ -260,19 +286,23 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                     }
                 }
 
-                if let UseSpans::PatUse(span) = move_spans {
-                    err.span_suggestion_verbose(
-                        span.shrink_to_lo(),
-                        &format!(
-                            "borrow this field in the pattern to avoid moving {}",
-                            self.describe_place(moved_place.as_ref())
-                                .map(|n| format!("`{}`", n))
-                                .unwrap_or_else(|| "the value".to_string())
-                        ),
-                        "ref ".to_string(),
-                        Applicability::MachineApplicable,
-                    );
-                    in_pattern = true;
+                if let (UseSpans::PatUse(span), []) =
+                    (move_spans, &maybe_reinitialized_locations[..])
+                {
+                    if maybe_reinitialized_locations.is_empty() {
+                        err.span_suggestion_verbose(
+                            span.shrink_to_lo(),
+                            &format!(
+                                "borrow this field in the pattern to avoid moving {}",
+                                self.describe_place(moved_place.as_ref())
+                                    .map(|n| format!("`{}`", n))
+                                    .unwrap_or_else(|| "the value".to_string())
+                            ),
+                            "ref ".to_string(),
+                            Applicability::MachineApplicable,
+                        );
+                        in_pattern = true;
+                    }
                 }
 
                 if let Some(DesugaringKind::ForLoop(_)) = move_span.desugaring_kind() {
@@ -1465,7 +1495,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         err
     }
 
-    fn get_moved_indexes(&mut self, location: Location, mpi: MovePathIndex) -> Vec<MoveSite> {
+    fn get_moved_indexes(
+        &mut self,
+        location: Location,
+        mpi: MovePathIndex,
+    ) -> (Vec<MoveSite>, Vec<Location>) {
         fn predecessor_locations(
             body: &'a mir::Body<'tcx>,
             location: Location,
@@ -1488,6 +1522,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         }));
 
         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() {
@@ -1529,6 +1565,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                             move_paths[path].place
                         );
                         result.push(MoveSite { moi: *moi, traversed_back_edge: is_back_edge });
+                        move_locations.insert(location);
 
                         // Strictly speaking, we could continue our DFS here. There may be
                         // other moves that can reach the point of error. But it is kind of
@@ -1565,6 +1602,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 },
             );
             if any_match {
+                reinits.push(location);
                 continue 'dfs;
             }
 
@@ -1574,7 +1612,25 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             }));
         }
 
-        result
+        // Check if we can reach these reinits from a move location.
+        let reinits_reachable = reinits
+            .into_iter()
+            .filter(|reinit| {
+                let mut visited = FxHashSet::default();
+                let mut stack = vec![*reinit];
+                while let Some(location) = stack.pop() {
+                    if !visited.insert(location) {
+                        continue;
+                    }
+                    if move_locations.contains(&location) {
+                        return true;
+                    }
+                    stack.extend(predecessor_locations(self.body, location));
+                }
+                false
+            })
+            .collect::<Vec<Location>>();
+        (result, reinits_reachable)
     }
 
     pub(in crate::borrow_check) fn report_illegal_mutation_of_borrowed(

src/test/ui/borrowck/issue-83760.rs

@@ -0,0 +1,40 @@
+struct Struct;
+
+fn test1() {
+    let mut val = Some(Struct);
+    while let Some(foo) = val { //~ ERROR use of moved value
+        if true {
+            val = None;
+        } else {
+
+        }
+    }
+}
+
+fn test2() {
+    let mut foo = Some(Struct);
+    let _x = foo.unwrap();
+    if true {
+        foo = Some(Struct);
+    } else {
+    }
+    let _y = foo; //~ ERROR use of moved value: `foo`
+}
+
+fn test3() {
+    let mut foo = Some(Struct);
+    let _x = foo.unwrap();
+    if true {
+        foo = Some(Struct);
+    } else if true {
+        foo = Some(Struct);
+    } else if true {
+        foo = Some(Struct);
+    } else if true {
+        foo = Some(Struct);
+    } else {
+    }
+    let _y = foo; //~ ERROR use of moved value: `foo`
+}
+
+fn main() {}

src/test/ui/borrowck/issue-83760.stderr

@@ -0,0 +1,62 @@
+error[E0382]: use of moved value
+  --> $DIR/issue-83760.rs:5:20
+   |
+LL |     while let Some(foo) = val {
+   |                    ^^^ value moved here, in previous iteration of loop
+LL |         if true {
+LL |             val = None;
+   |             ---------- this reinitialization might get skipped
+   |
+   = note: move occurs because value has type `Struct`, which does not implement the `Copy` trait
+
+error[E0382]: use of moved value: `foo`
+  --> $DIR/issue-83760.rs:21:14
+   |
+LL |     let mut foo = Some(Struct);
+   |         ------- move occurs because `foo` has type `Option<Struct>`, which does not implement the `Copy` trait
+LL |     let _x = foo.unwrap();
+   |                  -------- `foo` moved due to this method call
+LL |     if true {
+LL |         foo = Some(Struct);
+   |         ------------------ this reinitialization might get skipped
+...
+LL |     let _y = foo;
+   |              ^^^ value used here after move
+   |
+note: this function takes ownership of the receiver `self`, which moves `foo`
+  --> $SRC_DIR/core/src/option.rs:LL:COL
+   |
+LL |     pub const fn unwrap(self) -> T {
+   |                         ^^^^
+
+error[E0382]: use of moved value: `foo`
+  --> $DIR/issue-83760.rs:37:14
+   |
+LL |     let mut foo = Some(Struct);
+   |         ------- move occurs because `foo` has type `Option<Struct>`, which does not implement the `Copy` trait
+LL |     let _x = foo.unwrap();
+   |                  -------- `foo` moved due to this method call
+...
+LL |     let _y = foo;
+   |              ^^^ value used here after move
+   |
+note: these 3 reinitializations and 1 other might get skipped
+  --> $DIR/issue-83760.rs:30:9
+   |
+LL |         foo = Some(Struct);
+   |         ^^^^^^^^^^^^^^^^^^
+LL |     } else if true {
+LL |         foo = Some(Struct);
+   |         ^^^^^^^^^^^^^^^^^^
+LL |     } else if true {
+LL |         foo = Some(Struct);
+   |         ^^^^^^^^^^^^^^^^^^
+note: this function takes ownership of the receiver `self`, which moves `foo`
+  --> $SRC_DIR/core/src/option.rs:LL:COL
+   |
+LL |     pub const fn unwrap(self) -> T {
+   |                         ^^^^
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0382`.