Auto merge of rust-lang#118308 - Nadrieril:sound-exhaustive-patterns-take-3, r=compiler-errors

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

Exhaustiveness checking used to be naive about the possibility of a place containing invalid data. This could cause it to emit an "unreachable pattern" lint on an arm that was in fact reachable, as in rust-lang#117119.

This PR fixes that. We now track whether a place that is matched on may hold invalid data. This also forced me to be extra precise about how exhaustiveness manages empty types.

Note that this now errs in the opposite direction: the following arm is truly unreachable (because the binding causes a read of the value) but not linted as such. I'd rather not recommend writing a `match ... {}` that has the implicit side-effect of loading the value. [Never patterns](rust-lang#118155) will solve this cleanly.
```rust
match union.value {
    _x => unreachable!(),
}
```

I recommend reviewing commit by commit. I went all-in on the test suite because this went through a lot of iterations and I kept everything. The bit I'm least confident in is `is_known_valid_scrutinee` in `check_match.rs`.

Fixes rust-lang#117119.

Author: bors

compiler/rustc_middle/src/ty/inhabitedness/inhabited_predicate.rs

@@ -1,3 +1,5 @@
+use smallvec::SmallVec;
+
 use crate::ty::context::TyCtxt;
 use crate::ty::{self, DefId, ParamEnv, Ty};
 
@@ -31,27 +33,31 @@ impl<'tcx> InhabitedPredicate<'tcx> {
     /// Returns true if the corresponding type is inhabited in the given
     /// `ParamEnv` and module
     pub fn apply(self, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, module_def_id: DefId) -> bool {
-        let Ok(result) = self
-            .apply_inner::<!>(tcx, param_env, &|id| Ok(tcx.is_descendant_of(module_def_id, id)));
+        let Ok(result) = self.apply_inner::<!>(tcx, param_env, &mut Default::default(), &|id| {
+            Ok(tcx.is_descendant_of(module_def_id, id))
+        });
         result
     }
 
     /// Same as `apply`, but returns `None` if self contains a module predicate
     pub fn apply_any_module(self, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> Option<bool> {
-        self.apply_inner(tcx, param_env, &|_| Err(())).ok()
+        self.apply_inner(tcx, param_env, &mut Default::default(), &|_| Err(())).ok()
     }
 
     /// Same as `apply`, but `NotInModule(_)` predicates yield `false`. That is,
     /// privately uninhabited types are considered always uninhabited.
     pub fn apply_ignore_module(self, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> bool {
-        let Ok(result) = self.apply_inner::<!>(tcx, param_env, &|_| Ok(true));
+        let Ok(result) =
+            self.apply_inner::<!>(tcx, param_env, &mut Default::default(), &|_| Ok(true));
         result
     }
 
-    fn apply_inner<E>(
+    #[instrument(level = "debug", skip(tcx, param_env, in_module), ret)]
+    fn apply_inner<E: std::fmt::Debug>(
         self,
         tcx: TyCtxt<'tcx>,
         param_env: ParamEnv<'tcx>,
+        eval_stack: &mut SmallVec<[Ty<'tcx>; 1]>, // for cycle detection
         in_module: &impl Fn(DefId) -> Result<bool, E>,
     ) -> Result<bool, E> {
         match self {
@@ -71,11 +77,25 @@ impl<'tcx> InhabitedPredicate<'tcx> {
                 match normalized_pred {
                     // We don't have more information than we started with, so consider inhabited.
                     Self::GenericType(_) => Ok(true),
-                    pred => pred.apply_inner(tcx, param_env, in_module),
+                    pred => {
+                        // A type which is cyclic when monomorphized can happen here since the
+                        // layout error would only trigger later. See e.g. `tests/ui/sized/recursive-type-2.rs`.
+                        if eval_stack.contains(&t) {
+                            return Ok(true); // Recover; this will error later.
+                        }
+                        eval_stack.push(t);
+                        let ret = pred.apply_inner(tcx, param_env, eval_stack, in_module);
+                        eval_stack.pop();
+                        ret
+                    }
                 }
             }
-            Self::And([a, b]) => try_and(a, b, |x| x.apply_inner(tcx, param_env, in_module)),
-            Self::Or([a, b]) => try_or(a, b, |x| x.apply_inner(tcx, param_env, in_module)),
+            Self::And([a, b]) => {
+                try_and(a, b, |x| x.apply_inner(tcx, param_env, eval_stack, in_module))
+            }
+            Self::Or([a, b]) => {
+                try_or(a, b, |x| x.apply_inner(tcx, param_env, eval_stack, in_module))
+            }
         }
     }
 
@@ -197,7 +217,7 @@ impl<'tcx> InhabitedPredicate<'tcx> {
 
 // this is basically like `f(a)? && f(b)?` but different in the case of
 // `Ok(false) && Err(_) -> Ok(false)`
-fn try_and<T, E>(a: T, b: T, f: impl Fn(T) -> Result<bool, E>) -> Result<bool, E> {
+fn try_and<T, E>(a: T, b: T, mut f: impl FnMut(T) -> Result<bool, E>) -> Result<bool, E> {
     let a = f(a);
     if matches!(a, Ok(false)) {
         return Ok(false);
@@ -209,7 +229,7 @@ fn try_and<T, E>(a: T, b: T, f: impl Fn(T) -> Result<bool, E>) -> Result<bool, E
     }
 }
 
-fn try_or<T, E>(a: T, b: T, f: impl Fn(T) -> Result<bool, E>) -> Result<bool, E> {
+fn try_or<T, E>(a: T, b: T, mut f: impl FnMut(T) -> Result<bool, E>) -> Result<bool, E> {
     let a = f(a);
     if matches!(a, Ok(true)) {
         return Ok(true);

compiler/rustc_middle/src/ty/inhabitedness/mod.rs

@@ -103,6 +103,7 @@ impl<'tcx> VariantDef {
 }
 
 impl<'tcx> Ty<'tcx> {
+    #[instrument(level = "debug", skip(tcx), ret)]
     pub fn inhabited_predicate(self, tcx: TyCtxt<'tcx>) -> InhabitedPredicate<'tcx> {
         match self.kind() {
             // For now, unions are always considered inhabited

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

@@ -1,6 +1,6 @@
 use super::deconstruct_pat::{Constructor, DeconstructedPat, WitnessPat};
 use super::usefulness::{
-    compute_match_usefulness, MatchArm, MatchCheckCtxt, Reachability, UsefulnessReport,
+    compute_match_usefulness, MatchArm, MatchCheckCtxt, Usefulness, UsefulnessReport,
 };
 
 use crate::errors::*;
@@ -42,7 +42,7 @@ pub(crate) fn check_match(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Result<(), Err
 
     for param in thir.params.iter() {
         if let Some(box ref pattern) = param.pat {
-            visitor.check_binding_is_irrefutable(pattern, "function argument", None);
+            visitor.check_binding_is_irrefutable(pattern, "function argument", None, None);
         }
     }
     visitor.error
@@ -254,10 +254,11 @@ impl<'thir, 'p, 'tcx> MatchVisitor<'thir, 'p, 'tcx> {
                 self.with_lint_level(lint_level, |this| this.visit_land_rhs(&this.thir[value]))
             }
             ExprKind::Let { box ref pat, expr } => {
+                let expr = &self.thir()[expr];
                 self.with_let_source(LetSource::None, |this| {
-                    this.visit_expr(&this.thir()[expr]);
+                    this.visit_expr(expr);
                 });
-                Ok(Some((ex.span, self.is_let_irrefutable(pat)?)))
+                Ok(Some((ex.span, self.is_let_irrefutable(pat, Some(expr))?)))
             }
             _ => {
                 self.with_let_source(LetSource::None, |this| {
@@ -287,35 +288,114 @@ impl<'thir, 'p, 'tcx> MatchVisitor<'thir, 'p, 'tcx> {
         }
     }
 
+    /// Inspects the match scrutinee expression to determine whether the place it evaluates to may
+    /// hold invalid data.
+    fn is_known_valid_scrutinee(&self, scrutinee: &Expr<'tcx>) -> bool {
+        use ExprKind::*;
+        match &scrutinee.kind {
+            // Both pointers and references can validly point to a place with invalid data.
+            Deref { .. } => false,
+            // Inherit validity of the parent place, unless the parent is an union.
+            Field { lhs, .. } => {
+                let lhs = &self.thir()[*lhs];
+                match lhs.ty.kind() {
+                    ty::Adt(def, _) if def.is_union() => false,
+                    _ => self.is_known_valid_scrutinee(lhs),
+                }
+            }
+            // Essentially a field access.
+            Index { lhs, .. } => {
+                let lhs = &self.thir()[*lhs];
+                self.is_known_valid_scrutinee(lhs)
+            }
+
+            // No-op.
+            Scope { value, .. } => self.is_known_valid_scrutinee(&self.thir()[*value]),
+
+            // Casts don't cause a load.
+            NeverToAny { source }
+            | Cast { source }
+            | Use { source }
+            | PointerCoercion { source, .. }
+            | PlaceTypeAscription { source, .. }
+            | ValueTypeAscription { source, .. } => {
+                self.is_known_valid_scrutinee(&self.thir()[*source])
+            }
+
+            // These diverge.
+            Become { .. } | Break { .. } | Continue { .. } | Return { .. } => true,
+
+            // These are statements that evaluate to `()`.
+            Assign { .. } | AssignOp { .. } | InlineAsm { .. } | Let { .. } => true,
+
+            // These evaluate to a value.
+            AddressOf { .. }
+            | Adt { .. }
+            | Array { .. }
+            | Binary { .. }
+            | Block { .. }
+            | Borrow { .. }
+            | Box { .. }
+            | Call { .. }
+            | Closure { .. }
+            | ConstBlock { .. }
+            | ConstParam { .. }
+            | If { .. }
+            | Literal { .. }
+            | LogicalOp { .. }
+            | Loop { .. }
+            | Match { .. }
+            | NamedConst { .. }
+            | NonHirLiteral { .. }
+            | OffsetOf { .. }
+            | Repeat { .. }
+            | StaticRef { .. }
+            | ThreadLocalRef { .. }
+            | Tuple { .. }
+            | Unary { .. }
+            | UpvarRef { .. }
+            | VarRef { .. }
+            | ZstLiteral { .. }
+            | Yield { .. } => true,
+        }
+    }
+
     fn new_cx(
         &self,
         refutability: RefutableFlag,
-        match_span: Option<Span>,
+        whole_match_span: Option<Span>,
+        scrutinee: Option<&Expr<'tcx>>,
         scrut_span: Span,
     ) -> MatchCheckCtxt<'p, 'tcx> {
         let refutable = match refutability {
             Irrefutable => false,
             Refutable => true,
         };
+        // If we don't have a scrutinee we're either a function parameter or a `let x;`. Both cases
+        // require validity.
+        let known_valid_scrutinee =
+            scrutinee.map(|scrut| self.is_known_valid_scrutinee(scrut)).unwrap_or(true);
         MatchCheckCtxt {
             tcx: self.tcx,
             param_env: self.param_env,
             module: self.tcx.parent_module(self.lint_level).to_def_id(),
             pattern_arena: self.pattern_arena,
             match_lint_level: self.lint_level,
-            match_span,
+            whole_match_span,
             scrut_span,
             refutable,
+            known_valid_scrutinee,
         }
     }
 
     #[instrument(level = "trace", skip(self))]
     fn check_let(&mut self, pat: &Pat<'tcx>, scrutinee: Option<ExprId>, span: Span) {
         assert!(self.let_source != LetSource::None);
+        let scrut = scrutinee.map(|id| &self.thir[id]);
         if let LetSource::PlainLet = self.let_source {
-            self.check_binding_is_irrefutable(pat, "local binding", Some(span))
+            self.check_binding_is_irrefutable(pat, "local binding", scrut, Some(span))
         } else {
-            let Ok(refutability) = self.is_let_irrefutable(pat) else { return };
+            let Ok(refutability) = self.is_let_irrefutable(pat, scrut) else { return };
             if matches!(refutability, Irrefutable) {
                 report_irrefutable_let_patterns(
                     self.tcx,
@@ -336,7 +416,7 @@ impl<'thir, 'p, 'tcx> MatchVisitor<'thir, 'p, 'tcx> {
         expr_span: Span,
     ) {
         let scrut = &self.thir[scrut];
-        let cx = self.new_cx(Refutable, Some(expr_span), scrut.span);
+        let cx = self.new_cx(Refutable, Some(expr_span), Some(scrut), scrut.span);
 
         let mut tarms = Vec::with_capacity(arms.len());
         for &arm in arms {
@@ -377,7 +457,12 @@ impl<'thir, 'p, 'tcx> MatchVisitor<'thir, 'p, 'tcx> {
                 debug_assert_eq!(pat.span.desugaring_kind(), Some(DesugaringKind::ForLoop));
                 let PatKind::Variant { ref subpatterns, .. } = pat.kind else { bug!() };
                 let [pat_field] = &subpatterns[..] else { bug!() };
-                self.check_binding_is_irrefutable(&pat_field.pattern, "`for` loop binding", None);
+                self.check_binding_is_irrefutable(
+                    &pat_field.pattern,
+                    "`for` loop binding",
+                    None,
+                    None,
+                );
             } else {
                 self.error = Err(report_non_exhaustive_match(
                     &cx, self.thir, scrut_ty, scrut.span, witnesses, arms, expr_span,
@@ -457,16 +542,21 @@ impl<'thir, 'p, 'tcx> MatchVisitor<'thir, 'p, 'tcx> {
         &mut self,
         pat: &Pat<'tcx>,
         refutability: RefutableFlag,
+        scrut: Option<&Expr<'tcx>>,
     ) -> Result<(MatchCheckCtxt<'p, 'tcx>, UsefulnessReport<'p, 'tcx>), ErrorGuaranteed> {
-        let cx = self.new_cx(refutability, None, pat.span);
+        let cx = self.new_cx(refutability, None, scrut, pat.span);
         let pat = self.lower_pattern(&cx, pat)?;
         let arms = [MatchArm { pat, hir_id: self.lint_level, has_guard: false }];
         let report = compute_match_usefulness(&cx, &arms, pat.ty());
         Ok((cx, report))
     }
 
-    fn is_let_irrefutable(&mut self, pat: &Pat<'tcx>) -> Result<RefutableFlag, ErrorGuaranteed> {
-        let (cx, report) = self.analyze_binding(pat, Refutable)?;
+    fn is_let_irrefutable(
+        &mut self,
+        pat: &Pat<'tcx>,
+        scrut: Option<&Expr<'tcx>>,
+    ) -> Result<RefutableFlag, ErrorGuaranteed> {
+        let (cx, report) = self.analyze_binding(pat, Refutable, scrut)?;
         // Report if the pattern is unreachable, which can only occur when the type is uninhabited.
         // This also reports unreachable sub-patterns.
         report_arm_reachability(&cx, &report);
@@ -476,10 +566,16 @@ impl<'thir, 'p, 'tcx> MatchVisitor<'thir, 'p, 'tcx> {
     }
 
     #[instrument(level = "trace", skip(self))]
-    fn check_binding_is_irrefutable(&mut self, pat: &Pat<'tcx>, origin: &str, sp: Option<Span>) {
+    fn check_binding_is_irrefutable(
+        &mut self,
+        pat: &Pat<'tcx>,
+        origin: &str,
+        scrut: Option<&Expr<'tcx>>,
+        sp: Option<Span>,
+    ) {
         let pattern_ty = pat.ty;
 
-        let Ok((cx, report)) = self.analyze_binding(pat, Irrefutable) else { return };
+        let Ok((cx, report)) = self.analyze_binding(pat, Irrefutable, scrut) else { return };
         let witnesses = report.non_exhaustiveness_witnesses;
         if witnesses.is_empty() {
             // The pattern is irrefutable.
@@ -749,18 +845,18 @@ fn report_arm_reachability<'p, 'tcx>(
         );
     };
 
-    use Reachability::*;
+    use Usefulness::*;
     let mut catchall = None;
     for (arm, is_useful) in report.arm_usefulness.iter() {
         match is_useful {
-            Unreachable => report_unreachable_pattern(arm.pat.span(), arm.hir_id, catchall),
-            Reachable(unreachables) if unreachables.is_empty() => {}
-            // The arm is reachable, but contains unreachable subpatterns (from or-patterns).
-            Reachable(unreachables) => {
-                let mut unreachables = unreachables.clone();
+            Redundant => report_unreachable_pattern(arm.pat.span(), arm.hir_id, catchall),
+            Useful(redundant_spans) if redundant_spans.is_empty() => {}
+            // The arm is reachable, but contains redundant subpatterns (from or-patterns).
+            Useful(redundant_spans) => {
+                let mut redundant_spans = redundant_spans.clone();
                 // Emit lints in the order in which they occur in the file.
-                unreachables.sort_unstable();
-                for span in unreachables {
+                redundant_spans.sort_unstable();
+                for span in redundant_spans {
                     report_unreachable_pattern(span, arm.hir_id, None);
                 }
             }