Auto merge of #54720 - davidtwco:issue-51191, r=nikomatsakis

NLL fails to suggest "try removing `&mut` here"

Fixes #51191.

This PR adds ``try removing `&mut` here`` suggestions to functions where a mutable borrow is being taken of a `&mut self` or a `self: &mut Self`. This PR also enables the suggestion for adding a `mut` pattern to by-value implicit self arguments without `mut` patterns already.

r? @nikomatsakis

Author: bors

src/librustc/hir/lowering.rs

@@ -2044,11 +2044,31 @@ impl<'a> LoweringContext<'a> {
             inputs,
             output,
             variadic: decl.variadic,
-            has_implicit_self: decl.inputs.get(0).map_or(false, |arg| match arg.ty.node {
-                TyKind::ImplicitSelf => true,
-                TyKind::Rptr(_, ref mt) => mt.ty.node.is_implicit_self(),
-                _ => false,
-            }),
+            implicit_self: decl.inputs.get(0).map_or(
+                hir::ImplicitSelfKind::None,
+                |arg| {
+                    let is_mutable_pat = match arg.pat.node {
+                        PatKind::Ident(BindingMode::ByValue(mt), _, _) |
+                        PatKind::Ident(BindingMode::ByRef(mt), _, _) =>
+                            mt == Mutability::Mutable,
+                        _ => false,
+                    };
+
+                    match arg.ty.node {
+                        TyKind::ImplicitSelf if is_mutable_pat => hir::ImplicitSelfKind::Mut,
+                        TyKind::ImplicitSelf => hir::ImplicitSelfKind::Imm,
+                        // Given we are only considering `ImplicitSelf` types, we needn't consider
+                        // the case where we have a mutable pattern to a reference as that would
+                        // no longer be an `ImplicitSelf`.
+                        TyKind::Rptr(_, ref mt) if mt.ty.node.is_implicit_self() &&
+                            mt.mutbl == ast::Mutability::Mutable =>
+                                hir::ImplicitSelfKind::MutRef,
+                        TyKind::Rptr(_, ref mt) if mt.ty.node.is_implicit_self() =>
+                            hir::ImplicitSelfKind::ImmRef,
+                        _ => hir::ImplicitSelfKind::None,
+                    }
+                },
+            ),
         })
     }
 

src/librustc/hir/mod.rs

@@ -1782,9 +1782,34 @@ pub struct FnDecl {
     pub inputs: HirVec<Ty>,
     pub output: FunctionRetTy,
     pub variadic: bool,
-    /// True if this function has an `self`, `&self` or `&mut self` receiver
-    /// (but not a `self: Xxx` one).
-    pub has_implicit_self: bool,
+    /// Does the function have an implicit self?
+    pub implicit_self: ImplicitSelfKind,
+}
+
+/// Represents what type of implicit self a function has, if any.
+#[derive(Clone, Copy, RustcEncodable, RustcDecodable, Debug)]
+pub enum ImplicitSelfKind {
+    /// Represents a `fn x(self);`.
+    Imm,
+    /// Represents a `fn x(mut self);`.
+    Mut,
+    /// Represents a `fn x(&self);`.
+    ImmRef,
+    /// Represents a `fn x(&mut self);`.
+    MutRef,
+    /// Represents when a function does not have a self argument or
+    /// when a function has a `self: X` argument.
+    None
+}
+
+impl ImplicitSelfKind {
+    /// Does this represent an implicit self?
+    pub fn has_implicit_self(&self) -> bool {
+        match *self {
+            ImplicitSelfKind::None => false,
+            _ => true,
+        }
+    }
 }
 
 /// Is the trait definition an auto trait?

src/librustc/ich/impls_hir.rs

@@ -355,14 +355,22 @@ impl_stable_hash_for!(struct hir::FnDecl {
     inputs,
     output,
     variadic,
-    has_implicit_self
+    implicit_self
 });
 
 impl_stable_hash_for!(enum hir::FunctionRetTy {
     DefaultReturn(span),
     Return(t)
 });
 
+impl_stable_hash_for!(enum hir::ImplicitSelfKind {
+    Imm,
+    Mut,
+    ImmRef,
+    MutRef,
+    None
+});
+
 impl_stable_hash_for!(struct hir::TraitRef {
     // Don't hash the ref_id. It is tracked via the thing it is used to access
     ref_id -> _,

src/librustc/mir/mod.rs

@@ -583,11 +583,27 @@ pub enum BindingForm<'tcx> {
     /// This is a binding for a non-`self` binding, or a `self` that has an explicit type.
     Var(VarBindingForm<'tcx>),
     /// Binding for a `self`/`&self`/`&mut self` binding where the type is implicit.
-    ImplicitSelf,
+    ImplicitSelf(ImplicitSelfKind),
     /// Reference used in a guard expression to ensure immutability.
     RefForGuard,
 }
 
+/// Represents what type of implicit self a function has, if any.
+#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)]
+pub enum ImplicitSelfKind {
+    /// Represents a `fn x(self);`.
+    Imm,
+    /// Represents a `fn x(mut self);`.
+    Mut,
+    /// Represents a `fn x(&self);`.
+    ImmRef,
+    /// Represents a `fn x(&mut self);`.
+    MutRef,
+    /// Represents when a function does not have a self argument or
+    /// when a function has a `self: X` argument.
+    None
+}
+
 CloneTypeFoldableAndLiftImpls! { BindingForm<'tcx>, }
 
 impl_stable_hash_for!(struct self::VarBindingForm<'tcx> {
@@ -597,6 +613,14 @@ impl_stable_hash_for!(struct self::VarBindingForm<'tcx> {
     pat_span
 });
 
+impl_stable_hash_for!(enum self::ImplicitSelfKind {
+    Imm,
+    Mut,
+    ImmRef,
+    MutRef,
+    None
+});
+
 mod binding_form_impl {
     use ich::StableHashingContext;
     use rustc_data_structures::stable_hasher::{HashStable, StableHasher, StableHasherResult};
@@ -612,7 +636,7 @@ mod binding_form_impl {
 
             match self {
                 Var(binding) => binding.hash_stable(hcx, hasher),
-                ImplicitSelf => (),
+                ImplicitSelf(kind) => kind.hash_stable(hcx, hasher),
                 RefForGuard => (),
             }
         }
@@ -775,10 +799,9 @@ impl<'tcx> LocalDecl<'tcx> {
                 pat_span: _,
             }))) => true,
 
-            // FIXME: might be able to thread the distinction between
-            // `self`/`mut self`/`&self`/`&mut self` into the
-            // `BindingForm::ImplicitSelf` variant, (and then return
-            // true here for solely the first case).
+            Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf(ImplicitSelfKind::Imm)))
+                => true,
+
             _ => false,
         }
     }
@@ -795,7 +818,7 @@ impl<'tcx> LocalDecl<'tcx> {
                 pat_span: _,
             }))) => true,
 
-            Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf)) => true,
+            Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf(_))) => true,
 
             _ => false,
         }

src/librustc_borrowck/borrowck/mod.rs

@@ -1221,7 +1221,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
             if let Some(i) = arg_pos {
                 // The argument's `Ty`
                 (Some(&fn_like.decl().inputs[i]),
-                 i == 0 && fn_like.decl().has_implicit_self)
+                 i == 0 && fn_like.decl().implicit_self.has_implicit_self())
             } else {
                 (None, false)
             }

src/librustc_mir/borrow_check/mutability_errors.rs

@@ -16,6 +16,7 @@ use rustc::mir::TerminatorKind;
 use rustc::ty::{self, Const, DefIdTree, TyS, TyKind, TyCtxt};
 use rustc_data_structures::indexed_vec::Idx;
 use syntax_pos::Span;
+use syntax_pos::symbol::keywords;
 
 use dataflow::move_paths::InitLocation;
 use borrow_check::MirBorrowckCtxt;
@@ -217,6 +218,40 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
         debug!("report_mutability_error: act={:?}, acted_on={:?}", act, acted_on);
 
         match the_place_err {
+            // Suggest removing a `&mut` from the use of a mutable reference.
+            Place::Local(local)
+                if {
+                    self.mir.local_decls.get(*local).map(|local_decl| {
+                        if let ClearCrossCrate::Set(
+                            mir::BindingForm::ImplicitSelf(kind)
+                        ) = local_decl.is_user_variable.as_ref().unwrap() {
+                            // Check if the user variable is a `&mut self` and we can therefore
+                            // suggest removing the `&mut`.
+                            //
+                            // Deliberately fall into this case for all implicit self types,
+                            // so that we don't fall in to the next case with them.
+                            *kind == mir::ImplicitSelfKind::MutRef
+                        } else if Some(keywords::SelfValue.name()) == local_decl.name {
+                            // Otherwise, check if the name is the self kewyord - in which case
+                            // we have an explicit self. Do the same thing in this case and check
+                            // for a `self: &mut Self` to suggest removing the `&mut`.
+                            if let ty::TyKind::Ref(
+                                _, _, hir::Mutability::MutMutable
+                            ) = local_decl.ty.sty {
+                                true
+                            } else {
+                                false
+                            }
+                        } else {
+                            false
+                        }
+                    }).unwrap_or(false)
+                } =>
+            {
+                err.span_label(span, format!("cannot {ACT}", ACT = act));
+                err.span_label(span, "try removing `&mut` here");
+            },
+
             // We want to suggest users use `let mut` for local (user
             // variable) mutations...
             Place::Local(local) if self.mir.local_decls[*local].can_be_made_mutable() => {
@@ -316,7 +351,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
             {
                 let local_decl = &self.mir.local_decls[*local];
                 let suggestion = match local_decl.is_user_variable.as_ref().unwrap() {
-                    ClearCrossCrate::Set(mir::BindingForm::ImplicitSelf) => {
+                    ClearCrossCrate::Set(mir::BindingForm::ImplicitSelf(_)) => {
                         Some(suggest_ampmut_self(self.infcx.tcx, local_decl))
                     }
 

src/librustc_mir/build/mod.rs

@@ -125,8 +125,14 @@ pub fn mir_build<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> Mir<'t
                             let ty_hir_id = fn_decl.inputs[index].hir_id;
                             let ty_span = tcx.hir.span(tcx.hir.hir_to_node_id(ty_hir_id));
                             opt_ty_info = Some(ty_span);
-                            self_arg = if index == 0 && fn_decl.has_implicit_self {
-                                Some(ImplicitSelfBinding)
+                            self_arg = if index == 0 && fn_decl.implicit_self.has_implicit_self() {
+                                match fn_decl.implicit_self {
+                                    hir::ImplicitSelfKind::Imm => Some(ImplicitSelfKind::Imm),
+                                    hir::ImplicitSelfKind::Mut => Some(ImplicitSelfKind::Mut),
+                                    hir::ImplicitSelfKind::ImmRef => Some(ImplicitSelfKind::ImmRef),
+                                    hir::ImplicitSelfKind::MutRef => Some(ImplicitSelfKind::MutRef),
+                                    _ => None,
+                                }
                             } else {
                                 None
                             };
@@ -508,12 +514,10 @@ fn should_abort_on_panic<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
 ///////////////////////////////////////////////////////////////////////////
 /// the main entry point for building MIR for a function
 
-struct ImplicitSelfBinding;
-
 struct ArgInfo<'gcx>(Ty<'gcx>,
                      Option<Span>,
                      Option<&'gcx hir::Pat>,
-                     Option<ImplicitSelfBinding>);
+                     Option<ImplicitSelfKind>);
 
 fn construct_fn<'a, 'gcx, 'tcx, A>(hir: Cx<'a, 'gcx, 'tcx>,
                                    fn_id: ast::NodeId,
@@ -797,8 +801,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                     PatternKind::Binding { mutability, var, mode: BindingMode::ByValue, .. } => {
                         self.local_decls[local].mutability = mutability;
                         self.local_decls[local].is_user_variable =
-                            if let Some(ImplicitSelfBinding) = self_binding {
-                                Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf))
+                            if let Some(kind) = self_binding {
+                                Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf(*kind)))
                             } else {
                                 let binding_mode = ty::BindingMode::BindByValue(mutability.into());
                                 Some(ClearCrossCrate::Set(BindingForm::Var(VarBindingForm {

src/test/incremental/hashes/trait_defs.rs

@@ -270,7 +270,7 @@ trait TraitChangeModeSelfOwnToMut: Sized {
 #[rustc_clean(label="Hir", cfg="cfail2")]
 #[rustc_clean(label="Hir", cfg="cfail3")]
 trait TraitChangeModeSelfOwnToMut: Sized {
-    #[rustc_clean(label="Hir", cfg="cfail2")]
+    #[rustc_dirty(label="Hir", cfg="cfail2")]
     #[rustc_clean(label="Hir", cfg="cfail3")]
     #[rustc_dirty(label="HirBody", cfg="cfail2")]
     #[rustc_clean(label="HirBody", cfg="cfail3")]

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

@@ -2,15 +2,19 @@ error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
   --> $DIR/issue-31424.rs:17:9
    |
 LL |         (&mut self).bar(); //~ ERROR cannot borrow
-   |         ^^^^^^^^^^^ cannot borrow as mutable
+   |         ^^^^^^^^^^^
+   |         |
+   |         cannot borrow as mutable
+   |         try removing `&mut` here
 
 error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
   --> $DIR/issue-31424.rs:23:9
    |
-LL |     fn bar(self: &mut Self) {
-   |            ---- help: consider changing this to be mutable: `mut self`
 LL |         (&mut self).bar(); //~ ERROR cannot borrow
-   |         ^^^^^^^^^^^ cannot borrow as mutable
+   |         ^^^^^^^^^^^
+   |         |
+   |         cannot borrow as mutable
+   |         try removing `&mut` here
 
 error: aborting due to 2 previous errors
 

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

@@ -0,0 +1,42 @@
+// Copyright 2016 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)]
+
+struct Struct;
+
+impl Struct {
+    fn bar(self: &mut Self) {
+        (&mut self).bar();
+        //~^ ERROR cannot borrow `self` as mutable, as it is not declared as mutable [E0596]
+    }
+
+    fn imm(self) {
+        (&mut self).bar();
+        //~^ ERROR cannot borrow `self` as mutable, as it is not declared as mutable [E0596]
+    }
+
+    fn mtbl(mut self) {
+        (&mut self).bar();
+    }
+
+    fn immref(&self) {
+        (&mut self).bar();
+        //~^ ERROR cannot borrow `self` as mutable, as it is not declared as mutable [E0596]
+        //~^^ ERROR cannot borrow data in a `&` reference as mutable [E0596]
+    }
+
+    fn mtblref(&mut self) {
+        (&mut self).bar();
+        //~^ ERROR cannot borrow `self` as mutable, as it is not declared as mutable [E0596]
+    }
+}
+
+fn main () {}