Skip to content

Tweak error message when reborrowing &mut self as mut #47818

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Tweak error message when reborrowing &mut self as mut #47818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 33 additions & 19 deletions src/librustc_borrowck/borrowck/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,19 +397,16 @@ pub enum LoanPathElem<'tcx> {
LpInterior(Option<DefId>, InteriorKind),
}

fn closure_to_block(closure_id: LocalDefId,
tcx: TyCtxt) -> ast::NodeId {
fn closure_to_block(closure_id: LocalDefId, tcx: TyCtxt) -> ast::NodeId {
let closure_id = tcx.hir.local_def_id_to_node_id(closure_id);
match tcx.hir.get(closure_id) {
hir_map::NodeExpr(expr) => match expr.node {
hir::ExprClosure(.., body_id, _, _) => {
body_id.node_id
}
_ => {
bug!("encountered non-closure id: {}", closure_id)
}
_ => bug!("encountered non-closure id: {}", closure_id),
},
_ => bug!("encountered non-expr id: {}", closure_id)
_ => bug!("encountered non-closure id: {}", closure_id),
}
}

Expand Down Expand Up @@ -894,7 +891,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
}
};

self.note_and_explain_mutbl_error(&mut db, &err, &error_span);
self.note_and_explain_mutbl_error(&mut db, &err, &descr);
self.note_immutability_blame(&mut db, err.cmt.immutability_blame());
db.emit();
}
Expand Down Expand Up @@ -1282,8 +1279,10 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
}
}

fn note_and_explain_mutbl_error(&self, db: &mut DiagnosticBuilder, err: &BckError<'tcx>,
error_span: &Span) {
fn note_and_explain_mutbl_error(&self,
db: &mut DiagnosticBuilder,
err: &BckError<'tcx>,
descr: &str) {
match err.cmt.note {
mc::NoteClosureEnv(upvar_id) | mc::NoteUpvarRef(upvar_id) => {
// If this is an `Fn` closure, it simply can't mutate upvars.
Expand All @@ -1301,24 +1300,39 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
self by mutable reference");
}
}
_ => {
mc::NoteNone => {
if let Categorization::Deref(..) = err.cmt.cat {
db.span_label(*error_span, "cannot borrow as mutable");
db.span_label(err.span, "cannot borrow as mutable");
} else if let Categorization::Local(local_id) = err.cmt.cat {
let span = self.tcx.hir.span(local_id);
let mut generic_label = true;
let span = self.tcx.hir.span(local_id); // same as err.cmt.span
if let Ok(snippet) = self.tcx.sess.codemap().span_to_snippet(span) {
if snippet.starts_with("ref mut ") || snippet.starts_with("&mut ") {
db.span_label(*error_span, "cannot reborrow mutably");
db.span_label(*error_span, "try removing `&mut` here");
} else {
db.span_label(*error_span, "cannot borrow mutably");
db.span_label(span, "given this existing mutable borrow...");
if let Ok(s) = self.tcx.sess.codemap().span_to_snippet(err.span) {
// FIXME: this is a hack, error span should be pointing at the
// binding inside this closure, but instead it's pointing at the
// closure.
if !s.starts_with('|') {
db.span_label(err.span, "cannot reborrow mutably");
db.span_label(err.span, "try removing `&mut` here");
} else {
db.span_label(
err.span,
format!("cannot reborrow {} mutably in this closure",
descr),
);
}
generic_label = false;
}
}
} else {
db.span_label(*error_span, "cannot borrow mutably");
}
if generic_label {
db.span_label(err.span, "cannot borrow mutably");
}
} else if let Categorization::Interior(ref cmt, _) = err.cmt.cat {
if let mc::MutabilityCategory::McImmutable = cmt.mutbl {
db.span_label(*error_span,
db.span_label(err.span,
"cannot mutably borrow field of immutable binding");
}
}
Expand Down
24 changes: 24 additions & 0 deletions src/test/ui/borrowck/mut-reborrow-self.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

struct Foo {
u :u32,
}
impl Foo {
fn foo(&mut self) {
|| {
let r = &mut self;
r.u += 1;
};
}
//~^^^^^ ERROR closure cannot assign to immutable argument `self` [E0595]
}

fn main() {}
10 changes: 10 additions & 0 deletions src/test/ui/borrowck/mut-reborrow-self.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error[E0595]: closure cannot assign to immutable argument `self`
--> $DIR/mut-reborrow-self.rs:16:9
|
15 | fn foo(&mut self) {
| --------- given this existing mutable borrow...
16 | || {
| ^^ cannot reborrow immutable argument `self` mutably in this closure

error: aborting due to previous error

2 changes: 2 additions & 0 deletions src/test/ui/did_you_mean/issue-31424.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
error[E0596]: cannot borrow immutable argument `self` as mutable
--> $DIR/issue-31424.rs:17:15
|
16 | fn foo(&mut self) {
| --------- given this existing mutable borrow...
Copy link
Contributor

@nikomatsakis nikomatsakis Jan 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the existing message is confusing, but this doesn't seem quite right. The problem is that &mut self desugars to self: &mut Self, and hence the variable self itself is immutable and can't be borrowed mutably. I find pointing out the "existing mutable borrow" somewhat misleading, since it suggests that this is a problem of a borrow conflicting with another action, but that's not right right.

For example, this code compiles:

struct Foo {
    u :u32,
}
impl Foo {
    fn foo(mut self: &mut Self) {
        || {
            let r = &mut self;
            r.u += 1;
        };
    }
    //~^^^^^ ERROR closure cannot assign to immutable argument `self` [E0595]
}

fn main() {}

I have to wonder -- rather than improve the diagnostics here, maybe we should make &mut self desugar to mut self: &mut Self so that these code snippets work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that &mut self should desugar to mut self and should probably be accepted. That being said, I would like to keep the label to other cases like TestEnum::Item(ref mut x).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if they should all desugar the same way? It seems like the error message is equally misleading here (or at least I find it misleading). The problem is that the binding is not mutable -- but why isn't it, anyway? I feel like we've been moving towards a place where it's ok to &mut borrow an &mut -- thanks to deref coercions etc -- and I wonder if we should just embrace it.

Not sure how to make this decision. Feels like a thin RFC, but still a highly visible change in some sense, certainly would want at least FCP!

cc @rust-lang/lang -- thoughts? The context is that &mut self and ref mut foo both desugar to an immutable binding, but people sometimes stub their toe writing &mut self or &mut foo, which results in an error (where &mut *self and &mut *foo would work).

@estebank -- that reminds me, the suggestion to remove &mut (instead of adding *) is not necessarily a good one. Removing &mut can result in moves, where adding * would work more often and be closer to the existing "intent" of the code, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the semantics to accept this in all cases sounds eminently reasonable to me. I know I found it confusing at first, even though I'm now used to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, this also resolved something with match desugarings. I kind of forget the details now, but there was a minor inconsistency that centered about ref mut not being mut... maybe @cramertj remembers?

17 | (&mut self).bar(); //~ ERROR cannot borrow
| ^^^^
| |
Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/did_you_mean/issue-34126.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
error[E0596]: cannot borrow immutable argument `self` as mutable
--> $DIR/issue-34126.rs:16:23
|
15 | fn start(&mut self) {
| --------- given this existing mutable borrow...
16 | self.run(&mut self); //~ ERROR cannot borrow
| ^^^^
| |
Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/did_you_mean/issue-34337.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
error[E0596]: cannot borrow immutable local variable `key` as mutable
--> $DIR/issue-34337.rs:16:14
|
15 | let ref mut key = v[0];
| ----------- given this existing mutable borrow...
16 | get(&mut key); //~ ERROR cannot borrow
| ^^^
| |
Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/did_you_mean/issue-37139.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
error[E0596]: cannot borrow immutable local variable `x` as mutable
--> $DIR/issue-37139.rs:22:23
|
21 | TestEnum::Item(ref mut x) => {
| --------- given this existing mutable borrow...
22 | test(&mut x); //~ ERROR cannot borrow immutable
| ^
| |
Expand Down