Improve implicit self mutability suggestions.

davidtwco · davidtwco · commit 43b5d725d0b4 · 2018-10-02T00:35:15.000+02:00
This commit adds an `ImplicitSelfKind` to the HIR and the MIR that keeps
track of whether a implicit self argument is immutable by-value, mutable
by-value, immutable reference or mutable reference so that the addition
of the `mut` keyword can be suggested for the immutable by-value case.
diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs
@@ -2011,11 +2011,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,
+                    }
+                },
+            ),
         })
     }
 
diff --git a/src/librustc/hir/mod.rs b/src/librustc/hir/mod.rs
@@ -1769,9 +1769,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?
diff --git a/src/librustc/ich/impls_hir.rs b/src/librustc/ich/impls_hir.rs
@@ -349,14 +349,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 -> _,
diff --git a/src/librustc/mir/mod.rs b/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,
         }
diff --git a/src/librustc_borrowck/borrowck/mod.rs b/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)
             }
diff --git a/src/librustc_mir/borrow_check/mutability_errors.rs b/src/librustc_mir/borrow_check/mutability_errors.rs
@@ -316,7 +316,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))
                     }
 
diff --git a/src/librustc_mir/build/mod.rs b/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 {
diff --git a/src/test/incremental/hashes/trait_defs.rs b/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")]
diff --git a/src/test/ui/nll/issue-51191.rs b/src/test/ui/nll/issue-51191.rs
@@ -0,0 +1,37 @@
+// 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
+    }
+
+    fn imm(self) {
+        (&mut self).bar(); //~ ERROR cannot borrow
+    }
+
+    fn mtbl(mut self) {
+        (&mut self).bar();
+    }
+
+    fn immref(&self) {
+        (&mut self).bar(); //~ ERROR cannot borrow
+    }
+
+    fn mtblref(&mut self) {
+        (&mut self).bar();
+    }
+}
+
+fn main () {}
diff --git a/src/test/ui/nll/issue-51191.stderr b/src/test/ui/nll/issue-51191.stderr
@@ -0,0 +1,37 @@
+error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
+  --> $DIR/issue-51191.rs:17: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
+
+error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
+  --> $DIR/issue-51191.rs:21:9
+   |
+LL |     fn imm(self) {
+   |            ---- help: consider changing this to be mutable: `mut self`
+LL |         (&mut self).bar(); //~ ERROR cannot borrow
+   |         ^^^^^^^^^^^ cannot borrow as mutable
+
+error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
+  --> $DIR/issue-51191.rs:29:9
+   |
+LL |         (&mut self).bar(); //~ ERROR cannot borrow
+   |         ^^^^^^^^^^^ cannot borrow as mutable
+
+error[E0596]: cannot borrow data in a `&` reference as mutable
+  --> $DIR/issue-51191.rs:29:9
+   |
+LL |         (&mut self).bar(); //~ ERROR cannot borrow
+   |         ^^^^^^^^^^^ cannot borrow as mutable
+
+error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
+  --> $DIR/issue-51191.rs:33:9
+   |
+LL |         (&mut self).bar();
+   |         ^^^^^^^^^^^ cannot borrow as mutable
+
+error: aborting due to 5 previous errors
+
+For more information about this error, try `rustc --explain E0596`.

Original file line number	Diff line number	Diff line change
`@@ -1221,7 +1221,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {`
`1221`	`1221`	`if let Some(i) = arg_pos {`
`1222`	`1222`	// The argument's `Ty`
`1223`	`1223`	`(Some(&fn_like.decl().inputs[i]),`
`1224`		`- i == 0 && fn_like.decl().has_implicit_self)`
	`1224`	`+ i == 0 && fn_like.decl().implicit_self.has_implicit_self())`
`1225`	`1225`	`} else {`
`1226`	`1226`	`(None, false)`
`1227`	`1227`	`}`
Original file line number	Diff line number	Diff line change
`@@ -316,7 +316,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {`
`316`	`316`	`{`
`317`	`317`	`let local_decl = &self.mir.local_decls[*local];`
`318`	`318`	`let suggestion = match local_decl.is_user_variable.as_ref().unwrap() {`
`319`		`- ClearCrossCrate::Set(mir::BindingForm::ImplicitSelf) => {`
	`319`	`+ ClearCrossCrate::Set(mir::BindingForm::ImplicitSelf(_)) => {`
`320`	`320`	`Some(suggest_ampmut_self(self.infcx.tcx, local_decl))`
`321`	`321`	`}`
`322`	`322`