Auto merge of #51275 - pnkfelix:nll-diagnostics-revise-check-access-p…

…ermissions, r=nikomatsakis

NLL diagnostics: revise `fn check_access_permissions`

NLL: revise `fn check_access_permissions` so that its (still branchy) shares more code paths between the different cases, and also provide more diagnostics in more cases (though the added diagnostics still do not always meet the quality bar established by AST-borrowck)

----

Transcribing "checklist" suggested by Niko, except I am rendering it as a table to make it clear that I do not regard every item in the list to be a "must have" for landing this PR.

goal | does this PR do it?
-----|------------------------------
no suggestions for `ref mut` |  yes
suggestions for direct local assignment (`{ let x = 3; x = 4; }`) | yes (see commits at end)
suggestions for direct field assignment (`{ let x = (3, 4); x.0 = 5; }` | yes (see commits at end)
suggestions for upvars (`let x = 3; let c = \|\| { &mut x; }`) | yes

Note that I added support for a couple of rows via changes that are not strictly part of `fn check_access_permissions`. If desired I can remove those commits from this PR and leave them for a later PR.

Fix #51031
Fix #51032
(bug #51191 needs a little more investigation before closing.)
Fix #51578

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();
     }