Auto merge of #82536 - sexxi-goose:handle-patterns-take-2, r=nikomatsakis

2229: Handle patterns within closures correctly when `capture_disjoint_fields` is enabled

This PR fixes several issues related to handling patterns within closures when `capture_disjoint_fields` is enabled.
1. Matching is always considered a use of the place, even with `_` patterns
2. Compiler ICE when capturing fields in closures through `let` assignments

To do so, we

- Introduced new Fake Reads
- Delayed use of `Place` in favor of `PlaceBuilder`
- Ensured that `PlaceBuilder` can be resolved before attempting to extract `Place` in any of the pattern matching code

Closes rust-lang/project-rfc-2229/issues/27
Closes rust-lang/project-rfc-2229/issues/24
r? `@nikomatsakis`

Author: bors

compiler/rustc_middle/src/ty/context.rs

@@ -47,6 +47,7 @@ use rustc_hir::{
 };
 use rustc_index::vec::{Idx, IndexVec};
 use rustc_macros::HashStable;
+use rustc_middle::mir::FakeReadCause;
 use rustc_serialize::opaque::{FileEncodeResult, FileEncoder};
 use rustc_session::config::{BorrowckMode, CrateType, OutputFilenames};
 use rustc_session::lint::{Level, Lint};
@@ -430,6 +431,30 @@ pub struct TypeckResults<'tcx> {
     /// see `MinCaptureInformationMap` for more details.
     pub closure_min_captures: ty::MinCaptureInformationMap<'tcx>,
 
+    /// Tracks the fake reads required for a closure and the reason for the fake read.
+    /// When performing pattern matching for closures, there are times we don't end up
+    /// reading places that are mentioned in a closure (because of _ patterns). However,
+    /// to ensure the places are initialized, we introduce fake reads.
+    /// Consider these two examples:
+    /// ``` (discriminant matching with only wildcard arm)
+    /// let x: u8;
+    /// let c = || match x { _ => () };
+    /// ```
+    /// In this example, we don't need to actually read/borrow `x` in `c`, and so we don't
+    /// want to capture it. However, we do still want an error here, because `x` should have
+    /// to be initialized at the point where c is created. Therefore, we add a "fake read"
+    /// instead.
+    /// ``` (destructured assignments)
+    /// let c = || {
+    ///     let (t1, t2) = t;
+    /// }
+    /// ```
+    /// In the second example, we capture the disjoint fields of `t` (`t.0` & `t.1`), but
+    /// we never capture `t`. This becomes an issue when we build MIR as we require
+    /// information on `t` in order to create place `t.0` and `t.1`. We can solve this
+    /// issue by fake reading `t`.
+    pub closure_fake_reads: FxHashMap<DefId, Vec<(HirPlace<'tcx>, FakeReadCause, hir::HirId)>>,
+
     /// Stores the type, expression, span and optional scope span of all types
     /// that are live across the yield of this generator (if a generator).
     pub generator_interior_types: ty::Binder<Vec<GeneratorInteriorTypeCause<'tcx>>>,
@@ -464,6 +489,7 @@ impl<'tcx> TypeckResults<'tcx> {
             concrete_opaque_types: Default::default(),
             closure_captures: Default::default(),
             closure_min_captures: Default::default(),
+            closure_fake_reads: Default::default(),
             generator_interior_types: ty::Binder::dummy(Default::default()),
             treat_byte_string_as_slice: Default::default(),
         }
@@ -715,6 +741,7 @@ impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for TypeckResults<'tcx> {
             ref concrete_opaque_types,
             ref closure_captures,
             ref closure_min_captures,
+            ref closure_fake_reads,
             ref generator_interior_types,
             ref treat_byte_string_as_slice,
         } = *self;
@@ -750,6 +777,7 @@ impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for TypeckResults<'tcx> {
             concrete_opaque_types.hash_stable(hcx, hasher);
             closure_captures.hash_stable(hcx, hasher);
             closure_min_captures.hash_stable(hcx, hasher);
+            closure_fake_reads.hash_stable(hcx, hasher);
             generator_interior_types.hash_stable(hcx, hasher);
             treat_byte_string_as_slice.hash_stable(hcx, hasher);
         })

compiler/rustc_mir_build/src/build/expr/as_place.rs

@@ -10,14 +10,15 @@ use rustc_middle::hir::place::ProjectionKind as HirProjectionKind;
 use rustc_middle::middle::region;
 use rustc_middle::mir::AssertKind::BoundsCheck;
 use rustc_middle::mir::*;
+use rustc_middle::ty::AdtDef;
 use rustc_middle::ty::{self, CanonicalUserTypeAnnotation, Ty, TyCtxt, Variance};
 use rustc_span::Span;
 use rustc_target::abi::VariantIdx;
 
 use rustc_index::vec::Idx;
 
 /// The "outermost" place that holds this value.
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, Debug, PartialEq)]
 crate enum PlaceBase {
     /// Denotes the start of a `Place`.
     Local(Local),
@@ -67,7 +68,7 @@ crate enum PlaceBase {
 ///
 /// This is used internally when building a place for an expression like `a.b.c`. The fields `b`
 /// and `c` can be progressively pushed onto the place builder that is created when converting `a`.
-#[derive(Clone)]
+#[derive(Clone, Debug, PartialEq)]
 crate struct PlaceBuilder<'tcx> {
     base: PlaceBase,
     projection: Vec<PlaceElem<'tcx>>,
@@ -83,20 +84,23 @@ fn convert_to_hir_projections_and_truncate_for_capture<'tcx>(
     mir_projections: &[PlaceElem<'tcx>],
 ) -> Vec<HirProjectionKind> {
     let mut hir_projections = Vec::new();
+    let mut variant = None;
 
     for mir_projection in mir_projections {
         let hir_projection = match mir_projection {
             ProjectionElem::Deref => HirProjectionKind::Deref,
             ProjectionElem::Field(field, _) => {
-                // We will never encouter this for multivariant enums,
-                // read the comment for `Downcast`.
-                HirProjectionKind::Field(field.index() as u32, VariantIdx::new(0))
+                let variant = variant.unwrap_or(VariantIdx::new(0));
+                HirProjectionKind::Field(field.index() as u32, variant)
             }
-            ProjectionElem::Downcast(..) => {
-                // This projections exist only for enums that have
-                // multiple variants. Since such enums that are captured
-                // completely, we can stop here.
-                break;
+            ProjectionElem::Downcast(.., idx) => {
+                // We don't expect to see multi-variant enums here, as earlier
+                // phases will have truncated them already. However, there can
+                // still be downcasts, thanks to single-variant enums.
+                // We keep track of VariantIdx so we can use this information
+                // if the next ProjectionElem is a Field.
+                variant = Some(*idx);
+                continue;
             }
             ProjectionElem::Index(..)
             | ProjectionElem::ConstantIndex { .. }
@@ -106,7 +110,7 @@ fn convert_to_hir_projections_and_truncate_for_capture<'tcx>(
                 break;
             }
         };
-
+        variant = None;
         hir_projections.push(hir_projection);
     }
 
@@ -194,12 +198,12 @@ fn find_capture_matching_projections<'a, 'tcx>(
 /// Takes a PlaceBuilder and resolves the upvar (if any) within it, so that the
 /// `PlaceBuilder` now starts from `PlaceBase::Local`.
 ///
-/// Returns a Result with the error being the HirId of the Upvar that was not found.
+/// Returns a Result with the error being the PlaceBuilder (`from_builder`) that was not found.
 fn to_upvars_resolved_place_builder<'a, 'tcx>(
     from_builder: PlaceBuilder<'tcx>,
     tcx: TyCtxt<'tcx>,
     typeck_results: &'a ty::TypeckResults<'tcx>,
-) -> Result<PlaceBuilder<'tcx>, HirId> {
+) -> Result<PlaceBuilder<'tcx>, PlaceBuilder<'tcx>> {
     match from_builder.base {
         PlaceBase::Local(_) => Ok(from_builder),
         PlaceBase::Upvar { var_hir_id, closure_def_id, closure_kind } => {
@@ -230,13 +234,12 @@ fn to_upvars_resolved_place_builder<'a, 'tcx>(
                         from_builder.projection
                     )
                 } else {
-                    // FIXME(project-rfc-2229#24): Handle this case properly
                     debug!(
                         "No associated capture found for {:?}[{:#?}]",
                         var_hir_id, from_builder.projection,
                     );
                 }
-                return Err(var_hir_id);
+                return Err(from_builder);
             };
 
             let closure_ty = typeck_results
@@ -300,6 +303,25 @@ impl<'tcx> PlaceBuilder<'tcx> {
         to_upvars_resolved_place_builder(self, tcx, typeck_results).unwrap()
     }
 
+    /// Attempts to resolve the `PlaceBuilder`.
+    /// On success, it will return the resolved `PlaceBuilder`.
+    /// On failure, it will return itself.
+    ///
+    /// Upvars resolve may fail for a `PlaceBuilder` when attempting to
+    /// resolve a disjoint field whose root variable is not captured
+    /// (destructured assignments) or when attempting to resolve a root
+    /// variable (discriminant matching with only wildcard arm) that is
+    /// not captured. This can happen because the final mir that will be
+    /// generated doesn't require a read for this place. Failures will only
+    /// happen inside closures.
+    crate fn try_upvars_resolved<'a>(
+        self,
+        tcx: TyCtxt<'tcx>,
+        typeck_results: &'a ty::TypeckResults<'tcx>,
+    ) -> Result<PlaceBuilder<'tcx>, PlaceBuilder<'tcx>> {
+        to_upvars_resolved_place_builder(self, tcx, typeck_results)
+    }
+
     crate fn base(&self) -> PlaceBase {
         self.base
     }
@@ -308,15 +330,22 @@ impl<'tcx> PlaceBuilder<'tcx> {
         self.project(PlaceElem::Field(f, ty))
     }
 
-    fn deref(self) -> Self {
+    crate fn deref(self) -> Self {
         self.project(PlaceElem::Deref)
     }
 
+    crate fn downcast(self, adt_def: &'tcx AdtDef, variant_index: VariantIdx) -> Self {
+        self.project(PlaceElem::Downcast(
+            Some(adt_def.variants[variant_index].ident.name),
+            variant_index,
+        ))
+    }
+
     fn index(self, index: Local) -> Self {
         self.project(PlaceElem::Index(index))
     }
 
-    fn project(mut self, elem: PlaceElem<'tcx>) -> Self {
+    crate fn project(mut self, elem: PlaceElem<'tcx>) -> Self {
         self.projection.push(elem);
         self
     }
@@ -602,13 +631,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         // The "retagging" transformation (for Stacked Borrows) relies on this.
         let idx = unpack!(block = self.as_temp(block, temp_lifetime, index, Mutability::Not,));
 
-        block = self.bounds_check(
-            block,
-            base_place.clone().into_place(self.tcx, self.typeck_results),
-            idx,
-            expr_span,
-            source_info,
-        );
+        block = self.bounds_check(block, base_place.clone(), idx, expr_span, source_info);
 
         if is_outermost_index {
             self.read_fake_borrows(block, fake_borrow_temps, source_info)
@@ -629,7 +652,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     fn bounds_check(
         &mut self,
         block: BasicBlock,
-        slice: Place<'tcx>,
+        slice: PlaceBuilder<'tcx>,
         index: Local,
         expr_span: Span,
         source_info: SourceInfo,
@@ -641,7 +664,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         let lt = self.temp(bool_ty, expr_span);
 
         // len = len(slice)
-        self.cfg.push_assign(block, source_info, len, Rvalue::Len(slice));
+        self.cfg.push_assign(
+            block,
+            source_info,
+            len,
+            Rvalue::Len(slice.into_place(self.tcx, self.typeck_results)),
+        );
         // lt = idx < len
         self.cfg.push_assign(
             block,

compiler/rustc_mir_build/src/build/expr/as_rvalue.rs

@@ -8,6 +8,7 @@ use crate::build::{BlockAnd, BlockAndExtension, Builder};
 use crate::thir::*;
 use rustc_middle::middle::region;
 use rustc_middle::mir::AssertKind;
+use rustc_middle::mir::Place;
 use rustc_middle::mir::*;
 use rustc_middle::ty::{self, Ty, UpvarSubsts};
 use rustc_span::Span;
@@ -164,7 +165,41 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
                 block.and(Rvalue::Aggregate(box AggregateKind::Tuple, fields))
             }
-            ExprKind::Closure { closure_id, substs, upvars, movability } => {
+            ExprKind::Closure { closure_id, substs, upvars, movability, ref fake_reads } => {
+                // Convert the closure fake reads, if any, from `ExprRef` to mir `Place`
+                // and push the fake reads.
+                // This must come before creating the operands. This is required in case
+                // there is a fake read and a borrow of the same path, since otherwise the
+                // fake read might interfere with the borrow. Consider an example like this
+                // one:
+                // ```
+                // let mut x = 0;
+                // let c = || {
+                //     &mut x; // mutable borrow of `x`
+                //     match x { _ => () } // fake read of `x`
+                // };
+                // ```
+                // FIXME(RFC2229): Remove feature gate once diagnostics are improved
+                if this.tcx.features().capture_disjoint_fields {
+                    for (thir_place, cause, hir_id) in fake_reads.into_iter() {
+                        let place_builder =
+                            unpack!(block = this.as_place_builder(block, thir_place));
+
+                        if let Ok(place_builder_resolved) =
+                            place_builder.try_upvars_resolved(this.tcx, this.typeck_results)
+                        {
+                            let mir_place =
+                                place_builder_resolved.into_place(this.tcx, this.typeck_results);
+                            this.cfg.push_fake_read(
+                                block,
+                                this.source_info(this.tcx.hir().span(*hir_id)),
+                                *cause,
+                                mir_place,
+                            );
+                        }
+                    }
+                }
+
                 // see (*) above
                 let operands: Vec<_> = upvars
                     .into_iter()
@@ -203,6 +238,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                         }
                     })
                     .collect();
+
                 let result = match substs {
                     UpvarSubsts::Generator(substs) => {
                         // We implicitly set the discriminant to 0. See

compiler/rustc_mir_build/src/build/expr/mod.rs

@@ -62,7 +62,7 @@
 
 mod as_constant;
 mod as_operand;
-mod as_place;
+pub mod as_place;
 mod as_rvalue;
 mod as_temp;
 mod category;