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

Diagnostics for mutating non-DerefMut types are confusing #58864

Closed
Manishearth opened this issue Mar 2, 2019 · 2 comments · Fixed by #62468
Closed

Diagnostics for mutating non-DerefMut types are confusing #58864

Manishearth opened this issue Mar 2, 2019 · 2 comments · Fixed by #62468
Assignees
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints NLL-diagnostics Working towards the "diagnostic parity" goal

Comments

@Manishearth
Copy link
Member

Manishearth commented Mar 2, 2019

use std::rc::Rc;

fn main() {
    let mut x = Rc::new(vec![1,2,3,4]);
    x.push(5);
}

emits

warning: variable does not need to be mutable
 --> src/main.rs:5:9
  |
5 |     let mut x = Rc::new(vec![1,2,3,4]);
  |         ----^
  |         |
  |         help: remove this `mut`
  |
  = note: #[warn(unused_mut)] on by default

error[E0596]: cannot borrow data in a `&` reference as mutable
 --> src/main.rs:6:5
  |
6 |     x.push(5);
  |     ^ cannot borrow as mutable

error: aborting due to previous error

which is super confusing. Firstly, I have what appears to be two conflicting error messages. The second one tells me to make things mutable, and if I listen to it I get told that things don't need to be mutable. Secondly, the relevant error message (the second one) talks about & references which don't exist.

The actual issue here is that x is not DerefMut, of course. Which is obvious in this case, but that's not always true. I've often hit this error in other situations and I've basically learned to pattern match it to mean "something something DerefMut", but people seeing it for the first time typically don't.

We should explicitly make the diagnostics say something like "x is an Rc<T> which cannot be dereferenced mutably"

@Manishearth Manishearth added A-diagnostics Area: Messages for errors, warnings, and lints A-borrow-checker Area: The borrow checker labels Mar 2, 2019
@Manishearth
Copy link
Member Author

This is particularly bad for Pin where Pin's DerefMut implementation depends on Unpin being implemented, so you have two layers of introspection to do -- you have to first figure out that the &-reference error is talking about DerefMut, and then realize that DerefMut needs Unpin.

@asajeffrey hit this today

@Manishearth
Copy link
Member Author

A reduced example for the confusing pin diagnostics:

use std::pin::Pin;
use std::marker::PhantomData;
use std::marker::PhantomPinned;

struct Thing(
    String,
    PhantomData<PhantomPinned>, // Comment out this line and there's no error
);

// Uncomment this line and there's no error
// impl Unpin for Thing {}

impl Thing {
    fn foo<'a>(mut p: Pin<&'a mut Thing>) {
        // The error this produces is difficult to understand.
        // warning: variable does not need to be mutable
        // error: cannot borrow data in a `&` reference as mutable
        let _q: Pin<&mut String> = Pin::new(&mut p.0);
    }
}

(playground)

@matthewjasper matthewjasper self-assigned this Mar 2, 2019
@matthewjasper matthewjasper added the NLL-diagnostics Working towards the "diagnostic parity" goal label Mar 2, 2019
bors added a commit that referenced this issue Jul 13, 2019
…bank

Improve diagnostics for invalid mutation through overloaded operators

Closes #58864
Closes #52941
Closes #57839
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints NLL-diagnostics Working towards the "diagnostic parity" goal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants