2229: Fix issues with move closures and mutability

compiler/rustc_middle/src/ty/mod.rs

@@ -661,11 +661,28 @@ pub type RootVariableMinCaptureList<'tcx> = FxIndexMap<hir::HirId, MinCaptureLis
 /// Part of `MinCaptureInformationMap`; List of `CapturePlace`s.
 pub type MinCaptureList<'tcx> = Vec<CapturedPlace<'tcx>>;
 
-/// A `Place` and the corresponding `CaptureInfo`.
+/// A composite describing a `Place` that is captured by a closure.
 #[derive(PartialEq, Clone, Debug, TyEncodable, TyDecodable, TypeFoldable, HashStable)]
 pub struct CapturedPlace<'tcx> {
+    /// The `Place` that is captured.
     pub place: HirPlace<'tcx>,
+
+    /// `CaptureKind` and expression(s) that resulted in such capture of `place`.
     pub info: CaptureInfo<'tcx>,
+
+    /// Represents if `place` can be mutated or not.
+    pub mutability: hir::Mutability,
+}
+
+impl CapturedPlace<'tcx> {
+    /// Returns the hir-id of the root variable for the captured place.
+    /// e.g., if `a.b.c` was captured, would return the hir-id for `a`.
+    pub fn get_root_variable(&self) -> hir::HirId {
+        match self.place.base {
+            HirPlaceBase::Upvar(upvar_id) => upvar_id.var_path.hir_id,
+            base => bug!("Expected upvar, found={:?}", base),
+        }
+    }
 }
 
 pub fn place_to_string_for_capture(tcx: TyCtxt<'tcx>, place: &HirPlace<'tcx>) -> String {

compiler/rustc_mir/src/borrow_check/diagnostics/mod.rs

@@ -215,6 +215,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             PlaceRef { local, projection: [proj_base @ .., elem] } => {
                 match elem {
                     ProjectionElem::Deref => {
+                        // FIXME(project-rfc_2229#36): print capture precisely here.
                         let upvar_field_projection = self.is_upvar_field_projection(place);
                         if let Some(field) = upvar_field_projection {
                             let var_index = field.index();
@@ -259,6 +260,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                     ProjectionElem::Field(field, _ty) => {
                         autoderef = true;
 
+                        // FIXME(project-rfc_2229#36): print capture precisely here.
                         let upvar_field_projection = self.is_upvar_field_projection(place);
                         if let Some(field) = upvar_field_projection {
                             let var_index = field.index();

compiler/rustc_mir/src/borrow_check/diagnostics/move_errors.rs

@@ -345,7 +345,9 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                 };
 
                 let upvar = &self.upvars[upvar_field.unwrap().index()];
-                let upvar_hir_id = upvar.var_hir_id;
+                // FIXME(project-rfc-2229#8): Improve borrow-check diagnostics in case of precise
+                //                            capture.
+                let upvar_hir_id = upvar.place.get_root_variable();
                 let upvar_name = upvar.name;
                 let upvar_span = self.infcx.tcx.hir().span(upvar_hir_id);
 

compiler/rustc_mir/src/borrow_check/diagnostics/mutability_errors.rs

@@ -64,12 +64,29 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                     Place::ty_from(local, proj_base, self.body, self.infcx.tcx).ty
                 ));
 
-                item_msg = format!("`{}`", access_place_desc.unwrap());
-                if self.is_upvar_field_projection(access_place.as_ref()).is_some() {
-                    reason = ", as it is not declared as mutable".to_string();
+                let imm_borrow_derefed = self.upvars[upvar_index.index()]
+                    .place
+                    .place
+                    .deref_tys()
+                    .any(|ty| matches!(ty.kind(), ty::Ref(.., hir::Mutability::Not)));
+
+                // If the place is immutable then:
+                //
+                // - Either we deref a immutable ref to get to our final place.
+                //    - We don't capture derefs of raw ptrs
+                // - Or the final place is immut because the root variable of the capture
+                //   isn't marked mut and we should suggest that to the user.
+                if imm_borrow_derefed {
+                    // If we deref an immutable ref then the suggestion here doesn't help.
+                    return;
                 } else {
-                    let name = self.upvars[upvar_index.index()].name;
-                    reason = format!(", as `{}` is not declared as mutable", name);
+                    item_msg = format!("`{}`", access_place_desc.unwrap());
+                    if self.is_upvar_field_projection(access_place.as_ref()).is_some() {
+                        reason = ", as it is not declared as mutable".to_string();
+                    } else {
+                        let name = self.upvars[upvar_index.index()].name;
+                        reason = format!(", as `{}` is not declared as mutable", name);
+                    }
                 }
             }
 
@@ -259,9 +276,12 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                     Place::ty_from(local, proj_base, self.body, self.infcx.tcx).ty
                 ));
 
+                let captured_place = &self.upvars[upvar_index.index()].place;
+
                 err.span_label(span, format!("cannot {ACT}", ACT = act));
 
-                let upvar_hir_id = self.upvars[upvar_index.index()].var_hir_id;
+                let upvar_hir_id = captured_place.get_root_variable();
+
                 if let Some(Node::Binding(pat)) = self.infcx.tcx.hir().find(upvar_hir_id) {
                     if let hir::PatKind::Binding(
                         hir::BindingAnnotation::Unannotated,

compiler/rustc_mir/src/borrow_check/diagnostics/var_name.rs

@@ -12,7 +12,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         tcx: TyCtxt<'tcx>,
         body: &Body<'tcx>,
         local_names: &IndexVec<Local, Option<Symbol>>,
-        upvars: &[Upvar],
+        upvars: &[Upvar<'tcx>],
         fr: RegionVid,
     ) -> Option<(Option<Symbol>, Span)> {
         debug!("get_var_name_and_span_for_region(fr={:?})", fr);
@@ -21,6 +21,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         debug!("get_var_name_and_span_for_region: attempting upvar");
         self.get_upvar_index_for_region(tcx, fr)
             .map(|index| {
+                // FIXME(project-rfc-2229#8): Use place span for diagnostics
                 let (name, span) = self.get_upvar_name_and_span_for_region(tcx, upvars, index);
                 (Some(name), span)
             })
@@ -59,10 +60,10 @@ impl<'tcx> RegionInferenceContext<'tcx> {
     crate fn get_upvar_name_and_span_for_region(
         &self,
         tcx: TyCtxt<'tcx>,
-        upvars: &[Upvar],
+        upvars: &[Upvar<'tcx>],
         upvar_index: usize,
     ) -> (Symbol, Span) {
-        let upvar_hir_id = upvars[upvar_index].var_hir_id;
+        let upvar_hir_id = upvars[upvar_index].place.get_root_variable();
         debug!("get_upvar_name_and_span_for_region: upvar_hir_id={:?}", upvar_hir_id);
 
         let upvar_name = tcx.hir().name(upvar_hir_id);

compiler/rustc_mir/src/borrow_check/mod.rs

@@ -5,11 +5,10 @@ use rustc_data_structures::graph::dominators::Dominators;
 use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorReported};
 use rustc_hir as hir;
 use rustc_hir::def_id::LocalDefId;
-use rustc_hir::{HirId, Node};
+use rustc_hir::Node;
 use rustc_index::bit_set::BitSet;
 use rustc_index::vec::IndexVec;
 use rustc_infer::infer::{InferCtxt, TyCtxtInferExt};
-use rustc_middle::hir::place::PlaceBase as HirPlaceBase;
 use rustc_middle::mir::{
     traversal, Body, ClearCrossCrate, Local, Location, Mutability, Operand, Place, PlaceElem,
     PlaceRef, VarDebugInfoContents,
@@ -18,7 +17,7 @@ use rustc_middle::mir::{AggregateKind, BasicBlock, BorrowCheckResult, BorrowKind
 use rustc_middle::mir::{Field, ProjectionElem, Promoted, Rvalue, Statement, StatementKind};
 use rustc_middle::mir::{InlineAsmOperand, Terminator, TerminatorKind};
 use rustc_middle::ty::query::Providers;
-use rustc_middle::ty::{self, ParamEnv, RegionVid, TyCtxt};
+use rustc_middle::ty::{self, CapturedPlace, ParamEnv, RegionVid, TyCtxt};
 use rustc_session::lint::builtin::{MUTABLE_BORROW_RESERVATION_CONFLICT, UNUSED_MUT};
 use rustc_span::{Span, Symbol, DUMMY_SP};
 
@@ -73,16 +72,14 @@ crate use region_infer::RegionInferenceContext;
 
 // FIXME(eddyb) perhaps move this somewhere more centrally.
 #[derive(Debug)]
-crate struct Upvar {
+crate struct Upvar<'tcx> {
+    // FIXME(project-rfc_2229#36): print capture precisely here.
     name: Symbol,
 
-    // FIXME(project-rfc-2229#8): This should use Place or something similar
-    var_hir_id: HirId,
+    place: CapturedPlace<'tcx>,
 
     /// If true, the capture is behind a reference.
     by_ref: bool,
-
-    mutability: Mutability,
 }
 
 const DEREF_PROJECTION: &[PlaceElem<'_>; 1] = &[ProjectionElem::Deref];
@@ -161,26 +158,13 @@ fn do_mir_borrowck<'a, 'tcx>(
     let upvars: Vec<_> = tables
         .closure_min_captures_flattened(def.did.to_def_id())
         .map(|captured_place| {
-            let var_hir_id = match captured_place.place.base {
-                HirPlaceBase::Upvar(upvar_id) => upvar_id.var_path.hir_id,
-                _ => bug!("Expected upvar"),
-            };
+            let var_hir_id = captured_place.get_root_variable();
             let capture = captured_place.info.capture_kind;
             let by_ref = match capture {
                 ty::UpvarCapture::ByValue(_) => false,
                 ty::UpvarCapture::ByRef(..) => true,
             };
-            let mut upvar = Upvar {
-                name: tcx.hir().name(var_hir_id),
-                var_hir_id,
-                by_ref,
-                mutability: Mutability::Not,
-            };
-            let bm = *tables.pat_binding_modes().get(var_hir_id).expect("missing binding mode");
-            if bm == ty::BindByValue(hir::Mutability::Mut) {
-                upvar.mutability = Mutability::Mut;
-            }
-            upvar
+            Upvar { name: tcx.hir().name(var_hir_id), place: captured_place.clone(), by_ref }
         })
         .collect();
 
@@ -549,7 +533,7 @@ crate struct MirBorrowckCtxt<'cx, 'tcx> {
     dominators: Dominators<BasicBlock>,
 
     /// Information about upvars not necessarily preserved in types or MIR
-    upvars: Vec<Upvar>,
+    upvars: Vec<Upvar<'tcx>>,
 
     /// Names of local (user) variables (extracted from `var_debug_info`).
     local_names: IndexVec<Local, Option<Symbol>>,
@@ -1374,13 +1358,38 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
 
     fn propagate_closure_used_mut_upvar(&mut self, operand: &Operand<'tcx>) {
         let propagate_closure_used_mut_place = |this: &mut Self, place: Place<'tcx>| {
-            if !place.projection.is_empty() {
-                if let Some(field) = this.is_upvar_field_projection(place.as_ref()) {
+            // We have three possibilities here:
+            // a. We are modifying something through a mut-ref
+            // b. We are modifying something that is local to our parent
+            // c. Current body is a nested closure, and we are modifying path starting from
+            //    a Place captured by our parent closure.
+
+            // Handle (c), the path being modified is exactly the path captured by our parent
+            if let Some(field) = this.is_upvar_field_projection(place.as_ref()) {
+                this.used_mut_upvars.push(field);
+                return;
+            }
+
+            for (place_ref, proj) in place.iter_projections().rev() {
+                // Handle (a)
+                if proj == ProjectionElem::Deref {
+                    match place_ref.ty(this.body(), this.infcx.tcx).ty.kind() {
+                        // We aren't modifying a variable directly
+                        ty::Ref(_, _, hir::Mutability::Mut) => return,
+
+                        _ => {}
+                    }
+                }
+
+                // Handle (c)
+                if let Some(field) = this.is_upvar_field_projection(place_ref) {
                     this.used_mut_upvars.push(field);
+                    return;
                 }
-            } else {
-                this.used_mut.insert(place.local);
             }
+
+            // Handle(b)
+            this.used_mut.insert(place.local);
         };
 
         // This relies on the current way that by-value
@@ -2146,6 +2155,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         place: PlaceRef<'tcx>,
         is_local_mutation_allowed: LocalMutationIsAllowed,
     ) -> Result<RootPlace<'tcx>, PlaceRef<'tcx>> {
+        debug!("is_mutable: place={:?}, is_local...={:?}", place, is_local_mutation_allowed);
         match place.last_projection() {
             None => {
                 let local = &self.body.local_decls[place.local];
@@ -2227,11 +2237,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                         if let Some(field) = upvar_field_projection {
                             let upvar = &self.upvars[field.index()];
                             debug!(
-                                "upvar.mutability={:?} local_mutation_is_allowed={:?} \
-                                 place={:?}",
-                                upvar, is_local_mutation_allowed, place
+                                "is_mutable: upvar.mutability={:?} local_mutation_is_allowed={:?} \
+                                 place={:?}, place_base={:?}",
+                                upvar, is_local_mutation_allowed, place, place_base
                             );
-                            match (upvar.mutability, is_local_mutation_allowed) {
+                            match (upvar.place.mutability, is_local_mutation_allowed) {
                                 (
                                     Mutability::Not,
                                     LocalMutationIsAllowed::No

compiler/rustc_mir/src/borrow_check/nll.rs

@@ -165,7 +165,7 @@ pub(in crate::borrow_check) fn compute_regions<'cx, 'tcx>(
     flow_inits: &mut ResultsCursor<'cx, 'tcx, MaybeInitializedPlaces<'cx, 'tcx>>,
     move_data: &MoveData<'tcx>,
     borrow_set: &BorrowSet<'tcx>,
-    upvars: &[Upvar],
+    upvars: &[Upvar<'tcx>],
 ) -> NllOutput<'tcx> {
     let mut all_facts = AllFacts::enabled(infcx.tcx).then_some(AllFacts::default());
 

compiler/rustc_mir/src/borrow_check/path_utils.rs

@@ -143,7 +143,7 @@ pub(super) fn borrow_of_local_data(place: Place<'_>) -> bool {
 /// of a closure type.
 pub(crate) fn is_upvar_field_projection(
     tcx: TyCtxt<'tcx>,
-    upvars: &[Upvar],
+    upvars: &[Upvar<'tcx>],
     place_ref: PlaceRef<'tcx>,
     body: &Body<'tcx>,
 ) -> Option<Field> {

compiler/rustc_mir/src/borrow_check/type_check/mod.rs

@@ -132,7 +132,7 @@ pub(crate) fn type_check<'mir, 'tcx>(
     flow_inits: &mut ResultsCursor<'mir, 'tcx, MaybeInitializedPlaces<'mir, 'tcx>>,
     move_data: &MoveData<'tcx>,
     elements: &Rc<RegionValueElements>,
-    upvars: &[Upvar],
+    upvars: &[Upvar<'tcx>],
 ) -> MirTypeckResults<'tcx> {
     let implicit_region_bound = infcx.tcx.mk_region(ty::ReVar(universal_regions.fr_fn_body));
     let mut constraints = MirTypeckRegionConstraints {
@@ -821,7 +821,7 @@ struct BorrowCheckContext<'a, 'tcx> {
     all_facts: &'a mut Option<AllFacts>,
     borrow_set: &'a BorrowSet<'tcx>,
     constraints: &'a mut MirTypeckRegionConstraints<'tcx>,
-    upvars: &'a [Upvar],
+    upvars: &'a [Upvar<'tcx>],
 }
 
 crate struct MirTypeckResults<'tcx> {
@@ -2490,7 +2490,9 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
             body,
         );
         let category = if let Some(field) = field {
-            ConstraintCategory::ClosureUpvar(self.borrowck_context.upvars[field.index()].var_hir_id)
+            let var_hir_id = self.borrowck_context.upvars[field.index()].place.get_root_variable();
+            // FIXME(project-rfc-2229#8): Use Place for better diagnostics
+            ConstraintCategory::ClosureUpvar(var_hir_id)
         } else {
             ConstraintCategory::Boring
         };

compiler/rustc_mir_build/src/build/mod.rs

@@ -851,22 +851,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         _ => bug!("Expected an upvar")
                     };
 
-                    let mut mutability = Mutability::Not;
+                    let mutability = captured_place.mutability;
 
                     // FIXME(project-rfc-2229#8): Store more precise information
                     let mut name = kw::Empty;
                     if let Some(Node::Binding(pat)) = tcx_hir.find(var_id) {
                         if let hir::PatKind::Binding(_, _, ident, _) = pat.kind {
                             name = ident.name;
-                            match hir_typeck_results
-                                .extract_binding_mode(tcx.sess, pat.hir_id, pat.span)
-                            {
-                                Some(ty::BindByValue(hir::Mutability::Mut)) => {
-                                    mutability = Mutability::Mut;
-                                }
-                                Some(_) => mutability = Mutability::Not,
-                                _ => {}
-                            }
                         }
                     }