Diagnostics for cloning references are terrible

[Manishearth]

struct Foo;
fn main() {
    let x = Some(Foo);
    if let Some(ref y) = x {
        foo(y.clone());
    }
}

fn foo(x: Foo) {}

https://is.gd/f2AX0B

This gives the very unintuitive error:

error: mismatched types [--explain E0308]
 --> <anon>:5:13
5 |>         foo(y.clone());
  |>             ^^^^^^^^^ expected struct `Foo`, found &-ptr
note: expected type `Foo`
note:    found type `&Foo`

You expect y.clone() to return (*y).clone(), i.e. a Foo. It mysteriously returns an &Foo instead.

Because &T has a clone impl, it too, can be cloned (really, copied), returning a reference of the same lifetime. This is an operation you rarely want to do explicitly, except when dealing with generics.

This error can crop up when you've forgotten to implement Clone on Foo, and have a reference (which could have been silently inserted using ref, as in the code above). However, it's not very obvious what's happening here. It can also crop up if you forget a T: Clone bound

We should hint that Foo itself isn't cloneable and thus we fell back to cloning &Foo here.

This could be a lint on hitting the clone impl for references, but lints run too late.

I'm not sure how this can be implemented, since we need to see the source of a value and that's a bit tricky.

[Manishearth]

cc @nikomatsakis @jonathandturner

[nikomatsakis]

Interesting. This does seem like it would require some pretty special-case code to achieve, but I agree it'd be a nice case to capture.

[Manishearth]

A relatively easy way to catch most instances of this is to

• Pick up the expression which has a type error like this
• If it is a method call, check if that method call is a clone() call resolving to <&T as Clone>::clone
• If it is a variable, find the variable's def
• If the def is a local def, look at the init expr, check if it's a bad clone

This will miss things, but I think the majority of such bugs will be due to this. As it stands this feels like an incredibly easy case to hit (we've all forgotten to decorate structs).

[antoyo]

cc @GuillaumeGomez

[estebank]

CC #34629

[estebank]

As reported in #60875:

use std::collections::HashMap;

#[derive(Clone, Debug)]
struct Foo;

#[derive(Debug)]
struct Bar;

type H = HashMap<String, Vec<(Foo, Bar)>>;

fn wat(h: &H) -> H {
    let h: H = h.clone();
    h
}

fn main() {
    let h = H::new();
    println!("{:?}", wat(&h));
}

This reports:

  --> src/main.rs:12:16
   |
12 |     let h: H = h.clone();
   |                ^^^^^^^^^ expected struct `std::collections::HashMap`, found reference
   |
   = note: expected type `std::collections::HashMap<std::string::String, std::vec::Vec<(Foo, Bar)>>`
              found type `&std::collections::HashMap<std::string::String, std::vec::Vec<(Foo, Bar)>>`

error: aborting due to previous error

[estebank]

Current output is even worse for the original report:

error[E0596]: cannot borrow `sm.x` as mutable, as it is behind a `&` reference
  --> src/main.rs:10:9
   |
9  |     let mut sm = sr.clone();
   |         ------ help: consider changing this to be a mutable reference: `&mut Str`
10 |     foo(&mut sm.x);
   |         ^^^^^^^^^ `sm` is a `&` reference, so the data it refers to cannot be borrowed as mutable

The last comment's code is marginally different:

error[E0308]: mismatched types
  --> src/main.rs:12:16
   |
12 |     let h: H = h.clone();
   |            -   ^^^^^^^^^ expected struct `std::collections::HashMap`, found reference
   |            |
   |            expected due to this
   |
   = note: expected struct `std::collections::HashMap<std::string::String, std::vec::Vec<(Foo, Bar)>>`
           found reference `&std::collections::HashMap<std::string::String, std::vec::Vec<(Foo, Bar)>>`

[estebank]

Current output:

error[E0308]: mismatched types
 --> f54.rs:5:13
  |
5 |         foo(y.clone());
  |         --- ^^^^^^^^^ expected struct `Foo`, found `&Foo`
  |         |
  |         arguments to this function are incorrect
  |
note: `Foo` does not implement `Clone`, so `&Foo` was cloned instead
 --> f54.rs:5:13
  |
5 |         foo(y.clone());
  |             ^
note: function defined here
 --> f54.rs:9:4
  |
9 | fn foo(x: Foo) {}
  |    ^^^ ------

error[E0308]: mismatched types
  --> f54.rs:12:16
   |
12 |     let h: H = h.clone();
   |            -   ^^^^^^^^^ expected struct `HashMap`, found reference
   |            |
   |            expected due to this
   |
   = note: expected struct `HashMap<String, Vec<(Foo, Bar)>>`
           found reference `&HashMap<String, Vec<(Foo, Bar)>>`
note: `HashMap<String, Vec<(Foo, Bar)>>` does not implement `Clone`, so `&HashMap<String, Vec<(Foo, Bar)>>` was cloned instead
  --> f54.rs:12:16
   |
12 |     let h: H = h.clone();
   |                ^

error[E0308]: mismatched types
 --> f54.rs:2:5
  |
1 | fn wat<T>(t: &T) -> T {
  |        -            - expected `T` because of return type
  |        |
  |        this type parameter
2 |     t.clone()
  |     ^^^^^^^^^ expected type parameter `T`, found `&T`
  |
  = note: expected type parameter `T`
                  found reference `&T`
note: `T` does not implement `Clone`, so `&T` was cloned instead
 --> f54.rs:2:5
  |
2 |     t.clone()
  |     ^

The last case should suggest constraining T: Clone.

Edit: that was easier than I thought (we already had the suggestion machinery):