ExprUseVisitor: remove maybe_read_scrutinee

The split between walk_pat and maybe_read_scrutinee has now become
redundant.

Due to this change, one testcase within the testsuite has become similar
enough to a known ICE to also break. I am leaving this as future work,
as it requires feature(type_alias_impl_trait)

Author: meithecatte

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

@@ -7,9 +7,7 @@
 
 use std::cell::{Ref, RefCell};
 use std::ops::Deref;
-use std::slice::from_ref;
 
-use hir::Expr;
 use hir::def::DefKind;
 use hir::pat_util::EnumerateAndAdjustIterator as _;
 use rustc_abi::{FIRST_VARIANT, FieldIdx, VariantIdx};
@@ -312,7 +310,8 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
 
             let param_place = self.cat_rvalue(param.hir_id, param_ty);
 
-            self.walk_irrefutable_pat(&param_place, param.pat)?;
+            self.fake_read_scrutinee(&param_place, false)?;
+            self.walk_pat(&param_place, param.pat, false)?;
         }
 
         self.consume_expr(body.value)?;
@@ -454,13 +453,9 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
 
             hir::ExprKind::Match(discr, arms, _) => {
                 let discr_place = self.cat_expr(discr)?;
-                self.maybe_read_scrutinee(
-                    discr,
-                    discr_place.clone(),
-                    arms.iter().map(|arm| arm.pat),
-                )?;
+                self.fake_read_scrutinee(&discr_place, true)?;
+                self.walk_expr(discr)?;
 
-                // treatment of the discriminant is handled while walking the arms.
                 for arm in arms {
                     self.walk_arm(&discr_place, arm)?;
                 }
@@ -597,115 +592,25 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
         Ok(())
     }
 
-    fn maybe_read_scrutinee<'t>(
+    #[instrument(skip(self), level = "debug")]
+    fn fake_read_scrutinee(
         &self,
-        discr: &Expr<'_>,
-        discr_place: PlaceWithHirId<'tcx>,
-        pats: impl Iterator<Item = &'t hir::Pat<'t>>,
+        discr_place: &PlaceWithHirId<'tcx>,
+        refutable: bool,
     ) -> Result<(), Cx::Error> {
-        // 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.
-        let mut needs_to_be_read = false;
-        for pat in pats {
-            self.cat_pattern(discr_place.clone(), pat, &mut |place, pat| {
-                match &pat.kind {
-                    PatKind::Binding(.., opt_sub_pat) => {
-                        // If the opt_sub_pat is None, then the binding does not count as
-                        // a wildcard for the purpose of borrowing discr.
-                        if opt_sub_pat.is_none() {
-                            needs_to_be_read = true;
-                        }
-                    }
-                    PatKind::Never => {
-                        // A never pattern reads the value.
-                        // FIXME(never_patterns): does this do what I expect?
-                        needs_to_be_read = true;
-                    }
-                    PatKind::Expr(PatExpr { kind: PatExprKind::Path(qpath), hir_id, span }) => {
-                        // A `Path` pattern is just a name like `Foo`. This is either a
-                        // named constant or else it refers to an ADT variant
-
-                        let res = self.cx.typeck_results().qpath_res(qpath, *hir_id);
-                        match res {
-                            Res::Def(DefKind::Const, _) | Res::Def(DefKind::AssocConst, _) => {
-                                // Named constants have to be equated with the value
-                                // being matched, so that's a read of the value being matched.
-                                //
-                                // FIXME: We don't actually reads for ZSTs.
-                                needs_to_be_read = true;
-                            }
-                            _ => {
-                                // Otherwise, this is a struct/enum variant, and so it's
-                                // only a read if we need to read the discriminant.
-                                needs_to_be_read |=
-                                    self.is_multivariant_adt(place.place.ty(), *span);
-                            }
-                        }
-                    }
-                    PatKind::TupleStruct(..) | PatKind::Struct(..) | PatKind::Tuple(..) => {
-                        // For `Foo(..)`, `Foo { ... }` and `(...)` patterns, check if we are matching
-                        // against a multivariant enum or struct. In that case, we have to read
-                        // the discriminant. Otherwise this kind of pattern doesn't actually
-                        // read anything (we'll get invoked for the `...`, which may indeed
-                        // perform some reads).
-
-                        let place_ty = place.place.ty();
-                        needs_to_be_read |= self.is_multivariant_adt(place_ty, pat.span);
-                    }
-                    PatKind::Expr(_) | PatKind::Range(..) => {
-                        // If the PatKind is a Lit or a Range then we want
-                        // to borrow discr.
-                        needs_to_be_read = true;
-                    }
-                    PatKind::Slice(lhs, wild, rhs) => {
-                        // We don't need to test the length if the pattern is `[..]`
-                        if matches!((lhs, wild, rhs), (&[], Some(_), &[]))
-                            // Arrays have a statically known size, so
-                            // there is no need to read their length
-                            || place.place.ty().peel_refs().is_array()
-                        {
-                        } else {
-                            needs_to_be_read = true;
-                        }
-                    }
-                    PatKind::Or(_)
-                    | PatKind::Box(_)
-                    | PatKind::Deref(_)
-                    | PatKind::Ref(..)
-                    | PatKind::Guard(..)
-                    | PatKind::Wild
-                    | PatKind::Err(_) => {
-                        // If the PatKind is Or, Box, or Ref, the decision is made later
-                        // as these patterns contains subpatterns
-                        // If the PatKind is Wild or Err, the decision is made based on the other patterns
-                        // being examined
-                    }
-                }
-
-                Ok(())
-            })?
-        }
+        let closure_def_id = match discr_place.place.base {
+            PlaceBase::Upvar(upvar_id) => Some(upvar_id.closure_expr_id),
+            _ => None,
+        };
 
-        if needs_to_be_read {
-            self.borrow_expr(discr, BorrowKind::Immutable)?;
+        let cause = if refutable {
+            FakeReadCause::ForMatchedPlace(closure_def_id)
         } else {
-            let closure_def_id = match discr_place.place.base {
-                PlaceBase::Upvar(upvar_id) => Some(upvar_id.closure_expr_id),
-                _ => None,
-            };
+            FakeReadCause::ForLet(closure_def_id)
+        };
 
-            self.delegate.borrow_mut().fake_read(
-                &discr_place,
-                FakeReadCause::ForMatchedPlace(closure_def_id),
-                discr_place.hir_id,
-            );
+        self.delegate.borrow_mut().fake_read(discr_place, cause, discr_place.hir_id);
 
-            // We always want to walk the discriminant. We want to make sure, for instance,
-            // that the discriminant has been initialized.
-            self.walk_expr(discr)?;
-        }
         Ok(())
     }
 
@@ -722,12 +627,11 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
         self.walk_expr(expr)?;
         let expr_place = self.cat_expr(expr)?;
         f()?;
+        self.fake_read_scrutinee(&expr_place, els.is_some())?;
+        self.walk_pat(&expr_place, pat, false)?;
         if let Some(els) = els {
-            // borrowing because we need to test the discriminant
-            self.maybe_read_scrutinee(expr, expr_place.clone(), from_ref(pat).iter())?;
             self.walk_block(els)?;
         }
-        self.walk_irrefutable_pat(&expr_place, pat)?;
         Ok(())
     }
 
@@ -899,16 +803,6 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
         discr_place: &PlaceWithHirId<'tcx>,
         arm: &hir::Arm<'_>,
     ) -> Result<(), Cx::Error> {
-        let closure_def_id = match discr_place.place.base {
-            PlaceBase::Upvar(upvar_id) => Some(upvar_id.closure_expr_id),
-            _ => None,
-        };
-
-        self.delegate.borrow_mut().fake_read(
-            discr_place,
-            FakeReadCause::ForMatchedPlace(closure_def_id),
-            discr_place.hir_id,
-        );
         self.walk_pat(discr_place, arm.pat, arm.guard.is_some())?;
 
         if let Some(ref e) = arm.guard {
@@ -919,27 +813,6 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
         Ok(())
     }
 
-    /// Walks a pat that occurs in isolation (i.e., top-level of fn argument or
-    /// let binding, and *not* a match arm or nested pat.)
-    fn walk_irrefutable_pat(
-        &self,
-        discr_place: &PlaceWithHirId<'tcx>,
-        pat: &hir::Pat<'_>,
-    ) -> Result<(), Cx::Error> {
-        let closure_def_id = match discr_place.place.base {
-            PlaceBase::Upvar(upvar_id) => Some(upvar_id.closure_expr_id),
-            _ => None,
-        };
-
-        self.delegate.borrow_mut().fake_read(
-            discr_place,
-            FakeReadCause::ForLet(closure_def_id),
-            discr_place.hir_id,
-        );
-        self.walk_pat(discr_place, pat, false)?;
-        Ok(())
-    }
-
     /// The core driver for walking a pattern
     ///
     /// This should mirror how pattern-matching gets lowered to MIR, as
@@ -1928,8 +1801,11 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
     /// Here, we cannot perform such an accurate checks, because querying
     /// whether a type is inhabited requires that it has been fully inferred,
     /// which cannot be guaranteed at this point.
+    #[instrument(skip(self, span), level = "debug")]
     fn is_multivariant_adt(&self, ty: Ty<'tcx>, span: Span) -> bool {
-        if let ty::Adt(def, _) = self.cx.try_structurally_resolve_type(span, ty).kind() {
+        let ty = self.cx.try_structurally_resolve_type(span, ty);
+        debug!(?ty, "is_multivariant_adt: resolved");
+        if let ty::Adt(def, _) = ty.kind() {
             // Note that if a non-exhaustive SingleVariant is defined in another crate, we need
             // to assume that more cases will be added to the variant in the future. This mean
             // that we should handle non-exhaustive SingleVariant the same way we would handle

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

@@ -1447,6 +1447,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     pub(crate) fn try_structurally_resolve_type(&self, sp: Span, ty: Ty<'tcx>) -> Ty<'tcx> {
         let ty = self.resolve_vars_with_obligations(ty);
 
+        debug!("next_solver = {:?}", self.next_trait_solver());
         if self.next_trait_solver()
             && let ty::Alias(..) = ty.kind()
         {

tests/crashes/119786.rs renamed to tests/crashes/119786-1.rs

@@ -0,0 +1,15 @@
+//@ known-bug: #119786
+//@ edition:2021
+
+fn enum_upvar() {
+    type T = impl Copy;
+    let foo: T = Some((1u32, 2u32));
+    let x = move || {
+        match foo {
+            None => (),
+            Some(_) => (),
+        }
+    };
+}
+
+pub fn main() {}

tests/crashes/119786-2.rs

@@ -0,0 +1,15 @@
+//@ known-bug: #119786
+//@ edition:2021
+
+fn enum_upvar() {
+    type T = impl Copy;
+    let foo: T = Some((1u32, 2u32));
+    let x = move || {
+        match foo {
+            None => (),
+            Some((a, b)) => (),
+        }
+    };
+}
+
+pub fn main() {}

tests/crashes/119786-3.rs

@@ -4,7 +4,7 @@ error[E0505]: cannot move out of `ts` because it is borrowed
 LL |     let _b = || { match ts {
    |              --         -- borrow occurs due to use in closure
    |              |
-   |              borrow of `ts` occurs here
+   |              borrow of `ts.x` occurs here
 ...
 LL |     let mut mut_ts = ts;
    |                      ^^ move out of `ts` occurs here

tests/ui/closures/2229_closure_analysis/match/match-edge-cases_2.stderr

@@ -14,7 +14,6 @@ fn test_1_should_capture() {
     //~| Min Capture analysis includes:
         match variant {
         //~^ NOTE: Capturing variant[] -> Immutable
-        //~| NOTE: Capturing variant[] -> Immutable
         //~| NOTE: Min Capture variant[] -> Immutable
             Some(_) => {}
             _ => {}
@@ -65,9 +64,8 @@ fn test_6_should_capture_single_variant() {
     //~^ First Pass analysis includes:
     //~| Min Capture analysis includes:
         match variant {
-            //~^ NOTE: Capturing variant[] -> Immutable
-            //~| NOTE: Capturing variant[(0, 0)] -> Immutable
-            //~| NOTE: Min Capture variant[] -> Immutable
+            //~^ NOTE: Capturing variant[(0, 0)] -> Immutable
+            //~| NOTE: Min Capture variant[(0, 0)] -> Immutable
             SingleVariant::Points(a) => {
                 println!("{:?}", a);
             }
@@ -133,7 +131,6 @@ fn test_5_should_capture_multi_variant() {
     //~| Min Capture analysis includes:
         match variant {
         //~^ NOTE: Capturing variant[] -> Immutable
-        //~| NOTE: Capturing variant[] -> Immutable
         //~| NOTE: Min Capture variant[] -> Immutable
             MVariant::A => {}
             _ => {}
@@ -151,9 +148,8 @@ fn test_7_should_capture_slice_len() {
     //~^ First Pass analysis includes:
     //~| Min Capture analysis includes:
         match slice {
-            //~^ NOTE: Capturing slice[] -> Immutable
-            //~| NOTE: Capturing slice[Deref] -> Immutable
-            //~| NOTE: Min Capture slice[] -> Immutable
+            //~^ NOTE: Capturing slice[Deref] -> Immutable
+            //~| NOTE: Min Capture slice[Deref] -> Immutable
             [_,_,_] => {},
             _ => {}
         }
@@ -164,9 +160,8 @@ fn test_7_should_capture_slice_len() {
     //~^ First Pass analysis includes:
     //~| Min Capture analysis includes:
         match slice {
-            //~^ NOTE: Capturing slice[] -> Immutable
-            //~| NOTE: Capturing slice[Deref] -> Immutable
-            //~| NOTE: Min Capture slice[] -> Immutable
+            //~^ NOTE: Capturing slice[Deref] -> Immutable
+            //~| NOTE: Min Capture slice[Deref] -> Immutable
             [] => {},
             _ => {}
         }
@@ -177,9 +172,8 @@ fn test_7_should_capture_slice_len() {
     //~^ First Pass analysis includes:
     //~| Min Capture analysis includes:
         match slice {
-            //~^ NOTE: Capturing slice[] -> Immutable
-            //~| NOTE: Capturing slice[Deref] -> Immutable
-            //~| NOTE: Min Capture slice[] -> Immutable
+            //~^ NOTE: Capturing slice[Deref] -> Immutable
+            //~| NOTE: Min Capture slice[Deref] -> Immutable
             [_, .. ,_] => {},
             _ => {}
         }