From 8c378af72117e92bc894fd4a79e978ef0d1c0cc7 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Tue, 24 Mar 2020 04:40:58 -0700 Subject: [PATCH] missing match arms diagnostic --- Cargo.lock | 1 + crates/ra_hir/src/diagnostics.rs | 2 +- crates/ra_hir_ty/Cargo.toml | 1 + crates/ra_hir_ty/src/_match.rs | 944 ++++++++++++++++++++++++++++ crates/ra_hir_ty/src/diagnostics.rs | 20 +- crates/ra_hir_ty/src/expr.rs | 53 +- crates/ra_hir_ty/src/infer/pat.rs | 6 +- crates/ra_hir_ty/src/infer/path.rs | 12 +- crates/ra_hir_ty/src/lib.rs | 1 + crates/ra_hir_ty/src/test_db.rs | 6 +- crates/ra_hir_ty/src/tests.rs | 3 +- crates/ra_ide/src/diagnostics.rs | 10 +- 12 files changed, 1048 insertions(+), 11 deletions(-) create mode 100644 crates/ra_hir_ty/src/_match.rs diff --git a/Cargo.lock b/Cargo.lock index 91a57bf796..eb9824218f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -995,6 +995,7 @@ dependencies = [ "ra_prof", "ra_syntax", "rustc-hash", + "smallvec", "stdx", "test_utils", ] diff --git a/crates/ra_hir/src/diagnostics.rs b/crates/ra_hir/src/diagnostics.rs index a9040ea3d5..c82883d0c1 100644 --- a/crates/ra_hir/src/diagnostics.rs +++ b/crates/ra_hir/src/diagnostics.rs @@ -1,4 +1,4 @@ //! FIXME: write short doc here pub use hir_def::diagnostics::UnresolvedModule; pub use hir_expand::diagnostics::{AstDiagnostic, Diagnostic, DiagnosticSink}; -pub use hir_ty::diagnostics::{MissingFields, MissingOkInTailExpr, NoSuchField}; +pub use hir_ty::diagnostics::{MissingFields, MissingMatchArms, MissingOkInTailExpr, NoSuchField}; diff --git a/crates/ra_hir_ty/Cargo.toml b/crates/ra_hir_ty/Cargo.toml index 45be08430a..9a4a7aa6f7 100644 --- a/crates/ra_hir_ty/Cargo.toml +++ b/crates/ra_hir_ty/Cargo.toml @@ -9,6 +9,7 @@ doctest = false [dependencies] arrayvec = "0.5.1" +smallvec = "1.2.0" ena = "0.13.1" log = "0.4.8" rustc-hash = "1.1.0" diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs new file mode 100644 index 0000000000..ac66dd415b --- /dev/null +++ b/crates/ra_hir_ty/src/_match.rs @@ -0,0 +1,944 @@ +//! This module implements match statement exhaustiveness checking and usefulness checking +//! for match arms. +//! +//! It is modeled on the rustc module `librustc_mir_build::hair::pattern::_match`, which +//! contains very detailed documentation about the match checking algorithm. +use std::sync::Arc; + +use smallvec::{smallvec, SmallVec}; + +use crate::{ + db::HirDatabase, + expr::{Body, Expr, Literal, Pat, PatId}, + InferenceResult, +}; +use hir_def::{adt::VariantData, EnumVariantId, VariantId}; + +#[derive(Debug, Clone, Copy)] +enum PatIdOrWild { + PatId(PatId), + Wild, +} + +impl PatIdOrWild { + fn as_pat(self, cx: &MatchCheckCtx) -> Pat { + match self { + PatIdOrWild::PatId(id) => cx.body.pats[id].clone(), + PatIdOrWild::Wild => Pat::Wild, + } + } + + fn as_id(self) -> Option { + match self { + PatIdOrWild::PatId(id) => Some(id), + PatIdOrWild::Wild => None, + } + } +} + +impl From for PatIdOrWild { + fn from(pat_id: PatId) -> Self { + Self::PatId(pat_id) + } +} + +type PatStackInner = SmallVec<[PatIdOrWild; 2]>; +#[derive(Debug)] +pub(crate) struct PatStack(PatStackInner); + +impl PatStack { + pub(crate) fn from_pattern(pat_id: PatId) -> PatStack { + Self(smallvec!(pat_id.into())) + } + + pub(crate) fn from_wild() -> PatStack { + Self(smallvec!(PatIdOrWild::Wild)) + } + + fn from_slice(slice: &[PatIdOrWild]) -> PatStack { + Self(SmallVec::from_slice(slice)) + } + + fn from_vec(v: PatStackInner) -> PatStack { + Self(v) + } + + fn is_empty(&self) -> bool { + self.0.is_empty() + } + + fn head(&self) -> PatIdOrWild { + self.0[0] + } + + fn get_head(&self) -> Option { + self.0.first().copied() + } + + fn to_tail(&self) -> PatStack { + Self::from_slice(&self.0[1..]) + } + + fn replace_head_with(&self, pat_ids: &[PatId]) -> PatStack { + let mut patterns: PatStackInner = smallvec![]; + for pat in pat_ids { + patterns.push((*pat).into()); + } + for pat in &self.0[1..] { + patterns.push(*pat); + } + PatStack::from_vec(patterns) + } + + // Computes `D(self)`. + fn specialize_wildcard(&self, cx: &MatchCheckCtx) -> Option { + if matches!(self.head().as_pat(cx), Pat::Wild) { + Some(self.to_tail()) + } else { + None + } + } + + // Computes `S(constructor, self)`. + fn specialize_constructor( + &self, + cx: &MatchCheckCtx, + constructor: &Constructor, + ) -> Option { + match (self.head().as_pat(cx), constructor) { + (Pat::Tuple(ref pat_ids), Constructor::Tuple { arity }) => { + if pat_ids.len() != *arity { + return None; + } + + Some(self.replace_head_with(pat_ids)) + } + (Pat::Lit(_), Constructor::Bool(_)) => { + // for now we only support bool literals + Some(self.to_tail()) + } + (Pat::Wild, constructor) => Some(self.expand_wildcard(cx, constructor)), + (Pat::Path(_), Constructor::Enum(constructor)) => { + let pat_id = self.head().as_id().expect("we know this isn't a wild"); + if !enum_variant_matches(cx, pat_id, *constructor) { + return None; + } + // enums with no associated data become `Pat::Path` + Some(self.to_tail()) + } + (Pat::TupleStruct { args: ref pat_ids, .. }, Constructor::Enum(constructor)) => { + let pat_id = self.head().as_id().expect("we know this isn't a wild"); + if !enum_variant_matches(cx, pat_id, *constructor) { + return None; + } + + Some(self.replace_head_with(pat_ids)) + } + (Pat::Or(_), _) => unreachable!("we desugar or patterns so this should never happen"), + (a, b) => unimplemented!("{:?}, {:?}", a, b), + } + } + + fn expand_wildcard(&self, cx: &MatchCheckCtx, constructor: &Constructor) -> PatStack { + assert_eq!( + Pat::Wild, + self.head().as_pat(cx), + "expand_wildcard must only be called on PatStack with wild at head", + ); + + let mut patterns: PatStackInner = smallvec![]; + let arity = match constructor { + Constructor::Bool(_) => 0, + Constructor::Tuple { arity } => *arity, + Constructor::Enum(e) => { + match cx.db.enum_data(e.parent).variants[e.local_id].variant_data.as_ref() { + VariantData::Tuple(struct_field_data) => struct_field_data.len(), + VariantData::Unit => 0, + x => unimplemented!("{:?}", x), + } + } + }; + + for _ in 0..arity { + patterns.push(PatIdOrWild::Wild); + } + + for pat in &self.0[1..] { + patterns.push(*pat); + } + + PatStack::from_vec(patterns) + } +} + +#[derive(Debug)] +pub(crate) struct Matrix(Vec); + +impl Matrix { + pub(crate) fn empty() -> Self { + Self(vec![]) + } + + pub(crate) fn push(&mut self, cx: &MatchCheckCtx, row: PatStack) { + // if the pattern is an or pattern it should be expanded + if let Some(Pat::Or(pat_ids)) = row.get_head().map(|pat_id| pat_id.as_pat(cx)) { + for pat_id in pat_ids { + self.0.push(PatStack::from_pattern(pat_id)); + } + } else { + self.0.push(row); + } + } + + fn is_empty(&self) -> bool { + self.0.is_empty() + } + + fn heads(&self) -> Vec { + self.0.iter().map(|p| p.head()).collect() + } + + // Computes `D(self)`. + fn specialize_wildcard(&self, cx: &MatchCheckCtx) -> Self { + Self::collect(cx, self.0.iter().filter_map(|r| r.specialize_wildcard(cx))) + } + + // Computes `S(constructor, self)`. + fn specialize_constructor(&self, cx: &MatchCheckCtx, constructor: &Constructor) -> Self { + Self::collect(cx, self.0.iter().filter_map(|r| r.specialize_constructor(cx, constructor))) + } + + fn collect>(cx: &MatchCheckCtx, iter: T) -> Self { + let mut matrix = Matrix::empty(); + + for pat in iter { + // using push ensures we expand or-patterns + matrix.push(cx, pat); + } + + matrix + } +} + +#[derive(Clone, Debug, PartialEq)] +pub enum Usefulness { + Useful, + NotUseful, +} + +pub struct MatchCheckCtx<'a> { + pub body: Arc, + pub match_expr: &'a Expr, + pub infer: Arc, + pub db: &'a dyn HirDatabase, +} + +// see src/librustc_mir_build/hair/pattern/_match.rs +// It seems the rustc version of this method is able to assume that all the match arm +// patterns are valid (they are valid given a particular match expression), but I +// don't think we can make that assumption here. How should that be handled? +// +// Perhaps check that validity before passing the patterns into this method? +pub(crate) fn is_useful(cx: &MatchCheckCtx, matrix: &Matrix, v: &PatStack) -> Usefulness { + dbg!(matrix); + dbg!(v); + if v.is_empty() { + if matrix.is_empty() { + return Usefulness::Useful; + } else { + return Usefulness::NotUseful; + } + } + + if let Pat::Or(pat_ids) = v.head().as_pat(cx) { + let any_useful = pat_ids.iter().any(|&pat_id| { + let v = PatStack::from_pattern(pat_id); + + is_useful(cx, matrix, &v) == Usefulness::Useful + }); + + return if any_useful { Usefulness::Useful } else { Usefulness::NotUseful }; + } + + if let Some(constructor) = pat_constructor(cx, v.head()) { + let matrix = matrix.specialize_constructor(&cx, &constructor); + let v = v.specialize_constructor(&cx, &constructor).expect("todo handle this case"); + + is_useful(&cx, &matrix, &v) + } else { + dbg!("expanding wildcard"); + // expanding wildcard + let used_constructors: Vec = + matrix.heads().iter().filter_map(|&p| pat_constructor(cx, p)).collect(); + + // We assume here that the first constructor is the "correct" type. Since we + // only care about the "type" of the constructor (i.e. if it is a bool we + // don't care about the value), this assumption should be valid as long as + // the match statement is well formed. But potentially a better way to handle + // this is to use the match expressions type. + match &used_constructors.first() { + Some(constructor) if all_constructors_covered(&cx, constructor, &used_constructors) => { + dbg!("all constructors are covered"); + // If all constructors are covered, then we need to consider whether + // any values are covered by this wildcard. + // + // For example, with matrix '[[Some(true)], [None]]', all + // constructors are covered (`Some`/`None`), so we need + // to perform specialization to see that our wildcard will cover + // the `Some(false)` case. + let constructor = + matrix.heads().iter().filter_map(|&pat| pat_constructor(cx, pat)).next(); + + if let Some(constructor) = constructor { + dbg!("found constructor {:?}, specializing..", &constructor); + if let Constructor::Enum(e) = constructor { + // For enums we handle each variant as a distinct constructor, so + // here we create a constructor for each variant and then check + // usefulness after specializing for that constructor. + let any_useful = cx + .db + .enum_data(e.parent) + .variants + .iter() + .map(|(local_id, _)| { + Constructor::Enum(EnumVariantId { parent: e.parent, local_id }) + }) + .any(|constructor| { + let matrix = matrix.specialize_constructor(&cx, &constructor); + let v = v.expand_wildcard(&cx, &constructor); + + is_useful(&cx, &matrix, &v) == Usefulness::Useful + }); + + if any_useful { + Usefulness::Useful + } else { + Usefulness::NotUseful + } + } else { + let matrix = matrix.specialize_constructor(&cx, &constructor); + let v = v.expand_wildcard(&cx, &constructor); + + is_useful(&cx, &matrix, &v) + } + } else { + Usefulness::NotUseful + } + } + _ => { + // Either not all constructors are covered, or the only other arms + // are wildcards. Either way, this pattern is useful if it is useful + // when compared to those arms with wildcards. + let matrix = matrix.specialize_wildcard(&cx); + let v = v.to_tail(); + + is_useful(&cx, &matrix, &v) + } + } + } +} + +#[derive(Debug)] +enum Constructor { + Bool(bool), + Tuple { arity: usize }, + Enum(EnumVariantId), +} + +fn pat_constructor(cx: &MatchCheckCtx, pat: PatIdOrWild) -> Option { + match pat.as_pat(cx) { + Pat::Wild => None, + Pat::Tuple(pats) => Some(Constructor::Tuple { arity: pats.len() }), + Pat::Lit(lit_expr) => { + // for now we only support bool literals + match cx.body.exprs[lit_expr] { + Expr::Literal(Literal::Bool(val)) => Some(Constructor::Bool(val)), + _ => unimplemented!(), + } + } + Pat::TupleStruct { .. } | Pat::Path(_) => { + let pat_id = pat.as_id().expect("we already know this pattern is not a wild"); + let variant_id = + cx.infer.variant_resolution_for_pat(pat_id).unwrap_or_else(|| unimplemented!()); + match variant_id { + VariantId::EnumVariantId(enum_variant_id) => { + Some(Constructor::Enum(enum_variant_id)) + } + _ => unimplemented!(), + } + } + x => unimplemented!("{:?} not yet implemented", x), + } +} + +fn all_constructors_covered( + cx: &MatchCheckCtx, + constructor: &Constructor, + used_constructors: &[Constructor], +) -> bool { + match constructor { + Constructor::Tuple { arity } => { + used_constructors.iter().any(|constructor| match constructor { + Constructor::Tuple { arity: used_arity } => arity == used_arity, + _ => false, + }) + } + Constructor::Bool(_) => { + if used_constructors.is_empty() { + return false; + } + + let covers_true = + used_constructors.iter().any(|c| matches!(c, Constructor::Bool(true))); + let covers_false = + used_constructors.iter().any(|c| matches!(c, Constructor::Bool(false))); + + covers_true && covers_false + } + Constructor::Enum(e) => cx.db.enum_data(e.parent).variants.iter().all(|(id, _)| { + for constructor in used_constructors { + if let Constructor::Enum(e) = constructor { + if id == e.local_id { + return true; + } + } + } + + false + }), + } +} + +fn enum_variant_matches(cx: &MatchCheckCtx, pat_id: PatId, enum_variant_id: EnumVariantId) -> bool { + if let Some(VariantId::EnumVariantId(pat_variant_id)) = + cx.infer.variant_resolution_for_pat(pat_id) + { + if pat_variant_id.local_id == enum_variant_id.local_id { + return true; + } + } + false +} + +#[cfg(test)] +mod tests { + pub(super) use insta::assert_snapshot; + pub(super) use ra_db::fixture::WithFixture; + + pub(super) use crate::test_db::TestDB; + + pub(super) fn check_diagnostic_message(content: &str) -> String { + TestDB::with_single_file(content).0.diagnostics().0 + } + + pub(super) fn check_diagnostic_with_no_fix(content: &str) { + let diagnostic_count = TestDB::with_single_file(content).0.diagnostics().1; + + assert_eq!(1, diagnostic_count, "no diagnotic reported"); + } + + pub(super) fn check_no_diagnostic(content: &str) { + let diagnostic_count = TestDB::with_single_file(content).0.diagnostics().1; + + assert_eq!(0, diagnostic_count, "expected no diagnostic, found one"); + } + + #[test] + fn empty_tuple_no_arms_diagnostic_message() { + let content = r" + fn test_fn() { + match () { + } + } + "; + + assert_snapshot!( + check_diagnostic_message(content), + @"\"{\\n }\": Missing match arm\n" + ); + } + + #[test] + fn empty_tuple_no_arms() { + let content = r" + fn test_fn() { + match () { + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn empty_tuple_no_diagnostic() { + let content = r" + fn test_fn() { + match () { + () => {} + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn tuple_of_empty_tuple_no_arms() { + let content = r" + fn test_fn() { + match (()) { + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn tuple_of_empty_tuple_no_diagnostic() { + let content = r" + fn test_fn() { + match (()) { + (()) => {} + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn tuple_of_two_empty_tuple_no_arms() { + let content = r" + fn test_fn() { + match ((), ()) { + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn tuple_of_two_empty_tuple_no_diagnostic() { + let content = r" + fn test_fn() { + match ((), ()) { + ((), ()) => {} + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn bool_no_arms() { + let content = r" + fn test_fn() { + match false { + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn bool_missing_arm() { + let content = r" + fn test_fn() { + match false { + true => {} + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn bool_no_diagnostic() { + let content = r" + fn test_fn() { + match false { + true => {} + false => {} + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn tuple_of_bools_no_arms() { + let content = r" + fn test_fn() { + match (false, true) { + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn tuple_of_bools_missing_arms() { + let content = r" + fn test_fn() { + match (false, true) { + (true, true) => {}, + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn tuple_of_bools_no_diagnostic() { + let content = r" + fn test_fn() { + match (false, true) { + (true, true) => {}, + (true, false) => {}, + (false, true) => {}, + (false, false) => {}, + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn tuple_of_tuple_and_bools_no_arms() { + let content = r" + fn test_fn() { + match (false, ((), false)) { + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn tuple_of_tuple_and_bools_missing_arms() { + let content = r" + fn test_fn() { + match (false, ((), false)) { + (true, ((), true)) => {}, + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn tuple_of_tuple_and_bools_no_diagnostic() { + let content = r" + fn test_fn() { + match (false, ((), false)) { + (true, ((), true)) => {}, + (true, ((), false)) => {}, + (false, ((), true)) => {}, + (false, ((), false)) => {}, + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn tuple_of_tuple_and_bools_wildcard_missing_arms() { + let content = r" + fn test_fn() { + match (false, ((), false)) { + (true, _) => {}, + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn tuple_of_tuple_and_bools_wildcard_no_diagnostic() { + let content = r" + fn test_fn() { + match (false, ((), false)) { + (true, ((), true)) => {}, + (true, ((), false)) => {}, + (false, _) => {}, + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn enum_no_arms() { + let content = r" + enum Either { + A, + B, + } + fn test_fn() { + match Either::A { + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn enum_missing_arms() { + let content = r" + enum Either { + A, + B, + } + fn test_fn() { + match Either::B { + Either::A => {}, + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn enum_no_diagnostic() { + let content = r" + enum Either { + A, + B, + } + fn test_fn() { + match Either::B { + Either::A => {}, + Either::B => {}, + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn enum_containing_bool_no_arms() { + let content = r" + enum Either { + A(bool), + B, + } + fn test_fn() { + match Either::B { + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn enum_containing_bool_missing_arms() { + let content = r" + enum Either { + A(bool), + B, + } + fn test_fn() { + match Either::B { + Either::A(true) => (), + Either::B => (), + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn enum_containing_bool_no_diagnostic() { + let content = r" + enum Either { + A(bool), + B, + } + fn test_fn() { + match Either::B { + Either::A(true) => (), + Either::A(false) => (), + Either::B => (), + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn enum_containing_bool_with_wild_no_diagnostic() { + let content = r" + enum Either { + A(bool), + B, + } + fn test_fn() { + match Either::B { + Either::B => (), + _ => (), + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn enum_containing_bool_with_wild_2_no_diagnostic() { + let content = r" + enum Either { + A(bool), + B, + } + fn test_fn() { + match Either::B { + Either::A(_) => (), + Either::B => (), + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn enum_different_sizes_missing_arms() { + let content = r" + enum Either { + A(bool), + B(bool, bool), + } + fn test_fn() { + match Either::A(false) { + Either::A(_) => (), + Either::B(false, _) => (), + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn enum_different_sizes_no_diagnostic() { + let content = r" + enum Either { + A(bool), + B(bool, bool), + } + fn test_fn() { + match Either::A(false) { + Either::A(_) => (), + Either::B(true, _) => (), + Either::B(false, _) => (), + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn or_no_diagnostic() { + let content = r" + enum Either { + A(bool), + B(bool, bool), + } + fn test_fn() { + match Either::A(false) { + Either::A(true) | Either::A(false) => (), + Either::B(true, _) => (), + Either::B(false, _) => (), + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn tuple_of_enum_no_diagnostic() { + let content = r" + enum Either { + A(bool), + B(bool, bool), + } + enum Either2 { + C, + D, + } + fn test_fn() { + match (Either::A(false), Either2::C) { + (Either::A(true), _) | (Either::A(false), _) => (), + (Either::B(true, _), Either2::C) => (), + (Either::B(false, _), Either2::C) => (), + (Either::B(_, _), Either2::D) => (), + } + } + "; + + check_no_diagnostic(content); + } +} + +#[cfg(test)] +mod false_negatives { + //! The implementation of match checking here is a work in progress. As we roll this out, we + //! prefer false negatives to false positives (ideally there would be no false positives). This + //! test module should document known false negatives. Eventually we will have a complete + //! implementation of match checking and this module will be empty. + //! + //! The reasons for documenting known false negatives: + //! + //! 1. It acts as a backlog of work that can be done to improve the behavior of the system. + //! 2. It ensures the code doesn't panic when handling these cases. + + use super::tests::*; + + #[test] + fn mismatched_types() { + let content = r" + enum Either { + A, + B, + } + enum Either2 { + C, + D, + } + fn test_fn() { + match Either::A { + Either2::C => (), + Either2::D => (), + } + } + "; + + // This is a false negative. + // We don't currently check that the match arms actually + // match the type of the match expression. + check_no_diagnostic(content); + } +} diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs index 0f8522021e..3457905e22 100644 --- a/crates/ra_hir_ty/src/diagnostics.rs +++ b/crates/ra_hir_ty/src/diagnostics.rs @@ -6,7 +6,7 @@ use hir_expand::{db::AstDatabase, name::Name, HirFileId, InFile}; use ra_syntax::{ast, AstNode, AstPtr, SyntaxNodePtr}; use stdx::format_to; -pub use hir_def::diagnostics::UnresolvedModule; +pub use hir_def::{diagnostics::UnresolvedModule, expr::MatchArm}; pub use hir_expand::diagnostics::{AstDiagnostic, Diagnostic, DiagnosticSink}; #[derive(Debug)] @@ -62,6 +62,24 @@ impl AstDiagnostic for MissingFields { } } +#[derive(Debug)] +pub struct MissingMatchArms { + pub file: HirFileId, + pub arms: AstPtr, +} + +impl Diagnostic for MissingMatchArms { + fn message(&self) -> String { + String::from("Missing match arm") + } + fn source(&self) -> InFile { + InFile { file_id: self.file, value: self.arms.into() } + } + fn as_any(&self) -> &(dyn Any + Send + 'static) { + self + } +} + #[derive(Debug)] pub struct MissingOkInTailExpr { pub file: HirFileId, diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index b7b476b4c1..3caeeb3946 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -14,9 +14,10 @@ use rustc_hash::FxHashSet; use crate::{ db::HirDatabase, - diagnostics::{MissingFields, MissingOkInTailExpr}, + diagnostics::{MissingFields, MissingMatchArms, MissingOkInTailExpr}, utils::variant_data, ApplicationTy, InferenceResult, Ty, TypeCtor, + _match::{is_useful, MatchCheckCtx, Matrix, PatStack, Usefulness}, }; pub use hir_def::{ @@ -52,15 +53,63 @@ impl<'a, 'b> ExprValidator<'a, 'b> { for e in body.exprs.iter() { if let (id, Expr::RecordLit { path, fields, spread }) = e { self.validate_record_literal(id, path, fields, *spread, db); + } else if let (id, Expr::Match { expr, arms }) = e { + self.validate_match(id, *expr, arms, db, self.infer.clone()); } } let body_expr = &body[body.body_expr]; - if let Expr::Block { statements: _, tail: Some(t) } = body_expr { + if let Expr::Block { tail: Some(t), .. } = body_expr { self.validate_results_in_tail_expr(body.body_expr, *t, db); } } + fn validate_match( + &mut self, + id: ExprId, + expr: ExprId, + arms: &[MatchArm], + db: &dyn HirDatabase, + infer: Arc, + ) { + let (body, source_map): (Arc, Arc) = + db.body_with_source_map(self.func.into()); + + let match_expr: &hir_def::expr::Expr = &body[expr]; + + let cx = MatchCheckCtx { body: body.clone(), match_expr, infer, db }; + let pats = arms.iter().map(|arm| arm.pat); + + let mut seen = Matrix::empty(); + for pat in pats { + // If we had a NotUsefulMatchArm diagnostic, we could + // check the usefulness of each pattern as we added it + // to the matrix here. + let v = PatStack::from_pattern(pat); + seen.push(&cx, v); + } + + match is_useful(&cx, &seen, &PatStack::from_wild()) { + Usefulness::Useful => (), + // if a wildcard pattern is not useful, then all patterns are covered + Usefulness::NotUseful => return, + } + + if let Ok(source_ptr) = source_map.expr_syntax(id) { + if let Some(expr) = source_ptr.value.left() { + let root = source_ptr.file_syntax(db.upcast()); + if let ast::Expr::MatchExpr(match_expr) = expr.to_node(&root) { + if let Some(arms) = match_expr.match_arm_list() { + self.sink.push(MissingMatchArms { + file: source_ptr.file_id, + arms: AstPtr::new(&arms), + }) + } + } + } + } + } + fn validate_record_literal( &mut self, id: ExprId, diff --git a/crates/ra_hir_ty/src/infer/pat.rs b/crates/ra_hir_ty/src/infer/pat.rs index 86acd27f8d..69bbb4307f 100644 --- a/crates/ra_hir_ty/src/infer/pat.rs +++ b/crates/ra_hir_ty/src/infer/pat.rs @@ -21,9 +21,13 @@ impl<'a> InferenceContext<'a> { subpats: &[PatId], expected: &Ty, default_bm: BindingMode, + id: PatId, ) -> Ty { let (ty, def) = self.resolve_variant(path); let var_data = def.map(|it| variant_data(self.db.upcast(), it)); + if let Some(variant) = def { + self.write_variant_resolution(id.into(), variant); + } self.unify(&ty, expected); let substs = ty.substs().unwrap_or_else(Substs::empty); @@ -152,7 +156,7 @@ impl<'a> InferenceContext<'a> { Ty::apply_one(TypeCtor::Ref(*mutability), subty) } Pat::TupleStruct { path: p, args: subpats } => { - self.infer_tuple_struct_pat(p.as_ref(), subpats, expected, default_bm) + self.infer_tuple_struct_pat(p.as_ref(), subpats, expected, default_bm, pat) } Pat::Record { path: p, args: fields } => { self.infer_record_pat(p.as_ref(), fields, expected, default_bm, pat) diff --git a/crates/ra_hir_ty/src/infer/path.rs b/crates/ra_hir_ty/src/infer/path.rs index 318652c611..2b6bc0f798 100644 --- a/crates/ra_hir_ty/src/infer/path.rs +++ b/crates/ra_hir_ty/src/infer/path.rs @@ -67,8 +67,16 @@ impl<'a> InferenceContext<'a> { ValueNs::FunctionId(it) => it.into(), ValueNs::ConstId(it) => it.into(), ValueNs::StaticId(it) => it.into(), - ValueNs::StructId(it) => it.into(), - ValueNs::EnumVariantId(it) => it.into(), + ValueNs::StructId(it) => { + self.write_variant_resolution(id, it.into()); + + it.into() + } + ValueNs::EnumVariantId(it) => { + self.write_variant_resolution(id, it.into()); + + it.into() + } }; let ty = self.db.value_ty(typable); diff --git a/crates/ra_hir_ty/src/lib.rs b/crates/ra_hir_ty/src/lib.rs index 717399b6de..18f74d3b12 100644 --- a/crates/ra_hir_ty/src/lib.rs +++ b/crates/ra_hir_ty/src/lib.rs @@ -43,6 +43,7 @@ mod tests; #[cfg(test)] mod test_db; mod marks; +mod _match; use std::ops::Deref; use std::sync::Arc; diff --git a/crates/ra_hir_ty/src/test_db.rs b/crates/ra_hir_ty/src/test_db.rs index 208096aab6..3a4d58bf9c 100644 --- a/crates/ra_hir_ty/src/test_db.rs +++ b/crates/ra_hir_ty/src/test_db.rs @@ -105,8 +105,9 @@ impl TestDB { } // FIXME: don't duplicate this - pub fn diagnostics(&self) -> String { + pub fn diagnostics(&self) -> (String, u32) { let mut buf = String::new(); + let mut count = 0; let crate_graph = self.crate_graph(); for krate in crate_graph.iter() { let crate_def_map = self.crate_def_map(krate); @@ -133,13 +134,14 @@ impl TestDB { let infer = self.infer(f.into()); let mut sink = DiagnosticSink::new(|d| { format_to!(buf, "{:?}: {}\n", d.syntax_node(self).text(), d.message()); + count += 1; }); infer.add_diagnostics(self, f, &mut sink); let mut validator = ExprValidator::new(f, infer, &mut sink); validator.validate_body(self); } } - buf + (buf, count) } } diff --git a/crates/ra_hir_ty/src/tests.rs b/crates/ra_hir_ty/src/tests.rs index 027e5a8f88..e4a103d1be 100644 --- a/crates/ra_hir_ty/src/tests.rs +++ b/crates/ra_hir_ty/src/tests.rs @@ -309,7 +309,8 @@ fn no_such_field_diagnostics() { } ", ) - .diagnostics(); + .diagnostics() + .0; assert_snapshot!(diagnostics, @r###" "baz: 62": no such field diff --git a/crates/ra_ide/src/diagnostics.rs b/crates/ra_ide/src/diagnostics.rs index c1d7ddaf23..901ad104c1 100644 --- a/crates/ra_ide/src/diagnostics.rs +++ b/crates/ra_ide/src/diagnostics.rs @@ -101,6 +101,14 @@ pub(crate) fn diagnostics(db: &RootDatabase, file_id: FileId) -> Vec fix, }) }) + .on::(|d| { + res.borrow_mut().push(Diagnostic { + range: d.highlight_range(), + message: d.message(), + severity: Severity::Error, + fix: None, + }) + }) .on::(|d| { let node = d.ast(db); let replacement = format!("Ok({})", node.syntax()); @@ -291,7 +299,7 @@ mod tests { fn check_no_diagnostic(content: &str) { let (analysis, file_id) = single_file(content); let diagnostics = analysis.diagnostics(file_id).unwrap(); - assert_eq!(diagnostics.len(), 0); + assert_eq!(diagnostics.len(), 0, "expected no diagnostic, found one"); } #[test]