Uplift clippy::clone_double_ref to rustc

[fee1-dead]

In #109429, a snippet of code involves cloning references outputted a confusing compiler error:

use std::collections::hash_map::DefaultHasher;
use std::collections::HashMap;
use std::hash::BuildHasher;
use std::hash::Hash;

pub struct Hash128_1;

impl BuildHasher for Hash128_1 {
    type Hasher = DefaultHasher;
    fn build_hasher(&self) -> DefaultHasher { DefaultHasher::default() }
}

#[allow(unused)]
pub fn hashmap_copy<T, U>(
    map: &HashMap<T, U, Hash128_1>,
) where T: Hash + Clone, U: Clone
{
    let mut copy: Vec<U> = map.clone().into_values().collect();
}

pub fn make_map() -> HashMap<String, i64, Hash128_1>
{
    HashMap::with_hasher(Hash128_1)
}

error[E0507]: cannot move out of a shared reference
  --> src/lib.rs:19:28
   |
19 |     let mut copy: Vec<U> = map.clone().into_values().collect();
   |                            ^^^^^^^^^^^^-------------
   |                            |           |
   |                            |           value moved due to this method call
   |                            move occurs because value has type `HashMap<T, U, Hash128_1>`, which does not implement the `Copy` trait
   |
note: `HashMap::<K, V, S>::into_values` takes ownership of the receiver `self`, which moves value

Changing the diagnostic to provide help when we are trying to use a function that takes self after a .clone() call is nontrivial, since this is MIR borrowck and to my knowledge it is hard to know if it comes from the return value of a .clone() call.

Ideally we should have an error/warning like the one from clippy:

error: using `clone` on a double-reference; this will copy the reference of type `&HashMap<T, U, Hash128_1>` instead of cloning the inner type
  --> src/lib.rs:19:28
   |
19 |     let mut copy: Vec<U> = map.clone().into_values().collect();
   |                            ^^^^^^^^^^^

I think there are other ways this can be confusing, so uplifting can help with many issues related to this and not just the example I linked.

---

cc @rust-lang/lang: would uplifting be a good idea? I'd be willing to work on this if this gets approval.

@rustbot label +A-lint +T-compiler +T-lang +D-newcomer-roadblock

[scottmcm]

Personally this seems good, but I think I'd actually rather get @rust-lang/libs-api's take on it, rather than lang.

Do you consider this a reasonable use of clone? Or should it be linted?

Also, I wonder if this is something that would make more sense as an attribute on the impl or something, so it could be a general feature instead of clone-specific. For example, I'd put u32: TryFrom<u8> (and everything using that blanket impl from From) in the same "look, you can technically call try_from here, but in non-generic non-macro code you never should" bucket as calling clone on a reference.

[dtolnay]

I support uplifting this. I've hit the same issue repeatedly too; it is covered in https://dtolnay.github.io/rust-quiz/30.

[Zannick]

For example, I'd put u32: TryFrom<u8> (and everything using that blanket impl from From) in the same "look, you can technically call try_from here, but in non-generic non-macro code you never should" bucket as calling clone on a reference.

From a beginner's perspective, clone and other methods are always called on a ref, but it isn't obvious when calling a ref method that you are calling it on a double-ref, because it's implicitly deref'ed (I guess?). So my understanding was that you call clone on a reference to get a copy of the inner non-Copy type. In this case, the issue isn't truly the call on the double-ref but the Clone impl I want not being usable.

error: using clone on a double-reference; this will copy the reference of type &HashMap<T, U, Hash128_1> instead of cloning the inner type

For additional clarity, I would suggest something more like:

note: T is not Clone; using clone on &T will copy the reference instead of cloning the inner type

[m-ou-se]

This issue briefly came up in the libs-api meeting.

should it be linted?

Yes, this seems like a very useful lint to us. :)

[fee1-dead]

This is done as suspicious_double_ref_op and noop_method_call.