ExprUseVisitor: properly report discriminant reads

This should solve the "can't find the upvar" ICEs that resulted from
`maybe_read_scrutinee` being unfit for purpose.

Author: meithecatte

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

@@ -948,6 +948,21 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
     }
 
     /// The core driver for walking a pattern
+    ///
+    /// This should mirror how pattern-matching gets lowered to MIR, as
+    /// otherwise lowering will ICE when trying to resolve the upvars.
+    ///
+    /// However, it is okay to approximate it here by doing *more* accesses
+    /// than the actual MIR builder will, which is useful when some checks
+    /// are too cumbersome to perform here. For example, if only after type
+    /// inference it becomes clear that only one variant of an enum is
+    /// inhabited, and therefore a read of the discriminant is not necessary,
+    /// `walk_pat` will have over-approximated the necessary upvar capture
+    /// granularity. (Or, at least, that's what the code seems to be saying.
+    /// I didn't bother trying to craft an example where this actually happens).
+    ///
+    /// Do note that discrepancies like these do still create weird language
+    /// semantics, and should be avoided if possible.
     fn walk_pat(
         &self,
         discr_place: &PlaceWithHirId<'tcx>,
@@ -958,6 +973,11 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
 
         let tcx = self.cx.tcx();
         self.cat_pattern(discr_place.clone(), pat, &mut |place, pat| {
+            debug!("walk_pat: pat.kind={:?}", pat.kind);
+            let read_discriminant = || {
+                self.delegate.borrow_mut().borrow(place, discr_place.hir_id, BorrowKind::Immutable);
+            };
+
             match pat.kind {
                 PatKind::Binding(_, canonical_id, ..) => {
                     debug!("walk_pat: binding place={:?} pat={:?}", place, pat);
@@ -980,11 +1000,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
                     // binding when lowering pattern guards to ensure that the guard does not
                     // modify the scrutinee.
                     if has_guard {
-                        self.delegate.borrow_mut().borrow(
-                            place,
-                            discr_place.hir_id,
-                            BorrowKind::Immutable,
-                        );
+                        read_discriminant();
                     }
 
                     // It is also a borrow or copy/move of the value being matched.
@@ -1016,13 +1032,70 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
                 PatKind::Never => {
                     // A `!` pattern always counts as an immutable read of the discriminant,
                     // even in an irrefutable pattern.
-                    self.delegate.borrow_mut().borrow(
-                        place,
-                        discr_place.hir_id,
-                        BorrowKind::Immutable,
-                    );
+                    read_discriminant();
+                }
+                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: Does the MIR code skip this read when matching on a ZST?
+                            // If so, we can also skip it here.
+                            read_discriminant();
+                        }
+                        _ => {
+                            // Otherwise, this is a struct/enum variant, and so it's
+                            // only a read if we need to read the discriminant.
+                            if self.is_multivariant_adt(place.place.ty(), *span) {
+                                read_discriminant();
+                            }
+                        }
+                    }
+                }
+                PatKind::Expr(_) | PatKind::Range(..) => {
+                    // When matching against a literal or range, we need to
+                    // borrow the place to compare it against the pattern.
+                    //
+                    // FIXME: What if the type being matched only has one
+                    // possible value?
+                    // FIXME: What if the range is the full range of the type
+                    // and doesn't actually require a discriminant read?
+                    read_discriminant();
+                }
+                PatKind::Struct(..) | PatKind::TupleStruct(..) => {
+                    if self.is_multivariant_adt(place.place.ty(), pat.span) {
+                        read_discriminant();
+                    }
+                }
+                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()
+                    {
+                        // No read necessary
+                    } else {
+                        read_discriminant();
+                    }
+                }
+                PatKind::Or(_)
+                | PatKind::Box(_)
+                | PatKind::Ref(..)
+                | PatKind::Guard(..)
+                | PatKind::Tuple(..)
+                | PatKind::Wild
+                | PatKind::Err(_) => {
+                    // If the PatKind is Or, Box, Ref, Guard, or Tuple, the relevant accesses
+                    // are made later as these patterns contains subpatterns.
+                    // If the PatKind is Wild or Err, they are made when processing the other patterns
+                    // being examined
                 }
-                _ => {}
             }
 
             Ok(())
@@ -1858,6 +1931,14 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx
         Ok(())
     }
 
+    /// Checks whether a type has multiple variants, and therefore, whether a
+    /// read of the discriminant might be necessary. Note that the actual MIR
+    /// builder code does a more specific check, filtering out variants that
+    /// happen to be uninhabited.
+    ///
+    /// 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.
     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() {
             // Note that if a non-exhaustive SingleVariant is defined in another crate, we need

tests/ui/closures/or-patterns-issue-137467.rs

@@ -0,0 +1,142 @@
+//@ edition:2024
+//@ check-pass
+
+const X: u32 = 0;
+
+fn match_literal(x: (u32, u32, u32)) {
+    let _ = || {
+        let ((0, a, _) | (_, _, a)) = x;
+        a
+    };
+}
+
+fn match_range(x: (u32, u32, u32)) {
+    let _ = || {
+        let ((0..5, a, _) | (_, _, a)) = x;
+        a
+    };
+}
+
+fn match_const(x: (u32, u32, u32)) {
+    let _ = || {
+        let ((X, a, _) | (_, _, a)) = x;
+        a
+    };
+}
+
+enum Choice { A, B }
+
+fn match_unit_variant(x: (Choice, u32, u32)) {
+    let _ = || {
+        let ((Choice::A, a, _) | (Choice::B, _, a)) = x;
+        a
+    };
+}
+
+struct Unit;
+
+fn match_unit_struct(mut x: (Unit, u32)) {
+    let r = &mut x.0;
+    let _ = || {
+        let (Unit, a) = x;
+        a
+    };
+
+    let _ = *r;
+}
+
+enum Also { Unit }
+
+fn match_unit_enum(mut x: (Also, u32)) {
+    let r = &mut x.0;
+    let _ = || {
+        let (Also::Unit, a) = x;
+        a
+    };
+
+    let _ = *r;
+}
+
+enum TEnum {
+    A(u32),
+    B(u32),
+}
+
+enum SEnum {
+    A { a: u32 },
+    B { a: u32 },
+}
+
+fn match_tuple_enum(x: TEnum) {
+    let _ = || {
+        let (TEnum::A(a) | TEnum::B(a)) = x;
+        a
+    };
+}
+
+fn match_struct_enum(x: SEnum) {
+    let _ = || {
+        let (SEnum::A { a } | SEnum::B { a }) = x;
+        a
+    };
+}
+
+enum TSingle {
+    A(u32, u32),
+}
+
+enum SSingle {
+    A { a: u32, b: u32 },
+}
+
+struct TStruct(u32, u32);
+struct SStruct { a: u32, b: u32 }
+
+fn match_struct(mut x: SStruct) {
+    let r = &mut x.a;
+    let _ = || {
+        let SStruct { b, .. } = x;
+        b
+    };
+
+    let _ = *r;
+}
+
+fn match_tuple_struct(mut x: TStruct) {
+    let r = &mut x.0;
+    let _ = || {
+        let TStruct(_, a) = x;
+        a
+    };
+
+    let _ = *r;
+}
+
+fn match_singleton(mut x: SSingle) {
+    let SSingle::A { a: ref mut r, .. } = x;
+    let _ = || {
+        let SSingle::A { b, .. } = x;
+        b
+    };
+
+    let _ = *r;
+}
+
+fn match_tuple_singleton(mut x: TSingle) {
+    let TSingle::A(ref mut r, _) = x;
+    let _ = || {
+        let TSingle::A(_, a) = x;
+        a
+    };
+
+    let _ = *r;
+}
+
+fn match_slice(x: (&[u32], u32, u32)) {
+    let _ = || {
+        let (([], a, _) | ([_, ..], _, a)) = x;
+        a
+    };
+}
+
+fn main() {}