From 14473adf425623268252a4d10b5dd0d4f458daad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 26 Feb 2024 20:06:19 +0000 Subject: [PATCH 1/5] Detect when move of `!Copy` value occurs within `loop` and should likely not be cloned When encountering a move error on a value within a loop of any kind, identify if the moved value belongs to a call expression that should not be cloned and avoid the semantically incorrect suggestion. Also try to suggest moving the call expression outside of the loop instead. ``` error[E0382]: use of moved value: `vec` --> $DIR/recreating-value-in-loop-condition.rs:6:33 | LL | let vec = vec!["one", "two", "three"]; | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait LL | while let Some(item) = iter(vec).next() { | ----------------------------^^^-------- | | | | | value moved here, in previous iteration of loop | inside of this loop | note: consider changing this parameter type in function `iter` to borrow instead if owning the value isn't necessary --> $DIR/recreating-value-in-loop-condition.rs:1:17 | LL | fn iter(vec: Vec) -> impl Iterator { | ---- ^^^^^^ 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 = iter(vec); LL ~ while let Some(item) = value.next() { | ``` We use the presence of a `break` in the loop that would be affected by the moved value as a heuristic for "shouldn't be cloned". Fix #121466. --- .../src/diagnostics/conflict_errors.rs | 162 +++++++++++++++++- compiler/rustc_hir/src/hir.rs | 2 +- tests/ui/borrowck/mut-borrow-in-loop-2.fixed | 35 ---- tests/ui/borrowck/mut-borrow-in-loop-2.rs | 1 - tests/ui/borrowck/mut-borrow-in-loop-2.stderr | 10 +- .../liveness/liveness-move-call-arg-2.stderr | 6 + .../ui/liveness/liveness-move-call-arg.stderr | 6 + .../moves/borrow-closures-instead-of-move.rs | 2 +- .../borrow-closures-instead-of-move.stderr | 6 + .../nested-loop-moved-value-wrong-continue.rs | 24 +++ ...ted-loop-moved-value-wrong-continue.stderr | 35 ++++ .../recreating-value-in-loop-condition.rs | 57 ++++++ .../recreating-value-in-loop-condition.stderr | 133 ++++++++++++++ 13 files changed, 437 insertions(+), 42 deletions(-) delete mode 100644 tests/ui/borrowck/mut-borrow-in-loop-2.fixed create mode 100644 tests/ui/moves/nested-loop-moved-value-wrong-continue.rs create mode 100644 tests/ui/moves/nested-loop-moved-value-wrong-continue.stderr create mode 100644 tests/ui/moves/recreating-value-in-loop-condition.rs create mode 100644 tests/ui/moves/recreating-value-in-loop-condition.stderr diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index f81c74f3482c3..8a8fffe087671 100644 --- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs @@ -447,8 +447,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { err.span_note( span, format!( - "consider changing this parameter type in {descr} `{ident}` to \ - borrow instead if owning the value isn't necessary", + "consider changing this parameter type in {descr} `{ident}` to borrow \ + instead if owning the value isn't necessary", ), ); } @@ -747,8 +747,166 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { true } + /// In a move error that occurs on a call wihtin a loop, we try to identify cases where cloning + /// the value would lead to a logic error. We infer these cases by seeing if the moved value is + /// part of the logic to break the loop, either through an explicit `break` or if the expression + /// is part of a `while let`. + fn suggest_hoisting_call_outside_loop(&self, err: &mut Diag<'_>, expr: &hir::Expr<'_>) -> bool { + let tcx = self.infcx.tcx; + let mut can_suggest_clone = true; + + // If the moved value is a locally declared binding, we'll look upwards on the expression + // tree until the scope where it is defined, and no further, as suggesting to move the + // expression beyond that point would be illogical. + let local_hir_id = if let hir::ExprKind::Path(hir::QPath::Resolved( + _, + hir::Path { res: hir::def::Res::Local(local_hir_id), .. }, + )) = expr.kind + { + Some(local_hir_id) + } else { + // This case would be if the moved value comes from an argument binding, we'll just + // look within the entire item, that's fine. + None + }; + + /// This will allow us to look for a specific `HirId`, in our case `local_hir_id` where the + /// binding was declared, within any other expression. We'll use it to search for the + /// binding declaration within every scope we inspect. + struct Finder { + hir_id: hir::HirId, + found: bool, + } + impl<'hir> Visitor<'hir> for Finder { + fn visit_pat(&mut self, pat: &'hir hir::Pat<'hir>) { + if pat.hir_id == self.hir_id { + self.found = true; + } + hir::intravisit::walk_pat(self, pat); + } + fn visit_expr(&mut self, ex: &'hir hir::Expr<'hir>) { + if ex.hir_id == self.hir_id { + self.found = true; + } + hir::intravisit::walk_expr(self, ex); + } + } + // The immediate HIR parent of the moved expression. We'll look for it to be a call. + let mut parent = None; + // The top-most loop where the moved expression could be moved to a new binding. + let mut outer_most_loop: Option<&hir::Expr<'_>> = None; + for (_, node) in tcx.hir().parent_iter(expr.hir_id) { + let e = match node { + hir::Node::Expr(e) => e, + hir::Node::Local(hir::Local { els: Some(els), .. }) => { + let mut finder = BreakFinder { found_break: false }; + finder.visit_block(els); + if finder.found_break { + // Don't suggest clone as it could be will likely end in an infinite + // loop. + // let Some(_) = foo(non_copy.clone()) else { break; } + // --- ^^^^^^^^ ----- + can_suggest_clone = false; + } + continue; + } + _ => continue, + }; + if let Some(&hir_id) = local_hir_id { + let mut finder = Finder { hir_id, found: false }; + finder.visit_expr(e); + if finder.found { + // The current scope includes the declaration of the binding we're accessing, we + // can't look up any further for loops. + break; + } + } + if parent.is_none() { + parent = Some(e); + } + match e.kind { + hir::ExprKind::Let(_) => { + match tcx.parent_hir_node(e.hir_id) { + hir::Node::Expr(hir::Expr { + kind: hir::ExprKind::If(cond, ..), .. + }) => { + let mut finder = Finder { hir_id: expr.hir_id, found: false }; + finder.visit_expr(cond); + if finder.found { + // The expression where the move error happened is in a `while let` + // condition Don't suggest clone as it will likely end in an + // infinite loop. + // while let Some(_) = foo(non_copy.clone()) { } + // --------- ^^^^^^^^ + can_suggest_clone = false; + } + } + _ => {} + } + } + hir::ExprKind::Loop(..) => { + outer_most_loop = Some(e); + } + _ => {} + } + } + + /// 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, + } + 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; + } + 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 }; + 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() + }; + 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 + } + fn suggest_cloning(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, expr: &hir::Expr<'_>, span: Span) { let tcx = self.infcx.tcx; + if !self.suggest_hoisting_call_outside_loop(err, expr) { + // The place where the the type moves would be misleading to suggest clone. (#121466) + return; + } + // Try to find predicates on *generic params* that would allow copying `ty` let suggestion = if let Some(symbol) = tcx.hir().maybe_get_struct_pattern_shorthand_field(expr) { diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs index e21d31238e0dd..186b8716d9a51 100644 --- a/compiler/rustc_hir/src/hir.rs +++ b/compiler/rustc_hir/src/hir.rs @@ -2049,7 +2049,7 @@ impl LoopSource { } } -#[derive(Copy, Clone, Debug, HashStable_Generic)] +#[derive(Copy, Clone, Debug, PartialEq, HashStable_Generic)] pub enum LoopIdError { OutsideLoopScope, UnlabeledCfInWhileCondition, diff --git a/tests/ui/borrowck/mut-borrow-in-loop-2.fixed b/tests/ui/borrowck/mut-borrow-in-loop-2.fixed deleted file mode 100644 index cff3c372cdb90..0000000000000 --- a/tests/ui/borrowck/mut-borrow-in-loop-2.fixed +++ /dev/null @@ -1,35 +0,0 @@ -//@ run-rustfix -#![allow(dead_code)] - -struct Events(R); - -struct Other; - -pub trait Trait { - fn handle(value: T) -> Self; -} - -// Blanket impl. (If you comment this out, compiler figures out that it -// is passing an `&mut` to a method that must be expecting an `&mut`, -// and injects an auto-reborrow.) -impl Trait for T where T: From { - fn handle(_: U) -> Self { unimplemented!() } -} - -impl<'a, R> Trait<&'a mut Events> for Other { - fn handle(_: &'a mut Events) -> Self { unimplemented!() } -} - -fn this_compiles<'a, R>(value: &'a mut Events) { - for _ in 0..3 { - Other::handle(&mut *value); - } -} - -fn this_does_not<'a, R>(value: &'a mut Events) { - for _ in 0..3 { - Other::handle(&mut *value); //~ ERROR use of moved value: `value` - } -} - -fn main() {} diff --git a/tests/ui/borrowck/mut-borrow-in-loop-2.rs b/tests/ui/borrowck/mut-borrow-in-loop-2.rs index ba79b12042fba..f530dfca1a3ff 100644 --- a/tests/ui/borrowck/mut-borrow-in-loop-2.rs +++ b/tests/ui/borrowck/mut-borrow-in-loop-2.rs @@ -1,4 +1,3 @@ -//@ run-rustfix #![allow(dead_code)] struct Events(R); diff --git a/tests/ui/borrowck/mut-borrow-in-loop-2.stderr b/tests/ui/borrowck/mut-borrow-in-loop-2.stderr index 7b9a946f3ca3e..7a569d1da41c4 100644 --- a/tests/ui/borrowck/mut-borrow-in-loop-2.stderr +++ b/tests/ui/borrowck/mut-borrow-in-loop-2.stderr @@ -1,5 +1,5 @@ error[E0382]: use of moved value: `value` - --> $DIR/mut-borrow-in-loop-2.rs:31:23 + --> $DIR/mut-borrow-in-loop-2.rs:30:23 | LL | fn this_does_not<'a, R>(value: &'a mut Events) { | ----- move occurs because `value` has type `&mut Events`, which does not implement the `Copy` trait @@ -9,12 +9,18 @@ LL | Other::handle(value); | ^^^^^ value moved here, in previous iteration of loop | note: consider changing this parameter type in function `handle` to borrow instead if owning the value isn't necessary - --> $DIR/mut-borrow-in-loop-2.rs:9:22 + --> $DIR/mut-borrow-in-loop-2.rs:8:22 | LL | fn handle(value: T) -> Self; | ------ ^ 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 = Other::handle(value); +LL ~ for _ in 0..3 { +LL ~ value; + | help: consider creating a fresh reborrow of `value` here | LL | Other::handle(&mut *value); diff --git a/tests/ui/liveness/liveness-move-call-arg-2.stderr b/tests/ui/liveness/liveness-move-call-arg-2.stderr index f4252e1ac33c9..9b036541e8de6 100644 --- a/tests/ui/liveness/liveness-move-call-arg-2.stderr +++ b/tests/ui/liveness/liveness-move-call-arg-2.stderr @@ -16,6 +16,12 @@ 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 9697ed5cb114b..b1d831b9ef95f 100644 --- a/tests/ui/liveness/liveness-move-call-arg.stderr +++ b/tests/ui/liveness/liveness-move-call-arg.stderr @@ -16,6 +16,12 @@ 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 51771ced7f22f..ab8a280221358 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 { + loop { //~ HELP consider moving the expression out of the loop so it is only computed once 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 9a84ddef7e64e..b2787fd5c72f3 100644 --- a/tests/ui/moves/borrow-closures-instead-of-move.stderr +++ b/tests/ui/moves/borrow-closures-instead-of-move.stderr @@ -15,6 +15,12 @@ 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 new file mode 100644 index 0000000000000..618d820a2abea --- /dev/null +++ b/tests/ui/moves/nested-loop-moved-value-wrong-continue.rs @@ -0,0 +1,24 @@ +fn main() { + let foos = vec![String::new()]; + let bars = vec![""]; + let mut baz = vec![]; + let mut qux = vec![]; + for foo in foos { + //~^ NOTE this reinitialization might get skipped + //~| NOTE move occurs because `foo` has type `String` + for bar in &bars { + //~^ NOTE inside of this loop + //~| HELP consider moving the expression out of the loop + //~| NOTE in this expansion of desugaring of `for` loop + if foo == *bar { + baz.push(foo); + //~^ NOTE value moved here + //~| HELP consider cloning the value + continue; + } + } + qux.push(foo); + //~^ ERROR use of moved value + //~| NOTE value used here + } +} diff --git a/tests/ui/moves/nested-loop-moved-value-wrong-continue.stderr b/tests/ui/moves/nested-loop-moved-value-wrong-continue.stderr new file mode 100644 index 0000000000000..f04b5ecb935ce --- /dev/null +++ b/tests/ui/moves/nested-loop-moved-value-wrong-continue.stderr @@ -0,0 +1,35 @@ +error[E0382]: use of moved value: `foo` + --> $DIR/nested-loop-moved-value-wrong-continue.rs:20: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 + | +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()); + | ++++++++ + +error: aborting due to 1 previous error + +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 new file mode 100644 index 0000000000000..03df8102410d6 --- /dev/null +++ b/tests/ui/moves/recreating-value-in-loop-condition.rs @@ -0,0 +1,57 @@ +fn iter(vec: Vec) -> impl Iterator { + vec.into_iter() +} +fn foo() { + let vec = vec!["one", "two", "three"]; + while let Some(item) = iter(vec).next() { //~ ERROR use of moved value + println!("{:?}", item); + } +} +fn bar() { + let vec = vec!["one", "two", "three"]; + loop { + let Some(item) = iter(vec).next() else { //~ ERROR use of moved value + break; + }; + println!("{:?}", item); + } +} +fn baz() { + let vec = vec!["one", "two", "three"]; + loop { + let item = iter(vec).next(); //~ ERROR use of moved value + if item.is_none() { + break; + } + println!("{:?}", item); + } +} +fn qux() { + let vec = vec!["one", "two", "three"]; + loop { + if let Some(item) = iter(vec).next() { //~ ERROR use of moved value + println!("{:?}", item); + } + } +} +fn zap() { + loop { + let vec = vec!["one", "two", "three"]; + loop { + loop { + loop { + if let Some(item) = iter(vec).next() { //~ ERROR use of moved value + println!("{:?}", item); + } + } + } + } + } +} +fn main() { + foo(); + bar(); + baz(); + qux(); + zap(); +} diff --git a/tests/ui/moves/recreating-value-in-loop-condition.stderr b/tests/ui/moves/recreating-value-in-loop-condition.stderr new file mode 100644 index 0000000000000..ee2a68b72c103 --- /dev/null +++ b/tests/ui/moves/recreating-value-in-loop-condition.stderr @@ -0,0 +1,133 @@ +error[E0382]: use of moved value: `vec` + --> $DIR/recreating-value-in-loop-condition.rs:6:33 + | +LL | let vec = vec!["one", "two", "three"]; + | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait +LL | while let Some(item) = iter(vec).next() { + | ----------------------------^^^-------- + | | | + | | value moved here, in previous iteration of loop + | inside of this loop + | +note: consider changing this parameter type in function `iter` to borrow instead if owning the value isn't necessary + --> $DIR/recreating-value-in-loop-condition.rs:1:17 + | +LL | fn iter(vec: Vec) -> impl Iterator { + | ---- ^^^^^^ 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 = iter(vec); +LL ~ while let Some(item) = value.next() { + | + +error[E0382]: use of moved value: `vec` + --> $DIR/recreating-value-in-loop-condition.rs:13:31 + | +LL | let vec = vec!["one", "two", "three"]; + | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait +LL | loop { + | ---- inside of this loop +LL | let Some(item) = iter(vec).next() else { + | ^^^ value moved here, in previous iteration of loop + | +note: consider changing this parameter type in function `iter` to borrow instead if owning the value isn't necessary + --> $DIR/recreating-value-in-loop-condition.rs:1:17 + | +LL | fn iter(vec: Vec) -> impl Iterator { + | ---- ^^^^^^ 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 = iter(vec); +LL ~ loop { +LL ~ let Some(item) = value.next() else { + | + +error[E0382]: use of moved value: `vec` + --> $DIR/recreating-value-in-loop-condition.rs:22:25 + | +LL | let vec = vec!["one", "two", "three"]; + | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait +LL | loop { + | ---- inside of this loop +LL | let item = iter(vec).next(); + | ^^^ value moved here, in previous iteration of loop + | +note: consider changing this parameter type in function `iter` to borrow instead if owning the value isn't necessary + --> $DIR/recreating-value-in-loop-condition.rs:1:17 + | +LL | fn iter(vec: Vec) -> impl Iterator { + | ---- ^^^^^^ 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 = iter(vec); +LL ~ loop { +LL ~ let item = value.next(); + | +help: consider cloning the value if the performance cost is acceptable + | +LL | let item = iter(vec.clone()).next(); + | ++++++++ + +error[E0382]: use of moved value: `vec` + --> $DIR/recreating-value-in-loop-condition.rs:32:34 + | +LL | let vec = vec!["one", "two", "three"]; + | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait +LL | loop { + | ---- inside of this loop +LL | if let Some(item) = iter(vec).next() { + | ^^^ value moved here, in previous iteration of loop + | +note: consider changing this parameter type in function `iter` to borrow instead if owning the value isn't necessary + --> $DIR/recreating-value-in-loop-condition.rs:1:17 + | +LL | fn iter(vec: Vec) -> impl Iterator { + | ---- ^^^^^^ 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 = iter(vec); +LL ~ loop { +LL ~ if let Some(item) = value.next() { + | + +error[E0382]: use of moved value: `vec` + --> $DIR/recreating-value-in-loop-condition.rs:43:46 + | +LL | let vec = vec!["one", "two", "three"]; + | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait +LL | loop { + | ---- inside of this loop +LL | loop { + | ---- inside of this loop +LL | loop { + | ---- inside of this loop +LL | if let Some(item) = iter(vec).next() { + | ^^^ value moved here, in previous iteration of loop + | +note: consider changing this parameter type in function `iter` to borrow instead if owning the value isn't necessary + --> $DIR/recreating-value-in-loop-condition.rs:1:17 + | +LL | fn iter(vec: Vec) -> impl Iterator { + | ---- ^^^^^^ 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 = iter(vec); +LL ~ loop { +LL | loop { +LL | loop { +LL ~ if let Some(item) = value.next() { + | + +error: aborting due to 5 previous errors + +For more information about this error, try `rustc --explain E0382`. From 78d29ad8d6483ab01b8a380ff3a9598ae23cfa05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 26 Feb 2024 22:54:34 +0000 Subject: [PATCH 2/5] 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. --- .../src/diagnostics/conflict_errors.rs | 163 ++++++++++++++---- .../liveness/liveness-move-call-arg-2.stderr | 6 - .../ui/liveness/liveness-move-call-arg.stderr | 6 - .../moves/borrow-closures-instead-of-move.rs | 2 +- .../borrow-closures-instead-of-move.stderr | 6 - .../nested-loop-moved-value-wrong-continue.rs | 26 +++ ...ted-loop-moved-value-wrong-continue.stderr | 52 +++++- .../recreating-value-in-loop-condition.rs | 2 + .../recreating-value-in-loop-condition.stderr | 17 +- 9 files changed, 225 insertions(+), 55 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index 8a8fffe087671..5b61223a67f96 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 9b036541e8de6..f4252e1ac33c9 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 b1d831b9ef95f..9697ed5cb114b 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 ab8a280221358..51771ced7f22f 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 b2787fd5c72f3..9a84ddef7e64e 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 618d820a2abea..0235b291df540 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 f04b5ecb935ce..3247513d42cb8 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 03df8102410d6..000a8471e866f 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 ee2a68b72c103..fbf76c780d78c 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); From f216bac8618e6387cdc9cd25791347fc93889147 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 17 Mar 2024 21:45:03 +0000 Subject: [PATCH 3/5] Add `HELP` to test --- .../recreating-value-in-loop-condition.rs | 6 ++++++ .../recreating-value-in-loop-condition.stderr | 21 +++++++++++++------ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/tests/ui/moves/recreating-value-in-loop-condition.rs b/tests/ui/moves/recreating-value-in-loop-condition.rs index 000a8471e866f..ad012ff297813 100644 --- a/tests/ui/moves/recreating-value-in-loop-condition.rs +++ b/tests/ui/moves/recreating-value-in-loop-condition.rs @@ -4,12 +4,14 @@ fn iter(vec: Vec) -> impl Iterator { fn foo() { let vec = vec!["one", "two", "three"]; while let Some(item) = iter(vec).next() { //~ ERROR use of moved value + //~^ HELP consider moving the expression out of the loop so it is only moved once println!("{:?}", item); } } fn bar() { let vec = vec!["one", "two", "three"]; loop { + //~^ HELP consider moving the expression out of the loop so it is only moved once let Some(item) = iter(vec).next() else { //~ ERROR use of moved value break; }; @@ -19,7 +21,9 @@ fn bar() { fn baz() { let vec = vec!["one", "two", "three"]; loop { + //~^ HELP consider moving the expression out of the loop so it is only moved once let item = iter(vec).next(); //~ ERROR use of moved value + //~^ HELP consider cloning if item.is_none() { break; } @@ -29,6 +33,7 @@ fn baz() { fn qux() { let vec = vec!["one", "two", "three"]; loop { + //~^ HELP consider moving the expression out of the loop so it is only moved once if let Some(item) = iter(vec).next() { //~ ERROR use of moved value println!("{:?}", item); break; @@ -39,6 +44,7 @@ fn zap() { loop { let vec = vec!["one", "two", "three"]; loop { + //~^ HELP consider moving the expression out of the loop so it is only moved once loop { loop { if let Some(item) = iter(vec).next() { //~ ERROR use of moved value diff --git a/tests/ui/moves/recreating-value-in-loop-condition.stderr b/tests/ui/moves/recreating-value-in-loop-condition.stderr index fbf76c780d78c..75c9633bc0ae8 100644 --- a/tests/ui/moves/recreating-value-in-loop-condition.stderr +++ b/tests/ui/moves/recreating-value-in-loop-condition.stderr @@ -23,12 +23,13 @@ LL ~ while let Some(item) = value.next() { | error[E0382]: use of moved value: `vec` - --> $DIR/recreating-value-in-loop-condition.rs:13:31 + --> $DIR/recreating-value-in-loop-condition.rs:15:31 | LL | let vec = vec!["one", "two", "three"]; | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait LL | loop { | ---- inside of this loop +LL | LL | let Some(item) = iter(vec).next() else { | ^^^ value moved here, in previous iteration of loop | @@ -43,16 +44,18 @@ help: consider moving the expression out of the loop so it is only moved once | LL ~ let mut value = iter(vec); LL ~ loop { +LL | LL ~ let Some(item) = value.next() else { | error[E0382]: use of moved value: `vec` - --> $DIR/recreating-value-in-loop-condition.rs:22:25 + --> $DIR/recreating-value-in-loop-condition.rs:25:25 | LL | let vec = vec!["one", "two", "three"]; | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait LL | loop { | ---- inside of this loop +LL | LL | let item = iter(vec).next(); | ^^^ value moved here, in previous iteration of loop | @@ -67,6 +70,7 @@ help: consider moving the expression out of the loop so it is only moved once | LL ~ let mut value = iter(vec); LL ~ loop { +LL | LL ~ let item = value.next(); | help: consider cloning the value if the performance cost is acceptable @@ -75,12 +79,13 @@ LL | let item = iter(vec.clone()).next(); | ++++++++ error[E0382]: use of moved value: `vec` - --> $DIR/recreating-value-in-loop-condition.rs:32:34 + --> $DIR/recreating-value-in-loop-condition.rs:37:34 | LL | let vec = vec!["one", "two", "three"]; | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait LL | loop { | ---- inside of this loop +LL | LL | if let Some(item) = iter(vec).next() { | ^^^ value moved here, in previous iteration of loop | @@ -95,16 +100,18 @@ help: consider moving the expression out of the loop so it is only moved once | LL ~ let mut value = iter(vec); LL ~ loop { +LL | LL ~ if let Some(item) = value.next() { | error[E0382]: use of moved value: `vec` - --> $DIR/recreating-value-in-loop-condition.rs:44:46 + --> $DIR/recreating-value-in-loop-condition.rs:50:46 | LL | let vec = vec!["one", "two", "three"]; | --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait LL | loop { | ---- inside of this loop +LL | LL | loop { | ---- inside of this loop LL | loop { @@ -120,24 +127,26 @@ LL | fn iter(vec: Vec) -> impl Iterator { | | | in this function note: verify that your loop breaking logic is correct - --> $DIR/recreating-value-in-loop-condition.rs:46:25 + --> $DIR/recreating-value-in-loop-condition.rs:52:25 | LL | loop { | ---- LL | let vec = vec!["one", "two", "three"]; LL | loop { | ---- +LL | LL | loop { | ---- LL | loop { | ---- ... LL | break; - | ^^^^^ this `break` exits the loop at line 43 + | ^^^^^ this `break` exits the loop at line 49 help: consider moving the expression out of the loop so it is only moved once | LL ~ let mut value = iter(vec); LL ~ loop { +LL | LL | loop { LL | loop { LL ~ if let Some(item) = value.next() { From da2364d746291a5a1f68106d64df7402b6276df5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 17 Mar 2024 21:46:52 +0000 Subject: [PATCH 4/5] Move `Visitor` impl out to the `mod` level --- .../src/diagnostics/conflict_errors.rs | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index 5b61223a67f96..85e3788d1e3f2 100644 --- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs @@ -859,28 +859,6 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { }) .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_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>) { - 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); - } - } - let sm = tcx.sess.source_map(); if let Some(in_loop) = outer_most_loop { let mut finder = BreakFinder { found_breaks: vec![], found_continues: vec![] }; @@ -3943,6 +3921,28 @@ impl<'a, 'v> Visitor<'v> for ReferencedStatementsVisitor<'a> { } } +/// 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_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>) { + 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); + } +} + /// Given a set of spans representing statements initializing the relevant binding, visit all the /// function expressions looking for branching code paths that *do not* initialize the binding. struct ConditionVisitor<'b> { From 3b237d7d8ad46259856a42043ae37b25c6496f7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 17 Mar 2024 21:52:12 +0000 Subject: [PATCH 5/5] Move `suggest_hoisting_call_outside_loop` out of `suggest_cloning` --- .../rustc_borrowck/src/diagnostics/conflict_errors.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index 85e3788d1e3f2..0109f188772bc 100644 --- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs @@ -463,7 +463,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { } else if let UseSpans::FnSelfUse { kind: CallKind::Normal { .. }, .. } = move_spans { // We already suggest cloning for these cases in `explain_captures`. - } else { + } else if self.suggest_hoisting_call_outside_loop(err, expr) { + // The place where the the type moves would be misleading to suggest clone. + // #121466 self.suggest_cloning(err, ty, expr, move_span); } } @@ -977,11 +979,6 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { fn suggest_cloning(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, expr: &hir::Expr<'_>, span: Span) { let tcx = self.infcx.tcx; - if !self.suggest_hoisting_call_outside_loop(err, expr) { - // The place where the the type moves would be misleading to suggest clone. (#121466) - return; - } - // Try to find predicates on *generic params* that would allow copying `ty` let suggestion = if let Some(symbol) = tcx.hir().maybe_get_struct_pattern_shorthand_field(expr) {