From d1338354ca7afae7088f84756ed103ea94ce82f9 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 10 Apr 2020 20:14:53 -0700 Subject: fix match arm false positive --- crates/ra_hir_ty/src/_match.rs | 15 +++++++++------ crates/ra_hir_ty/src/expr.rs | 13 +++++++------ 2 files changed, 16 insertions(+), 12 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 9e9a9d047..342a78b45 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -1315,8 +1315,9 @@ mod tests { } "; - // Match arms with the incorrect type are filtered out. - check_diagnostic(content); + // Match statements with arms that don't match the + // expression pattern do not fire this diagnostic. + check_no_diagnostic(content); } #[test] @@ -1330,8 +1331,9 @@ mod tests { } "; - // Match arms with the incorrect type are filtered out. - check_diagnostic(content); + // Match statements with arms that don't match the + // expression pattern do not fire this diagnostic. + check_no_diagnostic(content); } #[test] @@ -1344,8 +1346,9 @@ mod tests { } "; - // Match arms with the incorrect type are filtered out. - check_diagnostic(content); + // Match statements with arms that don't match the + // expression pattern do not fire this diagnostic. + check_no_diagnostic(content); } #[test] diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 827b687de..03ef488b9 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -163,12 +163,6 @@ impl<'a, 'b> ExprValidator<'a, 'b> { let mut seen = Matrix::empty(); for pat in pats { - // We skip any patterns whose type we cannot resolve. - // - // This could lead to false positives in this diagnostic, so - // it might be better to skip the entire diagnostic if we either - // cannot resolve a match arm or determine that the match arm has - // the wrong type. if let Some(pat_ty) = infer.type_of_pat.get(pat) { // We only include patterns whose type matches the type // of the match expression. If we had a InvalidMatchArmPattern @@ -191,8 +185,15 @@ impl<'a, 'b> ExprValidator<'a, 'b> { // to the matrix here. let v = PatStack::from_pattern(pat); seen.push(&cx, v); + continue; } } + + // If we can't resolve the type of a pattern, or the pattern type doesn't + // fit the match expression, we skip this diagnostic. Skipping the entire + // diagnostic rather than just not including this match arm is preferred + // to avoid the chance of false positives. + return; } match is_useful(&cx, &seen, &PatStack::from_wild()) { -- cgit v1.2.3 From 26e5bb0a4efbe894d33cde3c1bc3f712fcf33cde Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 11 Apr 2020 07:07:47 -0700 Subject: missing match arms add tests for match expression diverging --- crates/ra_hir_ty/src/_match.rs | 77 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 342a78b45..4be08e9a3 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -1386,6 +1386,42 @@ mod tests { // we don't create a diagnostic). check_no_diagnostic(content); } + + #[test] + fn expr_diverges() { + let content = r" + enum Either { + A, + B, + } + fn test_fn() { + match loop {} { + Either::A => (), + Either::B => (), + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn expr_loop_with_break() { + let content = r" + enum Either { + A, + B, + } + fn test_fn() { + match loop { break Foo::A } { + Either::A => (), + Either::B => (), + } + } + "; + + check_no_diagnostic(content); + } } #[cfg(test)] @@ -1455,4 +1491,45 @@ mod false_negatives { // We do not currently handle patterns with internal `or`s. check_no_diagnostic(content); } + + #[test] + fn expr_diverges_missing_arm() { + let content = r" + enum Either { + A, + B, + } + fn test_fn() { + match loop {} { + Either::A => (), + } + } + "; + + // This is a false negative. + // Even though the match expression diverges, rustc fails + // to compile here since `Either::B` is missing. + check_no_diagnostic(content); + } + + #[test] + fn expr_loop_missing_arm() { + let content = r" + enum Either { + A, + B, + } + fn test_fn() { + match loop { break Foo::A } { + Either::A => (), + } + } + "; + + // This is a false negative. + // We currently infer the type of `loop { break Foo::A }` to `!`, which + // causes us to skip the diagnostic since `Either::A` doesn't type check + // with `!`. + check_no_diagnostic(content); + } } -- cgit v1.2.3 From aec20e50946ea427ceb6a44451459f0cb3a84a4f Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 11 Apr 2020 07:13:47 -0700 Subject: missing match arm add test for partially diverging type --- crates/ra_hir_ty/src/_match.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 4be08e9a3..596194dcf 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -1422,6 +1422,27 @@ mod tests { check_no_diagnostic(content); } + + #[test] + fn expr_partially_diverges() { + let content = r" + enum Either { + A(T), + B, + } + fn foo() -> Either { + Either::B + } + fn test_fn() -> u32 { + match foo() { + Either::A(val) => val, + Either::B => 0, + } + } + "; + + check_no_diagnostic(content); + } } #[cfg(test)] -- cgit v1.2.3 From 5e5eb6a108b00c573455d8d088742592012707be Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 11 Apr 2020 23:33:17 +0200 Subject: Align grammar for record patterns and literals The grammar now looks like this [name_ref :] pat --- crates/ra_hir_ty/src/tests/patterns.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/tests/patterns.rs b/crates/ra_hir_ty/src/tests/patterns.rs index 6e5d2247c..07cbc521a 100644 --- a/crates/ra_hir_ty/src/tests/patterns.rs +++ b/crates/ra_hir_ty/src/tests/patterns.rs @@ -1,7 +1,8 @@ -use super::{infer, infer_with_mismatches}; use insta::assert_snapshot; use test_utils::covers; +use super::{infer, infer_with_mismatches}; + #[test] fn infer_pattern() { assert_snapshot!( -- cgit v1.2.3 From a59179ac2d0894dc45d614242816665b9bd6ef8a Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 11 Apr 2020 20:50:54 -0700 Subject: missing match arms add test cases to demonstrate behavior of tuple with pattern --- crates/ra_hir_ty/src/_match.rs | 75 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 596194dcf..c9a6551ab 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -972,6 +972,47 @@ mod tests { check_no_diagnostic(content); } + #[test] + fn tuple_of_bools_with_ellipsis_at_end_no_diagnostic() { + let content = r" + fn test_fn() { + match (false, true, false) { + (false, ..) => {}, + (true, ..) => {}, + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn tuple_of_bools_with_ellipsis_at_beginning_no_diagnostic() { + let content = r" + fn test_fn() { + match (false, true, false) { + (.., false) => {}, + (.., true) => {}, + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn tuple_of_bools_with_ellipsis_no_diagnostic() { + let content = r" + fn test_fn() { + match (false, true, false) { + (..) => {}, + } + } + "; + + check_no_diagnostic(content); + } + #[test] fn tuple_of_tuple_and_bools_no_arms() { let content = r" @@ -1553,4 +1594,38 @@ mod false_negatives { // with `!`. check_no_diagnostic(content); } + + #[test] + fn tuple_of_bools_with_ellipsis_at_end_missing_arm() { + let content = r" + fn test_fn() { + match (false, true, false) { + (false, ..) => {}, + } + } + "; + + // This is a false negative. + // The `..` pattern is currently lowered to a single `Pat::Wild` + // no matter how many fields the `..` pattern is covering. This + // causes the match arm in this test not to type check against + // the match expression, which causes this diagnostic not to + // fire. + check_no_diagnostic(content); + } + + #[test] + fn tuple_of_bools_with_ellipsis_at_beginning_missing_arm() { + let content = r" + fn test_fn() { + match (false, true, false) { + (.., false) => {}, + } + } + "; + + // This is a false negative. + // See comments on `tuple_of_bools_with_ellipsis_at_end_missing_arm`. + check_no_diagnostic(content); + } } -- cgit v1.2.3 From bb2e5308b77e5d115df17411aaa2dd26be171b0a Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 11 Apr 2020 21:20:52 -0700 Subject: missing match arm add test cases to demonstrate enum tuple struct with ellipsis behavior --- crates/ra_hir_ty/src/_match.rs | 64 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index c9a6551ab..c482cf619 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -1484,6 +1484,45 @@ mod tests { check_no_diagnostic(content); } + + #[test] + fn enum_tuple_partial_ellipsis_no_diagnostic() { + let content = r" + enum Either { + A(bool, bool, bool, bool), + B, + } + fn test_fn() { + match Either::B { + Either::A(true, .., true) => {}, + Either::A(true, .., false) => {}, + Either::A(false, .., true) => {}, + Either::A(false, .., false) => {}, + Either::B => {}, + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn enum_tuple_ellipsis_no_diagnostic() { + let content = r" + enum Either { + A(bool, bool, bool, bool), + B, + } + fn test_fn() { + match Either::B { + Either::A(..) => {}, + Either::B => {}, + } + } + "; + + check_no_diagnostic(content); + } } #[cfg(test)] @@ -1628,4 +1667,29 @@ mod false_negatives { // See comments on `tuple_of_bools_with_ellipsis_at_end_missing_arm`. check_no_diagnostic(content); } + + #[test] + fn enum_tuple_partial_ellipsis_missing_arm() { + let content = r" + enum Either { + A(bool, bool, bool, bool), + B, + } + fn test_fn() { + match Either::B { + Either::A(true, .., true) => {}, + Either::A(true, .., false) => {}, + Either::A(false, .., false) => {}, + Either::B => {}, + } + } + "; + + // This is a false negative. + // The `..` pattern is currently lowered to a single `Pat::Wild` + // no matter how many fields the `..` pattern is covering. This + // causes us to return a `MatchCheckErr::MalformedMatchArm` in + // `Pat::specialize_constructor`. + check_no_diagnostic(content); + } } -- cgit v1.2.3 From ee822d19b7662a9055bc6693c4c40d8dcf752ea1 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sun, 12 Apr 2020 08:40:09 -0700 Subject: handle tuple patterns with ellipsis --- crates/ra_hir_ty/src/_match.rs | 160 +++++++++++++++++++++++++++----------- crates/ra_hir_ty/src/infer/pat.rs | 6 +- 2 files changed, 116 insertions(+), 50 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index c482cf619..a64be9848 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -289,7 +289,7 @@ impl PatStack { Self::from_slice(&self.0[1..]) } - fn replace_head_with(&self, pat_ids: &[PatId]) -> PatStack { + fn replace_head_with + Copy>(&self, pat_ids: &[T]) -> PatStack { let mut patterns: PatStackInner = smallvec![]; for pat in pat_ids { patterns.push((*pat).into()); @@ -320,12 +320,14 @@ impl PatStack { constructor: &Constructor, ) -> MatchCheckResult> { let result = match (self.head().as_pat(cx), constructor) { - (Pat::Tuple(ref pat_ids), Constructor::Tuple { arity }) => { - debug_assert_eq!( - pat_ids.len(), - *arity, - "we type check before calling this code, so we should never hit this case", - ); + (Pat::Tuple { args: ref pat_ids, ellipsis }, Constructor::Tuple { arity: _ }) => { + if ellipsis.is_some() { + // If there are ellipsis here, we should add the correct number of + // Pat::Wild patterns to `pat_ids`. We should be able to use the + // constructors arity for this, but at the time of writing we aren't + // correctly calculating this arity when ellipsis are present. + return Err(MatchCheckErr::NotImplemented); + } Some(self.replace_head_with(pat_ids)) } @@ -351,19 +353,47 @@ impl PatStack { Some(self.to_tail()) } } - (Pat::TupleStruct { args: ref pat_ids, .. }, Constructor::Enum(enum_constructor)) => { + ( + Pat::TupleStruct { args: ref pat_ids, ellipsis, .. }, + Constructor::Enum(enum_constructor), + ) => { let pat_id = self.head().as_id().expect("we know this isn't a wild"); if !enum_variant_matches(cx, pat_id, *enum_constructor) { None } else { - // If the enum variant matches, then we need to confirm - // that the number of patterns aligns with the expected - // number of patterns for that enum variant. - if pat_ids.len() != constructor.arity(cx)? { - return Err(MatchCheckErr::MalformedMatchArm); + let constructor_arity = constructor.arity(cx)?; + if let Some(ellipsis_position) = ellipsis { + // If there are ellipsis in the pattern, the ellipsis must take the place + // of at least one sub-pattern, so `pat_ids` should be smaller than the + // constructor arity. + if pat_ids.len() < constructor_arity { + let mut new_patterns: Vec = vec![]; + + for pat_id in &pat_ids[0..ellipsis_position] { + new_patterns.push((*pat_id).into()); + } + + for _ in 0..(constructor_arity - pat_ids.len()) { + new_patterns.push(PatIdOrWild::Wild); + } + + for pat_id in &pat_ids[ellipsis_position..pat_ids.len()] { + new_patterns.push((*pat_id).into()); + } + + Some(self.replace_head_with(&new_patterns)) + } else { + return Err(MatchCheckErr::MalformedMatchArm); + } + } else { + // 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)) + } else { + return Err(MatchCheckErr::MalformedMatchArm); + } } - - Some(self.replace_head_with(pat_ids)) } } (Pat::Or(_), _) => return Err(MatchCheckErr::NotImplemented), @@ -644,7 +674,11 @@ impl Constructor { 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() }), + // FIXME somehow create the Tuple constructor with the proper arity. If there are + // ellipsis, the arity is not equal to the number of patterns. + Pat::Tuple { args: pats, ellipsis } if ellipsis.is_none() => { + Some(Constructor::Tuple { arity: pats.len() }) + } Pat::Lit(lit_expr) => match cx.body.exprs[lit_expr] { Expr::Literal(Literal::Bool(val)) => Some(Constructor::Bool(val)), _ => return Err(MatchCheckErr::NotImplemented), @@ -1506,6 +1540,67 @@ mod tests { check_no_diagnostic(content); } + #[test] + fn enum_tuple_partial_ellipsis_2_no_diagnostic() { + let content = r" + enum Either { + A(bool, bool, bool, bool), + B, + } + fn test_fn() { + match Either::B { + Either::A(true, .., true) => {}, + Either::A(true, .., false) => {}, + Either::A(.., true) => {}, + Either::A(.., false) => {}, + Either::B => {}, + } + } + "; + + check_no_diagnostic(content); + } + + #[test] + fn enum_tuple_partial_ellipsis_missing_arm() { + let content = r" + enum Either { + A(bool, bool, bool, bool), + B, + } + fn test_fn() { + match Either::B { + Either::A(true, .., true) => {}, + Either::A(true, .., false) => {}, + Either::A(false, .., false) => {}, + Either::B => {}, + } + } + "; + + check_diagnostic(content); + } + + #[test] + fn enum_tuple_partial_ellipsis_2_missing_arm() { + let content = r" + enum Either { + A(bool, bool, bool, bool), + B, + } + fn test_fn() { + match Either::B { + Either::A(true, .., true) => {}, + Either::A(true, .., false) => {}, + Either::A(.., true) => {}, + Either::B => {}, + } + } + "; + + check_diagnostic(content); + } + #[test] fn enum_tuple_ellipsis_no_diagnostic() { let content = r" @@ -1645,11 +1740,7 @@ mod false_negatives { "; // This is a false negative. - // The `..` pattern is currently lowered to a single `Pat::Wild` - // no matter how many fields the `..` pattern is covering. This - // causes the match arm in this test not to type check against - // the match expression, which causes this diagnostic not to - // fire. + // We don't currently handle tuple patterns with ellipsis. check_no_diagnostic(content); } @@ -1664,32 +1755,7 @@ mod false_negatives { "; // This is a false negative. - // See comments on `tuple_of_bools_with_ellipsis_at_end_missing_arm`. - check_no_diagnostic(content); - } - - #[test] - fn enum_tuple_partial_ellipsis_missing_arm() { - let content = r" - enum Either { - A(bool, bool, bool, bool), - B, - } - fn test_fn() { - match Either::B { - Either::A(true, .., true) => {}, - Either::A(true, .., false) => {}, - Either::A(false, .., false) => {}, - Either::B => {}, - } - } - "; - - // This is a false negative. - // The `..` pattern is currently lowered to a single `Pat::Wild` - // no matter how many fields the `..` pattern is covering. This - // causes us to return a `MatchCheckErr::MalformedMatchArm` in - // `Pat::specialize_constructor`. + // We don't currently handle tuple patterns with ellipsis. check_no_diagnostic(content); } } diff --git a/crates/ra_hir_ty/src/infer/pat.rs b/crates/ra_hir_ty/src/infer/pat.rs index 078476f76..8ec4d4ace 100644 --- a/crates/ra_hir_ty/src/infer/pat.rs +++ b/crates/ra_hir_ty/src/infer/pat.rs @@ -85,7 +85,7 @@ impl<'a> InferenceContext<'a> { let body = Arc::clone(&self.body); // avoid borrow checker problem let is_non_ref_pat = match &body[pat] { - Pat::Tuple(..) + Pat::Tuple { .. } | Pat::Or(..) | Pat::TupleStruct { .. } | Pat::Record { .. } @@ -116,7 +116,7 @@ impl<'a> InferenceContext<'a> { let expected = expected; let ty = match &body[pat] { - Pat::Tuple(ref args) => { + Pat::Tuple { ref args, .. } => { let expectations = match expected.as_tuple() { Some(parameters) => &*parameters.0, _ => &[], @@ -155,7 +155,7 @@ impl<'a> InferenceContext<'a> { let subty = self.infer_pat(*pat, expectation, default_bm); Ty::apply_one(TypeCtor::Ref(*mutability), subty) } - Pat::TupleStruct { path: p, args: subpats } => { + Pat::TupleStruct { path: p, args: subpats, .. } => { self.infer_tuple_struct_pat(p.as_ref(), subpats, expected, default_bm, pat) } Pat::Record { path: p, args: fields, ellipsis: _ } => { -- cgit v1.2.3