Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving out of field error should emit resolution suggestions #64167

Open
oli-cosmian opened this issue Sep 5, 2019 · 3 comments
Open

Moving out of field error should emit resolution suggestions #64167

oli-cosmian opened this issue Sep 5, 2019 · 3 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oli-cosmian
Copy link

The current error doesn't help users resolve the problem. Even just including a message that one should try borrowing the field by prepending a & would be of help. I don't think it matters that this isn't possible for e.g. self.foo.consuming_method()

struct Foo {
    v: Vec<u32>,
}

impl Foo {
    fn bar(&self) {
        for _ in self.v {
            
        }
    }
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0507]: cannot move out of `self.v` which is behind a shared reference
 --> src/lib.rs:7:18
  |
7 |         for _ in self.v {
  |                  ^^^^^^ move occurs because `self.v` has type `std::vec::Vec<u32>`, which does not implement the `Copy` trait

error: aborting due to previous error

For more information about this error, try `rustc --explain E0507`.
error: Could not compile `playground`.

To learn more, run the command again with --verbose.

@Centril Centril added A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 5, 2019
@estebank
Copy link
Contributor

estebank commented Sep 5, 2019

I could have sworn we had it already :-|

@estebank estebank added the D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. label Oct 5, 2019
estebank added a commit to estebank/rust that referenced this issue Jan 21, 2020
tmandry added a commit to tmandry/rust that referenced this issue Jan 24, 2020
…y-vec, r=davidtwco

Suggest borrowing `Vec<NonCopy>` in for loop

Partially address rust-lang#64167.
tmandry added a commit to tmandry/rust that referenced this issue Jan 24, 2020
…y-vec, r=davidtwco

Suggest borrowing `Vec<NonCopy>` in for loop

Partially address rust-lang#64167.
@cmpute
Copy link

cmpute commented May 22, 2022

I have a case related to this issue:

use std::ops::Neg;

pub struct Foo<T>(pub T);

impl<T: Neg<Output = T>> Neg for Foo<T> {
    type Output = Self;
    fn neg(self) -> Self {
        Foo(-self.0)
    }
}

impl<T: Neg<Output = T> + Clone> Neg for &Foo<T> {
    type Output = Foo<T>;
    fn neg(self) -> Self::Output {
        Foo(-self.0.clone())
    }
}

// pub fn test0<T: Neg>(f: Foo<T>) -> Foo<T>{
//     f.neg()
// }
// pub fn test1<T: Neg>(f: Foo<T>) -> Foo<T>{
//     -f
// }
pub fn test2<T: Neg<Output = T>>(f: Foo<T>) -> Foo<T>{
    -f
}
// pub fn test3<T: Neg<Output = T>>(f: &Foo<T>) -> Foo<T>{
//     f.neg()
// }
pub fn test4<T: Neg<Output = T> + Clone>(f: &Foo<T>) -> Foo<T>{
    f.neg()
}

(Playground)

In this case, test2 and test4 are correct implementation while the others are incorrect because they lack trait bounds. But the error messages on test1 and test3 are not very helpful.

The error message for test0 is the most helpful one

error[[E0599]](https://doc.rust-lang.org/nightly/error-index.html#E0599): the method `neg` exists for struct `Foo<T>`, but its trait bounds were not satisfied
  --> src/lib.rs:20:7
   |
3  | pub struct Foo<T>(pub T);
   | -------------------------
   | |
   | method `neg` not found for this
   | doesn't satisfy `Foo<T>: Neg`
...
20 |     f.neg()
   |       ^^^ method cannot be called on `Foo<T>` due to unsatisfied trait bounds
   |

The error message for test1 is

error[[E0600]](https://doc.rust-lang.org/nightly/error-index.html#E0600): cannot apply unary operator `-` to type `Foo<T>`
  --> src/lib.rs:20:5
   |
20 |     -f
   |     ^^ cannot apply unary operator `-`

For more information about this error, try `rustc --explain E0600`.

The error message for test3 is

error[[E0507]](https://doc.rust-lang.org/nightly/error-index.html#E0507): cannot move out of `*f` which is behind a shared reference
   --> src/lib.rs:20:5
    |
20  |     f.neg()
    |     ^^-----
    |     | |
    |     | `*f` moved due to this method call
    |     move occurs because `*f` has type `Foo<T>`, which does not implement the `Copy` trait
    |
note: this function takes ownership of the receiver `self`, which moves `*f`

For more information about this error, try `rustc --explain E0507`.

@estebank
Copy link
Contributor

Current output:

error[E0507]: cannot move out of `self.v` which is behind a shared reference
   --> f75.rs:7:18
    |
7   |         for _ in self.v {
    |                  ^^^^^^
    |                  |
    |                  `self.v` moved due to this implicit call to `.into_iter()`
    |                  move occurs because `self.v` has type `Vec<u32>`, which does not implement the `Copy` trait
    |
note: `into_iter` takes ownership of the receiver `self`, which moves `self.v`
   --> /home/gh-estebank/rust/library/core/src/iter/traits/collect.rs:268:18
    |
268 |     fn into_iter(self) -> Self::IntoIter;
    |                  ^^^^
help: consider iterating over a slice of the `Vec<u32>`'s content to avoid moving into the `for` loop
    |
7   |         for _ in &self.v {
    |                  +

I believe that we are fully handling the original report. The report on the above comment is unchanged.

@estebank estebank removed C-enhancement Category: An issue proposing an enhancement or a PR with one. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. labels Nov 22, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 13, 2024
Detect borrow checker errors where `.clone()` would be an appropriate user action

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 rust-lang#49693, CC rust-lang#64167.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants