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

Unhelpful lifetime error on .clone() refers to &&T, could suggest adding Clone #40699

Closed
kw217 opened this issue Mar 21, 2017 · 8 comments
Closed
Labels
A-diagnostics Area: Messages for errors, warnings, and lints P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@kw217
Copy link

kw217 commented Mar 21, 2017

In code that works with boxed closures (e.g., futures code), it is common to clone an argument so that it can be moved into the closure. If the argument is passed by reference, clone() will clone the value if there is a Clone implementation, otherwise it will pointlessly clone the reference.

If the Clone implementation is not present, rustc reports E0495 quite unhelpfully.

Please could rustc spot this and suggest adding a Clone implementation? I think the signature is an invocation of clone() at &T when the type compatibility being checked is &&T.

Example:

// Uncomment the derive to make this compile.
//#[derive(Clone)]
struct Foo(String);

fn bar(p: &Foo) -> Box<Fn() -> ()> {
    let p = p.clone();
    let f = move || baz(&p);
    Box::new(f)
}

fn baz(_: &Foo) {}

pub fn main() {
    let foo = Foo("foo".to_string());
    let closure = bar(&foo);
    closure()
}

Error message:

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter `'a` due to conflicting requirements
 --> doubleref.rs:5:15
  |
5 |     let p = p.clone();
  |               ^^^^^
  |
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the body at 4:35...
 --> doubleref.rs:4:36
  |
4 |   fn bar(p: &Foo) -> Box<Fn() -> ()> {
  |  ____________________________________^ starting here...
5 | |     let p = p.clone();
6 | |     let f = move || baz(&p);
7 | |     Box::new(f)
8 | | }
  | |_^ ...ending here
note: ...so that types are compatible (expected &&Foo, found &&Foo)
 --> doubleref.rs:5:15
  |
5 |     let p = p.clone();
  |               ^^^^^
  = note: but, the lifetime must be valid for the static lifetime...
note: ...so that the type `[[email protected]:6:13: 6:28 p:&Foo]` will meet its required lifetime bounds
 --> doubleref.rs:7:5
  |
7 |     Box::new(f)
  |     ^^^^^^^^^^^
@kw217
Copy link
Author

kw217 commented Mar 28, 2017

Hi - do you need any further information for triage?

@Mark-Simulacrum Mark-Simulacrum added 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. labels Jun 20, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@steveklabnik
Copy link
Member

Triage: the error has changed slightly:

error[E0621]: explicit lifetime required in the type of `p`
 --> src/main.rs:7:5
  |
4 | fn bar(p: &Foo) -> Box<Fn() -> ()> {
  |           ---- help: add explicit lifetime `'static` to the type of `p`: `&'static Foo`
...
7 |     Box::new(f)
  |     ^^^^^^^^^^^ lifetime `'static` required

error: aborting due to previous error

@Dylan-DPC
Copy link
Member

Current error:

error: lifetime may not live long enough
 --> src/main.rs:6:5
  |
3 | fn bar(p: &Foo) -> Box<dyn Fn() -> ()> {
  |           - let's call the lifetime of this reference `'1`
...
6 |     Box::new(f)
  |     ^^^^^^^^^^^ returning this value requires that `'1` must outlive `'static`
  |
help: to declare that the trait object captures data from argument `p`, you can add an explicit `'_` lifetime bound
  |
3 | fn bar(p: &Foo) -> Box<dyn Fn() -> () + '_> {
  |                                       ++++

@estebank
Copy link
Contributor

After applying the suggestion mentioned above, the output is

warning: call to `.clone()` on a reference in this situation does nothing
 --> src/main.rs:6:14
  |
6 |     let p = p.clone();
  |              ^^^^^^^^ help: remove this redundant call
  |
  = note: the type `Foo` does not implement `Clone`, so calling `clone` on `&Foo` copies the reference, which does not do anything and can be removed
  = note: `#[warn(noop_method_call)]` on by default

Potentially the lint should mention #[derive(Clone)] as an option.

I believe that the direction the suggestions take us is perfectly valid given the available constraints (it makes sense to lead people towards borrowing more than cloning).

@estebank estebank added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. P-low Low priority and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Feb 20, 2024
estebank added a commit to estebank/rust that referenced this issue Feb 22, 2024
@estebank estebank removed the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Feb 22, 2024
@estebank
Copy link
Contributor

estebank commented Feb 22, 2024

The only thing left after #121471 is to mention T: !Clone in the lifetime error. Beyond that, we already provide enough information for people to get from this case to proper understanding what's going on (but not in one step).

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 23, 2024
When encountering `<&T as Clone>::clone(x)` because `T: Clone`, suggest `#[derive(Clone)]`

CC rust-lang#40699.

```
warning: call to `.clone()` on a reference in this situation does nothing
  --> $DIR/noop-method-call.rs:23:71
   |
LL |     let non_clone_type_ref_clone: &PlainType<u32> = non_clone_type_ref.clone();
   |                                                                       ^^^^^^^^
   |
   = note: the type `PlainType<u32>` does not implement `Clone`, so calling `clone` on `&PlainType<u32>` copies the reference, which does not do anything and can be removed
help: remove this redundant call
   |
LL -     let non_clone_type_ref_clone: &PlainType<u32> = non_clone_type_ref.clone();
LL +     let non_clone_type_ref_clone: &PlainType<u32> = non_clone_type_ref;
   |
help: if you meant to clone `PlainType<u32>`, implement `Clone` for it
   |
LL + #[derive(Clone)]
LL | struct PlainType<T>(T);
   |
```
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 23, 2024
Rollup merge of rust-lang#121471 - estebank:lint-clone, r=TaKO8Ki

When encountering `<&T as Clone>::clone(x)` because `T: Clone`, suggest `#[derive(Clone)]`

CC rust-lang#40699.

```
warning: call to `.clone()` on a reference in this situation does nothing
  --> $DIR/noop-method-call.rs:23:71
   |
LL |     let non_clone_type_ref_clone: &PlainType<u32> = non_clone_type_ref.clone();
   |                                                                       ^^^^^^^^
   |
   = note: the type `PlainType<u32>` does not implement `Clone`, so calling `clone` on `&PlainType<u32>` copies the reference, which does not do anything and can be removed
help: remove this redundant call
   |
LL -     let non_clone_type_ref_clone: &PlainType<u32> = non_clone_type_ref.clone();
LL +     let non_clone_type_ref_clone: &PlainType<u32> = non_clone_type_ref;
   |
help: if you meant to clone `PlainType<u32>`, implement `Clone` for it
   |
LL + #[derive(Clone)]
LL | struct PlainType<T>(T);
   |
```
@RodBurman
Copy link

RodBurman commented Oct 4, 2024

First I'll apologise for the extra long comment, but I thought it best to show the steps I went through.

Using this rust compiler:

$  rustc --V -v   
rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: aarch64-apple-darwin
release: 1.81.0
LLVM version: 18.1.7

To get a clean build I uncommented the derive clone giving:

// Uncomment the derive to make this compile.
#[derive(Clone)]
struct Foo(String);

fn bar(p: &Foo) -> Box<Fn() -> () + '_> {
    let p = p.clone();
    let f = move || baz(&p);
    Box::new(f)
}

fn baz(_: &Foo) {}

pub fn main() {
    let foo = Foo("foo".to_string());
    let closure = bar(&foo);
    closure()
}

This now gives an error:

error[E0782]: trait objects must include the `dyn` keyword
 --> src/main.rs:5:24
  |
5 | fn bar(p: &Foo) -> Box<Fn() -> () + '_> {
  |                        ^^^^^^^^^^^^^^^
  |
help: add `dyn` keyword before this trait
  |
5 | fn bar(p: &Foo) -> Box<dyn Fn() -> () + '_> {
  |                        +++

For more information about this error, try `rustc --explain E0782`.
error: could not compile `box-closure-clone` (bin "box-closure-clone") due to 1 previous error

Adding the dyn as specified and allowing dead code gives a clean no messages build.
Commenting out the derive clone again, gives:

#[allow(dead_code)]
// Uncomment the derive to make this compile.
// #[derive(Clone)]
struct Foo(String);

fn bar(p: &Foo) -> Box<dyn Fn() -> () + '_> {
    let p = p.clone();
    let f = move || baz(&p);
    Box::new(f)
}

fn baz(_: &Foo) {}

pub fn main() {
    let foo = Foo("foo".to_string());
    let closure = bar(&foo);
    closure()
}

Compiling this code, finally gives what I think was suggested originally, a suggestion to implement Clone for Foo:

   Compiling box-closure-clone v0.1.0 (/Users/rod/code/rust/triage/box-closure-clone)
warning: call to `.clone()` on a reference in this situation does nothing
 --> src/main.rs:7:14
  |
7 |     let p = p.clone();
  |              ^^^^^^^^
  |
  = note: the type `Foo` does not implement `Clone`, so calling `clone` on `&Foo` copies the reference, which does not do anything and can be removed
  = note: `#[warn(noop_method_call)]` on by default
help: remove this redundant call
  |
7 -     let p = p.clone();
7 +     let p = p;
  |
help: if you meant to clone `Foo`, implement `Clone` for it
  |
4 + #[derive(Clone)]
5 | struct Foo(String);
  |

warning: `box-closure-clone` (bin "box-closure-clone") generated 1 warning (run `cargo fix --bin "box-closure-clone"` to apply 1 suggestion)

@kw217
Copy link
Author

kw217 commented Oct 14, 2024

Thanks, this is definitely what I was after. Thanks!

@kw217
Copy link
Author

kw217 commented Oct 14, 2024

Closing as fixed. Thanks all.

@kw217 kw217 closed this as completed Oct 14, 2024
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 P-low Low priority 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

6 participants