Merge #5804

5804: Add type safety to diagnostic codes
 r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

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 {
+    fn code(&self) -> DiagnosticCode {
-        "unresolved-module"
+        DiagnosticCode("unresolved-module")
     }
     fn message(&self) -> String {
         "unresolved module".to_string()

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<SyntaxNodePtr>;

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 {
+    fn code(&self) -> DiagnosticCode {
-        "no-such-field"
+        DiagnosticCode("no-such-field")
     }
 
     fn message(&self) -> String {
@@ -58,8 +58,8 @@ pub struct MissingFields {
 }
 
 impl Diagnostic for MissingFields {
-    fn name(&self) -> &'static str {
+    fn code(&self) -> DiagnosticCode {
-        "missing-structure-fields"
+        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 {
+    fn code(&self) -> DiagnosticCode {
-        "missing-pat-fields"
+        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 {
+    fn code(&self) -> DiagnosticCode {
-        "missing-match-arm"
+        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 {
+    fn code(&self) -> DiagnosticCode {
-        "missing-ok-in-tail-expr"
+        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 {
+    fn code(&self) -> DiagnosticCode {
-        "break-outside-of-loop"
+        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 {
+    fn code(&self) -> DiagnosticCode {
-        "missing-unsafe"
+        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 {
+    fn code(&self) -> DiagnosticCode {
-        "mismatched-arg-count"
+        DiagnosticCode("mismatched-arg-count")
     }
     fn message(&self) -> String {
         let s = if self.expected == 1 { "" } else { "s" };

crates/ide/src/diagnostics.rs

@@ -25,7 +25,7 @@ use self::fixes::DiagnosticWithFix;
 
 #[derive(Debug)]
 pub struct Diagnostic {
-    pub name: Option<String>,
+    // pub name: Option<String>,
     pub message: String,
     pub range: TextRange,
     pub severity: Severity,
@@ -71,7 +71,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,
@@ -98,14 +98,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,
@@ -122,7 +122,7 @@ pub(crate) fn diagnostics(
 
 fn diagnostic_with_fix<D: DiagnosticWithFix>(d: &D, sema: &Semantics<RootDatabase>) -> 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,
@@ -149,7 +149,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,
@@ -196,7 +196,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,
@@ -294,54 +294,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::<Vec<_>>();
-        let analysis = mock.analysis();
-
-        let diagnostics = files
-            .clone()
-            .into_iter()
-            .flat_map(|file_id| analysis.diagnostics(&config, file_id).unwrap())
-            .collect::<Vec<_>>();
-
-        // 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::<Vec<_>>();
-
-        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();
@@ -604,9 +556,6 @@ fn test_fn() {
             expect![[r#"
                 [
                     Diagnostic {
-                        name: Some(
-                            "unresolved-module",
-                        ),
                         message: "unresolved module",
                         range: 0..8,
                         severity: Error,
@@ -783,6 +732,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());
     }
 }