Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misleading error message when mutably referencing members of cloned &T when T does not implement Clone #34629

Closed
peterjoel opened this issue Jul 3, 2016 · 5 comments · Fixed by #118076
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@peterjoel
Copy link
Contributor

Code example:

#[derive(Debug)]
struct Str {
   x: Option<i32>,
}

fn main() {
    let s = Str { x: None };
    let sr = &s;
    let mut sm = sr.clone();
    foo(&mut sm.x);
}

fn foo(s: &mut Option<i32>) {
    if s.is_none() {
        *s = Some(0);
    }
    println!("{:?}", s);
}

The Error is:

error: cannot borrow immutable field `sm.x` as mutable
  --> <anon>:10:14
   |>
10 |>     foo(&mut sm.x);
   |>              ^^^^

This is misleading because the fix is to add a Clone impl for Str

Meta

rustc --version --verbose:
rustc 1.9.0-nightly (d5a91e6 2016-03-26)
binary: rustc
commit-hash: d5a91e6
commit-date: 2016-03-26
host: x86_64-apple-darwin
release: 1.9.0-nightly

@GuillaumeGomez
Copy link
Member

The biggest question I have is: how can the clone method be called if it isn't defined on Str type?

@arielb1 arielb1 added the A-diagnostics Area: Messages for errors, warnings, and lints label Jul 4, 2016
@arielb1
Copy link
Contributor

arielb1 commented Jul 4, 2016

The issue is that <&Str>::clone is called through an autoref.

@steveklabnik steveklabnik removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 25, 2017
@pnkfelix
Copy link
Member

pnkfelix commented Jan 31, 2019

cc #48677, #34896

@crlf0710 crlf0710 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 11, 2020
@estebank
Copy link
Contributor

Current output:

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

Applying the suggestion:

error[E0308]: mismatched types
 --> src/main.rs:9:28
  |
9 |     let mut sm: &mut Str = sr.clone();
  |                 --------   ^^^^^^^^^^ types differ in mutability
  |                 |
  |                 expected due to this
  |
  = note: expected mutable reference `&mut Str`
                     found reference `&Str`

If you explicitly give sm the type Str expecting the clone to work:

error[E0308]: mismatched types
 --> src/main.rs:9:23
  |
9 |     let mut sm: Str = sr.clone();
  |                 ---   ^^^^^^^^^^ expected `Str`, found `&Str`
  |                 |
  |                 expected due to this
  |
note: `Str` does not implement `Clone`, so `&Str` was cloned instead
 --> src/main.rs:9:23
  |
9 |     let mut sm: Str = sr.clone();
  |                       ^^
help: consider annotating `Str` with `#[derive(Clone)]`
  |
2 + #[derive(Clone)]
3 | struct Str {

@estebank
Copy link
Contributor

Another case from #41825:

use std::collections::BTreeMap;
use std::collections::HashSet;

#[derive(Debug,Eq,PartialEq,Hash)]
enum Day {
    Mon,
}

struct Class {
    days: BTreeMap<u32, HashSet<Day>>
}

impl Class {
    fn do_stuff(&self) {
        for (_, v) in &self.days {
            let mut x = v.clone();
            let y: Vec<Day> = x.drain().collect();
            println!("{:?}", x);
        }
    }
}

fn fail() {
    let c = Class { days: BTreeMap::new() };
    c.do_stuff();
}
error[E0596]: cannot borrow `*x` as mutable, as it is behind a `&` reference
  --> f75.rs:17:31
   |
17 |             let y: Vec<Day> = x.drain().collect();
   |                               ^ `x` is a `&` reference, so the data it refers to cannot be borrowed as mutable
   |
help: consider specifying this binding's type
   |
16 |             let mut x: &mut HashSet<Day> = v.clone();
   |                      +++++++++++++++++++
error[E0308]: mismatched types
  --> f75.rs:16:44
   |
16 |             let mut x: &mut HashSet<Day> = v.clone();
   |                        -----------------   ^^^^^^^^^ types differ in mutability
   |                        |
   |                        expected due to this
   |
   = note: expected mutable reference `&mut HashSet<Day>`
                      found reference `&HashSet<Day>`
E0308]: mismatched types
  --> f75.rs:16:39
   |
16 |             let mut x: HashSet<Day> = v.clone();
   |                        ------------   ^^^^^^^^^ expected `HashSet<Day>`, found `&HashSet<Day>`
   |                        |
   |                        expected due to this
   |
   = note: expected struct `HashSet<Day>`
           found reference `&HashSet<Day>`
note: `HashSet<Day>` does not implement `Clone`, so `&HashSet<Day>` was cloned instead
  --> f75.rs:16:39
   |
16 |             let mut x: HashSet<Day> = v.clone();
   |                                       ^

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Dec 4, 2023
Tweak `.clone()` suggestion to work in more cases

When going through auto-deref, the `<T as Clone>` impl sometimes needs to be specified for rustc to actually clone the value and not the reference.

```
error[E0507]: cannot move out of dereference of `S`
  --> $DIR/needs-clone-through-deref.rs:15:18
   |
LL |         for _ in self.clone().into_iter() {}
   |                  ^^^^^^^^^^^^ ----------- value moved due to this method call
   |                  |
   |                  move occurs because value has type `Vec<usize>`, which does not implement the `Copy` trait
   |
note: `into_iter` takes ownership of the receiver `self`, which moves value
  --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
help: you can `clone` the value and consume it, but this might not be your desired behavior
   |
LL |         for _ in <Vec<usize> as Clone>::clone(&self.clone()).into_iter() {}
   |                  ++++++++++++++++++++++++++++++            +
```

When encountering a move error, look for implementations of `Clone` for the moved type. If there is one, check if all its obligations are met. If they are, we suggest cloning without caveats. If they aren't, we suggest cloning while mentioning the unmet obligations, potentially suggesting `#[derive(Clone)]` when appropriate.

```
error[E0507]: cannot move out of a shared reference
  --> $DIR/suggest-clone-when-some-obligation-is-unmet.rs:20:28
   |
LL |     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
  --> $SRC_DIR/std/src/collections/hash/map.rs:LL:COL
help: you could `clone` the value and consume it, if the `Hash128_1: Clone` trait bound could be satisfied
   |
LL |     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)]`
   |
LL + #[derive(Clone)]
LL | pub struct Hash128_1;
   |
```

Fix rust-lang#109429.

When encountering multiple mutable borrows, suggest cloning and adding
derive annotations as needed.

```
error[E0596]: cannot borrow `sm.x` as mutable, as it is behind a `&` reference
  --> $DIR/accidentally-cloning-ref-borrow-error.rs:32:9
   |
LL |     foo(&mut sm.x);
   |         ^^^^^^^^^ `sm` is a `&` reference, so the data it refers to cannot be borrowed as mutable
   |
help: `Str` doesn't implement `Clone`, so this call clones the reference `&Str`
  --> $DIR/accidentally-cloning-ref-borrow-error.rs:31:21
   |
LL |     let mut sm = sr.clone();
   |                     ^^^^^^^
help: consider annotating `Str` with `#[derive(Clone)]`
   |
LL + #[derive(Clone)]
LL | struct Str {
   |
help: consider specifying this binding's type
   |
LL |     let mut sm: &mut Str = sr.clone();
   |               ++++++++++
```

Fix rust-lang#34629. Fix rust-lang#76643. Fix rust-lang#91532.
@bors bors closed this as completed in f67523d Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants