improve diagnostics so that the message respect the locals storing upvars

... and treat them as such

Author: dingxiangfei2009

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

@@ -393,49 +393,18 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
                     Place::ty_from(local, proj_base, self.body, self.infcx.tcx).ty
                 ));
 
-                let captured_place = self.upvars[upvar_index.index()];
-
-                err.span_label(span, format!("cannot {act}"));
-
-                let upvar_hir_id = captured_place.get_root_variable();
-
-                if let Node::Pat(pat) = self.infcx.tcx.hir_node(upvar_hir_id)
-                    && let hir::PatKind::Binding(hir::BindingMode::NONE, _, upvar_ident, _) =
-                        pat.kind
-                {
-                    if upvar_ident.name == kw::SelfLower {
-                        for (_, node) in self.infcx.tcx.hir_parent_iter(upvar_hir_id) {
-                            if let Some(fn_decl) = node.fn_decl() {
-                                if !matches!(
-                                    fn_decl.implicit_self,
-                                    hir::ImplicitSelfKind::RefImm | hir::ImplicitSelfKind::RefMut
-                                ) {
-                                    err.span_suggestion_verbose(
-                                        upvar_ident.span.shrink_to_lo(),
-                                        "consider changing this to be mutable",
-                                        "mut ",
-                                        Applicability::MachineApplicable,
-                                    );
-                                    break;
-                                }
-                            }
-                        }
-                    } else {
-                        err.span_suggestion_verbose(
-                            upvar_ident.span.shrink_to_lo(),
-                            "consider changing this to be mutable",
-                            "mut ",
-                            Applicability::MachineApplicable,
-                        );
-                    }
-                }
+                self.suggest_mutable_upvar(*upvar_index, the_place_err, &mut err, span, act);
+            }
 
-                let tcx = self.infcx.tcx;
-                if let ty::Ref(_, ty, Mutability::Mut) = the_place_err.ty(self.body, tcx).ty.kind()
-                    && let ty::Closure(id, _) = *ty.kind()
-                {
-                    self.show_mutating_upvar(tcx, id.expect_local(), the_place_err, &mut err);
-                }
+            PlaceRef { local, projection: [] }
+                if let Some(upvar_index) = self
+                    .body
+                    .local_upvar_map
+                    .iter_enumerated()
+                    .filter_map(|(field, &local)| local.map(|local| (field, local)))
+                    .find_map(|(field, relocated)| (relocated == local).then_some(field)) =>
+            {
+                self.suggest_mutable_upvar(upvar_index, the_place_err, &mut err, span, act);
             }
 
             // complete hack to approximate old AST-borrowck
@@ -542,6 +511,58 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
         }
     }
 
+    fn suggest_mutable_upvar(
+        &self,
+        upvar_index: FieldIdx,
+        the_place_err: PlaceRef<'tcx>,
+        err: &mut Diag<'infcx>,
+        span: Span,
+        act: &str,
+    ) {
+        let captured_place = self.upvars[upvar_index.index()];
+
+        err.span_label(span, format!("cannot {act}"));
+
+        let upvar_hir_id = captured_place.get_root_variable();
+
+        if let Node::Pat(pat) = self.infcx.tcx.hir_node(upvar_hir_id)
+            && let hir::PatKind::Binding(hir::BindingMode::NONE, _, upvar_ident, _) = pat.kind
+        {
+            if upvar_ident.name == kw::SelfLower {
+                for (_, node) in self.infcx.tcx.hir_parent_iter(upvar_hir_id) {
+                    if let Some(fn_decl) = node.fn_decl() {
+                        if !matches!(
+                            fn_decl.implicit_self,
+                            hir::ImplicitSelfKind::RefImm | hir::ImplicitSelfKind::RefMut
+                        ) {
+                            err.span_suggestion_verbose(
+                                upvar_ident.span.shrink_to_lo(),
+                                "consider changing this to be mutable",
+                                "mut ",
+                                Applicability::MachineApplicable,
+                            );
+                            break;
+                        }
+                    }
+                }
+            } else {
+                err.span_suggestion_verbose(
+                    upvar_ident.span.shrink_to_lo(),
+                    "consider changing this to be mutable",
+                    "mut ",
+                    Applicability::MachineApplicable,
+                );
+            }
+        }
+
+        let tcx = self.infcx.tcx;
+        if let ty::Ref(_, ty, Mutability::Mut) = the_place_err.ty(self.body, tcx).ty.kind()
+            && let ty::Closure(id, _) = *ty.kind()
+        {
+            self.show_mutating_upvar(tcx, id.expect_local(), the_place_err, err);
+        }
+    }
+
     /// Suggest `map[k] = v` => `map.insert(k, v)` and the like.
     fn suggest_map_index_mut_alternatives(&self, ty: Ty<'tcx>, err: &mut Diag<'infcx>, span: Span) {
         let Some(adt) = ty.ty_adt_def() else { return };

compiler/rustc_borrowck/src/lib.rs

@@ -24,6 +24,7 @@ use std::ops::{ControlFlow, Deref};
 use rustc_abi::FieldIdx;
 use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
 use rustc_data_structures::graph::dominators::Dominators;
+use rustc_data_structures::unord::UnordMap;
 use rustc_errors::LintDiagnostic;
 use rustc_hir as hir;
 use rustc_hir::CRATE_HIR_ID;
@@ -262,6 +263,7 @@ fn do_mir_borrowck<'tcx>(
             regioncx: &regioncx,
             used_mut: Default::default(),
             used_mut_upvars: SmallVec::new(),
+            local_from_upvars: UnordMap::default(),
             borrow_set: &borrow_set,
             upvars: &[],
             local_names: IndexVec::from_elem(None, &promoted_body.local_decls),
@@ -287,6 +289,11 @@ fn do_mir_borrowck<'tcx>(
         promoted_mbcx.report_move_errors();
     }
 
+    let mut local_from_upvars = UnordMap::default();
+    for (field, &local) in body.local_upvar_map.iter_enumerated() {
+        let Some(local) = local else { continue };
+        local_from_upvars.insert(local, field);
+    }
     let mut mbcx = MirBorrowckCtxt {
         infcx: &infcx,
         body,
@@ -301,6 +308,7 @@ fn do_mir_borrowck<'tcx>(
         regioncx: &regioncx,
         used_mut: Default::default(),
         used_mut_upvars: SmallVec::new(),
+        local_from_upvars,
         borrow_set: &borrow_set,
         upvars: tcx.closure_captures(def),
         local_names,
@@ -556,6 +564,9 @@ struct MirBorrowckCtxt<'a, 'infcx, 'tcx> {
     /// If the function we're checking is a closure, then we'll need to report back the list of
     /// mutable upvars that have been used. This field keeps track of them.
     used_mut_upvars: SmallVec<[FieldIdx; 8]>,
+    /// Since upvars are moved to real locals, we need to map mutations to the locals back to
+    /// the upvars, so that used_mut_upvars is up-to-date.
+    local_from_upvars: UnordMap<Local, FieldIdx>,
     /// Region inference context. This contains the results from region inference and lets us e.g.
     /// find out which CFG points are contained in each borrow region.
     regioncx: &'a RegionInferenceContext<'tcx>,
@@ -2266,7 +2277,9 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
 
         // at this point, we have set up the error reporting state.
         if let Some(init_index) = previously_initialized {
-            if let (AccessKind::Mutate, Some(_)) = (error_access, place.as_local()) {
+            if let (AccessKind::Mutate, Some(local)) = (error_access, place.as_local())
+                && self.body.local_upvar_map.iter().flatten().all(|upvar| upvar != &local)
+            {
                 // If this is a mutate access to an immutable local variable with no projections
                 // report the error as an illegal reassignment
                 let init = &self.move_data.inits[init_index];
@@ -2294,10 +2307,12 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
                 // If the local may have been initialized, and it is now currently being
                 // mutated, then it is justified to be annotated with the `mut`
                 // keyword, since the mutation may be a possible reassignment.
-                if is_local_mutation_allowed != LocalMutationIsAllowed::Yes
-                    && self.is_local_ever_initialized(local, state).is_some()
-                {
-                    self.used_mut.insert(local);
+                if !matches!(is_local_mutation_allowed, LocalMutationIsAllowed::Yes) {
+                    if self.is_local_ever_initialized(local, state).is_some() {
+                        self.used_mut.insert(local);
+                    } else if let Some(&field) = self.local_from_upvars.get(&local) {
+                        self.used_mut_upvars.push(field);
+                    }
                 }
             }
             RootPlace {
@@ -2315,6 +2330,8 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
                     projection: place_projection,
                 }) {
                     self.used_mut_upvars.push(field);
+                } else if let Some(&field) = self.local_from_upvars.get(&place_local) {
+                    self.used_mut_upvars.push(field);
                 }
             }
         }

compiler/rustc_mir_transform/src/deref_separator.rs

@@ -40,6 +40,7 @@ impl<'a, 'tcx> MutVisitor<'tcx> for DerefChecker<'a, 'tcx> {
                         ty,
                         self.local_decls[p_ref.local].source_info.span,
                         LocalInfo::DerefTemp,
+                        false,
                     );
 
                     // We are adding current p_ref's projections to our

compiler/rustc_mir_transform/src/pass_manager.rs

@@ -294,11 +294,20 @@ fn run_passes_inner<'tcx>(
             if dump_enabled {
                 dump_mir_for_pass(tcx, body, name, true);
             }
+            let (dialect, phase_num) = body.phase.index();
             if validate {
-                validate_body(tcx, body, format!("after pass {name}"));
+                validate_body(
+                    tcx,
+                    body,
+                    format!("after pass {name} {dialect}-{phase_num}-{:03}", body.pass_count),
+                );
             }
             if lint {
-                lint_body(tcx, body, format!("after pass {name}"));
+                lint_body(
+                    tcx,
+                    body,
+                    format!("after pass {name} {dialect}-{phase_num}-{:03}", body.pass_count),
+                );
             }
 
             body.pass_count += 1;

compiler/rustc_mir_transform/src/patch.rs

@@ -154,10 +154,14 @@ impl<'tcx> MirPatch<'tcx> {
         ty: Ty<'tcx>,
         span: Span,
         local_info: LocalInfo<'tcx>,
+        immutable: bool,
     ) -> Local {
         let index = self.next_local;
         self.next_local += 1;
         let mut new_decl = LocalDecl::new(ty, span);
+        if immutable {
+            new_decl = new_decl.immutable();
+        }
         **new_decl.local_info.as_mut().unwrap_crate_local() = local_info;
         self.new_locals.push(new_decl);
         Local::new(index)

compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs

@@ -5,7 +5,7 @@ use std::borrow::Cow;
 use std::iter;
 
 use itertools::{EitherOrBoth, Itertools};
-use rustc_abi::ExternAbi;
+use rustc_abi::{ExternAbi, FIRST_VARIANT};
 use rustc_data_structures::fx::FxHashSet;
 use rustc_data_structures::stack::ensure_sufficient_stack;
 use rustc_errors::codes::*;
@@ -2360,18 +2360,28 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
             && let Some(coroutine_info) = self.tcx.mir_coroutine_witnesses(coroutine_did)
         {
             debug!(?coroutine_info);
-            'find_source: for (variant, source_info) in
-                coroutine_info.variant_fields.iter().zip(&coroutine_info.variant_source_info)
+            // Variants are scanned "in reverse" because suspension points
+            // tend to contain more diagnostic information than the unresumed state.
+            'find_source: for ((variant_idx, variant), source_info) in coroutine_info
+                .variant_fields
+                .iter_enumerated()
+                .zip(&coroutine_info.variant_source_info)
+                .rev()
             {
                 debug!(?variant);
                 for &local in variant {
                     let decl = &coroutine_info.field_tys[local];
                     debug!(?decl);
                     if ty_matches(ty::Binder::dummy(decl.ty)) && !decl.ignore_for_traits {
-                        interior_or_upvar_span = Some(CoroutineInteriorOrUpvar::Interior(
-                            decl.source_info.span,
-                            Some((source_info.span, from_awaited_ty)),
-                        ));
+                        let span = decl.source_info.span;
+                        if variant_idx == FIRST_VARIANT {
+                            interior_or_upvar_span = Some(CoroutineInteriorOrUpvar::Upvar(span));
+                        } else {
+                            interior_or_upvar_span = Some(CoroutineInteriorOrUpvar::Interior(
+                                span,
+                                Some((source_info.span, from_awaited_ty)),
+                            ));
+                        }
                         break 'find_source;
                     }
                 }