Rollup merge of rust-lang#80092 - sexxi-goose:restrict_precision, r=nikomatsakis

2229: Fix issues with move closures and mutability

This PR fixes two issues when feature `capture_disjoint_fields` is used.

1. Can't mutate using a mutable reference
2. Move closures try to move value out through a reference.

To do so, we
1. Compute the mutability of the capture and store it as part of the `CapturedPlace`  that is written in TypeckResults
2. Restrict capture precision. Note this is temporary for now, to allow the feature to be used with move closures and ByValue captures and might change depending on discussions with the lang team.
    - No Derefs are captured for ByValue captures, since that will result in value behind a reference getting moved.
    - No projections are applied to raw pointers since these require unsafe blocks. We capture
    them completely.

r? `@nikomatsakis`

Author: m-ou-se

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,
-                                _ => {}
-                            }
                         }
                     }