Auto merge of #51242 - ytausky:mut-ref, r=estebank

Suggest not mutably borrowing a mutable reference

This PR would (hopefully) solve #45392. I deviated a bit from @estebank's instructions since the error span only included the borrowed expression (e.g. the `b` in `&mut b`). I also didn't check the mutability of the local binding, since this whole case is concerned with an immutable local.

I can see two outstanding questions:
1. `note_immutability_blame` is called in two places, but I only have one test case. I think it covers the call in `report_bckerror`, but I'm not sure how to trigger the call from `report_aliasability_violation`.
2. There is one failing test, where the local binding is `self: &mut Self`. I'm not entirely sure what the correct output should be, but I think the new message should also apply. Unfortunately, since this parameter is parsed differently, its `let_span` covers both the pattern and the type, leading to a wrong suggestion text. I'm not sure how to correctly identify this case.

Author: bors

src/librustc_borrowck/borrowck/mod.rs

@@ -899,7 +899,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
                 };
 
                 self.note_and_explain_mutbl_error(&mut db, &err, &error_span);
-                self.note_immutability_blame(&mut db, err.cmt.immutability_blame());
+                self.note_immutability_blame(&mut db, err.cmt.immutability_blame(), err.cmt.id);
                 db.emit();
             }
             err_out_of_scope(super_scope, sub_scope, cause) => {
@@ -1105,7 +1105,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
                                                             Origin::Ast)
             }
         };
-        self.note_immutability_blame(&mut err, blame);
+        self.note_immutability_blame(&mut err, blame, cmt.id);
 
         if is_closure {
             err.help("closures behind references must be called via `&mut`");
@@ -1184,25 +1184,13 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
 
     fn note_immutability_blame(&self,
                                db: &mut DiagnosticBuilder,
-                               blame: Option<ImmutabilityBlame>) {
+                               blame: Option<ImmutabilityBlame>,
+                               error_node_id: ast::NodeId) {
         match blame {
             None => {}
             Some(ImmutabilityBlame::ClosureEnv(_)) => {}
             Some(ImmutabilityBlame::ImmLocal(node_id)) => {
-                let let_span = self.tcx.hir.span(node_id);
-                if let ty::BindByValue(..) = self.local_binding_mode(node_id) {
-                    if let Ok(snippet) = self.tcx.sess.codemap().span_to_snippet(let_span) {
-                        let (_, is_implicit_self) = self.local_ty(node_id);
-                        if is_implicit_self && snippet != "self" {
-                            // avoid suggesting `mut &self`.
-                            return
-                        }
-                        db.span_label(
-                            let_span,
-                            format!("consider changing this to `mut {}`", snippet)
-                        );
-                    }
-                }
+                self.note_immutable_local(db, error_node_id, node_id)
             }
             Some(ImmutabilityBlame::LocalDeref(node_id)) => {
                 let let_span = self.tcx.hir.span(node_id);
@@ -1242,6 +1230,46 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
         }
     }
 
+     // Suggest a fix when trying to mutably borrow an immutable local
+     // binding: either to make the binding mutable (if its type is
+     // not a mutable reference) or to avoid borrowing altogether
+    fn note_immutable_local(&self,
+                            db: &mut DiagnosticBuilder,
+                            borrowed_node_id: ast::NodeId,
+                            binding_node_id: ast::NodeId) {
+        let let_span = self.tcx.hir.span(binding_node_id);
+        if let ty::BindByValue(..) = self.local_binding_mode(binding_node_id) {
+            if let Ok(snippet) = self.tcx.sess.codemap().span_to_snippet(let_span) {
+                let (ty, is_implicit_self) = self.local_ty(binding_node_id);
+                if is_implicit_self && snippet != "self" {
+                    // avoid suggesting `mut &self`.
+                    return
+                }
+                if let Some(&hir::TyRptr(
+                    _,
+                    hir::MutTy {
+                        mutbl: hir::MutMutable,
+                        ..
+                    },
+                )) = ty.map(|t| &t.node)
+                {
+                    let borrow_expr_id = self.tcx.hir.get_parent_node(borrowed_node_id);
+                    db.span_suggestion(
+                        self.tcx.hir.span(borrow_expr_id),
+                        "consider removing the `&mut`, as it is an \
+                        immutable binding to a mutable reference",
+                        snippet
+                    );
+                } else {
+                    db.span_label(
+                        let_span,
+                        format!("consider changing this to `mut {}`", snippet),
+                    );
+                }
+            }
+        }
+    }
+
     fn report_out_of_scope_escaping_closure_capture(&self,
                                                     err: &BckError<'a, 'tcx>,
                                                     capture_span: Span)

src/libsyntax/parse/parser.rs

@@ -5190,88 +5190,90 @@ impl<'a> Parser<'a> {
         // Only a limited set of initial token sequences is considered self parameters, anything
         // else is parsed as a normal function parameter list, so some lookahead is required.
         let eself_lo = self.span;
-        let (eself, eself_ident) = match self.token {
+        let (eself, eself_ident, eself_hi) = match self.token {
             token::BinOp(token::And) => {
                 // &self
                 // &mut self
                 // &'lt self
                 // &'lt mut self
                 // &not_self
-                if isolated_self(self, 1) {
+                (if isolated_self(self, 1) {
                     self.bump();
-                    (SelfKind::Region(None, Mutability::Immutable), expect_ident(self))
+                    SelfKind::Region(None, Mutability::Immutable)
                 } else if self.look_ahead(1, |t| t.is_keyword(keywords::Mut)) &&
                           isolated_self(self, 2) {
                     self.bump();
                     self.bump();
-                    (SelfKind::Region(None, Mutability::Mutable), expect_ident(self))
+                    SelfKind::Region(None, Mutability::Mutable)
                 } else if self.look_ahead(1, |t| t.is_lifetime()) &&
                           isolated_self(self, 2) {
                     self.bump();
                     let lt = self.expect_lifetime();
-                    (SelfKind::Region(Some(lt), Mutability::Immutable), expect_ident(self))
+                    SelfKind::Region(Some(lt), Mutability::Immutable)
                 } else if self.look_ahead(1, |t| t.is_lifetime()) &&
                           self.look_ahead(2, |t| t.is_keyword(keywords::Mut)) &&
                           isolated_self(self, 3) {
                     self.bump();
                     let lt = self.expect_lifetime();
                     self.bump();
-                    (SelfKind::Region(Some(lt), Mutability::Mutable), expect_ident(self))
+                    SelfKind::Region(Some(lt), Mutability::Mutable)
                 } else {
                     return Ok(None);
-                }
+                }, expect_ident(self), self.prev_span)
             }
             token::BinOp(token::Star) => {
                 // *self
                 // *const self
                 // *mut self
                 // *not_self
                 // Emit special error for `self` cases.
-                if isolated_self(self, 1) {
+                (if isolated_self(self, 1) {
                     self.bump();
                     self.span_err(self.span, "cannot pass `self` by raw pointer");
-                    (SelfKind::Value(Mutability::Immutable), expect_ident(self))
+                    SelfKind::Value(Mutability::Immutable)
                 } else if self.look_ahead(1, |t| t.is_mutability()) &&
                           isolated_self(self, 2) {
                     self.bump();
                     self.bump();
                     self.span_err(self.span, "cannot pass `self` by raw pointer");
-                    (SelfKind::Value(Mutability::Immutable), expect_ident(self))
+                    SelfKind::Value(Mutability::Immutable)
                 } else {
                     return Ok(None);
-                }
+                }, expect_ident(self), self.prev_span)
             }
             token::Ident(..) => {
                 if isolated_self(self, 0) {
                     // self
                     // self: TYPE
                     let eself_ident = expect_ident(self);
-                    if self.eat(&token::Colon) {
+                    let eself_hi = self.prev_span;
+                    (if self.eat(&token::Colon) {
                         let ty = self.parse_ty()?;
-                        (SelfKind::Explicit(ty, Mutability::Immutable), eself_ident)
+                        SelfKind::Explicit(ty, Mutability::Immutable)
                     } else {
-                        (SelfKind::Value(Mutability::Immutable), eself_ident)
-                    }
+                        SelfKind::Value(Mutability::Immutable)
+                    }, eself_ident, eself_hi)
                 } else if self.token.is_keyword(keywords::Mut) &&
                           isolated_self(self, 1) {
                     // mut self
                     // mut self: TYPE
                     self.bump();
                     let eself_ident = expect_ident(self);
-                    if self.eat(&token::Colon) {
+                    let eself_hi = self.prev_span;
+                    (if self.eat(&token::Colon) {
                         let ty = self.parse_ty()?;
-                        (SelfKind::Explicit(ty, Mutability::Mutable), eself_ident)
+                        SelfKind::Explicit(ty, Mutability::Mutable)
                     } else {
-                        (SelfKind::Value(Mutability::Mutable), eself_ident)
-                    }
+                        SelfKind::Value(Mutability::Mutable)
+                    }, eself_ident, eself_hi)
                 } else {
                     return Ok(None);
                 }
             }
             _ => return Ok(None),
         };
 
-        let eself = codemap::respan(eself_lo.to(self.prev_span), eself);
+        let eself = codemap::respan(eself_lo.to(eself_hi), eself);
         Ok(Some(Arg::from_self(eself, eself_ident)))
     }
 

src/test/ui/borrowck/mut-borrow-of-mut-ref.nll.stderr

@@ -0,0 +1,9 @@
+error[E0596]: cannot borrow immutable item `b` as mutable
+  --> $DIR/mut-borrow-of-mut-ref.rs:18:7
+   |
+LL |     g(&mut b) //~ ERROR cannot borrow
+   |       ^^^^^^ cannot borrow as mutable
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0596`.

src/test/ui/borrowck/mut-borrow-of-mut-ref.rs

@@ -0,0 +1,21 @@
+// Copyright 2017 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.
+
+// Suggest not mutably borrowing a mutable reference
+
+fn main() {
+    f(&mut 0)
+}
+
+fn f(b: &mut i32) {
+    g(&mut b) //~ ERROR cannot borrow
+}
+
+fn g(_: &mut i32) {}

src/test/ui/borrowck/mut-borrow-of-mut-ref.stderr

@@ -0,0 +1,13 @@
+error[E0596]: cannot borrow immutable argument `b` as mutable
+  --> $DIR/mut-borrow-of-mut-ref.rs:18:12
+   |
+LL |     g(&mut b) //~ ERROR cannot borrow
+   |            ^ cannot borrow mutably
+help: consider removing the `&mut`, as it is an immutable binding to a mutable reference
+   |
+LL |     g(b) //~ ERROR cannot borrow
+   |       ^
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0596`.

src/test/ui/did_you_mean/issue-31424.stderr

@@ -10,10 +10,12 @@ LL |         (&mut self).bar(); //~ ERROR cannot borrow
 error[E0596]: cannot borrow immutable argument `self` as mutable
   --> $DIR/issue-31424.rs:23:15
    |
-LL |     fn bar(self: &mut Self) {
-   |            --------------- consider changing this to `mut self: &mut Self`
 LL |         (&mut self).bar(); //~ ERROR cannot borrow
    |               ^^^^ cannot borrow mutably
+help: consider removing the `&mut`, as it is an immutable binding to a mutable reference
+   |
+LL |         self.bar(); //~ ERROR cannot borrow
+   |         ^^^^
 
 error: aborting due to 2 previous errors