From 5b8b5478b63e91a51fadec87c6fed3e60d192b60 Mon Sep 17 00:00:00 2001 From: Kyle Brown Date: Fri, 18 Feb 2022 01:04:03 -0500 Subject: [PATCH] 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 --- CONTRIBUTING.md | 4 +- miette-derive/src/diagnostic.rs | 242 ++++++++++++++++++++------------ src/protocol.rs | 2 +- tests/test_derive_attr.rs | 147 +++++++++++++++++++ 4 files changed, 303 insertions(+), 92 deletions(-) create mode 100644 tests/test_derive_attr.rs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e39fc31..b38d029 100644 --- a/CONTRIBUTING.md +++ b/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. diff --git a/miette-derive/src/diagnostic.rs b/miette-derive/src/diagnostic.rs index 11e36b2..0d57841 100644 --- a/miette-derive/src/diagnostic.rs +++ b/miette-derive/src/diagnostic.rs @@ -69,123 +69,185 @@ pub struct DiagnosticConcreteArgs { } impl DiagnosticConcreteArgs { - fn parse( - _ident: &syn::Ident, - fields: &syn::Fields, - attr: &syn::Attribute, - args: impl Iterator, - ) -> Result { - 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); - } - } - } + fn for_fields(fields: &syn::Fields) -> Result { let labels = Labels::from_fields(fields)?; let source_code = SourceCode::from_fields(fields)?; let related = Related::from_fields(fields)?; - let concrete = DiagnosticConcreteArgs { - code, - help, + Ok(DiagnosticConcreteArgs { + code: None, + help: None, related, - severity, + severity: None, labels, - url, - forward, + url: None, + forward: None, source_code, - }; - Ok(concrete) + }) + } + + fn add_args( + &mut self, + attr: &syn::Attribute, + args: impl Iterator, + errors: &mut Vec, + ) { + 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 { - let args = - attr.parse_args_with(Punctuated::::parse_terminated)?; - if allow_transparent - && args.len() == 1 - && matches!(args.first(), Some(DiagnosticArg::Transparent)) - { - let forward = Forward::for_transparent_field(fields)?; - return Ok(Self::Transparent(forward)); - } else if args.iter().any(|x| matches!(x, DiagnosticArg::Transparent)) { - return Err(syn::Error::new_spanned( - attr, - if allow_transparent { - "diagnostic(transparent) not allowed in combination with other args" - } else { - "diagnostic(transparent) not allowed here" - }, - )); + let mut errors = Vec::new(); + + // Handle the only condition where Transparent is allowed + if allow_transparent && attrs.len() == 1 { + if let Ok(args) = + attrs[0].parse_args_with(Punctuated::::parse_terminated) + { + if matches!(args.first(), Some(DiagnosticArg::Transparent)) { + let forward = Forward::for_transparent_field(fields)?; + return Ok(Self::Transparent(forward)); + } + } + } + + // 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::::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 { + let input_attrs = input + .attrs + .iter() + .filter(|x| x.path.is_ident("diagnostic")) + .collect::>(); 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(&input.ident, &data_struct.fields, attr, true)?; - Diagnostic::Struct { - fields: data_struct.fields, - ident: input.ident, - generics: input.generics, - args, - } - } else { - Diagnostic::Struct { - fields: data_struct.fields, - ident: input.ident, - generics: input.generics, - args: DiagnosticDefArgs::Concrete(Default::default()), - } + let args = DiagnosticDefArgs::parse( + &input.ident, + &data_struct.fields, + &input_attrs, + true, + )?; + + Diagnostic::Struct { + fields: data_struct.fields, + ident: input.ident, + generics: input.generics, + args, } } 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 args = DiagnosticDefArgs::parse(&var.ident, &var.fields, attr, true)?; - vars.push(DiagnosticDef { - ident: var.ident, - fields: var.fields, - args, - }); - } + let mut variant_attrs = input_attrs.clone(); + variant_attrs + .extend(var.attrs.iter().filter(|x| x.path.is_ident("diagnostic"))); + let args = + DiagnosticDefArgs::parse(&var.ident, &var.fields, &variant_attrs, true)?; + vars.push(DiagnosticDef { + ident: var.ident, + fields: var.fields, + args, + }); } Diagnostic::Enum { ident: input.ident, diff --git a/src/protocol.rs b/src/protocol.rs index ae2f014..a2e68db 100644 --- a/src/protocol.rs +++ b/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, span: SourceSpan, diff --git a/tests/test_derive_attr.rs b/tests/test_derive_attr.rs new file mode 100644 index 0000000..df255ff --- /dev/null +++ b/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); +}