Point at continue and break that might be in the wrong place

Sometimes move errors are because of a misplaced `continue`, but we didn't
surface that anywhere. Now when there are more than one set of nested loops
we show them out and point at the `continue` and `break` expressions within
that might need to go elsewhere.

```
error[E0382]: use of moved value: `foo`
  --> $DIR/nested-loop-moved-value-wrong-continue.rs:46:18
   |
LL |     for foo in foos {
   |         ---
   |         |
   |         this reinitialization might get skipped
   |         move occurs because `foo` has type `String`, which does not implement the `Copy` trait
...
LL |         for bar in &bars {
   |         ---------------- inside of this loop
...
LL |                 baz.push(foo);
   |                          --- value moved here, in previous iteration of loop
...
LL |         qux.push(foo);
   |                  ^^^ value used here after move
   |
note: verify that your loop breaking logic is correct
  --> $DIR/nested-loop-moved-value-wrong-continue.rs:41:17
   |
LL |     for foo in foos {
   |     ---------------
...
LL |         for bar in &bars {
   |         ----------------
...
LL |                 continue;
   |                 ^^^^^^^^ this `continue` advances the loop at line 33
help: consider moving the expression out of the loop so it is only moved once
   |
LL ~         let mut value = baz.push(foo);
LL ~         for bar in &bars {
LL |
 ...
LL |             if foo == *bar {
LL ~                 value;
   |
help: consider cloning the value if the performance cost is acceptable
   |
LL |                 baz.push(foo.clone());
   |                             ++++++++
```

Fix #92531.

Author: estebank

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -9,7 +9,7 @@ use rustc_data_structures::fx::FxIndexSet;
 use rustc_errors::{codes::*, struct_span_code_err, Applicability, DiagnosticBuilder, MultiSpan};
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
-use rustc_hir::intravisit::{walk_block, walk_expr, Visitor};
+use rustc_hir::intravisit::{walk_block, walk_expr, Map, Visitor};
 use rustc_hir::{CoroutineDesugaring, PatField};
 use rustc_hir::{CoroutineKind, CoroutineSource, LangItem};
 use rustc_infer::traits::ObligationCause;
@@ -794,9 +794,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             let e = match node {
                 hir::Node::Expr(e) => e,
                 hir::Node::Local(hir::Local { els: Some(els), .. }) => {
-                    let mut finder = BreakFinder { found_break: false };
+                    let mut finder = BreakFinder { found_breaks: vec![], found_continues: vec![] };
                     finder.visit_block(els);
-                    if finder.found_break {
+                    if !finder.found_breaks.is_empty() {
                         // Don't suggest clone as it could be will likely end in an infinite
                         // loop.
                         // let Some(_) = foo(non_copy.clone()) else { break; }
@@ -845,51 +845,148 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 _ => {}
             }
         }
+        let loop_count: usize = tcx
+            .hir()
+            .parent_iter(expr.hir_id)
+            .map(|(_, node)| match node {
+                hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Loop(..), .. }) => 1,
+                _ => 0,
+            })
+            .sum();
 
         /// Look for `break` expressions within any arbitrary expressions. We'll do this to infer
         /// whether this is a case where the moved value would affect the exit of a loop, making it
         /// unsuitable for a `.clone()` suggestion.
         struct BreakFinder {
-            found_break: bool,
+            found_breaks: Vec<(hir::Destination, Span)>,
+            found_continues: Vec<(hir::Destination, Span)>,
         }
         impl<'hir> Visitor<'hir> for BreakFinder {
             fn visit_expr(&mut self, ex: &'hir hir::Expr<'hir>) {
-                if let hir::ExprKind::Break(..) = ex.kind {
-                    self.found_break = true;
+                match ex.kind {
+                    hir::ExprKind::Break(destination, _) => {
+                        self.found_breaks.push((destination, ex.span));
+                    }
+                    hir::ExprKind::Continue(destination) => {
+                        self.found_continues.push((destination, ex.span));
+                    }
+                    _ => {}
                 }
                 hir::intravisit::walk_expr(self, ex);
             }
         }
-        if let Some(in_loop) = outer_most_loop
-            && let Some(parent) = parent
-            && let hir::ExprKind::MethodCall(..) | hir::ExprKind::Call(..) = parent.kind
-        {
-            // FIXME: We could check that the call's *parent* takes `&mut val` to make the
-            // suggestion more targeted to the `mk_iter(val).next()` case. Maybe do that only to
-            // check for wheter to suggest `let value` or `let mut value`.
 
-            let span = in_loop.span;
-            let mut finder = BreakFinder { found_break: false };
+        let sm = tcx.sess.source_map();
+        if let Some(in_loop) = outer_most_loop {
+            let mut finder = BreakFinder { found_breaks: vec![], found_continues: vec![] };
             finder.visit_expr(in_loop);
-            let sm = tcx.sess.source_map();
-            if (finder.found_break || true)
-                && let Ok(value) = sm.span_to_snippet(parent.span)
-            {
-                // We know with high certainty that this move would affect the early return of a
-                // loop, so we suggest moving the expression with the move out of the loop.
-                let indent = if let Some(indent) = sm.indentation_before(span) {
-                    format!("\n{indent}")
-                } else {
-                    " ".to_string()
+            // All of the spans for `break` and `continue` expressions.
+            let spans = finder
+                .found_breaks
+                .iter()
+                .chain(finder.found_continues.iter())
+                .map(|(_, span)| *span)
+                .filter(|span| {
+                    !matches!(
+                        span.desugaring_kind(),
+                        Some(DesugaringKind::ForLoop | DesugaringKind::WhileLoop)
+                    )
+                })
+                .collect::<Vec<Span>>();
+            // All of the spans for the loops above the expression with the move error.
+            let loop_spans: Vec<_> = tcx
+                .hir()
+                .parent_iter(expr.hir_id)
+                .filter_map(|(_, node)| match node {
+                    hir::Node::Expr(hir::Expr { span, kind: hir::ExprKind::Loop(..), .. }) => {
+                        Some(*span)
+                    }
+                    _ => None,
+                })
+                .collect();
+            // It is possible that a user written `break` or `continue` is in the wrong place. We
+            // point them out at the user for them to make a determination. (#92531)
+            if !spans.is_empty() && loop_count > 1 {
+                // Getting fancy: if the spans of the loops *do not* overlap, we only use the line
+                // number when referring to them. If there *are* overlaps (multiple loops on the
+                // same line) then we use the more verbose span output (`file.rs:col:ll`).
+                let mut lines: Vec<_> =
+                    loop_spans.iter().map(|sp| sm.lookup_char_pos(sp.lo()).line).collect();
+                lines.sort();
+                lines.dedup();
+                let fmt_span = |span: Span| {
+                    if lines.len() == loop_spans.len() {
+                        format!("line {}", sm.lookup_char_pos(span.lo()).line)
+                    } else {
+                        sm.span_to_diagnostic_string(span)
+                    }
                 };
-                err.multipart_suggestion(
-                    "consider moving the expression out of the loop so it is only moved once",
-                    vec![
-                        (parent.span, "value".to_string()),
-                        (span.shrink_to_lo(), format!("let mut value = {value};{indent}")),
-                    ],
-                    Applicability::MaybeIncorrect,
-                );
+                let mut spans: MultiSpan = spans.clone().into();
+                // Point at all the `continue`s and explicit `break`s in the relevant loops.
+                for (desc, elements) in [
+                    ("`break` exits", &finder.found_breaks),
+                    ("`continue` advances", &finder.found_continues),
+                ] {
+                    for (destination, sp) in elements {
+                        if let Ok(hir_id) = destination.target_id
+                            && let hir::Node::Expr(expr) = tcx.hir().hir_node(hir_id)
+                            && !matches!(
+                                sp.desugaring_kind(),
+                                Some(DesugaringKind::ForLoop | DesugaringKind::WhileLoop)
+                            )
+                        {
+                            spans.push_span_label(
+                                *sp,
+                                format!("this {desc} the loop at {}", fmt_span(expr.span)),
+                            );
+                        }
+                    }
+                }
+                // Point at all the loops that are between this move and the parent item.
+                for span in loop_spans {
+                    spans.push_span_label(sm.guess_head_span(span), "");
+                }
+
+                // note: verify that your loop breaking logic is correct
+                //   --> $DIR/nested-loop-moved-value-wrong-continue.rs:41:17
+                //    |
+                // 28 |     for foo in foos {
+                //    |     ---------------
+                // ...
+                // 33 |         for bar in &bars {
+                //    |         ----------------
+                // ...
+                // 41 |                 continue;
+                //    |                 ^^^^^^^^ this `continue` advances the loop at line 33
+                err.span_note(spans, "verify that your loop breaking logic is correct");
+            }
+            if let Some(parent) = parent
+                && let hir::ExprKind::MethodCall(..) | hir::ExprKind::Call(..) = parent.kind
+            {
+                // FIXME: We could check that the call's *parent* takes `&mut val` to make the
+                // suggestion more targeted to the `mk_iter(val).next()` case. Maybe do that only to
+                // check for wheter to suggest `let value` or `let mut value`.
+
+                let span = in_loop.span;
+                if !finder.found_breaks.is_empty()
+                    && let Ok(value) = sm.span_to_snippet(parent.span)
+                {
+                    // We know with high certainty that this move would affect the early return of a
+                    // loop, so we suggest moving the expression with the move out of the loop.
+                    let indent = if let Some(indent) = sm.indentation_before(span) {
+                        format!("\n{indent}")
+                    } else {
+                        " ".to_string()
+                    };
+                    err.multipart_suggestion(
+                        "consider moving the expression out of the loop so it is only moved once",
+                        vec![
+                            (parent.span, "value".to_string()),
+                            (span.shrink_to_lo(), format!("let mut value = {value};{indent}")),
+                        ],
+                        Applicability::MaybeIncorrect,
+                    );
+                }
             }
         }
         can_suggest_clone

tests/ui/liveness/liveness-move-call-arg-2.stderr

@@ -16,12 +16,6 @@ LL | fn take(_x: Box<isize>) {}
    |    ----     ^^^^^^^^^^ this parameter takes ownership of the value
    |    |
    |    in this function
-help: consider moving the expression out of the loop so it is only moved once
-   |
-LL ~         let mut value = take(x);
-LL ~         loop {
-LL ~             value;
-   |
 help: consider cloning the value if the performance cost is acceptable
    |
 LL |             take(x.clone());

tests/ui/liveness/liveness-move-call-arg.stderr

@@ -16,12 +16,6 @@ LL | fn take(_x: Box<isize>) {}
    |    ----     ^^^^^^^^^^ this parameter takes ownership of the value
    |    |
    |    in this function
-help: consider moving the expression out of the loop so it is only moved once
-   |
-LL ~     let mut value = take(x);
-LL ~     loop {
-LL ~         value;
-   |
 help: consider cloning the value if the performance cost is acceptable
    |
 LL |         take(x.clone());

tests/ui/moves/borrow-closures-instead-of-move.rs

@@ -1,5 +1,5 @@
 fn takes_fn(f: impl Fn()) {
-    loop { //~ HELP consider moving the expression out of the loop so it is only computed once
+    loop {
         takes_fnonce(f);
         //~^ ERROR use of moved value
         //~| HELP consider borrowing

tests/ui/moves/borrow-closures-instead-of-move.stderr

@@ -15,12 +15,6 @@ LL | fn takes_fnonce(_: impl FnOnce()) {}
    |    ------------    ^^^^^^^^^^^^^ this parameter takes ownership of the value
    |    |
    |    in this function
-help: consider moving the expression out of the loop so it is only moved once
-   |
-LL ~     let mut value = takes_fnonce(f);
-LL ~     loop {
-LL ~         value;
-   |
 help: consider borrowing `f`
    |
 LL |         takes_fnonce(&f);

tests/ui/moves/nested-loop-moved-value-wrong-continue.rs

@@ -1,3 +1,27 @@
+fn foo() {
+    let foos = vec![String::new()];
+    let bars = vec![""];
+    let mut baz = vec![];
+    let mut qux = vec![];
+    for foo in foos { for bar in &bars { if foo == *bar {
+    //~^ NOTE this reinitialization might get skipped
+    //~| NOTE move occurs because `foo` has type `String`
+    //~| NOTE inside of this loop
+    //~| HELP consider moving the expression out of the loop
+    //~| NOTE in this expansion of desugaring of `for` loop
+        baz.push(foo);
+        //~^ NOTE value moved here
+        //~| HELP consider cloning the value
+        continue;
+        //~^ NOTE verify that your loop breaking logic is correct
+        //~| NOTE this `continue` advances the loop at $DIR/nested-loop-moved-value-wrong-continue.rs:6:23
+    } }
+    qux.push(foo);
+    //~^ ERROR use of moved value
+    //~| NOTE value used here
+    }
+}
+
 fn main() {
     let foos = vec![String::new()];
     let bars = vec![""];
@@ -15,6 +39,8 @@ fn main() {
                 //~^ NOTE value moved here
                 //~| HELP consider cloning the value
                 continue;
+                //~^ NOTE verify that your loop breaking logic is correct
+                //~| NOTE this `continue` advances the loop at line 33
             }
         }
         qux.push(foo);

tests/ui/moves/nested-loop-moved-value-wrong-continue.stderr

@@ -1,5 +1,42 @@
 error[E0382]: use of moved value: `foo`
-  --> $DIR/nested-loop-moved-value-wrong-continue.rs:20:18
+  --> $DIR/nested-loop-moved-value-wrong-continue.rs:19:14
+   |
+LL |     for foo in foos { for bar in &bars { if foo == *bar {
+   |         ---           ---------------- inside of this loop
+   |         |
+   |         this reinitialization might get skipped
+   |         move occurs because `foo` has type `String`, which does not implement the `Copy` trait
+...
+LL |         baz.push(foo);
+   |                  --- value moved here, in previous iteration of loop
+...
+LL |     qux.push(foo);
+   |              ^^^ value used here after move
+   |
+note: verify that your loop breaking logic is correct
+  --> $DIR/nested-loop-moved-value-wrong-continue.rs:15:9
+   |
+LL |     for foo in foos { for bar in &bars { if foo == *bar {
+   |     ---------------   ----------------
+...
+LL |         continue;
+   |         ^^^^^^^^ this `continue` advances the loop at $DIR/nested-loop-moved-value-wrong-continue.rs:6:23: 18:8
+help: consider moving the expression out of the loop so it is only moved once
+   |
+LL ~     for foo in foos { let mut value = baz.push(foo);
+LL ~     for bar in &bars { if foo == *bar {
+LL |
+ ...
+LL |
+LL ~         value;
+   |
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |         baz.push(foo.clone());
+   |                     ++++++++
+
+error[E0382]: use of moved value: `foo`
+  --> $DIR/nested-loop-moved-value-wrong-continue.rs:46:18
    |
 LL |     for foo in foos {
    |         ---
@@ -16,6 +53,17 @@ LL |                 baz.push(foo);
 LL |         qux.push(foo);
    |                  ^^^ value used here after move
    |
+note: verify that your loop breaking logic is correct
+  --> $DIR/nested-loop-moved-value-wrong-continue.rs:41:17
+   |
+LL |     for foo in foos {
+   |     ---------------
+...
+LL |         for bar in &bars {
+   |         ----------------
+...
+LL |                 continue;
+   |                 ^^^^^^^^ this `continue` advances the loop at line 33
 help: consider moving the expression out of the loop so it is only moved once
    |
 LL ~         let mut value = baz.push(foo);
@@ -30,6 +78,6 @@ help: consider cloning the value if the performance cost is acceptable
 LL |                 baz.push(foo.clone());
    |                             ++++++++
 
-error: aborting due to 1 previous error
+error: aborting due to 2 previous errors
 
 For more information about this error, try `rustc --explain E0382`.

tests/ui/moves/recreating-value-in-loop-condition.rs

@@ -31,6 +31,7 @@ fn qux() {
     loop {
         if let Some(item) = iter(vec).next() { //~ ERROR use of moved value
             println!("{:?}", item);
+            break;
         }
     }
 }
@@ -42,6 +43,7 @@ fn zap() {
                 loop {
                     if let Some(item) = iter(vec).next() { //~ ERROR use of moved value
                         println!("{:?}", item);
+                        break;
                     }
                 }
             }

tests/ui/moves/recreating-value-in-loop-condition.stderr

@@ -99,7 +99,7 @@ LL ~         if let Some(item) = value.next() {
    |
 
 error[E0382]: use of moved value: `vec`
-  --> $DIR/recreating-value-in-loop-condition.rs:43:46
+  --> $DIR/recreating-value-in-loop-condition.rs:44:46
    |
 LL |         let vec = vec!["one", "two", "three"];
    |             --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait
@@ -119,6 +119,21 @@ LL | fn iter<T>(vec: Vec<T>) -> impl Iterator<Item = T> {
    |    ----         ^^^^^^ this parameter takes ownership of the value
    |    |
    |    in this function
+note: verify that your loop breaking logic is correct
+  --> $DIR/recreating-value-in-loop-condition.rs:46:25
+   |
+LL |     loop {
+   |     ----
+LL |         let vec = vec!["one", "two", "three"];
+LL |         loop {
+   |         ----
+LL |             loop {
+   |             ----
+LL |                 loop {
+   |                 ----
+...
+LL |                         break;
+   |                         ^^^^^ this `break` exits the loop at line 43
 help: consider moving the expression out of the loop so it is only moved once
    |
 LL ~         let mut value = iter(vec);