Compiler unhelpful when clone surprisingly returns shared reference instead of owned object

[Zannick]

I tried this code, mimicking a usage of fasthash that I expected to work with HashMap:

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)
}

I expected to see one of these happen:

• hashmap_copy compiles, so function can copy the hashmap values into a vector and modify it as necessary.
• rustc gives an error that Clone is not implemented for HashMap<T, U, Hash128_1> because Hash128_1 does not have an implementation for Clone.

Instead, this happened:

error[[E0507]](https://doc.rust-lang.org/nightly/error_codes/E0507.html): 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
  --> /rustc/44f5180584404d18058cbbf224c55255db4fdcbb/library/std/src/collections/hash/map.rs:487:24

This appears to be a complaint that map.clone() is already borrowed somehow by the time I call into_values on it. On further research, there is a Clone implementation for shared references (#91805). I can get around this with map.values().map(Clone::clone).collect() to avoid needing Clone on the hasher param but:

1. I find it surprising that clone() can return a HashMap reference instead of an owned object based on whether I messed up the type parameters.
2. The error message was perplexing and I got lucky with my research while filing this bug report.

Built on 1.68.0 stable and 1.70.0-nightly in the playground (2023-03-20 44f5180)

[fee1-dead]

I took a look at this. The place that should insert a help message is here:

rust/compiler/rustc_borrowck/src/diagnostics/move_errors.rs

Lines 440 to 450 in a01b4cc

	GroupedMoveError::OtherIllegalMove { ref original_path, use_spans, .. } => {
	let span = use_spans.var_or_use();
	let place_ty = original_path.ty(self.body, self.infcx.tcx).ty;
	let place_desc = match self.describe_place(original_path.as_ref()) {
	Some(desc) => format!("`{desc}`"),
	None => "value".to_string(),
	};
	self.note_type_does_not_implement_copy(err, &place_desc, place_ty, Some(span), "");
	use_spans.args_span_label(err, format!("{place_desc} is moved here"));
	}

The debug implementation outputs the following:

OtherIllegalMove { original_path: (*_5), use_spans: FnSelfUse { var_span: test.rs:18:28: 18:53 (#0), fn_call_span: test.rs:18:40: 18:53 (#0), fn_span: /home/gh-fee1-dead/rust/library/std/src/collections/hash/map.rs:487:5: 487:49 (#0), kind: Normal { self_arg: Some(self#0), desugaring: None, method_did: DefId(1:755 ~ std[cc42]::collections::hash::map::{impl#1}::into_values), method_substs: [T, U, Hash128_1] } }, kind: BorrowedContent { target_place: (*_5) } 

Since this operates on mir it is hard to know whether the user has tried to clone it. Another way to do it would be to uplift the clippy lint clone_double_ref to rustc.

[fee1-dead]

Current progress:

• uplift clippy::clone_double_ref uplift clippy::clone_double_ref as suspicious_double_ref_op #110955
• turn noop_method_calls to warn
• Investigate how to run these lints after borrowck errors

[jyn514]

note that the compiler also emits an unhelpful diagnostic in this case:

help: you can `clone` the value and consume it, but this might not be your desired behavior
   |
13 |         for _ in self.clone().clone().into_iter() {}
   |                              ++++++++

which is probably easier to fix than running the lint even if there are errors.

code that replicates that error

use core::ops::Deref;
struct S(Vec<usize>);
impl Deref for S {
    type Target = Vec<usize>;
    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl S {
    fn foo(&self) {
        // `self.clone()` returns `&S`, not `Vec`
        for _ in self.clone().into_iter() {}
    }
}

[estebank]

For the last case, this is a place where auto-deref and method resolution interacts in a non-obvious way. Spelling out you want the Clone impl for Vec<usize> makes the code in the last comment work.