feat(derive): Make derive macro `diagnostic` attribute more flexible. (#115)

Fixes: #114 

* Improved defaulting

* Added correct combining logic
Added variable number of diagnostic attributes

* Error handling, testing, and docs improvements

Co-authored-by: Kyle Brown <kyleb@liquidrocketry.com>

CONTRIBUTING.md

@@ -88,10 +88,12 @@ If you want to go the usual route and run the project locally, though:
 
 Then in your terminal:
 * `cd path/to/your/clone`
-* `cargo test`
+* `cargo test --features fancy`
 
 And you should be ready to go!
 
+**Note:** If you don't include the "fancy" feature, one of the doc-tests will fail.
+
 ## Contribute Documentation
 
 Documentation is a super important, critical part of this project. Docs are how we keep track of what we're doing, how, and why. It's how we stay on the same page about our policies. And it's how we tell others everything they need in order to be able to use this project -- or contribute to it. So thank you in advance.

miette-derive/src/diagnostic.rs

@@ -69,123 +69,185 @@ pub struct DiagnosticConcreteArgs {
 }
 
 impl DiagnosticConcreteArgs {
-    fn parse(
+    fn for_fields(fields: &syn::Fields) -> Result<Self, syn::Error> {
-        _ident: &syn::Ident,
-        fields: &syn::Fields,
-        attr: &syn::Attribute,
-        args: impl Iterator<Item = DiagnosticArg>,
-    ) -> Result<Self, syn::Error> {
-        let mut code = None;
-        let mut severity = None;
-        let mut help = None;
-        let mut url = None;
-        let mut forward = None;
-        for arg in args {
-            match arg {
-                DiagnosticArg::Transparent => {
-                    return Err(syn::Error::new_spanned(attr, "transparent not allowed"));
-                }
-                DiagnosticArg::Forward(to_field) => {
-                    forward = Some(to_field);
-                }
-                DiagnosticArg::Code(new_code) => {
-                    // TODO: error on multiple?
-                    code = Some(new_code);
-                }
-                DiagnosticArg::Severity(sev) => {
-                    severity = Some(sev);
-                }
-                DiagnosticArg::Help(hl) => {
-                    help = Some(hl);
-                }
-                DiagnosticArg::Url(u) => {
-                    url = Some(u);
-                }
-            }
-        }
         let labels = Labels::from_fields(fields)?;
         let source_code = SourceCode::from_fields(fields)?;
         let related = Related::from_fields(fields)?;
-        let concrete = DiagnosticConcreteArgs {
+        Ok(DiagnosticConcreteArgs {
-            code,
+            code: None,
-            help,
+            help: None,
             related,
-            severity,
+            severity: None,
             labels,
-            url,
+            url: None,
-            forward,
+            forward: None,
             source_code,
-        };
+        })
-        Ok(concrete)
+    }
+
+    fn add_args(
+        &mut self,
+        attr: &syn::Attribute,
+        args: impl Iterator<Item = DiagnosticArg>,
+        errors: &mut Vec<syn::Error>,
+    ) {
+        for arg in args {
+            match arg {
+                DiagnosticArg::Transparent => {
+                    errors.push(syn::Error::new_spanned(attr, "transparent not allowed"));
+                }
+                DiagnosticArg::Forward(to_field) => {
+                    if self.forward.is_some() {
+                        errors.push(syn::Error::new_spanned(
+                            attr,
+                            "forward has already been specified",
+                        ));
+                    }
+                    self.forward = Some(to_field);
+                }
+                DiagnosticArg::Code(new_code) => {
+                    if self.code.is_some() {
+                        errors.push(syn::Error::new_spanned(
+                            attr,
+                            "code has already been specified",
+                        ));
+                    }
+                    self.code = Some(new_code);
+                }
+                DiagnosticArg::Severity(sev) => {
+                    if self.severity.is_some() {
+                        errors.push(syn::Error::new_spanned(
+                            attr,
+                            "severity has already been specified",
+                        ));
+                    }
+                    self.severity = Some(sev);
+                }
+                DiagnosticArg::Help(hl) => {
+                    if self.help.is_some() {
+                        errors.push(syn::Error::new_spanned(
+                            attr,
+                            "help has already been specified",
+                        ));
+                    }
+                    self.help = Some(hl);
+                }
+                DiagnosticArg::Url(u) => {
+                    if self.url.is_some() {
+                        errors.push(syn::Error::new_spanned(
+                            attr,
+                            "url has already been specified",
+                        ));
+                    }
+                    self.url = Some(u);
+                }
+            }
+        }
     }
 }
 
 impl DiagnosticDefArgs {
     fn parse(
-        ident: &syn::Ident,
+        _ident: &syn::Ident,
         fields: &syn::Fields,
-        attr: &syn::Attribute,
+        attrs: &[&syn::Attribute],
         allow_transparent: bool,
     ) -> syn::Result<Self> {
-        let args =
+        let mut errors = Vec::new();
-            attr.parse_args_with(Punctuated::<DiagnosticArg, Token![,]>::parse_terminated)?;
+
-        if allow_transparent
+        // Handle the only condition where Transparent is allowed
-            && args.len() == 1
+        if allow_transparent && attrs.len() == 1 {
-            && matches!(args.first(), Some(DiagnosticArg::Transparent))
+            if let Ok(args) =
-        {
+                attrs[0].parse_args_with(Punctuated::<DiagnosticArg, Token![,]>::parse_terminated)
-            let forward = Forward::for_transparent_field(fields)?;
+            {
-            return Ok(Self::Transparent(forward));
+                if matches!(args.first(), Some(DiagnosticArg::Transparent)) {
-        } else if args.iter().any(|x| matches!(x, DiagnosticArg::Transparent)) {
+                    let forward = Forward::for_transparent_field(fields)?;
-            return Err(syn::Error::new_spanned(
+                    return Ok(Self::Transparent(forward));
-                attr,
+                }
-                if allow_transparent {
+            }
-                    "diagnostic(transparent) not allowed in combination with other args"
+        }
-                } else {
+
-                    "diagnostic(transparent) not allowed here"
+        // Create errors for any appearances of Transparent
-                },
+        let error_message = if allow_transparent {
-            ));
+            "diagnostic(transparent) not allowed in combination with other args"
+        } else {
+            "diagnostic(transparent) not allowed here"
+        };
+        fn is_transparent(d: &DiagnosticArg) -> bool {
+            matches!(d, DiagnosticArg::Transparent)
+        }
+
+        let mut concrete = DiagnosticConcreteArgs::for_fields(fields)?;
+        for attr in attrs {
+            let args =
+                attr.parse_args_with(Punctuated::<DiagnosticArg, Token![,]>::parse_terminated);
+            let args = match args {
+                Ok(args) => args,
+                Err(error) => {
+                    errors.push(error);
+                    continue;
+                }
+            };
+
+            if args.iter().any(is_transparent) {
+                errors.push(syn::Error::new_spanned(attr, error_message));
+            }
+
+            let args = args
+                .into_iter()
+                .filter(|x| !matches!(x, DiagnosticArg::Transparent));
+
+            concrete.add_args(attr, args, &mut errors);
+        }
+
+        let combined_error = errors.into_iter().reduce(|mut lhs, rhs| {
+            lhs.combine(rhs);
+            lhs
+        });
+        if let Some(error) = combined_error {
+            Err(error)
+        } else {
+            Ok(DiagnosticDefArgs::Concrete(Box::new(concrete)))
         }
-        let args = args
-            .into_iter()
-            .filter(|x| !matches!(x, DiagnosticArg::Transparent));
-        let concrete = DiagnosticConcreteArgs::parse(ident, fields, attr, args)?;
-        Ok(DiagnosticDefArgs::Concrete(Box::new(concrete)))
     }
 }
 
 impl Diagnostic {
     pub fn from_derive_input(input: DeriveInput) -> Result<Self, syn::Error> {
+        let input_attrs = input
+            .attrs
+            .iter()
+            .filter(|x| x.path.is_ident("diagnostic"))
+            .collect::<Vec<&syn::Attribute>>();
         Ok(match input.data {
             syn::Data::Struct(data_struct) => {
-                if let Some(attr) = input.attrs.iter().find(|x| x.path.is_ident("diagnostic")) {
+                let args = DiagnosticDefArgs::parse(
-                    let args =
+                    &input.ident,
-                        DiagnosticDefArgs::parse(&input.ident, &data_struct.fields, attr, true)?;
+                    &data_struct.fields,
-                    Diagnostic::Struct {
+                    &input_attrs,
-                        fields: data_struct.fields,
+                    true,
-                        ident: input.ident,
+                )?;
-                        generics: input.generics,
+
-                        args,
+                Diagnostic::Struct {
-                    }
+                    fields: data_struct.fields,
-                } else {
+                    ident: input.ident,
-                    Diagnostic::Struct {
+                    generics: input.generics,
-                        fields: data_struct.fields,
+                    args,
-                        ident: input.ident,
-                        generics: input.generics,
-                        args: DiagnosticDefArgs::Concrete(Default::default()),
-                    }
                 }
             }
             syn::Data::Enum(syn::DataEnum { variants, .. }) => {
                 let mut vars = Vec::new();
                 for var in variants {
-                    if let Some(attr) = var.attrs.iter().find(|x| x.path.is_ident("diagnostic")) {
+                    let mut variant_attrs = input_attrs.clone();
-                        let args = DiagnosticDefArgs::parse(&var.ident, &var.fields, attr, true)?;
+                    variant_attrs
-                        vars.push(DiagnosticDef {
+                        .extend(var.attrs.iter().filter(|x| x.path.is_ident("diagnostic")));
-                            ident: var.ident,
+                    let args =
-                            fields: var.fields,
+                        DiagnosticDefArgs::parse(&var.ident, &var.fields, &variant_attrs, true)?;
-                            args,
+                    vars.push(DiagnosticDef {
-                        });
+                        ident: var.ident,
-                    }
+                        fields: var.fields,
+                        args,
+                    });
                 }
                 Diagnostic::Enum {
                     ident: input.ident,

src/protocol.rs

@@ -191,7 +191,7 @@ pub trait SourceCode: Send + Sync {
 /**
 A labeled [SourceSpan].
 */
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub struct LabeledSpan {
     label: Option<String>,
     span: SourceSpan,

tests/test_derive_attr.rs

@@ -0,0 +1,147 @@
+// Testing of the `diagnostic` attr used by derive(Diagnostic)
+use miette::{Diagnostic, LabeledSpan, NamedSource, SourceSpan};
+use thiserror::Error;
+
+#[test]
+fn enum_uses_base_attr() {
+    #[derive(Debug, Diagnostic, Error)]
+    #[error("oops!")]
+    #[diagnostic(code(error::on::base))]
+    enum MyBad {
+        Only {
+            #[source_code]
+            src: NamedSource,
+            #[label("this bit here")]
+            highlight: SourceSpan,
+        },
+    }
+
+    let src = "source\n  text\n    here".to_string();
+    let err = MyBad::Only {
+        src: NamedSource::new("bad_file.rs", src),
+        highlight: (9, 4).into(),
+    };
+    assert_eq!(err.code().unwrap().to_string(), "error::on::base");
+}
+
+#[test]
+fn enum_uses_variant_attr() {
+    #[derive(Debug, Diagnostic, Error)]
+    #[error("oops!")]
+    enum MyBad {
+        #[diagnostic(code(error::on::variant))]
+        Only {
+            #[source_code]
+            src: NamedSource,
+            #[label("this bit here")]
+            highlight: SourceSpan,
+        },
+    }
+
+    let src = "source\n  text\n    here".to_string();
+    let err = MyBad::Only {
+        src: NamedSource::new("bad_file.rs", src),
+        highlight: (9, 4).into(),
+    };
+    assert_eq!(err.code().unwrap().to_string(), "error::on::variant");
+}
+
+#[test]
+fn multiple_attrs_allowed_on_item() {
+    #[derive(Debug, Diagnostic, Error)]
+    #[error("oops!")]
+    #[diagnostic(code(error::on::base))]
+    #[diagnostic(help("try doing it correctly"))]
+    enum MyBad {
+        Only {
+            #[source_code]
+            src: NamedSource,
+            #[label("this bit here")]
+            highlight: SourceSpan,
+        },
+    }
+
+    let src = "source\n  text\n    here".to_string();
+    let err = MyBad::Only {
+        src: NamedSource::new("bad_file.rs", src),
+        highlight: (9, 4).into(),
+    };
+    assert_eq!(err.code().unwrap().to_string(), "error::on::base");
+    assert_eq!(err.help().unwrap().to_string(), "try doing it correctly");
+}
+
+#[test]
+fn multiple_attrs_allowed_on_variant() {
+    #[derive(Debug, Diagnostic, Error)]
+    #[error("oops!")]
+    enum MyBad {
+        #[diagnostic(code(error::on::variant))]
+        #[diagnostic(help("try doing it correctly"))]
+        Only {
+            #[source_code]
+            src: NamedSource,
+            #[label("this bit here")]
+            highlight: SourceSpan,
+        },
+    }
+
+    let src = "source\n  text\n    here".to_string();
+    let err = MyBad::Only {
+        src: NamedSource::new("bad_file.rs", src),
+        highlight: (9, 4).into(),
+    };
+    assert_eq!(err.code().unwrap().to_string(), "error::on::variant");
+    assert_eq!(err.help().unwrap().to_string(), "try doing it correctly");
+}
+
+#[test]
+fn attrs_can_be_split_between_item_and_variants() {
+    #[derive(Debug, Diagnostic, Error)]
+    #[error("oops!")]
+    #[diagnostic(code(error::on::base))]
+    enum MyBad {
+        #[diagnostic(help("try doing it correctly"))]
+        #[diagnostic(url("https://example.com/foo/bar"))]
+        Only {
+            #[source_code]
+            src: NamedSource,
+            #[label("this bit here")]
+            highlight: SourceSpan,
+        },
+    }
+
+    let src = "source\n  text\n    here".to_string();
+    let err = MyBad::Only {
+        src: NamedSource::new("bad_file.rs", src),
+        highlight: (9, 4).into(),
+    };
+    assert_eq!(err.code().unwrap().to_string(), "error::on::base");
+    assert_eq!(err.help().unwrap().to_string(), "try doing it correctly");
+    assert_eq!(
+        err.url().unwrap().to_string(),
+        "https://example.com/foo/bar".to_string()
+    );
+}
+
+#[test]
+fn attr_not_required() {
+    #[derive(Debug, Diagnostic, Error)]
+    #[error("oops!")]
+    enum MyBad {
+        Only {
+            #[source_code]
+            src: NamedSource,
+            #[label("this bit here")]
+            highlight: SourceSpan,
+        },
+    }
+
+    let src = "source\n  text\n    here".to_string();
+    let err = MyBad::Only {
+        src: NamedSource::new("bad_file.rs", src),
+        highlight: (9, 4).into(),
+    };
+    let err_span = err.labels().unwrap().next().unwrap();
+    let expectation = LabeledSpan::new(Some("this bit here".into()), 9usize.into(), 4usize.into());
+    assert_eq!(err_span, expectation);
+}