diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index 8a8fffe0876..5b61223a67f 100644 --- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs +++ b/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, Diag, 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_middle::hir::nested_filter::OnlyBodies; @@ -799,9 +799,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; } @@ -850,51 +850,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::>(); + // 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 diff --git a/tests/ui/liveness/liveness-move-call-arg-2.stderr b/tests/ui/liveness/liveness-move-call-arg-2.stderr index 9b036541e8d..f4252e1ac33 100644 --- a/tests/ui/liveness/liveness-move-call-arg-2.stderr +++ b/tests/ui/liveness/liveness-move-call-arg-2.stderr @@ -16,12 +16,6 @@ LL | fn take(_x: Box) {} | ---- ^^^^^^^^^^ 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()); diff --git a/tests/ui/liveness/liveness-move-call-arg.stderr b/tests/ui/liveness/liveness-move-call-arg.stderr index b1d831b9ef9..9697ed5cb11 100644 --- a/tests/ui/liveness/liveness-move-call-arg.stderr +++ b/tests/ui/liveness/liveness-move-call-arg.stderr @@ -16,12 +16,6 @@ LL | fn take(_x: Box) {} | ---- ^^^^^^^^^^ 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()); diff --git a/tests/ui/moves/borrow-closures-instead-of-move.rs b/tests/ui/moves/borrow-closures-instead-of-move.rs index ab8a2802213..51771ced7f2 100644 --- a/tests/ui/moves/borrow-closures-instead-of-move.rs +++ b/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 diff --git a/tests/ui/moves/borrow-closures-instead-of-move.stderr b/tests/ui/moves/borrow-closures-instead-of-move.stderr index b2787fd5c72..9a84ddef7e6 100644 --- a/tests/ui/moves/borrow-closures-instead-of-move.stderr +++ b/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); diff --git a/tests/ui/moves/nested-loop-moved-value-wrong-continue.rs b/tests/ui/moves/nested-loop-moved-value-wrong-continue.rs index 618d820a2ab..0235b291df5 100644 --- a/tests/ui/moves/nested-loop-moved-value-wrong-continue.rs +++ b/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); diff --git a/tests/ui/moves/nested-loop-moved-value-wrong-continue.stderr b/tests/ui/moves/nested-loop-moved-value-wrong-continue.stderr index f04b5ecb935..3247513d42c 100644 --- a/tests/ui/moves/nested-loop-moved-value-wrong-continue.stderr +++ b/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`. diff --git a/tests/ui/moves/recreating-value-in-loop-condition.rs b/tests/ui/moves/recreating-value-in-loop-condition.rs index 03df8102410..000a8471e86 100644 --- a/tests/ui/moves/recreating-value-in-loop-condition.rs +++ b/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; } } } diff --git a/tests/ui/moves/recreating-value-in-loop-condition.stderr b/tests/ui/moves/recreating-value-in-loop-condition.stderr index ee2a68b72c1..fbf76c780d7 100644 --- a/tests/ui/moves/recreating-value-in-loop-condition.stderr +++ b/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(vec: Vec) -> impl Iterator { | ---- ^^^^^^ 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);