Unintentionally cloning references can lead to confusing borrow checker errors

[HexyWitch]

(Note from pnkfelix: I have updated the example to continue illustrating the issue even in the presence of NLL.)

This issue is a result of all references implementing Clone. The following code will not compile:

fn foo<T: Default>(list: &mut Vec<T>) {
    let mut cloned_items = Vec::new();
    for v in list.iter() {
        cloned_items.push(v.clone())
    }
    list.push(T::default());
    drop(cloned_items);
}

producing the misleading error message:

error[E0502]: cannot borrow `*list` as mutable because it is also borrowed as immutable
 --> src/main.rs:6:5
  |
3 |     for v in list.iter() {
  |              ---- immutable borrow occurs here
...
6 |     list.push(T::default());
  |     ^^^^ mutable borrow occurs here
7 |     drop(cloned_items);
  |          ------------ immutable borrow later used here
  | - immutable borrow ends here

Because Clone is not a trait bound on T, where you'd expect v.clone() to return a cloned T it's actually returning a cloned &T. The type of cloned_items is inferred to be Vec<&T>, and the lifetime of the list immutable borrow is extended to the lifetime of cloned_items. This is hard to figure out from the error message. The solution is to add Clone as a trait bound to T. The following code builds:

fn foo<T: Default + Clone>(list: &mut Vec<T>) {
    let mut cloned_items = Vec::new();
    for v in list.iter() {
        cloned_items.push(v.clone())
    }
    list.push(T::default());
}

If you give cloned_items an explicit type

let mut cloned_items: Vec<T> = Vec::new();

it gives an error that more clearly explains what you're doing wrong:

error[E0308]: mismatched types
 --> src/main.rs:4:27
  |
4 |         cloned_items.push(v.clone())
  |                           ^^^^^^^^^ expected type parameter, found &T
  |
  = note: expected type `T`
             found type `&T`

I'm not sure what my proposed fix would be. I don't know if there are any use cases for explicitly cloning a reference. Ideally you'd get a warning on cloning a reference to a type that doesn't implement Clone.

[leoyvens]

Clippy has a clone-on-copy lint, moving it into rustc would be a way to push code like this in the right direction.

[memoryruins]

Triage: the original example now compiles on 2018 edition.
2015 errors:

error[E0502]: cannot borrow `*list` as mutable because it is also borrowed as immutable
 --> src/main.rs:6:5
  |
3 |     for v in list.iter() {
  |              ---- immutable borrow occurs here
...
6 |     list.push(T::default());
  |     ^^^^ mutable borrow occurs here
7 | }
  | - immutable borrow ends here

rustc: 1.32.0

[kyren]

Just to be clear, I don't think NLL really has anything to do with it, a simple modification makes the example not compile on rust 2018 edition:

fn foo<T: Default>(list: &mut Vec<T>) {
    let mut cloned_items = Vec::new();
    for v in list.iter() {
        cloned_items.push(v.clone())
    }
    list.push(T::default());
    // Use `cloned_items` to defeat NLL
    drop(cloned_items);
}

whereas ofc this compiles:

fn foo<T: Default + Clone>(list: &mut Vec<T>) {
    let mut cloned_items = Vec::new();
    for v in list.iter() {
        cloned_items.push(v.clone())
    }
    list.push(T::default());
    // Use `cloned_items` to defeat NLL
    drop(cloned_items);
}

[memoryruins]

Thank you for pointing that out, kyren. There was another issue I ran into during triage that only triggered the error when used, and I mistagged this. Enabling nll feature on 2015 does change if an error is produced (before use), but it does not imply a fix (nor does simply compiling imply one either, which is why I made a seperate issue to be reviewed in #57964).

[memoryruins]

and for the sake of update, when using kyren's first example, is

error[E0502]: cannot borrow `*list` as mutable because it is also borrowed as immutable
 --> src/lib.rs:6:5
  |
3 |     for v in list.iter() {
  |              ---- immutable borrow occurs here
...
6 |     list.push(T::default());
  |     ^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
7 |     drop(cloned_items);
  |          ------------ immutable borrow later used here

error: aborting due to previous error

which is not much change except for the spans in diagnostics.

[m4b]

Is this going to be fixed ? This is a pretty big fail imho. The error message is downright misleading. I’ve been programming in rust for over 3 years and this took me about 20-30 minutes to figure out.

In the issue I gave, which was closed and is more simpler imho with no user level generics at all, it highlights what’s particularly bothersome about this issue, which is that for any of the types used in the hashmap, for example might be a complex nesting of types, if a single one of them doesn’t have clone (which can be very hard to track down especially if it’s external code using external code) you will receive the essentially useless error message that we see today.

I don’t know immediately the fix here. It seems like a hard problem and so I think this needs to be escalated so we can get more eyes on it.

But in as strong language as I think is appropriate: the current status quo error message here is actively hostile to new and older users and this should be given a more serious treatment. This is a clear demerit on rusts reputation for having stellar error messages. We can do better.

[estebank]

Making this error in particular better would be hard, but I think there's an alternative: have an early deny by default lint on all <&T>::clone(self) calls. That way it gives people a fighting chance to understand what happens in this case in particular, but we also cover all the other linked cases.

[Aaron1011]

With #80723, we will start linting on calling clone on a reference.