NLL diagnostics: revise fn check_access_permissions

src/librustc/hir/map/mod.rs

@@ -170,6 +170,40 @@ impl<'hir> MapEntry<'hir> {
         })
     }
 
+    fn fn_decl(&self) -> Option<&FnDecl> {
+        match self {
+            EntryItem(_, _, ref item) => {
+                match item.node {
+                    ItemFn(ref fn_decl, _, _, _, _, _) => Some(&fn_decl),
+                    _ => None,
+                }
+            }
+
+            EntryTraitItem(_, _, ref item) => {
+                match item.node {
+                    TraitItemKind::Method(ref method_sig, _) => Some(&method_sig.decl),
+                    _ => None
+                }
+            }
+
+            EntryImplItem(_, _, ref item) => {
+                match item.node {
+                    ImplItemKind::Method(ref method_sig, _) => Some(&method_sig.decl),
+                    _ => None,
+                }
+            }
+
+            EntryExpr(_, _, ref expr) => {
+                match expr.node {
+                    ExprClosure(_, ref fn_decl, ..) => Some(&fn_decl),
+                    _ => None,
+                }
+            }
+
+            _ => None
+        }
+    }
+
     fn associated_body(self) -> Option<BodyId> {
         match self {
             EntryItem(_, _, item) => {
@@ -502,6 +536,14 @@ impl<'hir> Map<'hir> {
         self.forest.krate.body(id)
     }
 
+    pub fn fn_decl(&self, node_id: ast::NodeId) -> Option<FnDecl> {
+        if let Some(entry) = self.find_entry(node_id) {
+            entry.fn_decl().map(|fd| fd.clone())
+        } else {
+            bug!("no entry for node_id `{}`", node_id)
+        }
+    }
+
     /// Returns the `NodeId` that corresponds to the definition of
     /// which this is the body of, i.e. a `fn`, `const` or `static`
     /// item (possibly associated), a closure, or a `hir::AnonConst`.

src/librustc/mir/mod.rs

@@ -228,7 +228,7 @@ impl<'tcx> Mir<'tcx> {
     pub fn temps_iter<'a>(&'a self) -> impl Iterator<Item=Local> + 'a {
         (self.arg_count+1..self.local_decls.len()).filter_map(move |index| {
             let local = Local::new(index);
-            if self.local_decls[local].is_user_variable {
+            if self.local_decls[local].is_user_variable.is_some() {
                 None
             } else {
                 Some(local)
@@ -241,7 +241,7 @@ impl<'tcx> Mir<'tcx> {
     pub fn vars_iter<'a>(&'a self) -> impl Iterator<Item=Local> + 'a {
         (self.arg_count+1..self.local_decls.len()).filter_map(move |index| {
             let local = Local::new(index);
-            if self.local_decls[local].is_user_variable {
+            if self.local_decls[local].is_user_variable.is_some() {
                 Some(local)
             } else {
                 None
@@ -255,7 +255,7 @@ impl<'tcx> Mir<'tcx> {
         (1..self.local_decls.len()).filter_map(move |index| {
             let local = Local::new(index);
             let decl = &self.local_decls[local];
-            if (decl.is_user_variable || index < self.arg_count + 1)
+            if (decl.is_user_variable.is_some() || index < self.arg_count + 1)
                && decl.mutability == Mutability::Mut
             {
                 Some(local)
@@ -351,7 +351,7 @@ impl<'tcx> IndexMut<BasicBlock> for Mir<'tcx> {
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 pub enum ClearCrossCrate<T> {
     Clear,
     Set(T)
@@ -382,6 +382,16 @@ pub enum Mutability {
     Not,
 }
 
+impl From<Mutability> for hir::Mutability {
+    fn from(m: Mutability) -> Self {
+        match m {
+            Mutability::Mut => hir::MutMutable,
+            Mutability::Not => hir::MutImmutable,
+        }
+    }
+}
+
+
 #[derive(Copy, Clone, Debug, PartialEq, Eq, RustcEncodable, RustcDecodable)]
 pub enum BorrowKind {
     /// Data must be immutable and is aliasable.
@@ -463,6 +473,33 @@ pub enum LocalKind {
     ReturnPointer,
 }
 
+#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)]
+pub struct VarBindingForm {
+    /// Is variable bound via `x`, `mut x`, `ref x`, or `ref mut x`?
+    pub binding_mode: ty::BindingMode,
+    /// If an explicit type was provided for this variable binding,
+    /// this holds the source Span of that type.
+    ///
+    /// NOTE: If you want to change this to a `HirId`, be wary that
+    /// doing so breaks incremental compilation (as of this writing),
+    /// while a `Span` does not cause our tests to fail.
+    pub opt_ty_info: Option<Span>,
+}
+
+#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)]
+pub enum BindingForm {
+    /// This is a binding for a non-`self` binding, or a `self` that has an explicit type.
+    Var(VarBindingForm),
+    /// Binding for a `self`/`&self`/`&mut self` binding where the type is implicit.
+    ImplicitSelf,
+}
+
+CloneTypeFoldableAndLiftImpls! { BindingForm, }
+
+impl_stable_hash_for!(struct self::VarBindingForm { binding_mode, opt_ty_info });
+
+impl_stable_hash_for!(enum self::BindingForm { Var(binding), ImplicitSelf, });
+
 /// A MIR local.
 ///
 /// This can be a binding declared by the user, a temporary inserted by the compiler, a function
@@ -474,8 +511,14 @@ pub struct LocalDecl<'tcx> {
     /// Temporaries and the return place are always mutable.
     pub mutability: Mutability,
 
-    /// True if this corresponds to a user-declared local variable.
-    pub is_user_variable: bool,
+    /// Some(binding_mode) if this corresponds to a user-declared local variable.
+    ///
+    /// This is solely used for local diagnostics when generating
+    /// warnings/errors when compiling the current crate, and
+    /// therefore it need not be visible across crates. pnkfelix
+    /// currently hypothesized we *need* to wrap this in a
+    /// `ClearCrossCrate` as long as it carries as `HirId`.
+    pub is_user_variable: Option<ClearCrossCrate<BindingForm>>,
 
     /// True if this is an internal local
     ///
@@ -592,6 +635,45 @@ pub struct LocalDecl<'tcx> {
 }
 
 impl<'tcx> LocalDecl<'tcx> {
+    /// Returns true only if local is a binding that can itself be
+    /// made mutable via the addition of the `mut` keyword, namely
+    /// something like the occurrences of `x` in:
+    /// - `fn foo(x: Type) { ... }`,
+    /// - `let x = ...`,
+    /// - or `match ... { C(x) => ... }`
+    pub fn can_be_made_mutable(&self) -> bool
+    {
+        match self.is_user_variable {
+            Some(ClearCrossCrate::Set(BindingForm::Var(VarBindingForm {
+                binding_mode: ty::BindingMode::BindByValue(_),
+                opt_ty_info: _,
+            }))) => 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).
+            _ => false,
+        }
+    }
+
+    /// Returns true if local is definitely not a `ref ident` or
+    /// `ref mut ident` binding. (Such bindings cannot be made into
+    /// mutable bindings, but the inverse does not necessarily hold).
+    pub fn is_nonref_binding(&self) -> bool
+    {
+        match self.is_user_variable {
+            Some(ClearCrossCrate::Set(BindingForm::Var(VarBindingForm {
+                binding_mode: ty::BindingMode::BindByValue(_),
+                opt_ty_info: _,
+            }))) => true,
+
+            Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf)) => true,
+
+            _ => false,
+        }
+    }
+
     /// Create a new `LocalDecl` for a temporary.
     #[inline]
     pub fn new_temp(ty: Ty<'tcx>, span: Span) -> Self {
@@ -605,7 +687,7 @@ impl<'tcx> LocalDecl<'tcx> {
             },
             visibility_scope: OUTERMOST_SOURCE_SCOPE,
             internal: false,
-            is_user_variable: false
+            is_user_variable: None,
         }
     }
 
@@ -622,7 +704,7 @@ impl<'tcx> LocalDecl<'tcx> {
             },
             visibility_scope: OUTERMOST_SOURCE_SCOPE,
             internal: true,
-            is_user_variable: false
+            is_user_variable: None,
         }
     }
 
@@ -641,7 +723,7 @@ impl<'tcx> LocalDecl<'tcx> {
             visibility_scope: OUTERMOST_SOURCE_SCOPE,
             internal: false,
             name: None,     // FIXME maybe we do want some name here?
-            is_user_variable: false
+            is_user_variable: None,
         }
     }
 }

src/librustc/ty/binding.rs

@@ -18,6 +18,8 @@ pub enum BindingMode {
     BindByValue(Mutability),
 }
 
+CloneTypeFoldableAndLiftImpls! { BindingMode, }
+
 impl BindingMode {
     pub fn convert(ba: BindingAnnotation) -> BindingMode {
         match ba {

src/librustc_mir/borrow_check/error_reporting.rs

@@ -593,11 +593,18 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         err.emit();
     }
 
+    /// Reports an illegal reassignment; for example, an assignment to
+    /// (part of) a non-`mut` local that occurs potentially after that
+    /// local has already been initialized. `place` is the path being
+    /// assigned; `err_place` is a place providing a reason why
+    /// `place` is not mutable (e.g. the non-`mut` local `x` in an
+    /// assignment to `x.f`).
     pub(super) fn report_illegal_reassignment(
         &mut self,
         _context: Context,
         (place, span): (&Place<'tcx>, Span),
         assigned_span: Span,
+        err_place: &Place<'tcx>,
     ) {
         let is_arg = if let Place::Local(local) = place {
             if let LocalKind::Arg = self.mir.local_kind(*local) {
@@ -621,16 +628,23 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             "cannot assign twice to immutable variable"
         };
         if span != assigned_span {
-            if is_arg {
-                err.span_label(assigned_span, "argument not declared as `mut`");
-            } else {
+            if !is_arg {
                 let value_msg = match self.describe_place(place) {
                     Some(name) => format!("`{}`", name),
                     None => "value".to_owned(),
                 };
                 err.span_label(assigned_span, format!("first assignment to {}", value_msg));
             }
         }
+        if let Place::Local(local) = err_place {
+            let local_decl = &self.mir.local_decls[*local];
+            if let Some(name) = local_decl.name {
+                if local_decl.can_be_made_mutable() {
+                    err.span_label(local_decl.source_info.span,
+                                   format!("consider changing this to `mut {}`", name));
+                }
+            }
+        }
         err.span_label(span, msg);
         err.emit();
     }