From 8146669542dfc887956901b54a453c9a97fee7e3 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Tue, 18 Aug 2020 18:39:43 +0200 Subject: [PATCH] Add type safety to diagnostic codes --- crates/hir_def/src/diagnostics.rs | 6 +-- crates/hir_expand/src/diagnostics.rs | 11 +++- crates/hir_ty/src/diagnostics.rs | 34 ++++++------- crates/ide/src/diagnostics.rs | 76 +++++++--------------------- 4 files changed, 47 insertions(+), 80 deletions(-) diff --git a/crates/hir_def/src/diagnostics.rs b/crates/hir_def/src/diagnostics.rs index c7723de006..3e19d9117a 100644 --- a/crates/hir_def/src/diagnostics.rs +++ b/crates/hir_def/src/diagnostics.rs @@ -2,7 +2,7 @@ use std::any::Any; -use hir_expand::diagnostics::Diagnostic; +use hir_expand::diagnostics::{Diagnostic, DiagnosticCode}; use syntax::{ast, AstPtr, SyntaxNodePtr}; use hir_expand::{HirFileId, InFile}; @@ -15,8 +15,8 @@ pub struct UnresolvedModule { } impl Diagnostic for UnresolvedModule { - fn name(&self) -> &'static str { - "unresolved-module" + fn code(&self) -> DiagnosticCode { + DiagnosticCode("unresolved-module") } fn message(&self) -> String { "unresolved module".to_string() diff --git a/crates/hir_expand/src/diagnostics.rs b/crates/hir_expand/src/diagnostics.rs index 6c81b2501a..78ccc212c8 100644 --- a/crates/hir_expand/src/diagnostics.rs +++ b/crates/hir_expand/src/diagnostics.rs @@ -20,8 +20,17 @@ use syntax::SyntaxNodePtr; use crate::InFile; +#[derive(Copy, Clone, 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 name(&self) -> &'static str; + fn code(&self) -> DiagnosticCode; fn message(&self) -> String; /// Used in highlighting and related purposes fn display_source(&self) -> InFile; diff --git a/crates/hir_ty/src/diagnostics.rs b/crates/hir_ty/src/diagnostics.rs index 38fa24ee0a..9ba005fabd 100644 --- a/crates/hir_ty/src/diagnostics.rs +++ b/crates/hir_ty/src/diagnostics.rs @@ -6,7 +6,7 @@ mod unsafe_check; use std::any::Any; use hir_def::DefWithBodyId; -use hir_expand::diagnostics::{Diagnostic, DiagnosticSink}; +use hir_expand::diagnostics::{Diagnostic, DiagnosticCode, DiagnosticSink}; use hir_expand::{name::Name, HirFileId, InFile}; use stdx::format_to; use syntax::{ast, AstPtr, SyntaxNodePtr}; @@ -32,8 +32,8 @@ pub struct NoSuchField { } impl Diagnostic for NoSuchField { - fn name(&self) -> &'static str { - "no-such-field" + fn code(&self) -> DiagnosticCode { + DiagnosticCode("no-such-field") } fn message(&self) -> String { @@ -58,8 +58,8 @@ pub struct MissingFields { } impl Diagnostic for MissingFields { - fn name(&self) -> &'static str { - "missing-structure-fields" + fn code(&self) -> DiagnosticCode { + DiagnosticCode("missing-structure-fields") } fn message(&self) -> String { let mut buf = String::from("Missing structure fields:\n"); @@ -94,8 +94,8 @@ pub struct MissingPatFields { } impl Diagnostic for MissingPatFields { - fn name(&self) -> &'static str { - "missing-pat-fields" + fn code(&self) -> DiagnosticCode { + DiagnosticCode("missing-pat-fields") } fn message(&self) -> String { let mut buf = String::from("Missing structure fields:\n"); @@ -127,8 +127,8 @@ pub struct MissingMatchArms { } impl Diagnostic for MissingMatchArms { - fn name(&self) -> &'static str { - "missing-match-arm" + fn code(&self) -> DiagnosticCode { + DiagnosticCode("missing-match-arm") } fn message(&self) -> String { String::from("Missing match arm") @@ -148,8 +148,8 @@ pub struct MissingOkInTailExpr { } impl Diagnostic for MissingOkInTailExpr { - fn name(&self) -> &'static str { - "missing-ok-in-tail-expr" + fn code(&self) -> DiagnosticCode { + DiagnosticCode("missing-ok-in-tail-expr") } fn message(&self) -> String { "wrap return expression in Ok".to_string() @@ -169,8 +169,8 @@ pub struct BreakOutsideOfLoop { } impl Diagnostic for BreakOutsideOfLoop { - fn name(&self) -> &'static str { - "break-outside-of-loop" + fn code(&self) -> DiagnosticCode { + DiagnosticCode("break-outside-of-loop") } fn message(&self) -> String { "break outside of loop".to_string() @@ -190,8 +190,8 @@ pub struct MissingUnsafe { } impl Diagnostic for MissingUnsafe { - fn name(&self) -> &'static str { - "missing-unsafe" + fn code(&self) -> DiagnosticCode { + DiagnosticCode("missing-unsafe") } fn message(&self) -> String { format!("This operation is unsafe and requires an unsafe function or block") @@ -213,8 +213,8 @@ pub struct MismatchedArgCount { } impl Diagnostic for MismatchedArgCount { - fn name(&self) -> &'static str { - "mismatched-arg-count" + fn code(&self) -> DiagnosticCode { + DiagnosticCode("mismatched-arg-count") } fn message(&self) -> String { let s = if self.expected == 1 { "" } else { "s" }; diff --git a/crates/ide/src/diagnostics.rs b/crates/ide/src/diagnostics.rs index 1f85805d22..6922034bc7 100644 --- a/crates/ide/src/diagnostics.rs +++ b/crates/ide/src/diagnostics.rs @@ -25,7 +25,7 @@ use self::fixes::DiagnosticWithFix; #[derive(Debug)] pub struct Diagnostic { - pub name: Option, + // pub name: Option, pub message: String, pub range: TextRange, pub severity: Severity, @@ -76,7 +76,7 @@ pub(crate) fn diagnostics( // [#34344] Only take first 128 errors to prevent slowing down editor/ide, the number 128 is chosen arbitrarily. res.extend(parse.errors().iter().take(128).map(|err| Diagnostic { - name: None, + // name: None, range: err.range(), message: format!("Syntax Error: {}", err), severity: Severity::Error, @@ -103,14 +103,14 @@ pub(crate) fn diagnostics( }) // Only collect experimental diagnostics when they're enabled. .filter(|diag| !(diag.is_experimental() && config.disable_experimental)) - .filter(|diag| !config.disabled.contains(diag.name())); + .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 { - name: Some(d.name().into()), + // name: Some(d.name().into()), message: d.message(), range: sema.diagnostics_display_range(d).range, severity: Severity::Error, @@ -127,7 +127,7 @@ pub(crate) fn diagnostics( fn diagnostic_with_fix(d: &D, sema: &Semantics) -> Diagnostic { Diagnostic { - name: Some(d.name().into()), + // name: Some(d.name().into()), range: sema.diagnostics_display_range(d).range, message: d.message(), severity: Severity::Error, @@ -154,7 +154,7 @@ fn check_unnecessary_braces_in_use_statement( }); acc.push(Diagnostic { - name: None, + // name: None, range: use_range, message: "Unnecessary braces in use statement".to_string(), severity: Severity::WeakWarning, @@ -201,7 +201,7 @@ fn check_struct_shorthand_initialization( let field_range = record_field.syntax().text_range(); acc.push(Diagnostic { - name: None, + // name: None, range: field_range, message: "Shorthand struct initialization".to_string(), severity: Severity::WeakWarning, @@ -299,54 +299,6 @@ mod tests { assert_eq!(diagnostics.len(), 0, "unexpected diagnostics:\n{:#?}", diagnostics); } - /// Takes a multi-file input fixture with annotated cursor position and the list of disabled diagnostics, - /// and checks that provided diagnostics aren't spawned during analysis. - fn check_disabled_diagnostics(ra_fixture: &str, disabled_diagnostics: &[&'static str]) { - let mut config = DiagnosticsConfig::default(); - config.disabled = disabled_diagnostics.into_iter().map(|diag| diag.to_string()).collect(); - - let mock = MockAnalysis::with_files(ra_fixture); - let files = mock.files().map(|(it, _)| it).collect::>(); - let analysis = mock.analysis(); - - let diagnostics = files - .clone() - .into_iter() - .flat_map(|file_id| analysis.diagnostics(&config, file_id).unwrap()) - .collect::>(); - - // First, we have to check that diagnostic is not emitted when it's added to the disabled diagnostics list. - for diagnostic in diagnostics { - if let Some(name) = diagnostic.name { - assert!( - !disabled_diagnostics.contains(&name.as_str()), - "Diagnostic {} is disabled", - name - ); - } - } - - // Then, we must reset the config and repeat the check, so that we'll be sure that without - // config these diagnostics are emitted. - // This is required for tests to not become outdated if e.g. diagnostics name changes: - // without this additional run the test will pass simply because a diagnostic with an old name - // will no longer exist. - let diagnostics = files - .into_iter() - .flat_map(|file_id| { - analysis.diagnostics(&DiagnosticsConfig::default(), file_id).unwrap() - }) - .collect::>(); - - assert!( - diagnostics - .into_iter() - .filter_map(|diag| diag.name) - .any(|name| disabled_diagnostics.contains(&name.as_str())), - "At least one of the diagnostics was not emitted even without config; are the diagnostics names correct?" - ); - } - fn check_expect(ra_fixture: &str, expect: Expect) { let (analysis, file_id) = single_file(ra_fixture); let diagnostics = analysis.diagnostics(&DiagnosticsConfig::default(), file_id).unwrap(); @@ -609,9 +561,6 @@ fn test_fn() { expect![[r#" [ Diagnostic { - name: Some( - "unresolved-module", - ), message: "unresolved module", range: 0..8, severity: Error, @@ -788,6 +737,15 @@ struct Foo { #[test] fn test_disabled_diagnostics() { - check_disabled_diagnostics(r#"mod foo;"#, &["unresolved-module"]); + let mut config = DiagnosticsConfig::default(); + config.disabled.insert("unresolved-module".into()); + + let (analysis, file_id) = single_file(r#"mod foo;"#); + + let diagnostics = analysis.diagnostics(&config, file_id).unwrap(); + assert!(diagnostics.is_empty()); + + let diagnostics = analysis.diagnostics(&DiagnosticsConfig::default(), file_id).unwrap(); + assert!(!diagnostics.is_empty()); } }