Detect borrow checker errors where `.clone()` would be an appropriate user action #122603

estebank · 2024-03-16T18:41:38Z

When a value is moved twice, suggest cloning the earlier move:

error[E0509]: cannot move out of type `U2`, which implements the `Drop` trait
  --> $DIR/union-move.rs:49:18
   |
LL |         move_out(x.f1_nocopy);
   |                  ^^^^^^^^^^^
   |                  |
   |                  cannot move out of here
   |                  move occurs because `x.f1_nocopy` has type `ManuallyDrop<RefCell<i32>>`, which does not implement the `Copy` trait
   |
help: consider cloning the value if the performance cost is acceptable
   |
LL |         move_out(x.f1_nocopy.clone());
   |                             ++++++++

When a value is borrowed by an fn call, consider if cloning the result of the call would be reasonable, and suggest cloning that, instead of the argument:

error[E0505]: cannot move out of `a` because it is borrowed
  --> $DIR/variance-issue-20533.rs:53:14
   |
LL |         let a = AffineU32(1);
   |             - binding `a` declared here
LL |         let x = bat(&a);
   |                     -- borrow of `a` occurs here
LL |         drop(a);
   |              ^ move out of `a` occurs here
LL |         drop(x);
   |              - borrow later used here
   |
help: consider cloning the value if the performance cost is acceptable
   |
LL |         let x = bat(&a).clone();
   |                        ++++++++

otherwise, suggest cloning the argument:

error[E0505]: cannot move out of `a` because it is borrowed
  --> $DIR/variance-issue-20533.rs:59:14
   |
LL |         let a = ClonableAffineU32(1);
   |             - binding `a` declared here
LL |         let x = foo(&a);
   |                     -- borrow of `a` occurs here
LL |         drop(a);
   |              ^ move out of `a` occurs here
LL |         drop(x);
   |              - borrow later used here
   |
help: consider cloning the value if the performance cost is acceptable
   |
LL -         let x = foo(&a);
LL +         let x = foo(a.clone());
   |

This suggestion doesn't attempt to square out the types between what's cloned and what the fn expects, to allow the user to make a determination on whether to change the fn call or fn definition themselves.

Special case move errors caused by FnOnce:

error[E0382]: use of moved value: `blk`
  --> $DIR/once-cant-call-twice-on-heap.rs:8:5
   |
LL | fn foo<F:FnOnce()>(blk: F) {
   |                    --- move occurs because `blk` has type `F`, which does not implement the `Copy` trait
LL |     blk();
   |     ----- `blk` moved due to this call
LL |     blk();
   |     ^^^ value used here after move
   |
note: `FnOnce` closures can only be called once
  --> $DIR/once-cant-call-twice-on-heap.rs:6:10
   |
LL | fn foo<F:FnOnce()>(blk: F) {
   |          ^^^^^^^^ `F` is made to be an `FnOnce` closure here
LL |     blk();
   |     ----- this value implements `FnOnce`, which causes it to be moved when called

Account for redundant .clone() calls in resulting suggestions:

error[E0507]: cannot move out of dereference of `S`
  --> $DIR/needs-clone-through-deref.rs:15:18
   |
LL |         for _ in self.clone().into_iter() {}
   |                  ^^^^^^^^^^^^ ----------- value moved due to this method call
   |                  |
   |                  move occurs because value has type `Vec<usize>`, which does not implement the `Copy` trait
   |
note: `into_iter` takes ownership of the receiver `self`, which moves value
  --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
help: you can `clone` the value and consume it, but this might not be your desired behavior
   |
LL |         for _ in <Vec<usize> as Clone>::clone(&self).into_iter() {}
   |                  ++++++++++++++++++++++++++++++    ~

We use the presence of &mut values in a move error as a proxy for the user caring about side effects, so we don't emit a clone suggestion in that case:

error[E0505]: cannot move out of `s` because it is borrowed
  --> $DIR/borrowck-overloaded-index-move-index.rs:53:7
   |
LL |     let mut s = "hello".to_string();
   |         ----- binding `s` declared here
LL |     let rs = &mut s;
   |              ------ borrow of `s` occurs here
...
LL |     f[s] = 10;
   |       ^ move out of `s` occurs here
...
LL |     use_mut(rs);
   |             -- borrow later used here

We properly account for foo += foo; errors where we don't suggest foo.clone() += foo;, instead suggesting foo += foo.clone();.

Each commit can be reviewed in isolation. There are some "cleanup" commits, but kept them separate in order to show why specific changes were being made, and their effect on tests' output.

Fix #49693, CC #64167.

rustbot · 2024-03-16T18:41:45Z

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

bors · 2024-03-18T05:05:06Z

☔ The latest upstream changes (presumably #121652) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr

went through this PR and it mostly looks good.

I ended up only scimming over some of the functions as if let chains with 10 components are difficult to read.

nits, then r=me

lcnr · 2024-03-22T11:04:12Z