Don't warn an empty pattern unreachable if we're not sure the data is valid

Author: Nadrieril

compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs

@@ -783,9 +783,6 @@ pub(super) enum Constructor<'tcx> {
 }
 
 impl<'tcx> Constructor<'tcx> {
-    pub(super) fn is_wildcard(&self) -> bool {
-        matches!(self, Wildcard)
-    }
     pub(super) fn is_non_exhaustive(&self) -> bool {
         matches!(self, NonExhaustive)
     }
@@ -971,15 +968,17 @@ pub(super) enum ConstructorSet {
 /// constructors that exist in the type but are not present in the column.
 ///
 /// More formally, if we discard wildcards from the column, this respects the following constraints:
-/// 1. the union of `present` and `missing` covers the whole type
+/// 1. the union of `present`, `missing` an `missing_empty` covers all the constructors of the type
 /// 2. each constructor in `present` is covered by something in the column
-/// 3. no constructor in `missing` is covered by anything in the column
+/// 3. no constructor in `missing` or `missing_empty` is covered by anything in the column
 /// 4. each constructor in the column is equal to the union of one or more constructors in `present`
 /// 5. `missing` does not contain empty constructors (see discussion about emptiness at the top of
 ///    the file);
-/// 6. constructors in `present` and `missing` are split for the column; in other words, they are
-///    either fully included in or fully disjoint from each constructor in the column. In other
-///    words, there are no non-trivial intersections like between `0..10` and `5..15`.
+/// 6. `missing_empty` contains only empty constructors
+/// 7. constructors in `present`, `missing` and `missing_empty` are split for the column; in other
+///    words, they are either fully included in or fully disjoint from each constructor in the
+///    column. In yet other words, there are no non-trivial intersections like between `0..10` and
+///    `5..15`.
 ///
 /// We must be particularly careful with weird constructors like `Opaque`: they're not formally part
 /// of the `ConstructorSet` for the type, yet if we forgot to include them in `present` we would be
@@ -988,6 +987,7 @@ pub(super) enum ConstructorSet {
 pub(super) struct SplitConstructorSet<'tcx> {
     pub(super) present: SmallVec<[Constructor<'tcx>; 1]>,
     pub(super) missing: Vec<Constructor<'tcx>>,
+    pub(super) missing_empty: Vec<Constructor<'tcx>>,
 }
 
 impl ConstructorSet {
@@ -1136,10 +1136,10 @@ impl ConstructorSet {
         // Constructors in `ctors`, except wildcards and opaques.
         let mut seen = Vec::new();
         for ctor in ctors.cloned() {
-            if let Constructor::Opaque(..) = ctor {
-                present.push(ctor);
-            } else if !ctor.is_wildcard() {
-                seen.push(ctor);
+            match ctor {
+                Opaque(..) => present.push(ctor),
+                Wildcard => {} // discard wildcards
+                _ => seen.push(ctor),
             }
         }
 
@@ -1247,16 +1247,24 @@ impl ConstructorSet {
                 }
             }
             ConstructorSet::NoConstructors => {
-                if !pcx.is_top_level {
-                    missing_empty.push(NonExhaustive);
-                }
+                // In a `MaybeInvalid` place even an empty pattern may be reachable. We therefore
+                // add a dummy empty constructor here, which will be ignored if the place is
+                // `ValidOnly`.
+                missing_empty.push(NonExhaustive);
             }
         }
 
-        if !pcx.cx.tcx.features().exhaustive_patterns {
-            missing.extend(missing_empty);
+        // We have now grouped all the constructors into 3 buckets: present, missing, missing_empty.
+        // In the absence of the `exhaustive_patterns` feature however, we don't count nested empty
+        // types as empty. Only non-nested `!` or `enum Foo {}` are considered empty.
+        if !pcx.cx.tcx.features().exhaustive_patterns
+            && !(pcx.is_top_level && matches!(self, Self::NoConstructors))
+        {
+            // Treat all missing constructors as nonempty.
+            missing.extend(missing_empty.drain(..));
         }
-        SplitConstructorSet { present, missing }
+
+        SplitConstructorSet { present, missing, missing_empty }
     }
 }
 

compiler/rustc_mir_build/src/thir/pattern/usefulness.rs

@@ -631,32 +631,56 @@ impl<'a, 'p, 'tcx> fmt::Debug for PatCtxt<'a, 'p, 'tcx> {
     }
 }
 
-/// In the matrix, tracks whether a given place (aka column) is known to contain a valid value or
-/// not.
+/// Serves two purposes:
+/// - in a wildcard, tracks whether the wildcard matches only valid values (i.e. is a binding `_a`)
+///     or also invalid values (i.e. is a true `_` pattern).
+/// - in the matrix, track whether a given place (aka column) is known to contain a valid value or
+///     not.
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub(super) enum ValidityConstraint {
     ValidOnly,
     MaybeInvalid,
+    /// Option for backwards compatibility: the place is not known to be valid but we allow omitting
+    /// `useful && !reachable` arms anyway.
+    MaybeInvalidButAllowOmittingArms,
 }
 
 impl ValidityConstraint {
     pub(super) fn from_bool(is_valid_only: bool) -> Self {
         if is_valid_only { ValidOnly } else { MaybeInvalid }
     }
 
+    fn allow_omitting_side_effecting_arms(self) -> Self {
+        match self {
+            MaybeInvalid | MaybeInvalidButAllowOmittingArms => MaybeInvalidButAllowOmittingArms,
+            // There are no side-effecting empty arms here, nothing to do.
+            ValidOnly => ValidOnly,
+        }
+    }
+
+    pub(super) fn is_known_valid(self) -> bool {
+        matches!(self, ValidOnly)
+    }
+    pub(super) fn allows_omitting_empty_arms(self) -> bool {
+        matches!(self, ValidOnly | MaybeInvalidButAllowOmittingArms)
+    }
+
     /// If the place has validity given by `self` and we read that the value at the place has
     /// constructor `ctor`, this computes what we can assume about the validity of the constructor
     /// fields.
     ///
     /// Pending further opsem decisions, the current behavior is: validity is preserved, except
-    /// under `&` where validity is reset to `MaybeInvalid`.
+    /// inside `&` and union fields where validity is reset to `MaybeInvalid`.
     pub(super) fn specialize<'tcx>(
         self,
         pcx: &PatCtxt<'_, '_, 'tcx>,
         ctor: &Constructor<'tcx>,
     ) -> Self {
-        // We preserve validity except when we go under a reference.
-        if matches!(ctor, Constructor::Single) && matches!(pcx.ty.kind(), ty::Ref(..)) {
+        // We preserve validity except when we go inside a reference or a union field.
+        if matches!(ctor, Constructor::Single)
+            && (matches!(pcx.ty.kind(), ty::Ref(..))
+                || matches!(pcx.ty.kind(), ty::Adt(def, ..) if def.is_union()))
+        {
             // Validity of `x: &T` does not imply validity of `*x: T`.
             MaybeInvalid
         } else {
@@ -669,7 +693,7 @@ impl fmt::Display for ValidityConstraint {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let s = match self {
             ValidOnly => "✓",
-            MaybeInvalid => "?",
+            MaybeInvalid | MaybeInvalidButAllowOmittingArms => "?",
         };
         write!(f, "{s}")
     }
@@ -1192,9 +1216,9 @@ fn compute_exhaustiveness_and_usefulness<'p, 'tcx>(
         for row in matrix.rows_mut() {
             // All rows are useful until they're not.
             row.useful = true;
+            // When there's an unguarded row, the match is exhaustive and any subsequent row is not
+            // useful.
             if !row.is_under_guard {
-                // There's an unguarded row, so the match is exhaustive, and any subsequent row is
-                // unreachable.
                 return WitnessMatrix::empty();
             }
         }
@@ -1205,26 +1229,37 @@ fn compute_exhaustiveness_and_usefulness<'p, 'tcx>(
     debug!("ty: {ty:?}");
     let pcx = &PatCtxt { cx, ty, is_top_level };
 
+    // Whether the place/column we are inspecting is known to contain valid data.
+    let place_validity = matrix.place_validity[0];
+    // For backwards compability we allow omitting some empty arms that we ideally shouldn't.
+    let place_validity = place_validity.allow_omitting_side_effecting_arms();
+
     // Analyze the constructors present in this column.
     let ctors = matrix.heads().map(|p| p.ctor());
-    let split_set = ConstructorSet::for_ty(pcx.cx, pcx.ty).split(pcx, ctors);
-
+    let split_set = ConstructorSet::for_ty(cx, ty).split(pcx, ctors);
     let all_missing = split_set.present.is_empty();
-    let always_report_all = is_top_level && !IntRange::is_integral(pcx.ty);
-    // Whether we should report "Enum::A and Enum::C are missing" or "_ is missing".
-    let report_individual_missing_ctors = always_report_all || !all_missing;
 
+    // Build the set of constructors we will specialize with. It must cover the whole type.
     let mut split_ctors = split_set.present;
-    let mut only_report_missing = false;
     if !split_set.missing.is_empty() {
         // We need to iterate over a full set of constructors, so we add `Missing` to represent the
         // missing ones. This is explained under "Constructor Splitting" at the top of this file.
         split_ctors.push(Constructor::Missing);
-        // For diagnostic purposes we choose to only report the constructors that are missing. Since
-        // `Missing` matches only the wildcard rows, it matches fewer rows than any normal
-        // constructor and is therefore guaranteed to result in more witnesses. So skipping the
-        // other constructors does not jeopardize correctness.
-        only_report_missing = true;
+    } else if !split_set.missing_empty.is_empty() && !place_validity.is_known_valid() {
+        // The missing empty constructors are reachable if the place can contain invalid data.
+        split_ctors.push(Constructor::Missing);
+    }
+
+    // Decide what constructors to report.
+    let always_report_all = is_top_level && !IntRange::is_integral(pcx.ty);
+    // Whether we should report "Enum::A and Enum::C are missing" or "_ is missing".
+    let report_individual_missing_ctors = always_report_all || !all_missing;
+    // Which constructors are considered missing. We ensure that `!missing_ctors.is_empty() =>
+    // split_ctors.contains(Missing)`. The converse usually holds except in the
+    // `MaybeInvalidButAllowOmittingArms` backwards-compatibility case.
+    let mut missing_ctors = split_set.missing;
+    if !place_validity.allows_omitting_empty_arms() {
+        missing_ctors.extend(split_set.missing_empty);
     }
 
     let mut ret = WitnessMatrix::empty();
@@ -1236,11 +1271,19 @@ fn compute_exhaustiveness_and_usefulness<'p, 'tcx>(
             compute_exhaustiveness_and_usefulness(cx, &mut spec_matrix, false)
         });
 
-        if !only_report_missing || matches!(ctor, Constructor::Missing) {
+        let counts_for_exhaustiveness = match ctor {
+            Constructor::Missing => !missing_ctors.is_empty(),
+            // If there are missing constructors we'll report those instead. Since `Missing` matches
+            // only the wildcard rows, it matches fewer rows than this constructor, and is therefore
+            // guaranteed to result in the same or more witnesses. So skipping this does not
+            // jeopardize correctness.
+            _ => missing_ctors.is_empty(),
+        };
+        if counts_for_exhaustiveness {
             // Transform witnesses for `spec_matrix` into witnesses for `matrix`.
             witnesses.apply_constructor(
                 pcx,
-                &split_set.missing,
+                &missing_ctors,
                 &ctor,
                 report_individual_missing_ctors,
             );