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.

Sign up for GitHub

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

Jump to bottom

Suggest &foo when passing foo to a function expecting AsRef<T>... #41708

Open
oli-obk opened this issue May 2, 2017 · 9 comments · May be fixed by #124599
Open

Suggest &foo when passing foo to a function expecting AsRef<T>... #41708

oli-obk opened this issue May 2, 2017 · 9 comments · May be fixed by #124599
Labels
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. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented May 2, 2017

... and trying to use foo afterwards, which triggers

error[E0382]: use of moved value: foo

Example:

struct Bar;

impl AsRef<Bar> for Bar {
    fn as_ref(&self) -> &Bar {
        self
    }
}

fn foo<T: AsRef<Bar>>(_: T) {}

pub fn main() {
    let bar = Bar;
    foo(bar);
    let baa = bar;
}

currently yields

rustc 1.17.0 (56124baa9 2017-04-24)
error[E0382]: use of moved value: `bar`
  --> <anon>:15:9
   |
14 |     foo(bar);
   |         --- value moved here
15 |     let baa = bar;
   |         ^^^ value used here after move
   |
   = note: move occurs because `bar` has type `Bar`, which does not implement the `Copy` trait

error: aborting due to previous error

I suggest that it should also contain

help: if you meant to borrow `bar` use `foo(&bar)`?
@nagisa
Copy link
Member

nagisa commented May 2, 2017

AsRef is a library trait, so IMO we should not do that. Also to me "value moved here" is pretty much the "you need to use this as reference".

@oli-obk
Copy link
Contributor Author

oli-obk commented May 3, 2017

Also to me "value moved here" is pretty much the "you need to use this as reference".

While this might be obvious to us, I've had students struggling strongly with these errors. When I tell them "just add an ampersand", they're like "Oh, that makes total sense".

AsRef is a library trait, so IMO we should not do that.

I don't see that as reason not to do it. Could you elaborate?

@kennytm
Copy link
Member

kennytm commented May 3, 2017

This should also handle AsMut, Borrow, BorrowMut.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 3, 2017

AsMut and BorrowMut aren't as clear, since the object will be modified, so the user might also want to bar.clone(), but that can later be signalled through rust-lang/rfcs#1941 and/or multiple suggestions

@nagisa
Copy link
Member

nagisa commented May 3, 2017

I don't see that as reason not to do it. Could you elaborate?

Obvious way of doing this would involve making AsRef a language item or otherwise special in some other way. To me it is pretty clear that language and the standard library should be as independent as possible and IME a diagnostic is a pretty minor gain for the special casing involved.

One could imagine a similar diagnostic independent of library traits (e.g. always suggest a reference for "value moved here" if there’s a trait implementation for &T also), but then the problem of suggestion not being always correct kicks in.

So, I will not oppose a change that implements this without making any more of the libstd/libcore special and is always correct, but I have no idea how anybody even begin approaching this problem.

@nagisa nagisa added the A-diagnostics Area: Messages for errors, warnings, and lints label May 3, 2017
@oli-obk
Copy link
Contributor Author

oli-obk commented May 3, 2017

making AsRef a language item or otherwise special in some other way.

I fully agree that that is not desirable

In clippy we have similar diagnostics which in this case would simply check for core::convert::AsRef (see https://github.com/Manishearth/rust-clippy/blob/master/clippy_lints/src/utils/paths.rs#L3 for all the paths that we resolve manually).

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@estebank estebank added A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 15, 2019
@rylev
Copy link
Member

rylev commented Dec 9, 2020

Triage: this still produces the same error message.

Given the concerns voiced above, should this issue stay open? I suppose we could leave this open should someone come up with a clever way to handle this without special casing AsRef, but that seems unlikely.

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 9, 2020

We have diagnostic items nowadays, so we can make AsRef a diagnostic item and detect in in these situations. Though I'm not sure how easy it is to detect. The error may get reported far too late

@Dylan-DPC
Copy link
Member

Current output:

warning: unused variable: `baa`
  --> src/main.rs:14:9
   |
14 |     let baa = bar;
   |         ^^^ help: if this is intentional, prefix it with an underscore: `_baa`
   |
   = note: `#[warn(unused_variables)]` on by default

error[[E0382]](https://doc.rust-lang.org/nightly/error_codes/E0382.html): use of moved value: `bar`
  --> src/main.rs:14:15
   |
12 |     let bar = Bar;
   |         --- move occurs because `bar` has type `Bar`, which does not implement the `Copy` trait
13 |     foo(bar);
   |         --- value moved here
14 |     let baa = bar;
   |               ^^^ value used here after move
   |
note: consider changing this parameter type in function `foo` to borrow instead if owning the value isn't necessary
  --> src/main.rs:9:26
   |
9  | fn foo<T: AsRef<Bar>>(_: T) {}
   |    --- in this function  ^ this parameter takes ownership of the value

For more information about this error, try `rustc --explain E0382`.
warning: `playground` (bin "playground") generated 1 warning
error: could not compile `playground` (bin "playground") due to previous error; 1 warning emitted

@estebank estebank mentioned this issue Nov 22, 2023
Open
estebank added a commit to estebank/rust that referenced this issue May 2, 2024
@estebank
Suggest borrowing on fn argument that is impl AsRef
15c6593 
When encountering a move conflict, on an expression that is `!Copy` passed as an argument to an `fn` that is `impl AsRef`, suggest borrowing the expression.

```
error[E0382]: use of moved value: `bar`
  --> f204.rs:14:15
   |
12 |     let bar = Bar;
   |         --- move occurs because `bar` has type `Bar`, which does not implement the `Copy` trait
13 |     foo(bar);
   |         --- value moved here
14 |     let baa = bar;
   |               ^^^ value used here after move
   |
help: borrow the value to avoid moving it
   |
13 |     foo(&bar);
   |         +
```

Fix rust-lang#41708
estebank added a commit to estebank/rust that referenced this issue May 2, 2024
@estebank
Suggest borrowing on fn argument that is impl AsRef
78ff2b3 
When encountering a move conflict, on an expression that is `!Copy` passed as an argument to an `fn` that is `impl AsRef`, suggest borrowing the expression.

```
error[E0382]: use of moved value: `bar`
  --> f204.rs:14:15
   |
12 |     let bar = Bar;
   |         --- move occurs because `bar` has type `Bar`, which does not implement the `Copy` trait
13 |     foo(bar);
   |         --- value moved here
14 |     let baa = bar;
   |               ^^^ value used here after move
   |
help: borrow the value to avoid moving it
   |
13 |     foo(&bar);
   |         +
```

Fix rust-lang#41708
@estebank estebank linked a pull request May 2, 2024 that will close this issue
Open
estebank added a commit to estebank/rust that referenced this issue May 9, 2024
@estebank
Suggest borrowing on fn argument that is impl AsRef
83bf719 
When encountering a move conflict, on an expression that is `!Copy` passed as an argument to an `fn` that is `impl AsRef`, suggest borrowing the expression.

```
error[E0382]: use of moved value: `bar`
  --> f204.rs:14:15
   |
12 |     let bar = Bar;
   |         --- move occurs because `bar` has type `Bar`, which does not implement the `Copy` trait
13 |     foo(bar);
   |         --- value moved here
14 |     let baa = bar;
   |               ^^^ value used here after move
   |
help: borrow the value to avoid moving it
   |
13 |     foo(&bar);
   |         +
```

Fix rust-lang#41708
estebank added a commit to estebank/rust that referenced this issue May 9, 2024
@estebank
Suggest borrowing on fn argument that is impl AsRef
2df4f7d 
When encountering a move conflict, on an expression that is `!Copy` passed as an argument to an `fn` that is `impl AsRef`, suggest borrowing the expression.

```
error[E0382]: use of moved value: `bar`
  --> f204.rs:14:15
   |
12 |     let bar = Bar;
   |         --- move occurs because `bar` has type `Bar`, which does not implement the `Copy` trait
13 |     foo(bar);
   |         --- value moved here
14 |     let baa = bar;
   |               ^^^ value used here after move
   |
help: borrow the value to avoid moving it
   |
13 |     foo(&bar);
   |         +
```

Fix rust-lang#41708
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
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. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Suggest borrowing on fn argument that is impl AsRef estebank/rust
7 participants
@kennytm @oli-obk @nagisa @rylev @estebank @Mark-Simulacrum @Dylan-DPC