From b292e1b9da39813e2739cb450c263e7502c97c8d Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 13 Jun 2021 21:44:31 +0300 Subject: internal: refactor missing match arms diagnostics --- crates/hir/src/diagnostics.rs | 19 +- crates/hir/src/lib.rs | 13 +- crates/ide/src/diagnostics.rs | 909 +--------------------- crates/ide/src/diagnostics/missing_match_arms.rs | 924 +++++++++++++++++++++++ 4 files changed, 935 insertions(+), 930 deletions(-) create mode 100644 crates/ide/src/diagnostics/missing_match_arms.rs diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index c2d608eb5..5cffef47f 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -38,6 +38,7 @@ diagnostics![ MacroError, MismatchedArgCount, MissingFields, + MissingMatchArms, MissingOkOrSomeInTailExpr, MissingUnsafe, NoSuchField, @@ -149,9 +150,6 @@ pub struct MissingOkOrSomeInTailExpr { pub required: String, } -// Diagnostic: missing-match-arm -// -// This diagnostic is triggered if `match` block is missing one or more match arms. #[derive(Debug)] pub struct MissingMatchArms { pub file: HirFileId, @@ -159,21 +157,6 @@ pub struct MissingMatchArms { pub arms: AstPtr, } -impl Diagnostic for MissingMatchArms { - fn code(&self) -> DiagnosticCode { - DiagnosticCode("missing-match-arm") - } - fn message(&self) -> String { - String::from("Missing match arm") - } - fn display_source(&self) -> InFile { - InFile { file_id: self.file, value: self.match_expr.clone().into() } - } - fn as_any(&self) -> &(dyn Any + Send + 'static) { - self - } -} - #[derive(Debug)] pub struct InternalBailedOut { pub file: HirFileId, diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index fc147ade3..2e794ff4a 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -1209,11 +1209,14 @@ impl Function { if let (Some(match_expr), Some(arms)) = (match_expr.expr(), match_expr.match_arm_list()) { - sink.push(MissingMatchArms { - file: source_ptr.file_id, - match_expr: AstPtr::new(&match_expr), - arms: AstPtr::new(&arms), - }) + acc.push( + MissingMatchArms { + file: source_ptr.file_id, + match_expr: AstPtr::new(&match_expr), + arms: AstPtr::new(&arms), + } + .into(), + ) } } } diff --git a/crates/ide/src/diagnostics.rs b/crates/ide/src/diagnostics.rs index 7978c1fc2..01b68232e 100644 --- a/crates/ide/src/diagnostics.rs +++ b/crates/ide/src/diagnostics.rs @@ -10,6 +10,7 @@ mod incorrect_case; mod macro_error; mod mismatched_arg_count; mod missing_fields; +mod missing_match_arms; mod missing_ok_or_some_in_tail_expr; mod missing_unsafe; mod no_such_field; @@ -205,6 +206,7 @@ pub(crate) fn diagnostics( AnyDiagnostic::MacroError(d) => macro_error::macro_error(&ctx, &d), AnyDiagnostic::MismatchedArgCount(d) => mismatched_arg_count::mismatched_arg_count(&ctx, &d), AnyDiagnostic::MissingFields(d) => missing_fields::missing_fields(&ctx, &d), + AnyDiagnostic::MissingMatchArms(d) => missing_match_arms::missing_match_arms(&ctx, &d), AnyDiagnostic::MissingOkOrSomeInTailExpr(d) => missing_ok_or_some_in_tail_expr::missing_ok_or_some_in_tail_expr(&ctx, &d), AnyDiagnostic::MissingUnsafe(d) => missing_unsafe::missing_unsafe(&ctx, &d), AnyDiagnostic::NoSuchField(d) => no_such_field::no_such_field(&ctx, &d), @@ -545,910 +547,3 @@ pub struct Claims { ); } } - -#[cfg(test)] -pub(super) mod match_check_tests { - use crate::diagnostics::tests::check_diagnostics; - - #[test] - fn empty_tuple() { - check_diagnostics( - r#" -fn main() { - match () { } - //^^ Missing match arm - match (()) { } - //^^^^ Missing match arm - - match () { _ => (), } - match () { () => (), } - match (()) { (()) => (), } -} -"#, - ); - } - - #[test] - fn tuple_of_two_empty_tuple() { - check_diagnostics( - r#" -fn main() { - match ((), ()) { } - //^^^^^^^^ Missing match arm - - match ((), ()) { ((), ()) => (), } -} -"#, - ); - } - - #[test] - fn boolean() { - check_diagnostics( - r#" -fn test_main() { - match false { } - //^^^^^ Missing match arm - match false { true => (), } - //^^^^^ Missing match arm - match (false, true) {} - //^^^^^^^^^^^^^ Missing match arm - match (false, true) { (true, true) => (), } - //^^^^^^^^^^^^^ Missing match arm - match (false, true) { - //^^^^^^^^^^^^^ Missing match arm - (false, true) => (), - (false, false) => (), - (true, false) => (), - } - match (false, true) { (true, _x) => (), } - //^^^^^^^^^^^^^ Missing match arm - - match false { true => (), false => (), } - match (false, true) { - (false, _) => (), - (true, false) => (), - (_, true) => (), - } - match (false, true) { - (true, true) => (), - (true, false) => (), - (false, true) => (), - (false, false) => (), - } - match (false, true) { - (true, _x) => (), - (false, true) => (), - (false, false) => (), - } - match (false, true, false) { - (false, ..) => (), - (true, ..) => (), - } - match (false, true, false) { - (.., false) => (), - (.., true) => (), - } - match (false, true, false) { (..) => (), } -} -"#, - ); - } - - #[test] - fn tuple_of_tuple_and_bools() { - check_diagnostics( - r#" -fn main() { - match (false, ((), false)) {} - //^^^^^^^^^^^^^^^^^^^^ Missing match arm - match (false, ((), false)) { (true, ((), true)) => (), } - //^^^^^^^^^^^^^^^^^^^^ Missing match arm - match (false, ((), false)) { (true, _) => (), } - //^^^^^^^^^^^^^^^^^^^^ Missing match arm - - match (false, ((), false)) { - (true, ((), true)) => (), - (true, ((), false)) => (), - (false, ((), true)) => (), - (false, ((), false)) => (), - } - match (false, ((), false)) { - (true, ((), true)) => (), - (true, ((), false)) => (), - (false, _) => (), - } -} -"#, - ); - } - - #[test] - fn enums() { - check_diagnostics( - r#" -enum Either { A, B, } - -fn main() { - match Either::A { } - //^^^^^^^^^ Missing match arm - match Either::B { Either::A => (), } - //^^^^^^^^^ Missing match arm - - match &Either::B { - //^^^^^^^^^^ Missing match arm - Either::A => (), - } - - match Either::B { - Either::A => (), Either::B => (), - } - match &Either::B { - Either::A => (), Either::B => (), - } -} -"#, - ); - } - - #[test] - fn enum_containing_bool() { - check_diagnostics( - r#" -enum Either { A(bool), B } - -fn main() { - match Either::B { } - //^^^^^^^^^ Missing match arm - match Either::B { - //^^^^^^^^^ Missing match arm - Either::A(true) => (), Either::B => () - } - - match Either::B { - Either::A(true) => (), - Either::A(false) => (), - Either::B => (), - } - match Either::B { - Either::B => (), - _ => (), - } - match Either::B { - Either::A(_) => (), - Either::B => (), - } - -} - "#, - ); - } - - #[test] - fn enum_different_sizes() { - check_diagnostics( - r#" -enum Either { A(bool), B(bool, bool) } - -fn main() { - match Either::A(false) { - //^^^^^^^^^^^^^^^^ Missing match arm - Either::A(_) => (), - Either::B(false, _) => (), - } - - match Either::A(false) { - Either::A(_) => (), - Either::B(true, _) => (), - Either::B(false, _) => (), - } - match Either::A(false) { - Either::A(true) | Either::A(false) => (), - Either::B(true, _) => (), - Either::B(false, _) => (), - } -} -"#, - ); - } - - #[test] - fn tuple_of_enum_no_diagnostic() { - check_diagnostics( - r#" -enum Either { A(bool), B(bool, bool) } -enum Either2 { C, D } - -fn main() { - 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) => (), - } -} -"#, - ); - } - - #[test] - fn or_pattern_no_diagnostic() { - check_diagnostics( - r#" -enum Either {A, B} - -fn main() { - match (Either::A, Either::B) { - (Either::A | Either::B, _) => (), - } -}"#, - ) - } - - #[test] - fn mismatched_types() { - // Match statements with arms that don't match the - // expression pattern do not fire this diagnostic. - check_diagnostics( - r#" -enum Either { A, B } -enum Either2 { C, D } - -fn main() { - match Either::A { - Either2::C => (), - // ^^^^^^^^^^ Internal: match check bailed out - Either2::D => (), - } - match (true, false) { - (true, false, true) => (), - // ^^^^^^^^^^^^^^^^^^^ Internal: match check bailed out - (true) => (), - } - match (true, false) { (true,) => {} } - // ^^^^^^^ Internal: match check bailed out - match (0) { () => () } - // ^^ Internal: match check bailed out - match Unresolved::Bar { Unresolved::Baz => () } -} - "#, - ); - } - - #[test] - fn mismatched_types_in_or_patterns() { - check_diagnostics( - r#" -fn main() { - match false { true | () => {} } - // ^^^^^^^^^ Internal: match check bailed out - match (false,) { (true | (),) => {} } - // ^^^^^^^^^^^^ Internal: match check bailed out -} -"#, - ); - } - - #[test] - fn malformed_match_arm_tuple_enum_missing_pattern() { - // We are testing to be sure we don't panic here when the match - // arm `Either::B` is missing its pattern. - check_diagnostics( - r#" -enum Either { A, B(u32) } - -fn main() { - match Either::A { - Either::A => (), - Either::B() => (), - } -} -"#, - ); - } - - #[test] - fn malformed_match_arm_extra_fields() { - check_diagnostics( - r#" -enum A { B(isize, isize), C } -fn main() { - match A::B(1, 2) { - A::B(_, _, _) => (), - // ^^^^^^^^^^^^^ Internal: match check bailed out - } - match A::B(1, 2) { - A::C(_) => (), - // ^^^^^^^ Internal: match check bailed out - } -} -"#, - ); - } - - #[test] - fn expr_diverges() { - check_diagnostics( - r#" -enum Either { A, B } - -fn main() { - match loop {} { - Either::A => (), - // ^^^^^^^^^ Internal: match check bailed out - Either::B => (), - } - match loop {} { - Either::A => (), - // ^^^^^^^^^ Internal: match check bailed out - } - match loop { break Foo::A } { - //^^^^^^^^^^^^^^^^^^^^^ Missing match arm - Either::A => (), - } - match loop { break Foo::A } { - Either::A => (), - Either::B => (), - } -} -"#, - ); - } - - #[test] - fn expr_partially_diverges() { - check_diagnostics( - r#" -enum Either { A(T), B } - -fn foo() -> Either { Either::B } -fn main() -> u32 { - match foo() { - Either::A(val) => val, - Either::B => 0, - } -} -"#, - ); - } - - #[test] - fn enum_record() { - check_diagnostics( - r#" -enum Either { A { foo: bool }, B } - -fn main() { - let a = Either::A { foo: true }; - match a { } - //^ Missing match arm - match a { Either::A { foo: true } => () } - //^ Missing match arm - match a { - Either::A { } => (), - //^^^^^^^^^ Missing structure fields: - // | - foo - Either::B => (), - } - match a { - //^ Missing match arm - Either::A { } => (), - } //^^^^^^^^^ Missing structure fields: - // | - foo - - match a { - Either::A { foo: true } => (), - Either::A { foo: false } => (), - Either::B => (), - } - match a { - Either::A { foo: _ } => (), - Either::B => (), - } -} -"#, - ); - } - - #[test] - fn enum_record_fields_out_of_order() { - check_diagnostics( - r#" -enum Either { - A { foo: bool, bar: () }, - B, -} - -fn main() { - let a = Either::A { foo: true, bar: () }; - match a { - //^ Missing match arm - Either::A { bar: (), foo: false } => (), - Either::A { foo: true, bar: () } => (), - } - - match a { - Either::A { bar: (), foo: false } => (), - Either::A { foo: true, bar: () } => (), - Either::B => (), - } -} -"#, - ); - } - - #[test] - fn enum_record_ellipsis() { - check_diagnostics( - r#" -enum Either { - A { foo: bool, bar: bool }, - B, -} - -fn main() { - let a = Either::B; - match a { - //^ Missing match arm - Either::A { foo: true, .. } => (), - Either::B => (), - } - match a { - //^ Missing match arm - Either::A { .. } => (), - } - - match a { - Either::A { foo: true, .. } => (), - Either::A { foo: false, .. } => (), - Either::B => (), - } - - match a { - Either::A { .. } => (), - Either::B => (), - } -} -"#, - ); - } - - #[test] - fn enum_tuple_partial_ellipsis() { - check_diagnostics( - r#" -enum Either { - A(bool, bool, bool, bool), - B, -} - -fn main() { - match Either::B { - //^^^^^^^^^ Missing match arm - Either::A(true, .., true) => (), - Either::A(true, .., false) => (), - Either::A(false, .., false) => (), - Either::B => (), - } - match Either::B { - //^^^^^^^^^ Missing match arm - Either::A(true, .., true) => (), - Either::A(true, .., false) => (), - Either::A(.., true) => (), - Either::B => (), - } - - match Either::B { - Either::A(true, .., true) => (), - Either::A(true, .., false) => (), - Either::A(false, .., true) => (), - Either::A(false, .., false) => (), - Either::B => (), - } - match Either::B { - Either::A(true, .., true) => (), - Either::A(true, .., false) => (), - Either::A(.., true) => (), - Either::A(.., false) => (), - Either::B => (), - } -} -"#, - ); - } - - #[test] - fn never() { - check_diagnostics( - r#" -enum Never {} - -fn enum_(never: Never) { - match never {} -} -fn enum_ref(never: &Never) { - match never {} - //^^^^^ Missing match arm -} -fn bang(never: !) { - match never {} -} -"#, - ); - } - - #[test] - fn unknown_type() { - check_diagnostics( - r#" -enum Option { Some(T), None } - -fn main() { - // `Never` is deliberately not defined so that it's an uninferred type. - match Option::::None { - None => (), - Some(never) => match never {}, - // ^^^^^^^^^^^ Internal: match check bailed out - } - match Option::::None { - //^^^^^^^^^^^^^^^^^^^^^ Missing match arm - Option::Some(_never) => {}, - } -} -"#, - ); - } - - #[test] - fn tuple_of_bools_with_ellipsis_at_end_missing_arm() { - check_diagnostics( - r#" -fn main() { - match (false, true, false) { - //^^^^^^^^^^^^^^^^^^^^ Missing match arm - (false, ..) => (), - } -}"#, - ); - } - - #[test] - fn tuple_of_bools_with_ellipsis_at_beginning_missing_arm() { - check_diagnostics( - r#" -fn main() { - match (false, true, false) { - //^^^^^^^^^^^^^^^^^^^^ Missing match arm - (.., false) => (), - } -}"#, - ); - } - - #[test] - fn tuple_of_bools_with_ellipsis_in_middle_missing_arm() { - check_diagnostics( - r#" -fn main() { - match (false, true, false) { - //^^^^^^^^^^^^^^^^^^^^ Missing match arm - (true, .., false) => (), - } -}"#, - ); - } - - #[test] - fn record_struct() { - check_diagnostics( - r#"struct Foo { a: bool } -fn main(f: Foo) { - match f {} - //^ Missing match arm - match f { Foo { a: true } => () } - //^ Missing match arm - match &f { Foo { a: true } => () } - //^^ Missing match arm - match f { Foo { a: _ } => () } - match f { - Foo { a: true } => (), - Foo { a: false } => (), - } - match &f { - Foo { a: true } => (), - Foo { a: false } => (), - } -} -"#, - ); - } - - #[test] - fn tuple_struct() { - check_diagnostics( - r#"struct Foo(bool); -fn main(f: Foo) { - match f {} - //^ Missing match arm - match f { Foo(true) => () } - //^ Missing match arm - match f { - Foo(true) => (), - Foo(false) => (), - } -} -"#, - ); - } - - #[test] - fn unit_struct() { - check_diagnostics( - r#"struct Foo; -fn main(f: Foo) { - match f {} - //^ Missing match arm - match f { Foo => () } -} -"#, - ); - } - - #[test] - fn record_struct_ellipsis() { - check_diagnostics( - r#"struct Foo { foo: bool, bar: bool } -fn main(f: Foo) { - match f { Foo { foo: true, .. } => () } - //^ Missing match arm - match f { - //^ Missing match arm - Foo { foo: true, .. } => (), - Foo { bar: false, .. } => () - } - match f { Foo { .. } => () } - match f { - Foo { foo: true, .. } => (), - Foo { foo: false, .. } => () - } -} -"#, - ); - } - - #[test] - fn internal_or() { - check_diagnostics( - r#" -fn main() { - enum Either { A(bool), B } - match Either::B { - //^^^^^^^^^ Missing match arm - Either::A(true | false) => (), - } -} -"#, - ); - } - - #[test] - fn no_panic_at_unimplemented_subpattern_type() { - check_diagnostics( - r#" -struct S { a: char} -fn main(v: S) { - match v { S{ a } => {} } - match v { S{ a: _x } => {} } - match v { S{ a: 'a' } => {} } - //^^^^^^^^^^^ Internal: match check bailed out - match v { S{..} => {} } - match v { _ => {} } - match v { } - //^ Missing match arm -} -"#, - ); - } - - #[test] - fn binding() { - check_diagnostics( - r#" -fn main() { - match true { - _x @ true => {} - false => {} - } - match true { _x @ true => {} } - //^^^^ Missing match arm -} -"#, - ); - } - - #[test] - fn binding_ref_has_correct_type() { - // Asserts `PatKind::Binding(ref _x): bool`, not &bool. - // If that's not true match checking will panic with "incompatible constructors" - // FIXME: make facilities to test this directly like `tests::check_infer(..)` - check_diagnostics( - r#" -enum Foo { A } -fn main() { - // FIXME: this should not bail out but current behavior is such as the old algorithm. - // ExprValidator::validate_match(..) checks types of top level patterns incorrecly. - match Foo::A { - ref _x => {} - // ^^^^^^ Internal: match check bailed out - Foo::A => {} - } - match (true,) { - (ref _x,) => {} - (true,) => {} - } -} -"#, - ); - } - - #[test] - fn enum_non_exhaustive() { - check_diagnostics( - r#" -//- /lib.rs crate:lib -#[non_exhaustive] -pub enum E { A, B } -fn _local() { - match E::A { _ => {} } - match E::A { - E::A => {} - E::B => {} - } - match E::A { - E::A | E::B => {} - } -} - -//- /main.rs crate:main deps:lib -use lib::E; -fn main() { - match E::A { _ => {} } - match E::A { - //^^^^ Missing match arm - E::A => {} - E::B => {} - } - match E::A { - //^^^^ Missing match arm - E::A | E::B => {} - } -} -"#, - ); - } - - #[test] - fn match_guard() { - check_diagnostics( - r#" -fn main() { - match true { - true if false => {} - true => {} - false => {} - } - match true { - //^^^^ Missing match arm - true if false => {} - false => {} - } -} -"#, - ); - } - - #[test] - fn pattern_type_is_of_substitution() { - cov_mark::check!(match_check_wildcard_expanded_to_substitutions); - check_diagnostics( - r#" -struct Foo(T); -struct Bar; -fn main() { - match Foo(Bar) { - _ | Foo(Bar) => {} - } -} -"#, - ); - } - - #[test] - fn record_struct_no_such_field() { - check_diagnostics( - r#" -struct Foo { } -fn main(f: Foo) { - match f { Foo { bar } => () } - // ^^^^^^^^^^^ Internal: match check bailed out -} -"#, - ); - } - - #[test] - fn match_ergonomics_issue_9095() { - check_diagnostics( - r#" -enum Foo { A(T) } -fn main() { - match &Foo::A(true) { - _ => {} - Foo::A(_) => {} - } -} -"#, - ); - } - - 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::*; - - #[test] - fn integers() { - // We don't currently check integer exhaustiveness. - check_diagnostics( - r#" -fn main() { - match 5 { - 10 => (), - // ^^ Internal: match check bailed out - 11..20 => (), - } -} -"#, - ); - } - - #[test] - fn reference_patterns_at_top_level() { - check_diagnostics( - r#" -fn main() { - match &false { - &true => {} - // ^^^^^ Internal: match check bailed out - } -} - "#, - ); - } - - #[test] - fn reference_patterns_in_fields() { - check_diagnostics( - r#" -fn main() { - match (&false,) { - (true,) => {} - // ^^^^^^^ Internal: match check bailed out - } - match (&false,) { - (&true,) => {} - // ^^^^^^^^ Internal: match check bailed out - } -} - "#, - ); - } - } -} diff --git a/crates/ide/src/diagnostics/missing_match_arms.rs b/crates/ide/src/diagnostics/missing_match_arms.rs new file mode 100644 index 000000000..6b977fa59 --- /dev/null +++ b/crates/ide/src/diagnostics/missing_match_arms.rs @@ -0,0 +1,924 @@ +use hir::InFile; + +use crate::diagnostics::{Diagnostic, DiagnosticsContext}; + +// Diagnostic: missing-match-arm +// +// This diagnostic is triggered if `match` block is missing one or more match arms. +pub(super) fn missing_match_arms( + ctx: &DiagnosticsContext<'_>, + d: &hir::MissingMatchArms, +) -> Diagnostic { + Diagnostic::new( + "missing-match-arm", + "missing match arm", + ctx.sema.diagnostics_display_range(InFile::new(d.file, d.match_expr.clone().into())).range, + ) +} + +#[cfg(test)] +pub(super) mod tests { + use crate::diagnostics::tests::check_diagnostics; + + #[test] + fn empty_tuple() { + check_diagnostics( + r#" +fn main() { + match () { } + //^^ missing match arm + match (()) { } + //^^^^ missing match arm + + match () { _ => (), } + match () { () => (), } + match (()) { (()) => (), } +} +"#, + ); + } + + #[test] + fn tuple_of_two_empty_tuple() { + check_diagnostics( + r#" +fn main() { + match ((), ()) { } + //^^^^^^^^ missing match arm + + match ((), ()) { ((), ()) => (), } +} +"#, + ); + } + + #[test] + fn boolean() { + check_diagnostics( + r#" +fn test_main() { + match false { } + //^^^^^ missing match arm + match false { true => (), } + //^^^^^ missing match arm + match (false, true) {} + //^^^^^^^^^^^^^ missing match arm + match (false, true) { (true, true) => (), } + //^^^^^^^^^^^^^ missing match arm + match (false, true) { + //^^^^^^^^^^^^^ missing match arm + (false, true) => (), + (false, false) => (), + (true, false) => (), + } + match (false, true) { (true, _x) => (), } + //^^^^^^^^^^^^^ missing match arm + + match false { true => (), false => (), } + match (false, true) { + (false, _) => (), + (true, false) => (), + (_, true) => (), + } + match (false, true) { + (true, true) => (), + (true, false) => (), + (false, true) => (), + (false, false) => (), + } + match (false, true) { + (true, _x) => (), + (false, true) => (), + (false, false) => (), + } + match (false, true, false) { + (false, ..) => (), + (true, ..) => (), + } + match (false, true, false) { + (.., false) => (), + (.., true) => (), + } + match (false, true, false) { (..) => (), } +} +"#, + ); + } + + #[test] + fn tuple_of_tuple_and_bools() { + check_diagnostics( + r#" +fn main() { + match (false, ((), false)) {} + //^^^^^^^^^^^^^^^^^^^^ missing match arm + match (false, ((), false)) { (true, ((), true)) => (), } + //^^^^^^^^^^^^^^^^^^^^ missing match arm + match (false, ((), false)) { (true, _) => (), } + //^^^^^^^^^^^^^^^^^^^^ missing match arm + + match (false, ((), false)) { + (true, ((), true)) => (), + (true, ((), false)) => (), + (false, ((), true)) => (), + (false, ((), false)) => (), + } + match (false, ((), false)) { + (true, ((), true)) => (), + (true, ((), false)) => (), + (false, _) => (), + } +} +"#, + ); + } + + #[test] + fn enums() { + check_diagnostics( + r#" +enum Either { A, B, } + +fn main() { + match Either::A { } + //^^^^^^^^^ missing match arm + match Either::B { Either::A => (), } + //^^^^^^^^^ missing match arm + + match &Either::B { + //^^^^^^^^^^ missing match arm + Either::A => (), + } + + match Either::B { + Either::A => (), Either::B => (), + } + match &Either::B { + Either::A => (), Either::B => (), + } +} +"#, + ); + } + + #[test] + fn enum_containing_bool() { + check_diagnostics( + r#" +enum Either { A(bool), B } + +fn main() { + match Either::B { } + //^^^^^^^^^ missing match arm + match Either::B { + //^^^^^^^^^ missing match arm + Either::A(true) => (), Either::B => () + } + + match Either::B { + Either::A(true) => (), + Either::A(false) => (), + Either::B => (), + } + match Either::B { + Either::B => (), + _ => (), + } + match Either::B { + Either::A(_) => (), + Either::B => (), + } + +} + "#, + ); + } + + #[test] + fn enum_different_sizes() { + check_diagnostics( + r#" +enum Either { A(bool), B(bool, bool) } + +fn main() { + match Either::A(false) { + //^^^^^^^^^^^^^^^^ missing match arm + Either::A(_) => (), + Either::B(false, _) => (), + } + + match Either::A(false) { + Either::A(_) => (), + Either::B(true, _) => (), + Either::B(false, _) => (), + } + match Either::A(false) { + Either::A(true) | Either::A(false) => (), + Either::B(true, _) => (), + Either::B(false, _) => (), + } +} +"#, + ); + } + + #[test] + fn tuple_of_enum_no_diagnostic() { + check_diagnostics( + r#" +enum Either { A(bool), B(bool, bool) } +enum Either2 { C, D } + +fn main() { + 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) => (), + } +} +"#, + ); + } + + #[test] + fn or_pattern_no_diagnostic() { + check_diagnostics( + r#" +enum Either {A, B} + +fn main() { + match (Either::A, Either::B) { + (Either::A | Either::B, _) => (), + } +}"#, + ) + } + + #[test] + fn mismatched_types() { + // Match statements with arms that don't match the + // expression pattern do not fire this diagnostic. + check_diagnostics( + r#" +enum Either { A, B } +enum Either2 { C, D } + +fn main() { + match Either::A { + Either2::C => (), + // ^^^^^^^^^^ Internal: match check bailed out + Either2::D => (), + } + match (true, false) { + (true, false, true) => (), + // ^^^^^^^^^^^^^^^^^^^ Internal: match check bailed out + (true) => (), + } + match (true, false) { (true,) => {} } + // ^^^^^^^ Internal: match check bailed out + match (0) { () => () } + // ^^ Internal: match check bailed out + match Unresolved::Bar { Unresolved::Baz => () } +} + "#, + ); + } + + #[test] + fn mismatched_types_in_or_patterns() { + check_diagnostics( + r#" +fn main() { + match false { true | () => {} } + // ^^^^^^^^^ Internal: match check bailed out + match (false,) { (true | (),) => {} } + // ^^^^^^^^^^^^ Internal: match check bailed out +} +"#, + ); + } + + #[test] + fn malformed_match_arm_tuple_enum_missing_pattern() { + // We are testing to be sure we don't panic here when the match + // arm `Either::B` is missing its pattern. + check_diagnostics( + r#" +enum Either { A, B(u32) } + +fn main() { + match Either::A { + Either::A => (), + Either::B() => (), + } +} +"#, + ); + } + + #[test] + fn malformed_match_arm_extra_fields() { + check_diagnostics( + r#" +enum A { B(isize, isize), C } +fn main() { + match A::B(1, 2) { + A::B(_, _, _) => (), + // ^^^^^^^^^^^^^ Internal: match check bailed out + } + match A::B(1, 2) { + A::C(_) => (), + // ^^^^^^^ Internal: match check bailed out + } +} +"#, + ); + } + + #[test] + fn expr_diverges() { + check_diagnostics( + r#" +enum Either { A, B } + +fn main() { + match loop {} { + Either::A => (), + // ^^^^^^^^^ Internal: match check bailed out + Either::B => (), + } + match loop {} { + Either::A => (), + // ^^^^^^^^^ Internal: match check bailed out + } + match loop { break Foo::A } { + //^^^^^^^^^^^^^^^^^^^^^ missing match arm + Either::A => (), + } + match loop { break Foo::A } { + Either::A => (), + Either::B => (), + } +} +"#, + ); + } + + #[test] + fn expr_partially_diverges() { + check_diagnostics( + r#" +enum Either { A(T), B } + +fn foo() -> Either { Either::B } +fn main() -> u32 { + match foo() { + Either::A(val) => val, + Either::B => 0, + } +} +"#, + ); + } + + #[test] + fn enum_record() { + check_diagnostics( + r#" +enum Either { A { foo: bool }, B } + +fn main() { + let a = Either::A { foo: true }; + match a { } + //^ missing match arm + match a { Either::A { foo: true } => () } + //^ missing match arm + match a { + Either::A { } => (), + //^^^^^^^^^ Missing structure fields: + // | - foo + Either::B => (), + } + match a { + //^ missing match arm + Either::A { } => (), + } //^^^^^^^^^ Missing structure fields: + // | - foo + + match a { + Either::A { foo: true } => (), + Either::A { foo: false } => (), + Either::B => (), + } + match a { + Either::A { foo: _ } => (), + Either::B => (), + } +} +"#, + ); + } + + #[test] + fn enum_record_fields_out_of_order() { + check_diagnostics( + r#" +enum Either { + A { foo: bool, bar: () }, + B, +} + +fn main() { + let a = Either::A { foo: true, bar: () }; + match a { + //^ missing match arm + Either::A { bar: (), foo: false } => (), + Either::A { foo: true, bar: () } => (), + } + + match a { + Either::A { bar: (), foo: false } => (), + Either::A { foo: true, bar: () } => (), + Either::B => (), + } +} +"#, + ); + } + + #[test] + fn enum_record_ellipsis() { + check_diagnostics( + r#" +enum Either { + A { foo: bool, bar: bool }, + B, +} + +fn main() { + let a = Either::B; + match a { + //^ missing match arm + Either::A { foo: true, .. } => (), + Either::B => (), + } + match a { + //^ missing match arm + Either::A { .. } => (), + } + + match a { + Either::A { foo: true, .. } => (), + Either::A { foo: false, .. } => (), + Either::B => (), + } + + match a { + Either::A { .. } => (), + Either::B => (), + } +} +"#, + ); + } + + #[test] + fn enum_tuple_partial_ellipsis() { + check_diagnostics( + r#" +enum Either { + A(bool, bool, bool, bool), + B, +} + +fn main() { + match Either::B { + //^^^^^^^^^ missing match arm + Either::A(true, .., true) => (), + Either::A(true, .., false) => (), + Either::A(false, .., false) => (), + Either::B => (), + } + match Either::B { + //^^^^^^^^^ missing match arm + Either::A(true, .., true) => (), + Either::A(true, .., false) => (), + Either::A(.., true) => (), + Either::B => (), + } + + match Either::B { + Either::A(true, .., true) => (), + Either::A(true, .., false) => (), + Either::A(false, .., true) => (), + Either::A(false, .., false) => (), + Either::B => (), + } + match Either::B { + Either::A(true, .., true) => (), + Either::A(true, .., false) => (), + Either::A(.., true) => (), + Either::A(.., false) => (), + Either::B => (), + } +} +"#, + ); + } + + #[test] + fn never() { + check_diagnostics( + r#" +enum Never {} + +fn enum_(never: Never) { + match never {} +} +fn enum_ref(never: &Never) { + match never {} + //^^^^^ missing match arm +} +fn bang(never: !) { + match never {} +} +"#, + ); + } + + #[test] + fn unknown_type() { + check_diagnostics( + r#" +enum Option { Some(T), None } + +fn main() { + // `Never` is deliberately not defined so that it's an uninferred type. + match Option::::None { + None => (), + Some(never) => match never {}, + // ^^^^^^^^^^^ Internal: match check bailed out + } + match Option::::None { + //^^^^^^^^^^^^^^^^^^^^^ missing match arm + Option::Some(_never) => {}, + } +} +"#, + ); + } + + #[test] + fn tuple_of_bools_with_ellipsis_at_end_missing_arm() { + check_diagnostics( + r#" +fn main() { + match (false, true, false) { + //^^^^^^^^^^^^^^^^^^^^ missing match arm + (false, ..) => (), + } +}"#, + ); + } + + #[test] + fn tuple_of_bools_with_ellipsis_at_beginning_missing_arm() { + check_diagnostics( + r#" +fn main() { + match (false, true, false) { + //^^^^^^^^^^^^^^^^^^^^ missing match arm + (.., false) => (), + } +}"#, + ); + } + + #[test] + fn tuple_of_bools_with_ellipsis_in_middle_missing_arm() { + check_diagnostics( + r#" +fn main() { + match (false, true, false) { + //^^^^^^^^^^^^^^^^^^^^ missing match arm + (true, .., false) => (), + } +}"#, + ); + } + + #[test] + fn record_struct() { + check_diagnostics( + r#"struct Foo { a: bool } +fn main(f: Foo) { + match f {} + //^ missing match arm + match f { Foo { a: true } => () } + //^ missing match arm + match &f { Foo { a: true } => () } + //^^ missing match arm + match f { Foo { a: _ } => () } + match f { + Foo { a: true } => (), + Foo { a: false } => (), + } + match &f { + Foo { a: true } => (), + Foo { a: false } => (), + } +} +"#, + ); + } + + #[test] + fn tuple_struct() { + check_diagnostics( + r#"struct Foo(bool); +fn main(f: Foo) { + match f {} + //^ missing match arm + match f { Foo(true) => () } + //^ missing match arm + match f { + Foo(true) => (), + Foo(false) => (), + } +} +"#, + ); + } + + #[test] + fn unit_struct() { + check_diagnostics( + r#"struct Foo; +fn main(f: Foo) { + match f {} + //^ missing match arm + match f { Foo => () } +} +"#, + ); + } + + #[test] + fn record_struct_ellipsis() { + check_diagnostics( + r#"struct Foo { foo: bool, bar: bool } +fn main(f: Foo) { + match f { Foo { foo: true, .. } => () } + //^ missing match arm + match f { + //^ missing match arm + Foo { foo: true, .. } => (), + Foo { bar: false, .. } => () + } + match f { Foo { .. } => () } + match f { + Foo { foo: true, .. } => (), + Foo { foo: false, .. } => () + } +} +"#, + ); + } + + #[test] + fn internal_or() { + check_diagnostics( + r#" +fn main() { + enum Either { A(bool), B } + match Either::B { + //^^^^^^^^^ missing match arm + Either::A(true | false) => (), + } +} +"#, + ); + } + + #[test] + fn no_panic_at_unimplemented_subpattern_type() { + check_diagnostics( + r#" +struct S { a: char} +fn main(v: S) { + match v { S{ a } => {} } + match v { S{ a: _x } => {} } + match v { S{ a: 'a' } => {} } + //^^^^^^^^^^^ Internal: match check bailed out + match v { S{..} => {} } + match v { _ => {} } + match v { } + //^ missing match arm +} +"#, + ); + } + + #[test] + fn binding() { + check_diagnostics( + r#" +fn main() { + match true { + _x @ true => {} + false => {} + } + match true { _x @ true => {} } + //^^^^ missing match arm +} +"#, + ); + } + + #[test] + fn binding_ref_has_correct_type() { + // Asserts `PatKind::Binding(ref _x): bool`, not &bool. + // If that's not true match checking will panic with "incompatible constructors" + // FIXME: make facilities to test this directly like `tests::check_infer(..)` + check_diagnostics( + r#" +enum Foo { A } +fn main() { + // FIXME: this should not bail out but current behavior is such as the old algorithm. + // ExprValidator::validate_match(..) checks types of top level patterns incorrecly. + match Foo::A { + ref _x => {} + // ^^^^^^ Internal: match check bailed out + Foo::A => {} + } + match (true,) { + (ref _x,) => {} + (true,) => {} + } +} +"#, + ); + } + + #[test] + fn enum_non_exhaustive() { + check_diagnostics( + r#" +//- /lib.rs crate:lib +#[non_exhaustive] +pub enum E { A, B } +fn _local() { + match E::A { _ => {} } + match E::A { + E::A => {} + E::B => {} + } + match E::A { + E::A | E::B => {} + } +} + +//- /main.rs crate:main deps:lib +use lib::E; +fn main() { + match E::A { _ => {} } + match E::A { + //^^^^ missing match arm + E::A => {} + E::B => {} + } + match E::A { + //^^^^ missing match arm + E::A | E::B => {} + } +} +"#, + ); + } + + #[test] + fn match_guard() { + check_diagnostics( + r#" +fn main() { + match true { + true if false => {} + true => {} + false => {} + } + match true { + //^^^^ missing match arm + true if false => {} + false => {} + } +} +"#, + ); + } + + #[test] + fn pattern_type_is_of_substitution() { + cov_mark::check!(match_check_wildcard_expanded_to_substitutions); + check_diagnostics( + r#" +struct Foo(T); +struct Bar; +fn main() { + match Foo(Bar) { + _ | Foo(Bar) => {} + } +} +"#, + ); + } + + #[test] + fn record_struct_no_such_field() { + check_diagnostics( + r#" +struct Foo { } +fn main(f: Foo) { + match f { Foo { bar } => () } + // ^^^^^^^^^^^ Internal: match check bailed out +} +"#, + ); + } + + #[test] + fn match_ergonomics_issue_9095() { + check_diagnostics( + r#" +enum Foo { A(T) } +fn main() { + match &Foo::A(true) { + _ => {} + Foo::A(_) => {} + } +} +"#, + ); + } + + 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::*; + + #[test] + fn integers() { + // We don't currently check integer exhaustiveness. + check_diagnostics( + r#" +fn main() { + match 5 { + 10 => (), + // ^^ Internal: match check bailed out + 11..20 => (), + } +} +"#, + ); + } + + #[test] + fn reference_patterns_at_top_level() { + check_diagnostics( + r#" +fn main() { + match &false { + &true => {} + // ^^^^^ Internal: match check bailed out + } +} + "#, + ); + } + + #[test] + fn reference_patterns_in_fields() { + check_diagnostics( + r#" +fn main() { + match (&false,) { + (true,) => {} + // ^^^^^^^ Internal: match check bailed out + } + match (&false,) { + (&true,) => {} + // ^^^^^^^^ Internal: match check bailed out + } +} + "#, + ); + } + } +} -- cgit v1.2.3 From 935c53b92eea7c288b781ecd68436c9733ec8a83 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 13 Jun 2021 21:55:51 +0300 Subject: internal: use cov-mark rather than bailing out diagnostic --- crates/hir/src/diagnostics.rs | 23 ------ crates/hir/src/lib.rs | 28 ++----- crates/hir_ty/src/diagnostics/expr.rs | 17 +--- crates/ide/src/diagnostics.rs | 3 +- crates/ide/src/diagnostics/missing_match_arms.rs | 99 +++++++++++++----------- 5 files changed, 62 insertions(+), 108 deletions(-) diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index 5cffef47f..1f6a70006 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -3,8 +3,6 @@ //! //! This probably isn't the best way to do this -- ideally, diagnistics should //! be expressed in terms of hir types themselves. -use std::any::Any; - use cfg::{CfgExpr, CfgOptions}; use either::Either; use hir_def::path::ModPath; @@ -157,25 +155,4 @@ pub struct MissingMatchArms { pub arms: AstPtr, } -#[derive(Debug)] -pub struct InternalBailedOut { - pub file: HirFileId, - pub pat_syntax_ptr: SyntaxNodePtr, -} - -impl Diagnostic for InternalBailedOut { - fn code(&self) -> DiagnosticCode { - DiagnosticCode("internal:match-check-bailed-out") - } - fn message(&self) -> String { - format!("Internal: match check bailed out") - } - fn display_source(&self) -> InFile { - InFile { file_id: self.file, value: self.pat_syntax_ptr.clone() } - } - fn as_any(&self) -> &(dyn Any + Send + 'static) { - self - } -} - pub use hir_ty::diagnostics::IncorrectCase; diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 2e794ff4a..7f689cd41 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -86,8 +86,8 @@ use crate::{ pub use crate::{ attrs::{HasAttrs, Namespace}, diagnostics::{ - AnyDiagnostic, BreakOutsideOfLoop, InactiveCode, IncorrectCase, InternalBailedOut, - MacroError, MismatchedArgCount, MissingFields, MissingMatchArms, MissingOkOrSomeInTailExpr, + AnyDiagnostic, BreakOutsideOfLoop, InactiveCode, IncorrectCase, MacroError, + MismatchedArgCount, MissingFields, MissingMatchArms, MissingOkOrSomeInTailExpr, MissingUnsafe, NoSuchField, RemoveThisSemicolon, ReplaceFilterMapNextWithFindMap, UnimplementedBuiltinMacro, UnresolvedExternCrate, UnresolvedImport, UnresolvedMacroCall, UnresolvedModule, UnresolvedProcMacro, @@ -461,7 +461,6 @@ impl Module { self, db: &dyn HirDatabase, sink: &mut DiagnosticSink, - internal_diagnostics: bool, ) -> Vec { let _p = profile::span("Module::diagnostics").detail(|| { format!("{:?}", self.name(db).map_or("".into(), |name| name.to_string())) @@ -619,11 +618,11 @@ impl Module { } for decl in self.declarations(db) { match decl { - ModuleDef::Function(f) => acc.extend(f.diagnostics(db, sink, internal_diagnostics)), + ModuleDef::Function(f) => acc.extend(f.diagnostics(db, sink)), ModuleDef::Module(m) => { // Only add diagnostics from inline modules if def_map[m.id.local_id].origin.is_inline() { - acc.extend(m.diagnostics(db, sink, internal_diagnostics)) + acc.extend(m.diagnostics(db, sink)) } } _ => acc.extend(decl.diagnostics(db)), @@ -633,7 +632,7 @@ impl Module { for impl_def in self.impl_defs(db) { for item in impl_def.items(db) { if let AssocItem::Function(f) = item { - acc.extend(f.diagnostics(db, sink, internal_diagnostics)); + acc.extend(f.diagnostics(db, sink)); } } } @@ -1040,7 +1039,6 @@ impl Function { self, db: &dyn HirDatabase, sink: &mut DiagnosticSink, - internal_diagnostics: bool, ) -> Vec { let mut acc: Vec = Vec::new(); let krate = self.module(db).id.krate(); @@ -1100,9 +1098,7 @@ impl Function { } } - for diagnostic in - BodyValidationDiagnostic::collect(db, self.id.into(), internal_diagnostics) - { + for diagnostic in BodyValidationDiagnostic::collect(db, self.id.into()) { match diagnostic { BodyValidationDiagnostic::RecordMissingFields { record, @@ -1223,18 +1219,6 @@ impl Function { Err(SyntheticSyntax) => (), } } - BodyValidationDiagnostic::InternalBailedOut { pat } => { - match source_map.pat_syntax(pat) { - Ok(source_ptr) => { - let pat_syntax_ptr = source_ptr.value.either(Into::into, Into::into); - sink.push(InternalBailedOut { - file: source_ptr.file_id, - pat_syntax_ptr, - }); - } - Err(SyntheticSyntax) => (), - } - } } } diff --git a/crates/hir_ty/src/diagnostics/expr.rs b/crates/hir_ty/src/diagnostics/expr.rs index 2a211fd8e..b809b96a0 100644 --- a/crates/hir_ty/src/diagnostics/expr.rs +++ b/crates/hir_ty/src/diagnostics/expr.rs @@ -50,21 +50,13 @@ pub enum BodyValidationDiagnostic { MissingMatchArms { match_expr: ExprId, }, - InternalBailedOut { - pat: PatId, - }, } impl BodyValidationDiagnostic { - pub fn collect( - db: &dyn HirDatabase, - owner: DefWithBodyId, - internal_diagnostics: bool, - ) -> Vec { + pub fn collect(db: &dyn HirDatabase, owner: DefWithBodyId) -> Vec { let _p = profile::span("BodyValidationDiagnostic::collect"); let infer = db.infer(owner); let mut validator = ExprValidator::new(owner, infer.clone()); - validator.internal_diagnostics = internal_diagnostics; validator.validate_body(db); validator.diagnostics } @@ -74,12 +66,11 @@ struct ExprValidator { owner: DefWithBodyId, infer: Arc, pub(super) diagnostics: Vec, - internal_diagnostics: bool, } impl ExprValidator { fn new(owner: DefWithBodyId, infer: Arc) -> ExprValidator { - ExprValidator { owner, infer, diagnostics: Vec::new(), internal_diagnostics: false } + ExprValidator { owner, infer, diagnostics: Vec::new() } } fn validate_body(&mut self, db: &dyn HirDatabase) { @@ -308,9 +299,7 @@ impl ExprValidator { // 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. - if self.internal_diagnostics { - self.diagnostics.push(BodyValidationDiagnostic::InternalBailedOut { pat: arm.pat }) - } + cov_mark::hit!(validate_match_bailed_out); return; } diff --git a/crates/ide/src/diagnostics.rs b/crates/ide/src/diagnostics.rs index 01b68232e..fe6236e44 100644 --- a/crates/ide/src/diagnostics.rs +++ b/crates/ide/src/diagnostics.rs @@ -181,10 +181,9 @@ pub(crate) fn diagnostics( }); let mut diags = Vec::new(); - let internal_diagnostics = cfg!(test); let module = sema.to_module_def(file_id); if let Some(m) = module { - diags = m.diagnostics(db, &mut sink, internal_diagnostics) + diags = m.diagnostics(db, &mut sink) } drop(sink); diff --git a/crates/ide/src/diagnostics/missing_match_arms.rs b/crates/ide/src/diagnostics/missing_match_arms.rs index 6b977fa59..b636489b3 100644 --- a/crates/ide/src/diagnostics/missing_match_arms.rs +++ b/crates/ide/src/diagnostics/missing_match_arms.rs @@ -20,9 +20,14 @@ pub(super) fn missing_match_arms( pub(super) mod tests { use crate::diagnostics::tests::check_diagnostics; + fn check_diagnostics_no_bails(ra_fixture: &str) { + cov_mark::check_count!(validate_match_bailed_out, 0); + crate::diagnostics::tests::check_diagnostics(ra_fixture) + } + #[test] fn empty_tuple() { - check_diagnostics( + check_diagnostics_no_bails( r#" fn main() { match () { } @@ -40,7 +45,7 @@ fn main() { #[test] fn tuple_of_two_empty_tuple() { - check_diagnostics( + check_diagnostics_no_bails( r#" fn main() { match ((), ()) { } @@ -54,7 +59,7 @@ fn main() { #[test] fn boolean() { - check_diagnostics( + check_diagnostics_no_bails( r#" fn test_main() { match false { } @@ -107,7 +112,7 @@ fn test_main() { #[test] fn tuple_of_tuple_and_bools() { - check_diagnostics( + check_diagnostics_no_bails( r#" fn main() { match (false, ((), false)) {} @@ -135,7 +140,7 @@ fn main() { #[test] fn enums() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Either { A, B, } @@ -163,7 +168,7 @@ fn main() { #[test] fn enum_containing_bool() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Either { A(bool), B } @@ -196,7 +201,7 @@ fn main() { #[test] fn enum_different_sizes() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Either { A(bool), B(bool, bool) } @@ -224,7 +229,7 @@ fn main() { #[test] fn tuple_of_enum_no_diagnostic() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Either { A(bool), B(bool, bool) } enum Either2 { C, D } @@ -243,7 +248,7 @@ fn main() { #[test] fn or_pattern_no_diagnostic() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Either {A, B} @@ -257,6 +262,7 @@ fn main() { #[test] fn mismatched_types() { + cov_mark::check_count!(validate_match_bailed_out, 4); // Match statements with arms that don't match the // expression pattern do not fire this diagnostic. check_diagnostics( @@ -267,18 +273,14 @@ enum Either2 { C, D } fn main() { match Either::A { Either2::C => (), - // ^^^^^^^^^^ Internal: match check bailed out Either2::D => (), } match (true, false) { (true, false, true) => (), - // ^^^^^^^^^^^^^^^^^^^ Internal: match check bailed out (true) => (), } match (true, false) { (true,) => {} } - // ^^^^^^^ Internal: match check bailed out match (0) { () => () } - // ^^ Internal: match check bailed out match Unresolved::Bar { Unresolved::Baz => () } } "#, @@ -287,13 +289,12 @@ fn main() { #[test] fn mismatched_types_in_or_patterns() { + cov_mark::check_count!(validate_match_bailed_out, 2); check_diagnostics( r#" fn main() { match false { true | () => {} } - // ^^^^^^^^^ Internal: match check bailed out match (false,) { (true | (),) => {} } - // ^^^^^^^^^^^^ Internal: match check bailed out } "#, ); @@ -303,7 +304,7 @@ fn main() { fn malformed_match_arm_tuple_enum_missing_pattern() { // We are testing to be sure we don't panic here when the match // arm `Either::B` is missing its pattern. - check_diagnostics( + check_diagnostics_no_bails( r#" enum Either { A, B(u32) } @@ -319,17 +320,16 @@ fn main() { #[test] fn malformed_match_arm_extra_fields() { + cov_mark::check_count!(validate_match_bailed_out, 2); check_diagnostics( r#" enum A { B(isize, isize), C } fn main() { match A::B(1, 2) { A::B(_, _, _) => (), - // ^^^^^^^^^^^^^ Internal: match check bailed out } match A::B(1, 2) { A::C(_) => (), - // ^^^^^^^ Internal: match check bailed out } } "#, @@ -338,6 +338,7 @@ fn main() { #[test] fn expr_diverges() { + cov_mark::check_count!(validate_match_bailed_out, 2); check_diagnostics( r#" enum Either { A, B } @@ -345,12 +346,10 @@ enum Either { A, B } fn main() { match loop {} { Either::A => (), - // ^^^^^^^^^ Internal: match check bailed out Either::B => (), } match loop {} { Either::A => (), - // ^^^^^^^^^ Internal: match check bailed out } match loop { break Foo::A } { //^^^^^^^^^^^^^^^^^^^^^ missing match arm @@ -367,7 +366,7 @@ fn main() { #[test] fn expr_partially_diverges() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Either { A(T), B } @@ -384,7 +383,7 @@ fn main() -> u32 { #[test] fn enum_record() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Either { A { foo: bool }, B } @@ -422,7 +421,7 @@ fn main() { #[test] fn enum_record_fields_out_of_order() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Either { A { foo: bool, bar: () }, @@ -449,7 +448,7 @@ fn main() { #[test] fn enum_record_ellipsis() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Either { A { foo: bool, bar: bool }, @@ -485,7 +484,7 @@ fn main() { #[test] fn enum_tuple_partial_ellipsis() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Either { A(bool, bool, bool, bool), @@ -529,7 +528,7 @@ fn main() { #[test] fn never() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Never {} @@ -549,6 +548,8 @@ fn bang(never: !) { #[test] fn unknown_type() { + cov_mark::check_count!(validate_match_bailed_out, 1); + check_diagnostics( r#" enum Option { Some(T), None } @@ -558,7 +559,6 @@ fn main() { match Option::::None { None => (), Some(never) => match never {}, - // ^^^^^^^^^^^ Internal: match check bailed out } match Option::::None { //^^^^^^^^^^^^^^^^^^^^^ missing match arm @@ -571,7 +571,7 @@ fn main() { #[test] fn tuple_of_bools_with_ellipsis_at_end_missing_arm() { - check_diagnostics( + check_diagnostics_no_bails( r#" fn main() { match (false, true, false) { @@ -584,7 +584,7 @@ fn main() { #[test] fn tuple_of_bools_with_ellipsis_at_beginning_missing_arm() { - check_diagnostics( + check_diagnostics_no_bails( r#" fn main() { match (false, true, false) { @@ -597,7 +597,7 @@ fn main() { #[test] fn tuple_of_bools_with_ellipsis_in_middle_missing_arm() { - check_diagnostics( + check_diagnostics_no_bails( r#" fn main() { match (false, true, false) { @@ -610,7 +610,7 @@ fn main() { #[test] fn record_struct() { - check_diagnostics( + check_diagnostics_no_bails( r#"struct Foo { a: bool } fn main(f: Foo) { match f {} @@ -635,7 +635,7 @@ fn main(f: Foo) { #[test] fn tuple_struct() { - check_diagnostics( + check_diagnostics_no_bails( r#"struct Foo(bool); fn main(f: Foo) { match f {} @@ -653,7 +653,7 @@ fn main(f: Foo) { #[test] fn unit_struct() { - check_diagnostics( + check_diagnostics_no_bails( r#"struct Foo; fn main(f: Foo) { match f {} @@ -666,7 +666,7 @@ fn main(f: Foo) { #[test] fn record_struct_ellipsis() { - check_diagnostics( + check_diagnostics_no_bails( r#"struct Foo { foo: bool, bar: bool } fn main(f: Foo) { match f { Foo { foo: true, .. } => () } @@ -688,7 +688,7 @@ fn main(f: Foo) { #[test] fn internal_or() { - check_diagnostics( + check_diagnostics_no_bails( r#" fn main() { enum Either { A(bool), B } @@ -703,6 +703,8 @@ fn main() { #[test] fn no_panic_at_unimplemented_subpattern_type() { + cov_mark::check_count!(validate_match_bailed_out, 1); + check_diagnostics( r#" struct S { a: char} @@ -710,7 +712,6 @@ fn main(v: S) { match v { S{ a } => {} } match v { S{ a: _x } => {} } match v { S{ a: 'a' } => {} } - //^^^^^^^^^^^ Internal: match check bailed out match v { S{..} => {} } match v { _ => {} } match v { } @@ -722,7 +723,7 @@ fn main(v: S) { #[test] fn binding() { - check_diagnostics( + check_diagnostics_no_bails( r#" fn main() { match true { @@ -738,6 +739,8 @@ fn main() { #[test] fn binding_ref_has_correct_type() { + cov_mark::check_count!(validate_match_bailed_out, 1); + // Asserts `PatKind::Binding(ref _x): bool`, not &bool. // If that's not true match checking will panic with "incompatible constructors" // FIXME: make facilities to test this directly like `tests::check_infer(..)` @@ -749,7 +752,6 @@ fn main() { // ExprValidator::validate_match(..) checks types of top level patterns incorrecly. match Foo::A { ref _x => {} - // ^^^^^^ Internal: match check bailed out Foo::A => {} } match (true,) { @@ -763,7 +765,7 @@ fn main() { #[test] fn enum_non_exhaustive() { - check_diagnostics( + check_diagnostics_no_bails( r#" //- /lib.rs crate:lib #[non_exhaustive] @@ -799,7 +801,7 @@ fn main() { #[test] fn match_guard() { - check_diagnostics( + check_diagnostics_no_bails( r#" fn main() { match true { @@ -820,7 +822,7 @@ fn main() { #[test] fn pattern_type_is_of_substitution() { cov_mark::check!(match_check_wildcard_expanded_to_substitutions); - check_diagnostics( + check_diagnostics_no_bails( r#" struct Foo(T); struct Bar; @@ -835,12 +837,13 @@ fn main() { #[test] fn record_struct_no_such_field() { + cov_mark::check_count!(validate_match_bailed_out, 1); + check_diagnostics( r#" struct Foo { } fn main(f: Foo) { match f { Foo { bar } => () } - // ^^^^^^^^^^^ Internal: match check bailed out } "#, ); @@ -848,7 +851,7 @@ fn main(f: Foo) { #[test] fn match_ergonomics_issue_9095() { - check_diagnostics( + check_diagnostics_no_bails( r#" enum Foo { A(T) } fn main() { @@ -875,13 +878,14 @@ fn main() { #[test] fn integers() { + cov_mark::check_count!(validate_match_bailed_out, 1); + // We don't currently check integer exhaustiveness. check_diagnostics( r#" fn main() { match 5 { 10 => (), - // ^^ Internal: match check bailed out 11..20 => (), } } @@ -891,12 +895,13 @@ fn main() { #[test] fn reference_patterns_at_top_level() { + cov_mark::check_count!(validate_match_bailed_out, 1); + check_diagnostics( r#" fn main() { match &false { &true => {} - // ^^^^^ Internal: match check bailed out } } "#, @@ -905,16 +910,16 @@ fn main() { #[test] fn reference_patterns_in_fields() { + cov_mark::check_count!(validate_match_bailed_out, 2); + check_diagnostics( r#" fn main() { match (&false,) { (true,) => {} - // ^^^^^^^ Internal: match check bailed out } match (&false,) { (&true,) => {} - // ^^^^^^^^ Internal: match check bailed out } } "#, -- cgit v1.2.3 From ff52167c9a8dd6f99a56a35eae8d634d0ddf1286 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 13 Jun 2021 22:05:47 +0300 Subject: internal: kill diagnostic sink --- crates/hir/src/diagnostics.rs | 4 -- crates/hir/src/diagnostics_sink.rs | 109 ------------------------------------- crates/hir/src/lib.rs | 28 ++-------- crates/ide/src/diagnostics.rs | 48 +++++----------- 4 files changed, 19 insertions(+), 170 deletions(-) delete mode 100644 crates/hir/src/diagnostics_sink.rs diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index 1f6a70006..b4c505898 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -9,10 +9,6 @@ use hir_def::path::ModPath; use hir_expand::{name::Name, HirFileId, InFile}; use syntax::{ast, AstPtr, SyntaxNodePtr, TextRange}; -pub use crate::diagnostics_sink::{ - Diagnostic, DiagnosticCode, DiagnosticSink, DiagnosticSinkBuilder, -}; - macro_rules! diagnostics { ($($diag:ident,)*) => { pub enum AnyDiagnostic {$( diff --git a/crates/hir/src/diagnostics_sink.rs b/crates/hir/src/diagnostics_sink.rs deleted file mode 100644 index 084fa8b06..000000000 --- a/crates/hir/src/diagnostics_sink.rs +++ /dev/null @@ -1,109 +0,0 @@ -//! Semantic errors and warnings. -//! -//! The `Diagnostic` trait defines a trait object which can represent any -//! diagnostic. -//! -//! `DiagnosticSink` struct is used as an emitter for diagnostic. When creating -//! a `DiagnosticSink`, you supply a callback which can react to a `dyn -//! Diagnostic` or to any concrete diagnostic (downcasting is used internally). -//! -//! Because diagnostics store file offsets, it's a bad idea to store them -//! directly in salsa. For this reason, every hir subsytem defines it's own -//! strongly-typed closed set of diagnostics which use hir ids internally, are -//! stored in salsa and do *not* implement the `Diagnostic` trait. Instead, a -//! subsystem provides a separate, non-query-based API which can walk all stored -//! values and transform them into instances of `Diagnostic`. - -use std::{any::Any, fmt}; - -use hir_expand::InFile; -use syntax::SyntaxNodePtr; - -#[derive(Copy, Clone, Debug, PartialEq)] -pub struct DiagnosticCode(pub &'static str); - -impl DiagnosticCode { - pub fn as_str(&self) -> &str { - self.0 - } -} - -pub trait Diagnostic: Any + Send + Sync + fmt::Debug + 'static { - fn code(&self) -> DiagnosticCode; - fn message(&self) -> String; - /// Source element that triggered the diagnostics. - /// - /// Note that this should reflect "semantics", rather than specific span we - /// want to highlight. When rendering the diagnostics into an error message, - /// the IDE will fetch the `SyntaxNode` and will narrow the span - /// appropriately. - fn display_source(&self) -> InFile; - fn as_any(&self) -> &(dyn Any + Send + 'static); - fn is_experimental(&self) -> bool { - false - } -} - -pub struct DiagnosticSink<'a> { - callbacks: Vec Result<(), ()> + 'a>>, - filters: Vec bool + 'a>>, - default_callback: Box, -} - -impl<'a> DiagnosticSink<'a> { - pub fn push(&mut self, d: impl Diagnostic) { - let d: &dyn Diagnostic = &d; - self._push(d); - } - - fn _push(&mut self, d: &dyn Diagnostic) { - for filter in &mut self.filters { - if !filter(d) { - return; - } - } - for cb in &mut self.callbacks { - match cb(d) { - Ok(()) => return, - Err(()) => (), - } - } - (self.default_callback)(d) - } -} - -pub struct DiagnosticSinkBuilder<'a> { - callbacks: Vec Result<(), ()> + 'a>>, - filters: Vec bool + 'a>>, -} - -impl<'a> DiagnosticSinkBuilder<'a> { - pub fn new() -> Self { - Self { callbacks: Vec::new(), filters: Vec::new() } - } - - pub fn filter bool + 'a>(mut self, cb: F) -> Self { - self.filters.push(Box::new(cb)); - self - } - - pub fn on(mut self, mut cb: F) -> Self { - let cb = move |diag: &dyn Diagnostic| match diag.as_any().downcast_ref::() { - Some(d) => { - cb(d); - Ok(()) - } - None => Err(()), - }; - self.callbacks.push(Box::new(cb)); - self - } - - pub fn build(self, default_callback: F) -> DiagnosticSink<'a> { - DiagnosticSink { - callbacks: self.callbacks, - filters: self.filters, - default_callback: Box::new(default_callback), - } - } -} diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 7f689cd41..ce38396d0 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -27,7 +27,6 @@ mod attrs; mod has_source; pub mod diagnostics; -pub mod diagnostics_sink; pub mod db; mod display; @@ -78,10 +77,7 @@ use syntax::{ }; use tt::{Ident, Leaf, Literal, TokenTree}; -use crate::{ - db::{DefDatabase, HirDatabase}, - diagnostics_sink::DiagnosticSink, -}; +use crate::db::{DefDatabase, HirDatabase}; pub use crate::{ attrs::{HasAttrs, Namespace}, @@ -457,15 +453,10 @@ impl Module { self.id.def_map(db.upcast())[self.id.local_id].scope.visibility_of((*def).into()) } - pub fn diagnostics( - self, - db: &dyn HirDatabase, - sink: &mut DiagnosticSink, - ) -> Vec { + pub fn diagnostics(self, db: &dyn HirDatabase, acc: &mut Vec) { let _p = profile::span("Module::diagnostics").detail(|| { format!("{:?}", self.name(db).map_or("".into(), |name| name.to_string())) }); - let mut acc: Vec = Vec::new(); let def_map = self.id.def_map(db.upcast()); for diag in def_map.diagnostics() { if diag.in_module != self.id.local_id { @@ -618,11 +609,11 @@ impl Module { } for decl in self.declarations(db) { match decl { - ModuleDef::Function(f) => acc.extend(f.diagnostics(db, sink)), + ModuleDef::Function(f) => f.diagnostics(db, acc), ModuleDef::Module(m) => { // Only add diagnostics from inline modules if def_map[m.id.local_id].origin.is_inline() { - acc.extend(m.diagnostics(db, sink)) + m.diagnostics(db, acc) } } _ => acc.extend(decl.diagnostics(db)), @@ -632,11 +623,10 @@ impl Module { for impl_def in self.impl_defs(db) { for item in impl_def.items(db) { if let AssocItem::Function(f) = item { - acc.extend(f.diagnostics(db, sink)); + f.diagnostics(db, acc); } } } - acc } pub fn declarations(self, db: &dyn HirDatabase) -> Vec { @@ -1035,12 +1025,7 @@ impl Function { db.function_data(self.id).is_async() } - pub fn diagnostics( - self, - db: &dyn HirDatabase, - sink: &mut DiagnosticSink, - ) -> Vec { - let mut acc: Vec = Vec::new(); + pub fn diagnostics(self, db: &dyn HirDatabase, acc: &mut Vec) { let krate = self.module(db).id.krate(); let source_map = db.body_with_source_map(self.id.into()).1; @@ -1225,7 +1210,6 @@ impl Function { for diag in hir_ty::diagnostics::validate_module_item(db, krate, self.id.into()) { acc.push(diag.into()) } - acc } /// Whether this function declaration has a definition. diff --git a/crates/ide/src/diagnostics.rs b/crates/ide/src/diagnostics.rs index fe6236e44..c024e3e1e 100644 --- a/crates/ide/src/diagnostics.rs +++ b/crates/ide/src/diagnostics.rs @@ -26,12 +26,7 @@ mod unresolved_proc_macro; mod field_shorthand; -use std::cell::RefCell; - -use hir::{ - diagnostics::{AnyDiagnostic, DiagnosticCode, DiagnosticSinkBuilder}, - Semantics, -}; +use hir::{diagnostics::AnyDiagnostic, Semantics}; use ide_assists::AssistResolveStrategy; use ide_db::{base_db::SourceDatabase, RootDatabase}; use itertools::Itertools; @@ -45,6 +40,15 @@ use unlinked_file::UnlinkedFile; use crate::{Assist, AssistId, AssistKind, FileId, Label, SourceChange}; +#[derive(Copy, Clone, Debug, PartialEq)] +pub struct DiagnosticCode(pub &'static str); + +impl DiagnosticCode { + pub fn as_str(&self) -> &str { + self.0 + } +} + #[derive(Debug)] pub struct Diagnostic { // pub name: Option, @@ -113,10 +117,6 @@ impl Diagnostic { fn with_unused(self, unused: bool) -> Self { Self { unused, ..self } } - - fn with_code(self, code: Option) -> Self { - Self { code, ..self } - } } #[derive(Debug, Copy, Clone)] @@ -161,35 +161,13 @@ pub(crate) fn diagnostics( check_unnecessary_braces_in_use_statement(&mut res, file_id, &node); field_shorthand::check(&mut res, file_id, &node); } - let res = RefCell::new(res); - let sink_builder = DiagnosticSinkBuilder::new() - // Only collect experimental diagnostics when they're enabled. - .filter(|diag| !(diag.is_experimental() && config.disable_experimental)) - .filter(|diag| !config.disabled.contains(diag.code().as_str())); - - // Finalize the `DiagnosticSink` building process. - let mut sink = sink_builder - // Diagnostics not handled above get no fix and default treatment. - .build(|d| { - res.borrow_mut().push( - Diagnostic::error( - sema.diagnostics_display_range(d.display_source()).range, - d.message(), - ) - .with_code(Some(d.code())), - ); - }); let mut diags = Vec::new(); let module = sema.to_module_def(file_id); if let Some(m) = module { - diags = m.diagnostics(db, &mut sink) + m.diagnostics(db, &mut diags) } - drop(sink); - - let mut res = res.into_inner(); - let ctx = DiagnosticsContext { config, sema, resolve }; if module.is_none() { let d = UnlinkedFile { file: file_id }; @@ -350,8 +328,8 @@ mod tests { ) .unwrap() .pop() - .unwrap(); - let fix = &diagnostic.fixes.unwrap()[nth]; + .expect("no diagnostics"); + let fix = &diagnostic.fixes.expect("diagnostic misses fixes")[nth]; let actual = { let source_change = fix.source_change.as_ref().unwrap(); let file_id = *source_change.source_file_edits.keys().next().unwrap(); -- cgit v1.2.3