Auto merge of #52242 - ashtneoi:suggest-ref-mut, r=pnkfelix

NLL: Suggest `ref mut` and `&mut self`

Fixes #51244. Supersedes #51249, I think.

Under the old lexical lifetimes, the compiler provided helpful suggestions about adding `mut` when you tried to mutate a variable bound as `&self` or (explicit) `ref`. NLL doesn't have those suggestions yet. This pull request adds them.

I didn't bother making the help text exactly the same as without NLL, but I can if that's important.

(Originally this was supposed to be part of #51612, but I got bogged down trying to fit everything in one PR.)

Author: bors

src/librustc_borrowck/borrowck/mod.rs

@@ -39,6 +39,7 @@ use rustc::middle::free_region::RegionRelations;
 use rustc::ty::{self, Ty, TyCtxt};
 use rustc::ty::query::Providers;
 use rustc_mir::util::borrowck_errors::{BorrowckErrors, Origin};
+use rustc_mir::util::suggest_ref_mut;
 use rustc::util::nodemap::FxHashSet;
 
 use std::cell::RefCell;
@@ -1206,15 +1207,15 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
                 self.note_immutable_local(db, error_node_id, node_id)
             }
             Some(ImmutabilityBlame::LocalDeref(node_id)) => {
-                let let_span = self.tcx.hir.span(node_id);
                 match self.local_binding_mode(node_id) {
                     ty::BindByReference(..) => {
-                        let snippet = self.tcx.sess.codemap().span_to_snippet(let_span);
-                        if let Ok(snippet) = snippet {
-                            db.span_label(
+                        let let_span = self.tcx.hir.span(node_id);
+                        let suggestion = suggest_ref_mut(self.tcx, let_span);
+                        if let Some((let_span, replace_str)) = suggestion {
+                            db.span_suggestion(
                                 let_span,
-                                format!("consider changing this to `{}`",
-                                         snippet.replace("ref ", "ref mut "))
+                                "use a mutable reference instead",
+                                replace_str,
                             );
                         }
                     }

src/librustc_mir/borrow_check/mod.rs

@@ -44,6 +44,7 @@ use dataflow::{EverInitializedPlaces, MovingOutStatements};
 use dataflow::{MaybeInitializedPlaces, MaybeUninitializedPlaces};
 use util::borrowck_errors::{BorrowckErrors, Origin};
 use util::collect_writes::FindAssignments;
+use util::suggest_ref_mut;
 
 use self::borrow_set::{BorrowData, BorrowSet};
 use self::flows::Flows;
@@ -1837,17 +1838,41 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             Place::Projection(box Projection {
                 base: Place::Local(local),
                 elem: ProjectionElem::Deref,
-            }) if self.mir.local_decls[*local].is_nonref_binding() =>
-            {
-                let (err_help_span, suggested_code) =
-                    find_place_to_suggest_ampmut(self.tcx, self.mir, *local);
-                err.span_suggestion(
-                    err_help_span,
-                    "consider changing this to be a mutable reference",
-                    suggested_code,
-                );
-
+            }) if self.mir.local_decls[*local].is_user_variable.is_some() => {
                 let local_decl = &self.mir.local_decls[*local];
+                let suggestion = match local_decl.is_user_variable.as_ref().unwrap() {
+                    ClearCrossCrate::Set(mir::BindingForm::ImplicitSelf) => {
+                        Some(suggest_ampmut_self(local_decl))
+                    },
+
+                    ClearCrossCrate::Set(mir::BindingForm::Var(mir::VarBindingForm {
+                        binding_mode: ty::BindingMode::BindByValue(_),
+                        opt_ty_info,
+                        ..
+                    })) => Some(suggest_ampmut(
+                        self.tcx,
+                        self.mir,
+                        *local,
+                        local_decl,
+                        *opt_ty_info,
+                    )),
+
+                    ClearCrossCrate::Set(mir::BindingForm::Var(mir::VarBindingForm {
+                        binding_mode: ty::BindingMode::BindByReference(_),
+                        ..
+                    })) => suggest_ref_mut(self.tcx, local_decl.source_info.span),
+
+                    ClearCrossCrate::Clear => bug!("saw cleared local state"),
+                };
+
+                if let Some((err_help_span, suggested_code)) = suggestion {
+                    err.span_suggestion(
+                        err_help_span,
+                        "consider changing this to be a mutable reference",
+                        suggested_code,
+                    );
+                }
+
                 if let Some(name) = local_decl.name {
                     err.span_label(
                         span,
@@ -1874,13 +1899,16 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         err.emit();
         return true;
 
-        // Returns the span to highlight and the associated text to
-        // present when suggesting that the user use an `&mut`.
-        //
+        fn suggest_ampmut_self<'cx, 'gcx, 'tcx>(
+            local_decl: &mir::LocalDecl<'tcx>,
+        ) -> (Span, String) {
+            (local_decl.source_info.span, "&mut self".to_string())
+        }
+
         // When we want to suggest a user change a local variable to be a `&mut`, there
         // are three potential "obvious" things to highlight:
         //
-        // let ident [: Type] [= RightHandSideExresssion];
+        // let ident [: Type] [= RightHandSideExpression];
         //     ^^^^^    ^^^^     ^^^^^^^^^^^^^^^^^^^^^^^
         //     (1.)     (2.)              (3.)
         //
@@ -1889,48 +1917,44 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         // for example, if the RHS is present and the Type is not, then the type is going to
         // be inferred *from* the RHS, which means we should highlight that (and suggest
         // that they borrow the RHS mutably).
-        fn find_place_to_suggest_ampmut<'cx, 'gcx, 'tcx>(
+        //
+        // This implementation attempts to emulate AST-borrowck prioritization
+        // by trying (3.), then (2.) and finally falling back on (1.).
+        fn suggest_ampmut<'cx, 'gcx, 'tcx>(
             tcx: TyCtxt<'cx, 'gcx, 'tcx>,
             mir: &Mir<'tcx>,
             local: Local,
+            local_decl: &mir::LocalDecl<'tcx>,
+            opt_ty_info: Option<Span>,
         ) -> (Span, String) {
-            // This implementation attempts to emulate AST-borrowck prioritization
-            // by trying (3.), then (2.) and finally falling back on (1.).
             let locations = mir.find_assignments(local);
             if locations.len() > 0 {
                 let assignment_rhs_span = mir.source_info(locations[0]).span;
                 let snippet = tcx.sess.codemap().span_to_snippet(assignment_rhs_span);
                 if let Ok(src) = snippet {
-                    // pnkfelix inherited code; believes intention is
-                    // highlighted text will always be `&<expr>` and
-                    // thus can transform to `&mut` by slicing off
-                    // first ASCII character and prepending "&mut ".
                     if src.starts_with('&') {
                         let borrowed_expr = src[1..].to_string();
-                        return (assignment_rhs_span, format!("&mut {}", borrowed_expr));
+                        return (
+                            assignment_rhs_span,
+                            format!("&mut {}", borrowed_expr),
+                        );
                     }
                 }
             }
 
-            let local_decl = &mir.local_decls[local];
-            let highlight_span = match local_decl.is_user_variable {
+            let highlight_span = match opt_ty_info {
                 // if this is a variable binding with an explicit type,
                 // try to highlight that for the suggestion.
-                Some(ClearCrossCrate::Set(mir::BindingForm::Var(mir::VarBindingForm {
-                    opt_ty_info: Some(ty_span),
-                    ..
-                }))) => ty_span,
-
-                Some(ClearCrossCrate::Clear) => bug!("saw cleared local state"),
+                Some(ty_span) => ty_span,
 
                 // otherwise, just highlight the span associated with
                 // the (MIR) LocalDecl.
-                _ => local_decl.source_info.span,
+                None => local_decl.source_info.span,
             };
 
             let ty_mut = local_decl.ty.builtin_deref(true).unwrap();
             assert_eq!(ty_mut.mutbl, hir::MutImmutable);
-            return (highlight_span, format!("&mut {}", ty_mut.ty));
+            (highlight_span, format!("&mut {}", ty_mut.ty))
         }
     }
 

src/librustc_mir/lib.rs

@@ -33,6 +33,7 @@ Rust MIR: a lowered representation of Rust. Also: an experiment!
 #![feature(never_type)]
 #![feature(specialization)]
 #![feature(try_trait)]
+#![feature(unicode_internals)]
 
 #![recursion_limit="256"]
 
@@ -56,6 +57,7 @@ extern crate rustc_target;
 extern crate log_settings;
 extern crate rustc_apfloat;
 extern crate byteorder;
+extern crate core;
 
 mod diagnostics;
 

src/librustc_mir/util/mod.rs

@@ -8,6 +8,10 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+use core::unicode::property::Pattern_White_Space;
+use rustc::ty;
+use syntax_pos::Span;
+
 pub mod borrowck_errors;
 pub mod elaborate_drops;
 pub mod def_use;
@@ -23,3 +27,19 @@ pub use self::alignment::is_disaligned;
 pub use self::pretty::{dump_enabled, dump_mir, write_mir_pretty, PassWhere};
 pub use self::graphviz::{write_mir_graphviz};
 pub use self::graphviz::write_node_label as write_graphviz_node_label;
+
+/// If possible, suggest replacing `ref` with `ref mut`.
+pub fn suggest_ref_mut<'cx, 'gcx, 'tcx>(
+    tcx: ty::TyCtxt<'cx, 'gcx, 'tcx>,
+    pattern_span: Span,
+) -> Option<(Span, String)> {
+    let hi_src = tcx.sess.codemap().span_to_snippet(pattern_span).unwrap();
+    if hi_src.starts_with("ref")
+        && hi_src["ref".len()..].starts_with(Pattern_White_Space)
+    {
+        let replacement = format!("ref mut{}", &hi_src["ref".len()..]);
+        Some((pattern_span, replacement))
+    } else {
+        None
+    }
+}

src/test/ui/did_you_mean/issue-38147-1.nll.stderr

@@ -2,7 +2,7 @@ error[E0596]: cannot borrow immutable item `*self.s` as mutable
   --> $DIR/issue-38147-1.rs:27:9
    |
 LL |     fn f(&self) {
-   |          ----- help: consider changing this to be a mutable reference: `&mut Foo<'_>`
+   |          ----- help: consider changing this to be a mutable reference: `&mut self`
 LL |         self.s.push('x'); //~ ERROR cannot borrow data mutably
    |         ^^^^^^ `self` is a `&` reference, so the data it refers to cannot be borrowed as mutable
 

src/test/ui/did_you_mean/issue-39544.nll.stderr

@@ -10,15 +10,15 @@ error[E0596]: cannot borrow immutable item `self.x` as mutable
   --> $DIR/issue-39544.rs:26:17
    |
 LL |     fn foo<'z>(&'z self) {
-   |                -------- help: consider changing this to be a mutable reference: `&mut Z`
+   |                -------- help: consider changing this to be a mutable reference: `&mut self`
 LL |         let _ = &mut self.x; //~ ERROR cannot borrow
    |                 ^^^^^^^^^^^ `self` is a `&` reference, so the data it refers to cannot be borrowed as mutable
 
 error[E0596]: cannot borrow immutable item `self.x` as mutable
   --> $DIR/issue-39544.rs:30:17
    |
 LL |     fn foo1(&self, other: &Z) {
-   |             ----- help: consider changing this to be a mutable reference: `&mut Z`
+   |             ----- help: consider changing this to be a mutable reference: `&mut self`
 LL |         let _ = &mut self.x; //~ ERROR cannot borrow
    |                 ^^^^^^^^^^^ `self` is a `&` reference, so the data it refers to cannot be borrowed as mutable
 
@@ -35,7 +35,7 @@ error[E0596]: cannot borrow immutable item `self.x` as mutable
   --> $DIR/issue-39544.rs:35:17
    |
 LL |     fn foo2<'a>(&'a self, other: &Z) {
-   |                 -------- help: consider changing this to be a mutable reference: `&mut Z`
+   |                 -------- help: consider changing this to be a mutable reference: `&mut self`
 LL |         let _ = &mut self.x; //~ ERROR cannot borrow
    |                 ^^^^^^^^^^^ `self` is a `&` reference, so the data it refers to cannot be borrowed as mutable
 

src/test/ui/nll/issue-51244.rs

@@ -0,0 +1,17 @@
+// 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.
+
+#![feature(nll)]
+
+fn main() {
+    let ref my_ref @ _ = 0;
+    *my_ref = 0;
+    //~^ ERROR cannot assign to `*my_ref` which is behind a `&` reference [E0594]
+}

src/test/ui/nll/issue-51244.stderr

@@ -0,0 +1,11 @@
+error[E0594]: cannot assign to `*my_ref` which is behind a `&` reference
+  --> $DIR/issue-51244.rs:15:5
+   |
+LL |     let ref my_ref @ _ = 0;
+   |         -------------- help: consider changing this to be a mutable reference: `ref mut my_ref @ _`
+LL |     *my_ref = 0;
+   |     ^^^^^^^^^^^ `my_ref` is a `&` reference, so the data it refers to cannot be written
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0594`.

src/test/ui/rfc-2005-default-binding-mode/enum.nll.stderr

@@ -2,19 +2,19 @@ error[E0594]: cannot assign to `*x` which is behind a `&` reference
   --> $DIR/enum.rs:19:5
    |
 LL |     *x += 1; //~ ERROR cannot assign to immutable
-   |     ^^^^^^^ cannot assign
+   |     ^^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written
 
 error[E0594]: cannot assign to `*x` which is behind a `&` reference
   --> $DIR/enum.rs:23:9
    |
 LL |         *x += 1; //~ ERROR cannot assign to immutable
-   |         ^^^^^^^ cannot assign
+   |         ^^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written
 
 error[E0594]: cannot assign to `*x` which is behind a `&` reference
   --> $DIR/enum.rs:29:9
    |
 LL |         *x += 1; //~ ERROR cannot assign to immutable
-   |         ^^^^^^^ cannot assign
+   |         ^^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written
 
 error: aborting due to 3 previous errors
 

src/test/ui/rfc-2005-default-binding-mode/enum.stderr

@@ -1,24 +1,18 @@
 error[E0594]: cannot assign to immutable borrowed content `*x`
   --> $DIR/enum.rs:19:5
    |
-LL |     let Wrap(x) = &Wrap(3);
-   |              - consider changing this to `x`
 LL |     *x += 1; //~ ERROR cannot assign to immutable
    |     ^^^^^^^ cannot borrow as mutable
 
 error[E0594]: cannot assign to immutable borrowed content `*x`
   --> $DIR/enum.rs:23:9
    |
-LL |     if let Some(x) = &Some(3) {
-   |                 - consider changing this to `x`
 LL |         *x += 1; //~ ERROR cannot assign to immutable
    |         ^^^^^^^ cannot borrow as mutable
 
 error[E0594]: cannot assign to immutable borrowed content `*x`
   --> $DIR/enum.rs:29:9
    |
-LL |     while let Some(x) = &Some(3) {
-   |                    - consider changing this to `x`
 LL |         *x += 1; //~ ERROR cannot assign to immutable
    |         ^^^^^^^ cannot borrow as mutable
 

src/test/ui/rfc-2005-default-binding-mode/explicit-mut.nll.stderr

@@ -2,19 +2,19 @@ error[E0594]: cannot assign to `*n` which is behind a `&` reference
   --> $DIR/explicit-mut.rs:17:13
    |
 LL |             *n += 1; //~ ERROR cannot assign to immutable
-   |             ^^^^^^^ cannot assign
+   |             ^^^^^^^ `n` is a `&` reference, so the data it refers to cannot be written
 
 error[E0594]: cannot assign to `*n` which is behind a `&` reference
   --> $DIR/explicit-mut.rs:25:13
    |
 LL |             *n += 1; //~ ERROR cannot assign to immutable
-   |             ^^^^^^^ cannot assign
+   |             ^^^^^^^ `n` is a `&` reference, so the data it refers to cannot be written
 
 error[E0594]: cannot assign to `*n` which is behind a `&` reference
   --> $DIR/explicit-mut.rs:33:13
    |
 LL |             *n += 1; //~ ERROR cannot assign to immutable
-   |             ^^^^^^^ cannot assign
+   |             ^^^^^^^ `n` is a `&` reference, so the data it refers to cannot be written
 
 error: aborting due to 3 previous errors
 

src/test/ui/rfc-2005-default-binding-mode/explicit-mut.stderr

@@ -1,24 +1,18 @@
 error[E0594]: cannot assign to immutable borrowed content `*n`
   --> $DIR/explicit-mut.rs:17:13
    |
-LL |         Some(n) => {
-   |              - consider changing this to `n`
 LL |             *n += 1; //~ ERROR cannot assign to immutable
    |             ^^^^^^^ cannot borrow as mutable
 
 error[E0594]: cannot assign to immutable borrowed content `*n`
   --> $DIR/explicit-mut.rs:25:13
    |
-LL |         Some(n) => {
-   |              - consider changing this to `n`
 LL |             *n += 1; //~ ERROR cannot assign to immutable
    |             ^^^^^^^ cannot borrow as mutable
 
 error[E0594]: cannot assign to immutable borrowed content `*n`
   --> $DIR/explicit-mut.rs:33:13
    |
-LL |         Some(n) => {
-   |              - consider changing this to `n`
 LL |             *n += 1; //~ ERROR cannot assign to immutable
    |             ^^^^^^^ cannot borrow as mutable
 

src/test/ui/suggestions/issue-51244.nll.stderr

@@ -0,0 +1,11 @@
+error[E0594]: cannot assign to `*my_ref` which is behind a `&` reference
+  --> $DIR/issue-51244.rs:13:5
+   |
+LL |     let ref my_ref @ _ = 0;
+   |         -------------- help: consider changing this to be a mutable reference: `ref mut my_ref @ _`
+LL |     *my_ref = 0; //~ ERROR cannot assign to immutable borrowed content `*my_ref` [E0594]
+   |     ^^^^^^^^^^^ `my_ref` is a `&` reference, so the data it refers to cannot be written
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0594`.