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.

[estebank]

Would there be anything you'd like to add to the following?

error[E0507]: cannot move out of a shared reference
   --> f75.rs:18:28
    |
18  |     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
   --> /home/gh-estebank/rust/library/std/src/collections/hash/map.rs:486:24
    |
486 |     pub fn into_values(self) -> IntoValues<K, V> {
    |                        ^^^^
help: you could `clone` the value and consume it, if the `Hash128_1: Clone` trait bound could be satisfied
    |
18  |     let mut copy: Vec<U> = <HashMap<T, U, Hash128_1> as Clone>::clone(&map.clone()).into_values().collect();
    |                            ++++++++++++++++++++++++++++++++++++++++++++           +
help: consider annotating `Hash128_1` with `#[derive(Clone)]`
    |
6   + #[derive(Clone)]
7   | pub struct Hash128_1;
    |

Would it be worth-while to also point at where the trait bounds where introduced, like we do in other errors?

[thenorili]

When you say 'where the trait bounds were introduced', do you mean the place where other bounds were introduced for hash128? Is that not covered in the last two lines of the message you shared?

[jyn514]

probably esteban meant here: https://doc.rust-lang.org/1.74.0/src/std/collections/hash/map.rs.html#1264

[estebank]

probably esteban meant here: https://doc.rust-lang.org/1.74.0/src/std/collections/hash/map.rs.html#1264

Correct. Like in the following:

[Zannick]

Would there be anything you'd like to add to the following?

error[E0507]: cannot move out of a shared reference
   --> f75.rs:18:28
    |
18  |     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
   --> /home/gh-estebank/rust/library/std/src/collections/hash/map.rs:486:24
    |
486 |     pub fn into_values(self) -> IntoValues<K, V> {
    |                        ^^^^

This part is still the same as my initial report from what I can tell, hard to recognize that the clone() function is returning a shared reference.

Here's what I might suggest to add here to add some clarity:

note: `map.clone()` understood here as:
        <&HashMap<T, U, Hash128_1> as Clone>::clone(map)
help: but perhaps you wanted (if `HashMap<T, U, Hash128_1>` implements `Clone`):
        <HashMap<T, U, Hash128_1> as Clone>::clone(map)
    |
18  |     let mut copy: Vec<U> = <HashMap<T, U, Hash128_1> as Clone>::clone(map).clone().into_values().collect();
    |                            +++++++++++++++++++++++++++++++++++++++++++   +--------

The first two lines of this introduce the context I need to understand this error (the trait method call), and the other lines are an ideal suggestion (if a bit wordy to spell out the full invocation).

I also want to note:

help: you could `clone` the value and consume it, if the `Hash128_1: Clone` trait bound could be satisfied
    |
18  |     let mut copy: Vec<U> = <HashMap<T, U, Hash128_1> as Clone>::clone(&map.clone()).into_values().collect();
    |                            ++++++++++++++++++++++++++++++++++++++++++++           +

Appears to introduce a clone of a clone. That's not great, but I suppose it's because the added clone is the generic advice for avoiding this use-after-move, so it doesn't necessary notice that the call it's wrapping is another clone.

[estebank]

This part is still the same as my initial report from what I can tell, hard to recognize that the clone() function is returning a shared reference.

I'll see what I can do.

Appears to introduce a clone of a clone.

The clone of a clone will be difficult to remove from the suggestion (as we don't have access to the hir tree at that point), but you're also less likely to introduce it in the first place as you won't get the suggestion for map.clone() in the first place, instead telling you to use the fully qualified path and to add the derive.