From d1338354ca7afae7088f84756ed103ea94ce82f9 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 10 Apr 2020 20:14:53 -0700 Subject: [PATCH 1/5] 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(-) diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 9e9a9d04751..342a78b4514 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -1315,8 +1315,9 @@ fn test_fn() { } "; - // 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 @@ fn test_fn() { } "; - // 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 @@ fn test_fn() { } "; - // 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 827b687de17..03ef488b9df 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -163,12 +163,6 @@ fn validate_match( 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 @@ fn validate_match( // 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()) { From 26e5bb0a4efbe894d33cde3c1bc3f712fcf33cde Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 11 Apr 2020 07:07:47 -0700 Subject: [PATCH 2/5] missing match arms add tests for match expression diverging --- crates/ra_hir_ty/src/_match.rs | 77 ++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 342a78b4514..4be08e9a385 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -1386,6 +1386,42 @@ fn test_fn() { // 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 @@ enum Either { // 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); + } } From aec20e50946ea427ceb6a44451459f0cb3a84a4f Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 11 Apr 2020 07:13:47 -0700 Subject: [PATCH 3/5] missing match arm add test for partially diverging type --- crates/ra_hir_ty/src/_match.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 4be08e9a385..596194dcf25 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -1422,6 +1422,27 @@ fn test_fn() { 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)] From a59179ac2d0894dc45d614242816665b9bd6ef8a Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 11 Apr 2020 20:50:54 -0700 Subject: [PATCH 4/5] 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(+) diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 596194dcf25..c9a6551ab01 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -972,6 +972,47 @@ fn test_fn() { 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 @@ fn test_fn() { // 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); + } } From bb2e5308b77e5d115df17411aaa2dd26be171b0a Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 11 Apr 2020 21:20:52 -0700 Subject: [PATCH 5/5] 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(+) diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index c9a6551ab01..c482cf6190a 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -1484,6 +1484,45 @@ fn test_fn() -> u32 { 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 @@ fn test_fn() { // 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); + } }