Only borrow place for matching under specific conditions

Author: roxelo

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

@@ -100,7 +100,7 @@ fn convert_to_hir_projections_and_truncate_for_capture<'tcx>(
                 // single and multiple variants.
                 // For single variants, enums are not captured completely.
                 // We keep track of VariantIdx so we can use this information
-                // if the next ProjectionElem is a Field
+                // if the next ProjectionElem is a Field.
                 variant = Some(*idx);
                 continue;
             }

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

@@ -420,6 +420,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             | ExprKind::PlaceTypeAscription { .. }
             | ExprKind::ValueTypeAscription { .. } => {
                 debug_assert!(Category::of(&expr.kind) == Some(Category::Place));
+
                 let place = unpack!(block = this.as_place(block, expr));
                 let rvalue = Rvalue::Use(this.consume_by_copy_or_move(place));
                 this.cfg.push_assign(block, source_info, destination, rvalue);
@@ -436,6 +437,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 }
 
                 debug_assert!(Category::of(&expr.kind) == Some(Category::Place));
+
                 let place = unpack!(block = this.as_place(block, expr));
                 let rvalue = Rvalue::Use(this.consume_by_copy_or_move(place));
                 this.cfg.push_assign(block, source_info, destination, rvalue);

compiler/rustc_mir_build/src/build/matches/simplify.rs

@@ -68,7 +68,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             let match_pairs = mem::take(&mut candidate.match_pairs);
 
             if let [MatchPair { pattern: Pat { kind: box PatKind::Or { pats }, .. }, place }] =
-                &*match_pairs.clone()
+                &*match_pairs
             {
                 existing_bindings.extend_from_slice(&new_bindings);
                 mem::swap(&mut candidate.bindings, &mut existing_bindings);
@@ -155,12 +155,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 ascription: thir::pattern::Ascription { variance, user_ty, user_ty_span },
             } => {
                 // Apply the type ascription to the value at `match_pair.place`, which is the
-                // value being matched, taking the variance field into account.
-                let place = match_pair.place.clone().into_place(self.tcx, self.typeck_results);
                 candidate.ascriptions.push(Ascription {
                     span: user_ty_span,
                     user_ty,
-                    source: place,
+                    source: match_pair.place.clone().into_place(self.tcx, self.typeck_results),
                     variance,
                 });
 
@@ -175,12 +173,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             }
 
             PatKind::Binding { name, mutability, mode, var, ty, ref subpattern, is_primary: _ } => {
-                let place = match_pair.place.clone().into_place(self.tcx, self.typeck_results);
                 candidate.bindings.push(Binding {
                     name,
                     mutability,
                     span: match_pair.pattern.span,
-                    source: place,
+                    source: match_pair.place.clone().into_place(self.tcx, self.typeck_results),
                     var_id: var,
                     var_ty: ty,
                     binding_mode: mode,

compiler/rustc_mir_build/src/thir/cx/expr.rs

@@ -455,32 +455,18 @@ impl<'thir, 'tcx> Cx<'thir, 'tcx> {
                 );
 
                 let fake_reads = match self.typeck_results().closure_fake_reads.get(&def_id) {
-                    Some(vals) => {
-                        Some(
-                            vals.iter()
-                                .map(|(place, cause)| {
-                                    (
-                                        self.arena.alloc(
-                                            self.convert_captured_hir_place(expr, place.clone()),
-                                        ),
-                                        *cause,
-                                    )
-                                    // let var_hir_id = match val.base {
-                                    //     HirPlaceBase::Upvar(upvar_id) => {
-                                    //         debug!("upvar");
-                                    //         upvar_id.var_path.hir_id
-                                    //     }
-                                    //     _ => {
-                                    //         bug!(
-                                    //             "Do not know how to get HirId out of Rvalue and StaticItem"
-                                    //         );
-                                    //     }
-                                    // };
-                                    // self.fake_read_capture_upvar(expr, val.clone(), var_hir_id)
-                                })
-                                .collect(),
-                        )
-                    }
+                    Some(vals) => Some(
+                        vals.iter()
+                            .map(|(place, cause)| {
+                                (
+                                    self.arena.alloc(
+                                        self.convert_captured_hir_place(expr, place.clone()),
+                                    ),
+                                    *cause,
+                                )
+                            })
+                            .collect(),
+                    ),
                     None => None,
                 };
 
@@ -1058,6 +1044,11 @@ impl<'thir, 'tcx> Cx<'thir, 'tcx> {
         let temp_lifetime = self.region_scope_tree.temporary_scope(closure_expr.hir_id.local_id);
         let var_ty = place.base_ty;
 
+        // The result of capture analysis in `rustc_typeck/check/upvar.rs`represents a captured path
+        // as it's seen for use within the closure and not at the time of closure creation.
+        //
+        // That is we see expect to see it start from a captured upvar and not something that is local
+        // to the closure's parent.
         let var_hir_id = match place.base {
             HirPlaceBase::Upvar(upvar_id) => upvar_id.var_path.hir_id,
             base => bug!("Expected an upvar, found {:?}", base),

compiler/rustc_typeck/src/expr_use_visitor.rs

@@ -8,10 +8,10 @@ pub use self::ConsumeMode::*;
 pub use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, Projection};
 
 use rustc_hir as hir;
-use rustc_hir::def::{DefKind, Res};
+use rustc_hir::def::Res;
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::PatKind;
-use rustc_hir::QPath;
+//use rustc_hir::QPath;
 use rustc_index::vec::Idx;
 use rustc_infer::infer::InferCtxt;
 use rustc_middle::hir::place::ProjectionKind;
@@ -54,7 +54,6 @@ pub trait Delegate<'tcx> {
     // `diag_expr_id` is the id used for diagnostics (see `consume` for more details).
     fn mutate(&mut self, assignee_place: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId);
 
-    // [FIXME] RFC2229 This should also affect clippy ref: https://github.com/sexxi-goose/rust/pull/27
     fn fake_read(&mut self, place: Place<'tcx>, cause: FakeReadCause);
 }
 
@@ -235,24 +234,30 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
             hir::ExprKind::Match(ref discr, arms, _) => {
                 let discr_place = return_if_err!(self.mc.cat_expr(&discr));
 
+                // Matching should not always be considered a use of the place, hence
+                // discr does not necessarily need to be borrowed.
                 // We only want to borrow discr if the pattern contain something other
-                // than wildcards
+                // than wildcards.
                 let ExprUseVisitor { ref mc, body_owner: _, delegate: _ } = *self;
                 let mut needs_to_be_read = false;
                 for arm in arms.iter() {
                     return_if_err!(mc.cat_pattern(discr_place.clone(), &arm.pat, |_place, pat| {
-                        if let PatKind::Binding(_, _, _, opt_sub_pat) = pat.kind {
-                            if let None = opt_sub_pat {
-                                needs_to_be_read = true;
-                            }
-                        } else if let PatKind::TupleStruct(qpath, _, _) = &pat.kind {
-                            // If a TupleStruct has a Some PathSegment, we should read the discr_place
-                            // regardless if it contains a Wild pattern later
-                            if let QPath::Resolved(_, path) = qpath {
-                                if let Res::Def(DefKind::Ctor(_, _), _) = path.res {
+                        match &pat.kind {
+                            PatKind::Binding(_, _, _, opt_sub_pat) => {
+                                // If the opt_sub_pat is None, than the binding does not count as
+                                // a wildcard for the purpose of borrowing discr
+                                if let None = opt_sub_pat {
                                     needs_to_be_read = true;
                                 }
                             }
+                            PatKind::TupleStruct(_, _, _)
+                            | PatKind::Struct(_, _, _)
+                            | PatKind::Lit(_) => {
+                                // If the PatKind is a TupleStruct, Struct, or Lit then we want
+                                // to borrow discr
+                                needs_to_be_read = true;
+                            }
+                            _ => {}
                         }
                     }));
                 }
@@ -629,6 +634,8 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
     /// - When reporting the Place back to the Delegate, ensure that the UpvarId uses the enclosing
     /// closure as the DefId.
     fn walk_captures(&mut self, closure_expr: &hir::Expr<'_>) {
+        debug!("walk_captures({:?})", closure_expr);
+
         let closure_def_id = self.tcx().hir().local_def_id(closure_expr.hir_id).to_def_id();
         let upvars = self.tcx().upvars_mentioned(self.body_owner);
 
@@ -645,10 +652,9 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
                         if upvars.map_or(body_owner_is_closure, |upvars| {
                             !upvars.contains_key(&upvar_id.var_path.hir_id)
                         }) {
-                            // [FIXME] RFC2229 Update this comment
-                            // The nested closure might be capturing the current (enclosing) closure's local variables.
+                            // The nested closure might be fake reading the current (enclosing) closure's local variables.
                             // We check if the root variable is ever mentioned within the enclosing closure, if not
-                            // then for the current body (if it's a closure) these aren't captures, we will ignore them.
+                            // then for the current body (if it's a closure) these do not require fake_read, we will ignore them.
                             continue;
                         }
                     }

src/test/ui/closures/2229_closure_analysis/run_pass/lit-pattern-matching-with-methods.rs

@@ -0,0 +1,24 @@
+//check-pass
+#![feature(capture_disjoint_fields)]
+//~^ WARNING: the feature `capture_disjoint_fields` is incomplete
+#![warn(unused)]
+#![feature(rustc_attrs)]
+#![feature(btree_drain_filter)]
+
+use std::collections::BTreeMap;
+use std::panic::{catch_unwind, AssertUnwindSafe};
+
+fn main() {
+    let mut map = BTreeMap::new();
+    map.insert("a", ());
+    map.insert("b", ());
+    map.insert("c", ());
+
+    {
+        let mut it = map.drain_filter(|_, _| true);
+        catch_unwind(AssertUnwindSafe(|| while it.next().is_some() {})).unwrap_err();
+        let result = catch_unwind(AssertUnwindSafe(|| it.next()));
+        assert!(matches!(result, Ok(None)));
+    }
+
+}

src/test/ui/closures/2229_closure_analysis/run_pass/lit-pattern-matching-with-methods.stderr

@@ -0,0 +1,11 @@
+warning: the feature `capture_disjoint_fields` is incomplete and may not be safe to use and/or cause compiler crashes
+  --> $DIR/lit-pattern-matching-with-methods.rs:2:12
+   |
+LL | #![feature(capture_disjoint_fields)]
+   |            ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `#[warn(incomplete_features)]` on by default
+   = note: see issue #53488 <https://github.com/rust-lang/rust/issues/53488> for more information
+
+warning: 1 warning emitted
+

src/test/ui/closures/2229_closure_analysis/run_pass/struct-pattern-matching-with-methods.rs

@@ -0,0 +1,48 @@
+//check-pass
+#![feature(capture_disjoint_fields)]
+//~^ WARNING: the feature `capture_disjoint_fields` is incomplete
+#![warn(unused)]
+#![feature(rustc_attrs)]
+
+#[derive(Debug, Clone, Copy)]
+enum PointType {
+    TwoD { x: u32, y: u32 },
+
+    ThreeD{ x: u32, y: u32, z: u32 }
+}
+
+struct Points {
+    points: Vec<PointType>,
+}
+
+impl Points {
+    pub fn test1(&mut self) -> Vec<usize> {
+        (0..self.points.len())
+            .filter_map(|i| {
+                let idx = i as usize;
+                match self.test2(idx) {
+                    PointType::TwoD { .. } => Some(i),
+                    PointType::ThreeD { .. } => None,
+                }
+            })
+            .collect()
+    }
+
+    pub fn test2(&mut self, i: usize) -> PointType {
+        self.points[i]
+    }
+}
+
+fn main() {
+    let mut points = Points {
+        points: Vec::<PointType>::new()
+    };
+
+    points.points.push(PointType::ThreeD { x:0, y:0, z:0 });
+    points.points.push(PointType::TwoD{ x:0, y:0 });
+    points.points.push(PointType::ThreeD{ x:0, y:0, z:0 });
+    points.points.push(PointType::TwoD{ x:0, y:0 });
+
+    println!("{:?}", points.test1());
+    println!("{:?}", points.points);
+}

src/test/ui/closures/2229_closure_analysis/run_pass/struct-pattern-matching-with-methods.stderr

@@ -0,0 +1,11 @@
+warning: the feature `capture_disjoint_fields` is incomplete and may not be safe to use and/or cause compiler crashes
+  --> $DIR/struct-pattern-matching-with-methods.rs:2:12
+   |
+LL | #![feature(capture_disjoint_fields)]
+   |            ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `#[warn(incomplete_features)]` on by default
+   = note: see issue #53488 <https://github.com/rust-lang/rust/issues/53488> for more information
+
+warning: 1 warning emitted
+

src/test/ui/closures/2229_closure_analysis/run_pass/tuple-struct-pattern-matching-with-methods.rs

@@ -0,0 +1,43 @@
+//check-pass
+#![feature(capture_disjoint_fields)]
+//~^ WARNING: the feature `capture_disjoint_fields` is incomplete
+
+#[derive(Copy, Clone)]
+enum PointType {
+    TwoD(u32, u32),
+    ThreeD(u32, u32, u32)
+}
+
+struct Points {
+    points: Vec<PointType>,
+}
+
+impl Points {
+    pub fn test1(&mut self) -> Vec<usize> {
+        (0..self.points.len())
+            .filter_map(|i| {
+                match self.test2(i) {
+                    PointType::TwoD (..) => Some(i),
+                    PointType::ThreeD (..) => None,
+                }
+            })
+            .collect()
+    }
+
+    pub fn test2(&mut self, i: usize) -> PointType {
+        self.points[i]
+    }
+}
+
+fn main() {
+    let mut points = Points {
+        points: Vec::<PointType>::new()
+    };
+
+    points.points.push(PointType::ThreeD(0,0,0));
+    points.points.push(PointType::TwoD(0,0));
+    points.points.push(PointType::ThreeD(0,0,1));
+    points.points.push(PointType::TwoD(0,1));
+
+    println!("{:?}", points.test1());
+}