From 8c378af72117e92bc894fd4a79e978ef0d1c0cc7 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Tue, 24 Mar 2020 04:40:58 -0700 Subject: missing match arms diagnostic --- crates/ra_hir_ty/src/_match.rs | 944 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 944 insertions(+) create mode 100644 crates/ra_hir_ty/src/_match.rs (limited to 'crates/ra_hir_ty/src/_match.rs') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs new file mode 100644 index 000000000..ac66dd415 --- /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); + } +} -- cgit v1.2.3 From b87b7a088f34ff794fc19e57ee2ae1cfe81a12df Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 4 Apr 2020 18:02:27 -0700 Subject: remove panics --- crates/ra_hir_ty/src/_match.rs | 273 +++++++++++++++++++++++++++++++---------- 1 file changed, 209 insertions(+), 64 deletions(-) (limited to 'crates/ra_hir_ty/src/_match.rs') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index ac66dd415..de291c1f6 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -42,6 +42,10 @@ impl From for PatIdOrWild { } } +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct MatchCheckNotImplemented; +pub type MatchCheckResult = Result; + type PatStackInner = SmallVec<[PatIdOrWild; 2]>; #[derive(Debug)] pub(crate) struct PatStack(PatStackInner); @@ -104,42 +108,49 @@ impl PatStack { &self, cx: &MatchCheckCtx, constructor: &Constructor, - ) -> Option { - match (self.head().as_pat(cx), constructor) { + ) -> MatchCheckResult> { + let result = match (self.head().as_pat(cx), constructor) { (Pat::Tuple(ref pat_ids), Constructor::Tuple { arity }) => { if pat_ids.len() != *arity { - return None; + None + } else { + Some(self.replace_head_with(pat_ids)) } - - 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::Wild, constructor) => Some(self.expand_wildcard(cx, constructor)?), (Pat::Path(_), Constructor::Enum(constructor)) => { + // enums with no associated data become `Pat::Path` 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; + None + } else { + Some(self.to_tail()) } - // 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; + None + } else { + Some(self.replace_head_with(pat_ids)) } - - Some(self.replace_head_with(pat_ids)) } (Pat::Or(_), _) => unreachable!("we desugar or patterns so this should never happen"), - (a, b) => unimplemented!("{:?}, {:?}", a, b), - } + (_, _) => return Err(MatchCheckNotImplemented), + }; + + Ok(result) } - fn expand_wildcard(&self, cx: &MatchCheckCtx, constructor: &Constructor) -> PatStack { + fn expand_wildcard( + &self, + cx: &MatchCheckCtx, + constructor: &Constructor, + ) -> MatchCheckResult { assert_eq!( Pat::Wild, self.head().as_pat(cx), @@ -154,7 +165,7 @@ impl PatStack { 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), + _ => return Err(MatchCheckNotImplemented), } } }; @@ -167,7 +178,7 @@ impl PatStack { patterns.push(*pat); } - PatStack::from_vec(patterns) + Ok(PatStack::from_vec(patterns)) } } @@ -204,8 +215,19 @@ impl Matrix { } // 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 specialize_constructor( + &self, + cx: &MatchCheckCtx, + constructor: &Constructor, + ) -> MatchCheckResult { + let mut new_matrix = Matrix::empty(); + for pat in &self.0 { + if let Some(pat) = pat.specialize_constructor(cx, constructor)? { + new_matrix.push(cx, pat); + } + } + + Ok(new_matrix) } fn collect>(cx: &MatchCheckCtx, iter: T) -> Self { @@ -239,37 +261,56 @@ pub struct MatchCheckCtx<'a> { // 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); +pub(crate) fn is_useful( + cx: &MatchCheckCtx, + matrix: &Matrix, + v: &PatStack, +) -> MatchCheckResult { if v.is_empty() { - if matrix.is_empty() { - return Usefulness::Useful; - } else { - return Usefulness::NotUseful; - } + let result = if matrix.is_empty() { Usefulness::Useful } else { Usefulness::NotUseful }; + + return Ok(result); } if let Pat::Or(pat_ids) = v.head().as_pat(cx) { + let mut found_unimplemented = false; let any_useful = pat_ids.iter().any(|&pat_id| { let v = PatStack::from_pattern(pat_id); - is_useful(cx, matrix, &v) == Usefulness::Useful + match is_useful(cx, matrix, &v) { + Ok(Usefulness::Useful) => true, + Ok(Usefulness::NotUseful) => false, + _ => { + found_unimplemented = true; + false + } + } }); - return if any_useful { Usefulness::Useful } else { Usefulness::NotUseful }; + return if any_useful { + Ok(Usefulness::Useful) + } else if found_unimplemented { + Err(MatchCheckNotImplemented) + } else { + Ok(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"); + if let Some(constructor) = pat_constructor(cx, v.head())? { + let matrix = matrix.specialize_constructor(&cx, &constructor)?; + let v = v + .specialize_constructor(&cx, &constructor)? + .expect("we know this can't fail because we get the constructor from `v.head()` above"); 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(); + let mut used_constructors: Vec = vec![]; + for pat in matrix.heads() { + if let Some(constructor) = pat_constructor(cx, pat)? { + used_constructors.push(constructor); + } + } // 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 @@ -278,7 +319,6 @@ pub(crate) fn is_useful(cx: &MatchCheckCtx, matrix: &Matrix, v: &PatStack) -> Us // 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. // @@ -286,43 +326,48 @@ pub(crate) fn is_useful(cx: &MatchCheckCtx, matrix: &Matrix, v: &PatStack) -> Us // 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(); + let mut constructor = None; + for pat in matrix.heads() { + if let Some(c) = pat_constructor(cx, pat)? { + constructor = Some(c); + break; + } + } 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, _)| { + let mut found_unimplemented = false; + for constructor in + 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 - }); + { + let matrix = matrix.specialize_constructor(&cx, &constructor)?; + let v = v.expand_wildcard(&cx, &constructor)?; + + match is_useful(&cx, &matrix, &v) { + Ok(Usefulness::Useful) => return Ok(Usefulness::Useful), + Ok(Usefulness::NotUseful) => continue, + _ => found_unimplemented = true, + }; + } - if any_useful { - Usefulness::Useful + if found_unimplemented { + Err(MatchCheckNotImplemented) } else { - Usefulness::NotUseful + Ok(Usefulness::NotUseful) } } else { - let matrix = matrix.specialize_constructor(&cx, &constructor); - let v = v.expand_wildcard(&cx, &constructor); + let matrix = matrix.specialize_constructor(&cx, &constructor)?; + let v = v.expand_wildcard(&cx, &constructor)?; is_useful(&cx, &matrix, &v) } } else { - Usefulness::NotUseful + Ok(Usefulness::NotUseful) } } _ => { @@ -345,30 +390,32 @@ enum Constructor { Enum(EnumVariantId), } -fn pat_constructor(cx: &MatchCheckCtx, pat: PatIdOrWild) -> Option { - match pat.as_pat(cx) { +fn pat_constructor(cx: &MatchCheckCtx, pat: PatIdOrWild) -> MatchCheckResult> { + let res = 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!(), + _ => return Err(MatchCheckNotImplemented), } } 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!()); + cx.infer.variant_resolution_for_pat(pat_id).ok_or(MatchCheckNotImplemented)?; match variant_id { VariantId::EnumVariantId(enum_variant_id) => { Some(Constructor::Enum(enum_variant_id)) } - _ => unimplemented!(), + _ => return Err(MatchCheckNotImplemented), } } - x => unimplemented!("{:?} not yet implemented", x), - } + _ => return Err(MatchCheckNotImplemented), + }; + + Ok(res) } fn all_constructors_covered( @@ -613,6 +660,34 @@ mod tests { check_no_diagnostic(content); } + #[test] + fn tuple_of_bools_binding_missing_arms() { + let content = r" + fn test_fn() { + match (false, true) { + (true, _x) => {}, + } + } + "; + + check_diagnostic_with_no_fix(content); + } + + #[test] + fn tuple_of_bools_binding_no_diagnostic() { + let content = r" + fn test_fn() { + match (false, true) { + (true, _x) => {}, + (false, true) => {}, + (false, false) => {}, + } + } + "; + + check_no_diagnostic(content); + } + #[test] fn tuple_of_tuple_and_bools_no_arms() { let content = r" @@ -941,4 +1016,74 @@ mod false_negatives { // match the type of the match expression. check_no_diagnostic(content); } + + #[test] + fn mismatched_types_with_different_arity() { + let content = r" + fn test_fn() { + match (true, false) { + (true, false, true) => (), + (true) => (), + } + } + "; + + // This is a false negative. + // We don't currently check that the match arms actually + // match the type of the match expression. This test + // checks to ensure we don't panic when the code we are + // checking is malformed in such a way that the arity of the + // constructors doesn't match. + check_no_diagnostic(content); + } + + #[test] + fn integers() { + let content = r" + fn test_fn() { + match 5 { + 10 => (), + 11..20 => (), + } + } + "; + + // This is a false negative. + // We don't currently check integer exhaustiveness. + check_no_diagnostic(content); + } + + #[test] + fn enum_record() { + let content = r" + enum Either { + A { foo: u32 }, + B, + } + fn test_fn() { + match Either::B { + Either::A { foo: 5 } => (), + } + } + "; + + // This is a false negative. + // We don't currently handle enum record types. + check_no_diagnostic(content); + } + + #[test] + fn enum_not_in_scope() { + let content = r" + fn test_fn() { + match Foo::Bar { + Foo::Baz => (), + } + } + "; + + // This is a false negative. + // The enum is not in scope so we don't perform exhaustiveness checking. + check_no_diagnostic(content); + } } -- cgit v1.2.3 From 43dfd894934cf7c9161e473495a4e24965239475 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sun, 5 Apr 2020 10:16:34 -0700 Subject: handle non matching enum pattern types --- crates/ra_hir_ty/src/_match.rs | 68 ++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 33 deletions(-) (limited to 'crates/ra_hir_ty/src/_match.rs') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index de291c1f6..91206adc3 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -250,17 +250,13 @@ pub enum Usefulness { 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? +/// Given a set of patterns `matrix`, and pattern to consider `v`, determines +/// whether `v` is useful. A pattern is useful if it covers cases which were +/// not previously covered. pub(crate) fn is_useful( cx: &MatchCheckCtx, matrix: &Matrix, @@ -517,6 +513,19 @@ mod tests { check_diagnostic_with_no_fix(content); } + #[test] + fn empty_tuple_wild() { + let content = r" + fn test_fn() { + match () { + _ => {} + } + } + "; + + check_no_diagnostic(content); + } + #[test] fn empty_tuple_no_diagnostic() { let content = r" @@ -976,21 +985,6 @@ mod tests { 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() { @@ -1011,10 +1005,8 @@ mod false_negatives { } "; - // 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); + // Match arms with the incorrect type are filtered out. + check_diagnostic_with_no_fix(content); } #[test] @@ -1028,14 +1020,24 @@ mod false_negatives { } "; - // This is a false negative. - // We don't currently check that the match arms actually - // match the type of the match expression. This test - // checks to ensure we don't panic when the code we are - // checking is malformed in such a way that the arity of the - // constructors doesn't match. - check_no_diagnostic(content); + // Match arms with the incorrect type are filtered out. + check_diagnostic_with_no_fix(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 integers() { -- cgit v1.2.3 From 5b4316377b9897f064b213a52a7efe8622d48487 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sun, 5 Apr 2020 11:28:53 -0700 Subject: improving documentation --- crates/ra_hir_ty/src/_match.rs | 80 +++++++++++++++++++++++++++++++++--------- 1 file changed, 64 insertions(+), 16 deletions(-) (limited to 'crates/ra_hir_ty/src/_match.rs') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 91206adc3..8b9bdb7cd 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -2,7 +2,7 @@ //! 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. +//! contains very detailed documentation about the algorithms used here. use std::sync::Arc; use smallvec::{smallvec, SmallVec}; @@ -15,6 +15,14 @@ use crate::{ use hir_def::{adt::VariantData, EnumVariantId, VariantId}; #[derive(Debug, Clone, Copy)] +/// Either a pattern from the source code being analyzed, represented as +/// as `PatId`, or a `Wild` pattern which is created as an intermediate +/// step in the match checking algorithm and thus is not backed by a +/// real `PatId`. +/// +/// Note that it is totally valid for the `PatId` variant to contain +/// a `PatId` which resolves to a `Wild` pattern, if that wild pattern +/// exists in the source code being analyzed. enum PatIdOrWild { PatId(PatId), Wild, @@ -44,11 +52,22 @@ impl From for PatIdOrWild { #[derive(Debug, Clone, Copy, PartialEq)] pub struct MatchCheckNotImplemented; + +/// The return type of `is_useful` is either an indication of usefulness +/// of the match arm, or an error in the case the match statement +/// is made up of types for which exhaustiveness checking is currently +/// not completely implemented. +/// +/// The `std::result::Result` type is used here rather than a custom enum +/// to allow the use of `?`. pub type MatchCheckResult = Result; -type PatStackInner = SmallVec<[PatIdOrWild; 2]>; #[derive(Debug)] +/// A row in a Matrix. +/// +/// This type is modeled from the struct of the same name in `rustc`. pub(crate) struct PatStack(PatStackInner); +type PatStackInner = SmallVec<[PatIdOrWild; 2]>; impl PatStack { pub(crate) fn from_pattern(pat_id: PatId) -> PatStack { @@ -94,7 +113,9 @@ impl PatStack { PatStack::from_vec(patterns) } - // Computes `D(self)`. + /// Computes `D(self)`. + /// + /// See the module docs and the associated documentation in rustc for details. fn specialize_wildcard(&self, cx: &MatchCheckCtx) -> Option { if matches!(self.head().as_pat(cx), Pat::Wild) { Some(self.to_tail()) @@ -103,7 +124,9 @@ impl PatStack { } } - // Computes `S(constructor, self)`. + /// Computes `S(constructor, self)`. + /// + /// See the module docs and the associated documentation in rustc for details. fn specialize_constructor( &self, cx: &MatchCheckCtx, @@ -146,6 +169,11 @@ impl PatStack { Ok(result) } + /// A special case of `specialize_constructor` where the head of the pattern stack + /// is a Wild pattern. + /// + /// Replaces the Wild pattern at the head of the pattern stack with N Wild patterns + /// (N >= 0), where N is the arity of the given constructor. fn expand_wildcard( &self, cx: &MatchCheckCtx, @@ -183,6 +211,9 @@ impl PatStack { } #[derive(Debug)] +/// A collection of PatStack. +/// +/// This type is modeled from the struct of the same name in `rustc`. pub(crate) struct Matrix(Vec); impl Matrix { @@ -191,8 +222,8 @@ impl Matrix { } 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)) { + // Or patterns are expanded here for pat_id in pat_ids { self.0.push(PatStack::from_pattern(pat_id)); } @@ -209,12 +240,16 @@ impl Matrix { self.0.iter().map(|p| p.head()).collect() } - // Computes `D(self)`. + /// Computes `D(self)` for each contained PatStack. + /// + /// See the module docs and the associated documentation in rustc for details. fn specialize_wildcard(&self, cx: &MatchCheckCtx) -> Self { Self::collect(cx, self.0.iter().filter_map(|r| r.specialize_wildcard(cx))) } - // Computes `S(constructor, self)`. + /// Computes `S(constructor, self)` for each contained PatStack. + /// + /// See the module docs and the associated documentation in rustc for details. fn specialize_constructor( &self, cx: &MatchCheckCtx, @@ -243,6 +278,11 @@ impl Matrix { } #[derive(Clone, Debug, PartialEq)] +/// An indication of the usefulness of a given match arm, where +/// usefulness is defined as matching some patterns which were +/// not matched by an prior match arms. +/// +/// We may eventually need an `Unknown` variant here. pub enum Usefulness { Useful, NotUseful, @@ -257,6 +297,11 @@ pub struct MatchCheckCtx<'a> { /// Given a set of patterns `matrix`, and pattern to consider `v`, determines /// whether `v` is useful. A pattern is useful if it covers cases which were /// not previously covered. +/// +/// When calling this function externally (that is, not the recursive calls) it +/// expected that you have already type checked the match arms. All patterns in +/// matrix should be the same type as v, as well as they should all be the same +/// type as the match expression. pub(crate) fn is_useful( cx: &MatchCheckCtx, matrix: &Matrix, @@ -311,8 +356,9 @@ pub(crate) fn is_useful( // 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. + // the match statement is well formed. We currently uphold this invariant by + // filtering match arms before calling `is_useful`, only passing in match arms + // whose type matches the type of the match expression. match &used_constructors.first() { Some(constructor) if all_constructors_covered(&cx, constructor, &used_constructors) => { // If all constructors are covered, then we need to consider whether @@ -380,23 +426,25 @@ pub(crate) fn is_useful( } #[derive(Debug)] +/// Similar to TypeCtor, but includes additional information about the specific +/// value being instantiated. For example, TypeCtor::Bool doesn't contain the +/// boolean value. enum Constructor { Bool(bool), Tuple { arity: usize }, Enum(EnumVariantId), } +/// Returns the constructor for the given pattern. Should only return None +/// in the case of a Wild pattern. fn pat_constructor(cx: &MatchCheckCtx, pat: PatIdOrWild) -> MatchCheckResult> { let res = 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)), - _ => return Err(MatchCheckNotImplemented), - } - } + Pat::Lit(lit_expr) => match cx.body.exprs[lit_expr] { + Expr::Literal(Literal::Bool(val)) => Some(Constructor::Bool(val)), + _ => return Err(MatchCheckNotImplemented), + }, Pat::TupleStruct { .. } | Pat::Path(_) => { let pat_id = pat.as_id().expect("we already know this pattern is not a wild"); let variant_id = -- cgit v1.2.3 From 5fe608fb31430f43a404312e284a71d6f7cfa038 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sun, 5 Apr 2020 12:42:24 -0700 Subject: handle match auto-deref --- crates/ra_hir_ty/src/_match.rs | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) (limited to 'crates/ra_hir_ty/src/_match.rs') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 8b9bdb7cd..f502a9208 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -865,6 +865,41 @@ mod tests { check_no_diagnostic(content); } + #[test] + fn enum_ref_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_ref_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" -- cgit v1.2.3 From da6752d5f9a18ba58adb6a2e72d30a83532ec8a6 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Mon, 6 Apr 2020 06:55:25 -0700 Subject: missing match arms diagnostic change source to match expression --- crates/ra_hir_ty/src/_match.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'crates/ra_hir_ty/src/_match.rs') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index f502a9208..02472e0c0 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -545,7 +545,7 @@ mod tests { assert_snapshot!( check_diagnostic_message(content), - @"\"{\\n }\": Missing match arm\n" + @"\"()\": Missing match arm\n" ); } -- cgit v1.2.3 From a208de15b7846911856e6c069f7df03676c18a03 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Mon, 6 Apr 2020 15:38:20 -0700 Subject: PR feedback implementation --- crates/ra_hir_ty/src/_match.rs | 425 ++++++++++++++++++++++++++++++++--------- 1 file changed, 331 insertions(+), 94 deletions(-) (limited to 'crates/ra_hir_ty/src/_match.rs') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 02472e0c0..f29a25505 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -2,7 +2,191 @@ //! for match arms. //! //! It is modeled on the rustc module `librustc_mir_build::hair::pattern::_match`, which -//! contains very detailed documentation about the algorithms used here. +//! contains very detailed documentation about the algorithms used here. I've duplicated +//! most of that documentation below. +//! +//! This file includes the logic for exhaustiveness and usefulness checking for +//! pattern-matching. Specifically, given a list of patterns for a type, we can +//! tell whether: +//! (a) the patterns cover every possible constructor for the type [exhaustiveness] +//! (b) each pattern is necessary [usefulness] +//! +//! The algorithm implemented here is a modified version of the one described in: +//! http://moscova.inria.fr/~maranget/papers/warn/index.html +//! However, to save future implementors from reading the original paper, we +//! summarise the algorithm here to hopefully save time and be a little clearer +//! (without being so rigorous). +//! +//! The core of the algorithm revolves about a "usefulness" check. In particular, we +//! are trying to compute a predicate `U(P, p)` where `P` is a list of patterns (we refer to this as +//! a matrix). `U(P, p)` represents whether, given an existing list of patterns +//! `P_1 ..= P_m`, adding a new pattern `p` will be "useful" (that is, cover previously- +//! uncovered values of the type). +//! +//! If we have this predicate, then we can easily compute both exhaustiveness of an +//! entire set of patterns and the individual usefulness of each one. +//! (a) the set of patterns is exhaustive iff `U(P, _)` is false (i.e., adding a wildcard +//! match doesn't increase the number of values we're matching) +//! (b) a pattern `P_i` is not useful if `U(P[0..=(i-1), P_i)` is false (i.e., adding a +//! pattern to those that have come before it doesn't increase the number of values +//! we're matching). +//! +//! During the course of the algorithm, the rows of the matrix won't just be individual patterns, +//! but rather partially-deconstructed patterns in the form of a list of patterns. The paper +//! calls those pattern-vectors, and we will call them pattern-stacks. The same holds for the +//! new pattern `p`. +//! +//! For example, say we have the following: +//! ``` +//! // x: (Option, Result<()>) +//! match x { +//! (Some(true), _) => {} +//! (None, Err(())) => {} +//! (None, Err(_)) => {} +//! } +//! ``` +//! Here, the matrix `P` starts as: +//! [ +//! [(Some(true), _)], +//! [(None, Err(()))], +//! [(None, Err(_))], +//! ] +//! We can tell it's not exhaustive, because `U(P, _)` is true (we're not covering +//! `[(Some(false), _)]`, for instance). In addition, row 3 is not useful, because +//! all the values it covers are already covered by row 2. +//! +//! A list of patterns can be thought of as a stack, because we are mainly interested in the top of +//! the stack at any given point, and we can pop or apply constructors to get new pattern-stacks. +//! To match the paper, the top of the stack is at the beginning / on the left. +//! +//! There are two important operations on pattern-stacks necessary to understand the algorithm: +//! 1. We can pop a given constructor off the top of a stack. This operation is called +//! `specialize`, and is denoted `S(c, p)` where `c` is a constructor (like `Some` or +//! `None`) and `p` a pattern-stack. +//! If the pattern on top of the stack can cover `c`, this removes the constructor and +//! pushes its arguments onto the stack. It also expands OR-patterns into distinct patterns. +//! Otherwise the pattern-stack is discarded. +//! This essentially filters those pattern-stacks whose top covers the constructor `c` and +//! discards the others. +//! +//! For example, the first pattern above initially gives a stack `[(Some(true), _)]`. If we +//! pop the tuple constructor, we are left with `[Some(true), _]`, and if we then pop the +//! `Some` constructor we get `[true, _]`. If we had popped `None` instead, we would get +//! nothing back. +//! +//! This returns zero or more new pattern-stacks, as follows. We look at the pattern `p_1` +//! on top of the stack, and we have four cases: +//! 1.1. `p_1 = c(r_1, .., r_a)`, i.e. the top of the stack has constructor `c`. We +//! push onto the stack the arguments of this constructor, and return the result: +//! r_1, .., r_a, p_2, .., p_n +//! 1.2. `p_1 = c'(r_1, .., r_a')` where `c ≠ c'`. We discard the current stack and +//! return nothing. +//! 1.3. `p_1 = _`. We push onto the stack as many wildcards as the constructor `c` has +//! arguments (its arity), and return the resulting stack: +//! _, .., _, p_2, .., p_n +//! 1.4. `p_1 = r_1 | r_2`. We expand the OR-pattern and then recurse on each resulting +//! stack: +//! S(c, (r_1, p_2, .., p_n)) +//! S(c, (r_2, p_2, .., p_n)) +//! +//! 2. We can pop a wildcard off the top of the stack. This is called `D(p)`, where `p` is +//! a pattern-stack. +//! This is used when we know there are missing constructor cases, but there might be +//! existing wildcard patterns, so to check the usefulness of the matrix, we have to check +//! all its *other* components. +//! +//! It is computed as follows. We look at the pattern `p_1` on top of the stack, +//! and we have three cases: +//! 1.1. `p_1 = c(r_1, .., r_a)`. We discard the current stack and return nothing. +//! 1.2. `p_1 = _`. We return the rest of the stack: +//! p_2, .., p_n +//! 1.3. `p_1 = r_1 | r_2`. We expand the OR-pattern and then recurse on each resulting +//! stack. +//! D((r_1, p_2, .., p_n)) +//! D((r_2, p_2, .., p_n)) +//! +//! Note that the OR-patterns are not always used directly in Rust, but are used to derive the +//! exhaustive integer matching rules, so they're written here for posterity. +//! +//! Both those operations extend straightforwardly to a list or pattern-stacks, i.e. a matrix, by +//! working row-by-row. Popping a constructor ends up keeping only the matrix rows that start with +//! the given constructor, and popping a wildcard keeps those rows that start with a wildcard. +//! +//! +//! The algorithm for computing `U` +//! ------------------------------- +//! The algorithm is inductive (on the number of columns: i.e., components of tuple patterns). +//! That means we're going to check the components from left-to-right, so the algorithm +//! operates principally on the first component of the matrix and new pattern-stack `p`. +//! This algorithm is realised in the `is_useful` function. +//! +//! Base case. (`n = 0`, i.e., an empty tuple pattern) +//! - If `P` already contains an empty pattern (i.e., if the number of patterns `m > 0`), +//! then `U(P, p)` is false. +//! - Otherwise, `P` must be empty, so `U(P, p)` is true. +//! +//! Inductive step. (`n > 0`, i.e., whether there's at least one column +//! [which may then be expanded into further columns later]) +//! We're going to match on the top of the new pattern-stack, `p_1`. +//! - If `p_1 == c(r_1, .., r_a)`, i.e. we have a constructor pattern. +//! Then, the usefulness of `p_1` can be reduced to whether it is useful when +//! we ignore all the patterns in the first column of `P` that involve other constructors. +//! This is where `S(c, P)` comes in: +//! `U(P, p) := U(S(c, P), S(c, p))` +//! This special case is handled in `is_useful_specialized`. +//! +//! For example, if `P` is: +//! [ +//! [Some(true), _], +//! [None, 0], +//! ] +//! and `p` is [Some(false), 0], then we don't care about row 2 since we know `p` only +//! matches values that row 2 doesn't. For row 1 however, we need to dig into the +//! arguments of `Some` to know whether some new value is covered. So we compute +//! `U([[true, _]], [false, 0])`. +//! +//! - If `p_1 == _`, then we look at the list of constructors that appear in the first +//! component of the rows of `P`: +//! + If there are some constructors that aren't present, then we might think that the +//! wildcard `_` is useful, since it covers those constructors that weren't covered +//! before. +//! That's almost correct, but only works if there were no wildcards in those first +//! components. So we need to check that `p` is useful with respect to the rows that +//! start with a wildcard, if there are any. This is where `D` comes in: +//! `U(P, p) := U(D(P), D(p))` +//! +//! For example, if `P` is: +//! [ +//! [_, true, _], +//! [None, false, 1], +//! ] +//! and `p` is [_, false, _], the `Some` constructor doesn't appear in `P`. So if we +//! only had row 2, we'd know that `p` is useful. However row 1 starts with a +//! wildcard, so we need to check whether `U([[true, _]], [false, 1])`. +//! +//! + Otherwise, all possible constructors (for the relevant type) are present. In this +//! case we must check whether the wildcard pattern covers any unmatched value. For +//! that, we can think of the `_` pattern as a big OR-pattern that covers all +//! possible constructors. For `Option`, that would mean `_ = None | Some(_)` for +//! example. The wildcard pattern is useful in this case if it is useful when +//! specialized to one of the possible constructors. So we compute: +//! `U(P, p) := ∃(k ϵ constructors) U(S(k, P), S(k, p))` +//! +//! For example, if `P` is: +//! [ +//! [Some(true), _], +//! [None, false], +//! ] +//! and `p` is [_, false], both `None` and `Some` constructors appear in the first +//! components of `P`. We will therefore try popping both constructors in turn: we +//! compute U([[true, _]], [_, false]) for the `Some` constructor, and U([[false]], +//! [false]) for the `None` constructor. The first case returns true, so we know that +//! `p` is useful for `P`. Indeed, it matches `[Some(false), _]` that wasn't matched +//! before. +//! +//! - If `p_1 == r_1 | r_2`, then the usefulness depends on each `r_i` separately: +//! `U(P, p) := U(P, (r_1, p_2, .., p_n)) +//! || U(P, (r_2, p_2, .., p_n))` use std::sync::Arc; use smallvec::{smallvec, SmallVec}; @@ -134,16 +318,26 @@ impl PatStack { ) -> MatchCheckResult> { let result = match (self.head().as_pat(cx), constructor) { (Pat::Tuple(ref pat_ids), Constructor::Tuple { arity }) => { - if pat_ids.len() != *arity { - None - } else { - Some(self.replace_head_with(pat_ids)) + debug_assert_eq!( + pat_ids.len(), + *arity, + "we type check before calling this code, so we should never hit this case", + ); + + Some(self.replace_head_with(pat_ids)) + } + (Pat::Lit(lit_expr), Constructor::Bool(constructor_val)) => { + match cx.body.exprs[lit_expr] { + Expr::Literal(Literal::Bool(pat_val)) if *constructor_val == pat_val => { + Some(self.to_tail()) + } + // it was a bool but the value doesn't match + Expr::Literal(Literal::Bool(_)) => None, + // perhaps this is actually unreachable given we have + // already checked that these match arms have the appropriate type? + _ => return Err(MatchCheckNotImplemented), } } - (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)) => { // enums with no associated data become `Pat::Path` @@ -162,7 +356,7 @@ impl PatStack { Some(self.replace_head_with(pat_ids)) } } - (Pat::Or(_), _) => unreachable!("we desugar or patterns so this should never happen"), + (Pat::Or(_), _) => return Err(MatchCheckNotImplemented), (_, _) => return Err(MatchCheckNotImplemented), }; @@ -186,19 +380,8 @@ impl PatStack { ); 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, - _ => return Err(MatchCheckNotImplemented), - } - } - }; - for _ in 0..arity { + for _ in 0..constructor.arity(cx)? { patterns.push(PatIdOrWild::Wild); } @@ -368,46 +551,23 @@ pub(crate) fn is_useful( // constructors are covered (`Some`/`None`), so we need // to perform specialization to see that our wildcard will cover // the `Some(false)` case. - let mut constructor = None; - for pat in matrix.heads() { - if let Some(c) = pat_constructor(cx, pat)? { - constructor = Some(c); - break; - } + // + // Here we create a constructor for each variant and then check + // usefulness after specializing for that constructor. + let mut found_unimplemented = false; + for constructor in constructor.all_constructors(cx) { + let matrix = matrix.specialize_constructor(&cx, &constructor)?; + let v = v.expand_wildcard(&cx, &constructor)?; + + match is_useful(&cx, &matrix, &v) { + Ok(Usefulness::Useful) => return Ok(Usefulness::Useful), + Ok(Usefulness::NotUseful) => continue, + _ => found_unimplemented = true, + }; } - if let Some(constructor) = 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 mut found_unimplemented = false; - for constructor in - cx.db.enum_data(e.parent).variants.iter().map(|(local_id, _)| { - Constructor::Enum(EnumVariantId { parent: e.parent, local_id }) - }) - { - let matrix = matrix.specialize_constructor(&cx, &constructor)?; - let v = v.expand_wildcard(&cx, &constructor)?; - - match is_useful(&cx, &matrix, &v) { - Ok(Usefulness::Useful) => return Ok(Usefulness::Useful), - Ok(Usefulness::NotUseful) => continue, - _ => found_unimplemented = true, - }; - } - - if found_unimplemented { - Err(MatchCheckNotImplemented) - } else { - Ok(Usefulness::NotUseful) - } - } else { - let matrix = matrix.specialize_constructor(&cx, &constructor)?; - let v = v.expand_wildcard(&cx, &constructor)?; - - is_useful(&cx, &matrix, &v) - } + if found_unimplemented { + Err(MatchCheckNotImplemented) } else { Ok(Usefulness::NotUseful) } @@ -425,7 +585,7 @@ pub(crate) fn is_useful( } } -#[derive(Debug)] +#[derive(Debug, Clone, Copy)] /// Similar to TypeCtor, but includes additional information about the specific /// value being instantiated. For example, TypeCtor::Bool doesn't contain the /// boolean value. @@ -435,6 +595,40 @@ enum Constructor { Enum(EnumVariantId), } +impl Constructor { + fn arity(&self, cx: &MatchCheckCtx) -> MatchCheckResult { + let arity = match self { + 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, + _ => return Err(MatchCheckNotImplemented), + } + } + }; + + Ok(arity) + } + + fn all_constructors(&self, cx: &MatchCheckCtx) -> Vec { + match self { + Constructor::Bool(_) => vec![Constructor::Bool(true), Constructor::Bool(false)], + Constructor::Tuple { .. } => vec![*self], + Constructor::Enum(e) => cx + .db + .enum_data(e.parent) + .variants + .iter() + .map(|(local_id, _)| { + Constructor::Enum(EnumVariantId { parent: e.parent, local_id }) + }) + .collect(), + } + } +} + /// Returns the constructor for the given pattern. Should only return None /// in the case of a Wild pattern. fn pat_constructor(cx: &MatchCheckCtx, pat: PatIdOrWild) -> MatchCheckResult> { @@ -501,14 +695,7 @@ fn all_constructors_covered( } 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 + Some(enum_variant_id.into()) == cx.infer.variant_resolution_for_pat(pat_id) } #[cfg(test)] @@ -522,10 +709,10 @@ mod tests { TestDB::with_single_file(content).0.diagnostics().0 } - pub(super) fn check_diagnostic_with_no_fix(content: &str) { + pub(super) fn check_diagnostic(content: &str) { let diagnostic_count = TestDB::with_single_file(content).0.diagnostics().1; - assert_eq!(1, diagnostic_count, "no diagnotic reported"); + assert_eq!(1, diagnostic_count, "no diagnostic reported"); } pub(super) fn check_no_diagnostic(content: &str) { @@ -558,7 +745,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -596,7 +783,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -621,7 +808,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -646,7 +833,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -659,7 +846,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -685,7 +872,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -698,7 +885,37 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); + } + + #[test] + fn tuple_of_bools_missing_arm() { + let content = r" + fn test_fn() { + match (false, true) { + (false, true) => {}, + (false, false) => {}, + (true, false) => {}, + } + } + "; + + check_diagnostic(content); + } + + #[test] + fn tuple_of_bools_with_wilds() { + let content = r" + fn test_fn() { + match (false, true) { + (false, _) => {}, + (true, false) => {}, + (_, true) => {}, + } + } + "; + + check_no_diagnostic(content); } #[test] @@ -727,7 +944,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -754,7 +971,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -767,7 +984,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -796,7 +1013,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -827,7 +1044,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -844,7 +1061,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -879,7 +1096,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -913,7 +1130,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -931,7 +1148,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -1004,7 +1221,7 @@ mod tests { } "; - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -1089,7 +1306,7 @@ mod tests { "; // Match arms with the incorrect type are filtered out. - check_diagnostic_with_no_fix(content); + check_diagnostic(content); } #[test] @@ -1104,7 +1321,23 @@ mod tests { "; // Match arms with the incorrect type are filtered out. - check_diagnostic_with_no_fix(content); + check_diagnostic(content); + } + + #[test] + fn enum_not_in_scope() { + let content = r" + fn test_fn() { + match Foo::Bar { + Foo::Baz => (), + } + } + "; + + // The enum is not in scope so we don't perform exhaustiveness + // checking, but we want to be sure we don't panic here (and + // we don't create a diagnostic). + check_no_diagnostic(content); } } @@ -1158,17 +1391,21 @@ mod false_negatives { } #[test] - fn enum_not_in_scope() { + fn internal_or() { let content = r" fn test_fn() { - match Foo::Bar { - Foo::Baz => (), + enum Either { + A(bool), + B, + } + match Either::B { + Either::A(true | false) => (), } } "; // This is a false negative. - // The enum is not in scope so we don't perform exhaustiveness checking. + // We do not currently handle patterns with internal `or`s. check_no_diagnostic(content); } } -- cgit v1.2.3