From 7adb681b1f8dbdc0205efbe22698c28ab9da6378 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Thu, 16 Apr 2020 17:08:24 -0700 Subject: missing match arm diagnostic support enum record type --- crates/ra_hir_ty/src/_match.rs | 336 ++++++++++++++++++++++++++++++++++++---- crates/ra_hir_ty/src/test_db.rs | 36 +++-- 2 files changed, 331 insertions(+), 41 deletions(-) diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 688026a04..779e78574 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -235,10 +235,19 @@ impl From for PatIdOrWild { } } +impl From<&PatId> for PatIdOrWild { + fn from(pat_id: &PatId) -> Self { + Self::PatId(*pat_id) + } +} + #[derive(Debug, Clone, Copy, PartialEq)] pub enum MatchCheckErr { NotImplemented, MalformedMatchArm, + /// Used when type inference cannot resolve the type of + /// a pattern or expression. + Unknown, } /// The return type of `is_useful` is either an indication of usefulness @@ -290,10 +299,14 @@ impl PatStack { Self::from_slice(&self.0[1..]) } - fn replace_head_with + Copy>(&self, pat_ids: &[T]) -> PatStack { + fn replace_head_with(&self, pats: I) -> PatStack + where + I: Iterator, + T: Into, + { let mut patterns: PatStackInner = smallvec![]; - for pat in pat_ids { - patterns.push((*pat).into()); + for pat in pats { + patterns.push(pat.into()); } for pat in &self.0[1..] { patterns.push(*pat); @@ -330,7 +343,7 @@ impl PatStack { return Err(MatchCheckErr::NotImplemented); } - Some(self.replace_head_with(pat_ids)) + Some(self.replace_head_with(pat_ids.iter())) } (Pat::Lit(lit_expr), Constructor::Bool(constructor_val)) => { match cx.body.exprs[lit_expr] { @@ -382,7 +395,7 @@ impl PatStack { new_patterns.push((*pat_id).into()); } - Some(self.replace_head_with(&new_patterns)) + Some(self.replace_head_with(new_patterns.into_iter())) } else { return Err(MatchCheckErr::MalformedMatchArm); } @@ -390,13 +403,41 @@ impl PatStack { // If there is no ellipsis in the tuple pattern, the number // of patterns must equal the constructor arity. if pat_ids.len() == constructor_arity { - Some(self.replace_head_with(pat_ids)) + Some(self.replace_head_with(pat_ids.into_iter())) } else { return Err(MatchCheckErr::MalformedMatchArm); } } } } + (Pat::Record { args: ref arg_patterns, .. }, Constructor::Enum(e)) => { + let pat_id = self.head().as_id().expect("we know this isn't a wild"); + if !enum_variant_matches(cx, pat_id, *e) { + None + } else { + match cx.db.enum_data(e.parent).variants[e.local_id].variant_data.as_ref() { + VariantData::Record(struct_field_arena) => { + // Here we treat any missing fields in the record as the wild pattern, as + // if the record has ellipsis. We want to do this here even if the + // record does not contain ellipsis, because it allows us to continue + // enforcing exhaustiveness for the rest of the match statement. + // + // Creating the diagnostic for the missing field in the pattern + // should be done in a different diagnostic. + let patterns = struct_field_arena.iter().map(|(_, struct_field)| { + arg_patterns + .iter() + .find(|pat| pat.name == struct_field.name) + .map(|pat| PatIdOrWild::from(pat.pat)) + .unwrap_or(PatIdOrWild::Wild) + }); + + Some(self.replace_head_with(patterns)) + } + _ => return Err(MatchCheckErr::Unknown), + } + } + } (Pat::Or(_), _) => return Err(MatchCheckErr::NotImplemented), (_, _) => return Err(MatchCheckErr::NotImplemented), }; @@ -655,8 +696,8 @@ impl Constructor { 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::Record(struct_field_data) => struct_field_data.len(), VariantData::Unit => 0, - _ => return Err(MatchCheckErr::NotImplemented), } } }; @@ -695,10 +736,10 @@ fn pat_constructor(cx: &MatchCheckCtx, pat: PatIdOrWild) -> MatchCheckResult Some(Constructor::Bool(val)), _ => return Err(MatchCheckErr::NotImplemented), }, - Pat::TupleStruct { .. } | Pat::Path(_) => { + Pat::TupleStruct { .. } | Pat::Path(_) | Pat::Record { .. } => { 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).ok_or(MatchCheckErr::NotImplemented)?; + cx.infer.variant_resolution_for_pat(pat_id).ok_or(MatchCheckErr::Unknown)?; match variant_id { VariantId::EnumVariantId(enum_variant_id) => { Some(Constructor::Enum(enum_variant_id)) @@ -759,20 +800,22 @@ mod tests { pub(super) use insta::assert_snapshot; pub(super) use ra_db::fixture::WithFixture; - pub(super) use crate::test_db::TestDB; + pub(super) use crate::{diagnostics::MissingMatchArms, test_db::TestDB}; pub(super) fn check_diagnostic_message(content: &str) -> String { - TestDB::with_single_file(content).0.diagnostics().0 + TestDB::with_single_file(content).0.diagnostic::().0 } pub(super) fn check_diagnostic(content: &str) { - let diagnostic_count = TestDB::with_single_file(content).0.diagnostics().1; + let diagnostic_count = + TestDB::with_single_file(content).0.diagnostic::().1; assert_eq!(1, diagnostic_count, "no diagnostic reported"); } pub(super) fn check_no_diagnostic(content: &str) { - let diagnostic_count = TestDB::with_single_file(content).0.diagnostics().1; + let diagnostic_count = + TestDB::with_single_file(content).0.diagnostic::().1; assert_eq!(0, diagnostic_count, "expected no diagnostic, found one"); } @@ -1531,6 +1574,236 @@ mod tests { check_no_diagnostic(content); } + #[test] + fn enum_record_no_arms() { + let content = r" + enum Either { + A { foo: bool }, + B, + } + fn test_fn() { + let a = Either::A { foo: true }; + match a { + } + } + "; + + check_diagnostic(content); + } + + #[test] + fn enum_record_missing_arms() { + let content = r" + enum Either { + A { foo: bool }, + B, + } + fn test_fn() { + let a = Either::A { foo: true }; + match a { + Either::A { foo: true } => (), + } + } + "; + + check_diagnostic(content); + } + + #[test] + fn enum_record_no_diagnostic() { + let content = r" + enum Either { + A { foo: bool }, + B, + } + fn test_fn() { + let a = Either::A { foo: true }; + match a { + Either::A { foo: true } => (), + Either::A { foo: false } => (), + Either::B => (), + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn enum_record_missing_field_no_diagnostic() { + let content = r" + enum Either { + A { foo: bool }, + B, + } + fn test_fn() { + let a = Either::B; + match a { + Either::A { } => (), + Either::B => (), + } + } + "; + + // When `Either::A` is missing a struct member, we don't want + // to fire the missing match arm diagnostic. This should fire + // some other diagnostic. + check_no_diagnostic(content); + } + + #[test] + fn enum_record_missing_field_missing_match_arm() { + let content = r" + enum Either { + A { foo: bool }, + B, + } + fn test_fn() { + let a = Either::B; + match a { + Either::A { } => (), + } + } + "; + + // Even though `Either::A` is missing fields, we still want to fire + // the missing arm diagnostic here, since we know `Either::B` is missing. + check_diagnostic(content); + } + + #[test] + fn enum_record_no_diagnostic_wild() { + let content = r" + enum Either { + A { foo: bool }, + B, + } + fn test_fn() { + let a = Either::A { foo: true }; + match a { + Either::A { foo: _ } => (), + Either::B => (), + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn enum_record_fields_out_of_order_missing_arm() { + let content = r" + enum Either { + A { foo: bool, bar: () }, + B, + } + fn test_fn() { + let a = Either::A { foo: true }; + match a { + Either::A { bar: (), foo: false } => (), + Either::A { foo: true, bar: () } => (), + } + } + "; + + check_diagnostic(content); + } + + #[test] + fn enum_record_fields_out_of_order_no_diagnostic() { + let content = r" + enum Either { + A { foo: bool, bar: () }, + B, + } + fn test_fn() { + let a = Either::A { foo: true }; + match a { + Either::A { bar: (), foo: false } => (), + Either::A { foo: true, bar: () } => (), + Either::B => (), + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn enum_record_ellipsis_missing_arm() { + let content = r" + enum Either { + A { foo: bool, bar: bool }, + B, + } + fn test_fn() { + match Either::B { + Either::A { foo: true, .. } => (), + Either::B => (), + } + } + "; + + check_diagnostic(content); + } + + #[test] + fn enum_record_ellipsis_no_diagnostic() { + let content = r" + enum Either { + A { foo: bool, bar: bool }, + B, + } + fn test_fn() { + let a = Either::A { foo: true }; + match a { + Either::A { foo: true, .. } => (), + Either::A { foo: false, .. } => (), + Either::B => (), + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn enum_record_ellipsis_all_fields_missing_arm() { + let content = r" + enum Either { + A { foo: bool, bar: bool }, + B, + } + fn test_fn() { + let a = Either::B; + match a { + Either::A { .. } => (), + } + } + "; + + check_diagnostic(content); + } + + #[test] + fn enum_record_ellipsis_all_fields_no_diagnostic() { + let content = r" + enum Either { + A { foo: bool, bar: bool }, + B, + } + fn test_fn() { + let a = Either::B; + match a { + Either::A { .. } => (), + Either::B => (), + } + } + "; + + check_no_diagnostic(content); + } + #[test] fn enum_tuple_partial_ellipsis_no_diagnostic() { let content = r" @@ -1688,25 +1961,6 @@ mod false_negatives { 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 internal_or() { let content = r" @@ -1796,4 +2050,22 @@ mod false_negatives { // We don't currently handle tuple patterns with ellipsis. check_no_diagnostic(content); } + + #[test] + fn struct_missing_arm() { + let content = r" + struct Foo { + a: bool, + } + fn test_fn(f: Foo) { + match f { + Foo { a: true } => {}, + } + } + "; + + // This is a false negative. + // We don't currently handle structs. + check_no_diagnostic(content); + } } diff --git a/crates/ra_hir_ty/src/test_db.rs b/crates/ra_hir_ty/src/test_db.rs index 3a4d58bf9..8498d3d96 100644 --- a/crates/ra_hir_ty/src/test_db.rs +++ b/crates/ra_hir_ty/src/test_db.rs @@ -12,7 +12,7 @@ use ra_db::{ }; use stdx::format_to; -use crate::{db::HirDatabase, expr::ExprValidator}; +use crate::{db::HirDatabase, diagnostics::Diagnostic, expr::ExprValidator}; #[salsa::database( ra_db::SourceDatabaseExtStorage, @@ -104,10 +104,7 @@ impl TestDB { panic!("Can't find module for file") } - // FIXME: don't duplicate this - pub fn diagnostics(&self) -> (String, u32) { - let mut buf = String::new(); - let mut count = 0; + fn diag(&self, mut cb: F) { let crate_graph = self.crate_graph(); for krate in crate_graph.iter() { let crate_def_map = self.crate_def_map(krate); @@ -132,15 +129,36 @@ impl TestDB { for f in fns { 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; - }); + let mut sink = DiagnosticSink::new(&mut cb); infer.add_diagnostics(self, f, &mut sink); let mut validator = ExprValidator::new(f, infer, &mut sink); validator.validate_body(self); } } + } + + pub fn diagnostics(&self) -> (String, u32) { + let mut buf = String::new(); + let mut count = 0; + self.diag(|d| { + format_to!(buf, "{:?}: {}\n", d.syntax_node(self).text(), d.message()); + count += 1; + }); + (buf, count) + } + + /// Like `diagnostics`, but filtered for a single diagnostic. + pub fn diagnostic(&self) -> (String, u32) { + let mut buf = String::new(); + let mut count = 0; + self.diag(|d| { + // We want to filter diagnostics by the particular one we are testing for, to + // avoid surprising results in tests. + if d.downcast_ref::().is_some() { + format_to!(buf, "{:?}: {}\n", d.syntax_node(self).text(), d.message()); + count += 1; + }; + }); (buf, count) } } -- cgit v1.2.3