Auto merge of #16303 - rosefromthedead:non-exhaustive-let, r=Veykril

bors · bors · commit ff8fe522e899 · 2024-02-19T11:51:06.000Z
feat: add non-exhaustive-let diagnostic

I want this to have a quickfix to add an else branch but I couldn't figure out how to do that, so here's the diagnostic on its own. It pretends a `let` is a match with one arm, and asks the match checking whether that match would be exhaustive.

Previously the pattern was checked based on its own type, but that was causing a panic in `match_check` (while processing e.g. `crates/hir/src/lib.rs`) so I changed it to use the initialiser's type instead, to align with the checking of actual match expressions. I think the panic can still happen, but I hear that `match_check` is going to be updated to a new version from rustc, so I'm posting this now in the hopes that the panic will magically go away when that happens.
diff --git a/crates/hir-ty/src/diagnostics/expr.rs b/crates/hir-ty/src/diagnostics/expr.rs
@@ -12,6 +12,7 @@ use hir_expand::name;
 use itertools::Itertools;
 use rustc_hash::FxHashSet;
 use rustc_pattern_analysis::usefulness::{compute_match_usefulness, ValidityConstraint};
+use tracing::debug;
 use triomphe::Arc;
 use typed_arena::Arena;
 
@@ -44,6 +45,10 @@ pub enum BodyValidationDiagnostic {
         match_expr: ExprId,
         uncovered_patterns: String,
     },
+    NonExhaustiveLet {
+        pat: PatId,
+        uncovered_patterns: String,
+    },
     RemoveTrailingReturn {
         return_expr: ExprId,
     },
@@ -57,26 +62,25 @@ impl BodyValidationDiagnostic {
         let _p =
             tracing::span!(tracing::Level::INFO, "BodyValidationDiagnostic::collect").entered();
         let infer = db.infer(owner);
-        let mut validator = ExprValidator::new(owner, infer);
+        let body = db.body(owner);
+        let mut validator = ExprValidator { owner, body, infer, diagnostics: Vec::new() };
         validator.validate_body(db);
         validator.diagnostics
     }
 }
 
 struct ExprValidator {
     owner: DefWithBodyId,
+    body: Arc<Body>,
     infer: Arc<InferenceResult>,
-    pub(super) diagnostics: Vec<BodyValidationDiagnostic>,
+    diagnostics: Vec<BodyValidationDiagnostic>,
 }
 
 impl ExprValidator {
-    fn new(owner: DefWithBodyId, infer: Arc<InferenceResult>) -> ExprValidator {
-        ExprValidator { owner, infer, diagnostics: Vec::new() }
-    }
-
     fn validate_body(&mut self, db: &dyn HirDatabase) {
-        let body = db.body(self.owner);
         let mut filter_map_next_checker = None;
+        // we'll pass &mut self while iterating over body.exprs, so they need to be disjoint
+        let body = Arc::clone(&self.body);
 
         if matches!(self.owner, DefWithBodyId::FunctionId(_)) {
             self.check_for_trailing_return(body.body_expr, &body);
@@ -106,6 +110,9 @@ impl ExprValidator {
                 Expr::If { .. } => {
                     self.check_for_unnecessary_else(id, expr, &body);
                 }
+                Expr::Block { .. } => {
+                    self.validate_block(db, expr);
+                }
                 _ => {}
             }
         }
@@ -162,8 +169,6 @@ impl ExprValidator {
         arms: &[MatchArm],
         db: &dyn HirDatabase,
     ) {
-        let body = db.body(self.owner);
-
         let scrut_ty = &self.infer[scrutinee_expr];
         if scrut_ty.is_unknown() {
             return;
@@ -191,12 +196,12 @@ impl ExprValidator {
                         .as_reference()
                         .map(|(match_expr_ty, ..)| match_expr_ty == pat_ty)
                         .unwrap_or(false))
-                    && types_of_subpatterns_do_match(arm.pat, &body, &self.infer)
+                    && types_of_subpatterns_do_match(arm.pat, &self.body, &self.infer)
                 {
                     // If we had a NotUsefulMatchArm diagnostic, we could
                     // check the usefulness of each pattern as we added it
                     // to the matrix here.
-                    let pat = self.lower_pattern(&cx, arm.pat, db, &body, &mut has_lowering_errors);
+                    let pat = self.lower_pattern(&cx, arm.pat, db, &mut has_lowering_errors);
                     let m_arm = pat_analysis::MatchArm {
                         pat: pattern_arena.alloc(pat),
                         has_guard: arm.guard.is_some(),
@@ -234,20 +239,63 @@ impl ExprValidator {
         if !witnesses.is_empty() {
             self.diagnostics.push(BodyValidationDiagnostic::MissingMatchArms {
                 match_expr,
-                uncovered_patterns: missing_match_arms(&cx, scrut_ty, witnesses, arms),
+                uncovered_patterns: missing_match_arms(&cx, scrut_ty, witnesses, m_arms.is_empty()),
             });
         }
     }
 
+    fn validate_block(&mut self, db: &dyn HirDatabase, expr: &Expr) {
+        let Expr::Block { statements, .. } = expr else { return };
+        let pattern_arena = Arena::new();
+        let cx = MatchCheckCtx::new(self.owner.module(db.upcast()), self.owner, db);
+        for stmt in &**statements {
+            let &Statement::Let { pat, initializer, else_branch: None, .. } = stmt else {
+                continue;
+            };
+            let Some(initializer) = initializer else { continue };
+            let ty = &self.infer[initializer];
+
+            let mut have_errors = false;
+            let deconstructed_pat = self.lower_pattern(&cx, pat, db, &mut have_errors);
+            let match_arm = rustc_pattern_analysis::MatchArm {
+                pat: pattern_arena.alloc(deconstructed_pat),
+                has_guard: false,
+                arm_data: (),
+            };
+            if have_errors {
+                continue;
+            }
+
+            let report = match compute_match_usefulness(
+                &cx,
+                &[match_arm],
+                ty.clone(),
+                ValidityConstraint::ValidOnly,
+            ) {
+                Ok(v) => v,
+                Err(e) => {
+                    debug!(?e, "match usefulness error");
+                    continue;
+                }
+            };
+            let witnesses = report.non_exhaustiveness_witnesses;
+            if !witnesses.is_empty() {
+                self.diagnostics.push(BodyValidationDiagnostic::NonExhaustiveLet {
+                    pat,
+                    uncovered_patterns: missing_match_arms(&cx, ty, witnesses, false),
+                });
+            }
+        }
+    }
+
     fn lower_pattern<'p>(
         &self,
         cx: &MatchCheckCtx<'p>,
         pat: PatId,
         db: &dyn HirDatabase,
-        body: &Body,
         have_errors: &mut bool,
     ) -> DeconstructedPat<'p> {
-        let mut patcx = match_check::PatCtxt::new(db, &self.infer, body);
+        let mut patcx = match_check::PatCtxt::new(db, &self.infer, &self.body);
         let pattern = patcx.lower_pattern(pat);
         let pattern = cx.lower_pat(&pattern);
         if !patcx.errors.is_empty() {
@@ -448,7 +496,7 @@ fn missing_match_arms<'p>(
     cx: &MatchCheckCtx<'p>,
     scrut_ty: &Ty,
     witnesses: Vec<WitnessPat<'p>>,
-    arms: &[MatchArm],
+    arms_is_empty: bool,
 ) -> String {
     struct DisplayWitness<'a, 'p>(&'a WitnessPat<'p>, &'a MatchCheckCtx<'p>);
     impl fmt::Display for DisplayWitness<'_, '_> {
@@ -463,7 +511,7 @@ fn missing_match_arms<'p>(
         Some((AdtId::EnumId(e), _)) => !cx.db.enum_data(e).variants.is_empty(),
         _ => false,
     };
-    if arms.is_empty() && !non_empty_enum {
+    if arms_is_empty && !non_empty_enum {
         format!("type `{}` is non-empty", scrut_ty.display(cx.db))
     } else {
         let pat_display = |witness| DisplayWitness(witness, cx);
diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs
@@ -64,6 +64,7 @@ diagnostics![
     MissingUnsafe,
     MovedOutOfRef,
     NeedMut,
+    NonExhaustiveLet,
     NoSuchField,
     PrivateAssocItem,
     PrivateField,
@@ -280,6 +281,12 @@ pub struct MissingMatchArms {
     pub uncovered_patterns: String,
 }
 
+#[derive(Debug)]
+pub struct NonExhaustiveLet {
+    pub pat: InFile<AstPtr<ast::Pat>>,
+    pub uncovered_patterns: String,
+}
+
 #[derive(Debug)]
 pub struct TypeMismatch {
     pub expr_or_pat: InFile<AstPtr<Either<ast::Expr, ast::Pat>>>,
@@ -456,6 +463,22 @@ impl AnyDiagnostic {
                     Err(SyntheticSyntax) => (),
                 }
             }
+            BodyValidationDiagnostic::NonExhaustiveLet { pat, uncovered_patterns } => {
+                match source_map.pat_syntax(pat) {
+                    Ok(source_ptr) => {
+                        if let Some(ast_pat) = source_ptr.value.cast::<ast::Pat>() {
+                            return Some(
+                                NonExhaustiveLet {
+                                    pat: InFile::new(source_ptr.file_id, ast_pat),
+                                    uncovered_patterns,
+                                }
+                                .into(),
+                            );
+                        }
+                    }
+                    Err(SyntheticSyntax) => {}
+                }
+            }
             BodyValidationDiagnostic::RemoveTrailingReturn { return_expr } => {
                 if let Ok(source_ptr) = source_map.expr_syntax(return_expr) {
                     // Filters out desugared return expressions (e.g. desugared try operators).
diff --git a/crates/ide-diagnostics/src/handlers/mutability_errors.rs b/crates/ide-diagnostics/src/handlers/mutability_errors.rs
@@ -817,7 +817,7 @@ fn f() {
 //- minicore: option
 fn f(_: i32) {}
 fn main() {
-    let ((Some(mut x), None) | (_, Some(mut x))) = (None, Some(7));
+    let ((Some(mut x), None) | (_, Some(mut x))) = (None, Some(7)) else { return };
              //^^^^^ 💡 warn: variable does not need to be mutable
     f(x);
 }
diff --git a/crates/ide-diagnostics/src/handlers/non_exhaustive_let.rs b/crates/ide-diagnostics/src/handlers/non_exhaustive_let.rs
@@ -0,0 +1,47 @@
+use crate::{Diagnostic, DiagnosticCode, DiagnosticsContext};
+
+// Diagnostic: non-exhaustive-let
+//
+// This diagnostic is triggered if a `let` statement without an `else` branch has a non-exhaustive
+// pattern.
+pub(crate) fn non_exhaustive_let(
+    ctx: &DiagnosticsContext<'_>,
+    d: &hir::NonExhaustiveLet,
+) -> Diagnostic {
+    Diagnostic::new_with_syntax_node_ptr(
+        ctx,
+        DiagnosticCode::RustcHardError("E0005"),
+        format!("non-exhaustive pattern: {}", d.uncovered_patterns),
+        d.pat.map(Into::into),
+    )
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::tests::check_diagnostics;
+
+    #[test]
+    fn option_nonexhaustive() {
+        check_diagnostics(
+            r#"
+//- minicore: option
+fn main() {
+    let None = Some(5);
+      //^^^^ error: non-exhaustive pattern: `Some(_)` not covered
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn option_exhaustive() {
+        check_diagnostics(
+            r#"
+//- minicore: option
+fn main() {
+    let Some(_) | None = Some(5);
+}
+"#,
+        );
+    }
+}
diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs
@@ -41,6 +41,7 @@ mod handlers {
     pub(crate) mod moved_out_of_ref;
     pub(crate) mod mutability_errors;
     pub(crate) mod no_such_field;
+    pub(crate) mod non_exhaustive_let;
     pub(crate) mod private_assoc_item;
     pub(crate) mod private_field;
     pub(crate) mod remove_trailing_return;
@@ -359,6 +360,7 @@ pub fn diagnostics(
             AnyDiagnostic::MissingUnsafe(d) => handlers::missing_unsafe::missing_unsafe(&ctx, &d),
             AnyDiagnostic::MovedOutOfRef(d) => handlers::moved_out_of_ref::moved_out_of_ref(&ctx, &d),
             AnyDiagnostic::NeedMut(d) => handlers::mutability_errors::need_mut(&ctx, &d),
+            AnyDiagnostic::NonExhaustiveLet(d) => handlers::non_exhaustive_let::non_exhaustive_let(&ctx, &d),
             AnyDiagnostic::NoSuchField(d) => handlers::no_such_field::no_such_field(&ctx, &d),
             AnyDiagnostic::PrivateAssocItem(d) => handlers::private_assoc_item::private_assoc_item(&ctx, &d),
             AnyDiagnostic::PrivateField(d) => handlers::private_field::private_field(&ctx, &d),

Original file line number	Diff line number	Diff line change
`@@ -817,7 +817,7 @@ fn f() {`
`817`	`817`	`//- minicore: option`
`818`	`818`	`fn f(_: i32) {}`
`819`	`819`	`fn main() {`
`820`		`- let ((Some(mut x), None) \| (_, Some(mut x))) = (None, Some(7));`
	`820`	`+ let ((Some(mut x), None) \| (_, Some(mut x))) = (None, Some(7)) else { return };`
`821`	`821`	`//^^^^^ 💡 warn: variable does not need to be mutable`
`822`	`822`	`f(x);`
`823`	`823`	`}`