feat(rome_diagnostics): refactor some existing diagnostics to use the…

… new Diagnostic trait (#3315) * feat(rome_diagnostics): refactor existing diagnostics to the new Diagnostic trait * address PR review * update snapshots
rome · Oct 4, 2022 · f3ade33 · f3ade33
1 parent 8c69826
commit f3ade33
Show file tree

Hide file tree

Showing 22 changed files with 613 additions and 309 deletions.
diff --git a/crates/rome_cli/src/reports/mod.rs b/crates/rome_cli/src/reports/mod.rs
@@ -2,7 +2,7 @@ pub mod formatter;
 
 use crate::reports::formatter::{FormatterReportFileDetail, FormatterReportSummary};
 use formatter::FormatterReport;
-use rome_diagnostics::{v2::Category, Severity};
+use rome_diagnostics::v2::{Category, Severity};
 use rome_service::RomeError;
 use serde::Serialize;
 use std::collections::HashMap;

diff --git a/crates/rome_cli/src/traversal.rs b/crates/rome_cli/src/traversal.rs
diff --git a/crates/rome_diagnostics/src/emit.rs b/crates/rome_diagnostics/src/emit.rs
@@ -136,7 +136,7 @@ impl<'a> Display for DiagnosticPrinter<'a> {
 }
 
 impl v2::Diagnostic for DiagnosticPrinter<'_> {
-    fn category(&self) -> Option<&Category> {
+    fn category(&self) -> Option<&'static Category> {
         self.d.code
     }
 

diff --git a/crates/rome_diagnostics/src/v2/adapters.rs b/crates/rome_diagnostics/src/v2/adapters.rs
@@ -53,7 +53,7 @@ impl From<io::Error> for IoError {
 }
 
 impl Diagnostic for IoError {
-    fn category(&self) -> Option<&Category> {
+    fn category(&self) -> Option<&'static Category> {
         Some(category!("internalError/io"))
     }
 

diff --git a/crates/rome_diagnostics/src/v2/context.rs b/crates/rome_diagnostics/src/v2/context.rs
@@ -182,7 +182,7 @@ mod internal {
     }
 
     impl<M: fmt::Display + 'static, E: AsDiagnostic> Diagnostic for ContextDiagnostic<M, E> {
-        fn category(&self) -> Option<&Category> {
+        fn category(&self) -> Option<&'static Category> {
             self.source.as_diagnostic().category()
         }
 
@@ -280,7 +280,7 @@ mod internal {
     }
 
     impl<E: AsDiagnostic> Diagnostic for CategoryDiagnostic<E> {
-        fn category(&self) -> Option<&Category> {
+        fn category(&self) -> Option<&'static Category> {
             Some(
                 self.source
                     .as_diagnostic()
@@ -335,7 +335,7 @@ mod internal {
     }
 
     impl<E: AsDiagnostic> Diagnostic for FilePathDiagnostic<E> {
-        fn category(&self) -> Option<&Category> {
+        fn category(&self) -> Option<&'static Category> {
             self.source.as_diagnostic().category()
         }
 
@@ -410,7 +410,7 @@ mod internal {
     }
 
     impl<E: AsDiagnostic> Diagnostic for FileSourceCodeDiagnostic<E> {
-        fn category(&self) -> Option<&Category> {
+        fn category(&self) -> Option<&'static Category> {
             self.source.as_diagnostic().category()
         }
 

diff --git a/crates/rome_diagnostics/src/v2/diagnostic.rs b/crates/rome_diagnostics/src/v2/diagnostic.rs
@@ -29,7 +29,7 @@ pub trait Diagnostic: Debug {
     /// The category of a diagnostic uniquely identifying this
     /// diagnostic type, such as `lint/correctness/noArguments`, `args/invalid`
     /// or `format/disabled`.
-    fn category(&self) -> Option<&Category> {
+    fn category(&self) -> Option<&'static Category> {
         None
     }
 

diff --git a/crates/rome_diagnostics/src/v2/display.rs b/crates/rome_diagnostics/src/v2/display.rs
@@ -589,7 +589,7 @@ mod tests {
     }
 
     impl<A: Advices + std::fmt::Debug> Diagnostic for TestDiagnostic<A> {
-        fn category(&self) -> Option<&Category> {
+        fn category(&self) -> Option<&'static Category> {
             Some(category!("internalError/io"))
         }
 

diff --git a/crates/rome_diagnostics/src/v2/serde.rs b/crates/rome_diagnostics/src/v2/serde.rs
@@ -18,7 +18,7 @@ use super::{
 #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))]
 #[cfg_attr(test, derive(Eq, PartialEq))]
 pub struct Diagnostic {
-    category: Option<String>,
+    category: Option<&'static Category>,
     severity: Severity,
     description: String,
     message: MarkupBuf,
@@ -35,7 +35,7 @@ impl Diagnostic {
     }
 
     fn new_impl<D: super::Diagnostic + ?Sized>(diag: &D) -> Self {
-        let category = diag.category().map(|category| category.name().to_string());
+        let category = diag.category();
 
         let severity = diag.severity();
 
@@ -75,15 +75,8 @@ impl Diagnostic {
 }
 
 impl super::Diagnostic for Diagnostic {
-    fn category(&self) -> Option<&Category> {
-        self.category.as_deref().and_then(|name| {
-            let category: Option<&Category> = name.parse().ok();
-            debug_assert!(
-                category.is_some(),
-                "diagnostic category {name:?} does not exist in the static registry"
-            );
-            category
-        })
+    fn category(&self) -> Option<&'static Category> {
+        self.category
     }
 
     fn severity(&self) -> Severity {

diff --git a/crates/rome_diagnostics_categories/src/lib.rs b/crates/rome_diagnostics_categories/src/lib.rs
@@ -53,17 +53,53 @@ impl serde::Serialize for &'static Category {
     }
 }
 
+#[cfg(feature = "serde")]
+struct CategoryVisitor;
+
+#[cfg(feature = "serde")]
+fn deserialize_parse<E: serde::de::Error>(code: &str) -> Result<&'static Category, E> {
+    code.parse().map_err(|()| {
+        serde::de::Error::custom(format_args!("failed to deserialize category from {code}"))
+    })
+}
+
+#[cfg(feature = "serde")]
+impl<'de> serde::de::Visitor<'de> for CategoryVisitor {
+    type Value = &'static Category;
+
+    fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+        formatter.write_str("a borrowed string")
+    }
+
+    fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
+    where
+        E: serde::de::Error,
+    {
+        deserialize_parse(v)
+    }
+
+    fn visit_borrowed_str<E>(self, v: &'de str) -> Result<Self::Value, E>
+    where
+        E: serde::de::Error,
+    {
+        deserialize_parse(v)
+    }
+
+    fn visit_string<E>(self, v: String) -> Result<Self::Value, E>
+    where
+        E: serde::de::Error,
+    {
+        deserialize_parse(&v)
+    }
+}
+
 #[cfg(feature = "serde")]
 impl<'de> serde::Deserialize<'de> for &'static Category {
     fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
     where
         D: serde::Deserializer<'de>,
     {
-        <&str>::deserialize(deserializer).and_then(|code| {
-            code.parse().map_err(|()| {
-                serde::de::Error::custom(format_args!("failed to deserialize category from {code}"))
-            })
-        })
+        deserializer.deserialize_str(CategoryVisitor)
     }
 }
 

diff --git a/crates/rome_diagnostics_macros/src/generate.rs b/crates/rome_diagnostics_macros/src/generate.rs
@@ -54,7 +54,7 @@ fn generate_category(input: &DeriveInput) -> TokenStream {
     };
 
     quote! {
-        fn category(&self) -> Option<&rome_diagnostics::v2::Category> {
+        fn category(&self) -> Option<&'static rome_diagnostics::v2::Category> {
             Some(#category)
         }
     }

diff --git a/crates/rome_fs/src/fs.rs b/crates/rome_fs/src/fs.rs
@@ -6,13 +6,14 @@ use std::{
 };
 
 use crate::{PathInterner, RomePath};
-use rome_diagnostics::{file::FileId, v2::Category};
+use rome_diagnostics::file::FileId;
 
 mod memory;
 mod os;
 
 pub use memory::MemoryFileSystem;
 pub use os::OsFileSystem;
+use rome_diagnostics::v2::Error;
 pub const CONFIG_NAME: &str = "rome.json";
 
 pub trait FileSystem: Send + Sync + RefUnwindSafe {
@@ -127,7 +128,7 @@ pub trait TraversalContext: Sync {
 
     /// Called by the traversal process to emit an error diagnostic associated
     /// with a particular file ID when an IO error happens
-    fn push_diagnostic(&self, file_id: FileId, code: &'static Category, message: String);
+    fn push_diagnostic(&self, error: Error);
 
     /// Checks if the traversal context can handle a particular path, used as
     /// an optimization to bail out of scheduling a file handler if it wouldn't

diff --git a/crates/rome_fs/src/fs/memory.rs b/crates/rome_fs/src/fs/memory.rs
@@ -155,12 +155,13 @@ mod tests {
     };
 
     use parking_lot::Mutex;
+    use rome_diagnostics::v2::Error;
 
     use crate::fs::FileSystemExt;
     use crate::{
         AtomicInterner, FileSystem, MemoryFileSystem, PathInterner, RomePath, TraversalContext,
     };
-    use rome_diagnostics::{file::FileId, v2::Category};
+    use rome_diagnostics::file::FileId;
 
     #[test]
     fn file_read_write() {
@@ -225,11 +226,8 @@ mod tests {
                 &self.interner
             }
 
-            fn push_diagnostic(&self, file_id: FileId, code: &'static Category, message: String) {
-                panic!(
-                    "unexpected error {:?} in file {file_id:?}: {message}",
-                    code.name()
-                )
+            fn push_diagnostic(&self, err: Error) {
+                panic!("unexpected error {err:?}")
             }
 
             fn can_handle(&self, _: &RomePath) -> bool {

diff --git a/crates/rome_fs/src/fs/os.rs b/crates/rome_fs/src/fs/os.rs
@@ -6,8 +6,7 @@ use crate::{
     FileSystem, RomePath,
 };
 use rayon::{scope, Scope};
-use rome_diagnostics::file::FileId;
-use rome_diagnostics::v2::category;
+use rome_diagnostics::v2::{adapters::IoError, Diagnostic, DiagnosticExt, Error, FileId};
 use std::{
     ffi::OsStr,
     fs,
@@ -108,7 +107,7 @@ impl<'scope> TraversalScope<'scope> for OsTraversalScope<'scope> {
         let file_type = match path.metadata() {
             Ok(meta) => meta.file_type(),
             Err(err) => {
-                ctx.push_diagnostic(file_id, category!("internalError/fs"), err.to_string());
+                ctx.push_diagnostic(IoError::from(err).with_file_path(file_id));
                 return;
             }
         };
@@ -127,11 +126,7 @@ impl<'scope> TraversalScope<'scope> for OsTraversalScope<'scope> {
             return;
         }
 
-        ctx.push_diagnostic(
-            file_id,
-            category!("internalError/fs"),
-            "unhandled file type".into(),
-        );
+        ctx.push_diagnostic(Error::from(UnhandledDiagnostic { file_id }));
     }
 }
 
@@ -157,7 +152,7 @@ fn handle_dir<'scope>(
     let iter = match fs::read_dir(path) {
         Ok(iter) => iter,
         Err(err) => {
-            ctx.push_diagnostic(file_id, category!("internalError/fs"), err.to_string());
+            ctx.push_diagnostic(IoError::from(err).with_file_path(file_id));
             return;
         }
     };
@@ -166,7 +161,7 @@ fn handle_dir<'scope>(
         let entry = match entry {
             Ok(entry) => entry,
             Err(err) => {
-                ctx.push_diagnostic(file_id, category!("internalError/fs"), err.to_string());
+                ctx.push_diagnostic(IoError::from(err).with_file_path(file_id));
                 continue;
             }
         };
@@ -177,7 +172,7 @@ fn handle_dir<'scope>(
         let file_type = match entry.file_type() {
             Ok(file_type) => file_type,
             Err(err) => {
-                ctx.push_diagnostic(file_id, category!("internalError/fs"), err.to_string());
+                ctx.push_diagnostic(IoError::from(err).with_file_path(file_id));
                 continue;
             }
         };
@@ -205,10 +200,13 @@ fn handle_dir<'scope>(
             continue;
         }
 
-        ctx.push_diagnostic(
-            file_id,
-            category!("internalError/fs"),
-            "unhandled file type".into(),
-        );
+        ctx.push_diagnostic(Error::from(UnhandledDiagnostic { file_id }));
     }
 }
+
+#[derive(Debug, Diagnostic)]
+#[diagnostic(category = "internalError/fs", message = "unhandled file type")]
+struct UnhandledDiagnostic {
+    #[location(resource)]
+    file_id: FileId,
+}