Skip to content

Commit

Permalink
Detect when move of !Copy value occurs within loop and should lik…
Browse files Browse the repository at this point in the history
…ely 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<T>(vec: Vec<T>) -> impl Iterator<Item = T> {
   |    ----         ^^^^^^ 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 rust-lang#121466.
  • Loading branch information
estebank committed Feb 26, 2024
1 parent 933a05b commit 24c4bad
Show file tree
Hide file tree
Showing 13 changed files with 441 additions and 42 deletions.
166 changes: 164 additions & 2 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -443,8 +443,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",
),
);
}
Expand Down Expand Up @@ -738,6 +738,163 @@ 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 DiagnosticBuilder<'_>,
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 DiagnosticBuilder<'_>,
Expand All @@ -746,6 +903,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
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) {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2049,7 +2049,7 @@ impl LoopSource {
}
}

#[derive(Copy, Clone, Debug, HashStable_Generic)]
#[derive(Copy, Clone, Debug, PartialEq, HashStable_Generic)]
pub enum LoopIdError {
OutsideLoopScope,
UnlabeledCfInWhileCondition,
Expand Down
35 changes: 0 additions & 35 deletions tests/ui/borrowck/mut-borrow-in-loop-2.fixed

This file was deleted.

1 change: 0 additions & 1 deletion tests/ui/borrowck/mut-borrow-in-loop-2.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
//@ run-rustfix
#![allow(dead_code)]

struct Events<R>(R);
Expand Down
10 changes: 8 additions & 2 deletions tests/ui/borrowck/mut-borrow-in-loop-2.stderr
Original file line number Diff line number Diff line change
@@ -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<R>) {
| ----- move occurs because `value` has type `&mut Events<R>`, which does not implement the `Copy` trait
Expand All @@ -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);
Expand Down
6 changes: 6 additions & 0 deletions tests/ui/liveness/liveness-move-call-arg-2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ 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());
Expand Down
6 changes: 6 additions & 0 deletions tests/ui/liveness/liveness-move-call-arg.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ 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());
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/moves/borrow-closures-instead-of-move.rs
Original file line number Diff line number Diff line change
@@ -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
Expand Down
6 changes: 6 additions & 0 deletions tests/ui/moves/borrow-closures-instead-of-move.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
24 changes: 24 additions & 0 deletions tests/ui/moves/nested-loop-moved-value-wrong-continue.rs
Original file line number Diff line number Diff line change
@@ -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
}
}
35 changes: 35 additions & 0 deletions tests/ui/moves/nested-loop-moved-value-wrong-continue.stderr
Original file line number Diff line number Diff line change
@@ -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`.
Loading

0 comments on commit 24c4bad

Please sign in to comment.