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] 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 f81c74f3482..8a8fffe0876 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 e21d31238e0..186b8716d9a 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 cff3c372cdb..00000000000 --- 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 ba79b12042f..f530dfca1a3 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 7b9a946f3ca..7a569d1da41 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 f4252e1ac33..9b036541e8d 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 9697ed5cb11..b1d831b9ef9 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 51771ced7f2..ab8a2802213 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 9a84ddef7e6..b2787fd5c72 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 00000000000..618d820a2ab --- /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 00000000000..f04b5ecb935 --- /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 00000000000..03df8102410 --- /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 00000000000..ee2a68b72c1 --- /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`.