diff --git a/compatibility-tests/compile-fail/tests/ui/attribute-duplication.rs b/compatibility-tests/compile-fail/tests/ui/attribute-duplication.rs new file mode 100644 index 00000000..0a5f38d9 --- /dev/null +++ b/compatibility-tests/compile-fail/tests/ui/attribute-duplication.rs @@ -0,0 +1,45 @@ +mod enum_duplication { + use snafu::Snafu; + + #[derive(Debug, Snafu)] + #[snafu(visibility(pub))] + #[snafu(visibility(pub))] + enum EnumError { + #[snafu(display("an error variant"))] + #[snafu(display("should not allow duplicate display"))] + #[snafu(visibility(pub))] + #[snafu(visibility(pub))] + AVariant { + #[snafu(source(from(EnumError, Box::new)))] + #[snafu(backtrace(delegate))] + source: Box, + #[snafu(source)] + #[snafu(backtrace(delegate))] + source2: String, + #[snafu(source)] + #[snafu(backtrace(delegate))] + source3: String, + + #[snafu(backtrace)] + backtrace1: String, + #[snafu(backtrace)] + backtrace2: String, + #[snafu(backtrace)] + backtrace3: String, + }, + } +} + +mod struct_duplication { + use snafu::Snafu; + + #[derive(Debug, Snafu)] + enum UsableError {} + + #[derive(Debug, Snafu)] + #[snafu(source(from(UsableError, Box::new)))] + #[snafu(source(from(UsableError, Box::new)))] + struct StructError(Box); +} + +fn main() {} diff --git a/compatibility-tests/compile-fail/tests/ui/attribute-duplication.stderr b/compatibility-tests/compile-fail/tests/ui/attribute-duplication.stderr new file mode 100644 index 00000000..02493f95 --- /dev/null +++ b/compatibility-tests/compile-fail/tests/ui/attribute-duplication.stderr @@ -0,0 +1,71 @@ +error: Only one field can be designated as 'visibility'; found a duplicate here: + --> $DIR/attribute-duplication.rs:7:10 + | +7 | enum EnumError { + | ^^^^^^^^^ + +error: Only one field can be designated as 'source'; found a duplicate here: + --> $DIR/attribute-duplication.rs:18:13 + | +18 | source2: String, + | ^^^^^^^ + +error: Only one field can be designated as 'source'; found a duplicate here: + --> $DIR/attribute-duplication.rs:21:13 + | +21 | source3: String, + | ^^^^^^^ + +error: Only one field can be designated as 'backtrace'; found a duplicate here: + --> $DIR/attribute-duplication.rs:26:13 + | +26 | backtrace2: String, + | ^^^^^^^^^^ + +error: Only one field can be designated as 'backtrace'; found a duplicate here: + --> $DIR/attribute-duplication.rs:28:13 + | +28 | backtrace3: String, + | ^^^^^^^^^^ + +error: Only one field can be designated as 'backtrace(delegate)'; found a duplicate here: + --> $DIR/attribute-duplication.rs:18:13 + | +18 | source2: String, + | ^^^^^^^ + +error: Only one field can be designated as 'backtrace(delegate)'; found a duplicate here: + --> $DIR/attribute-duplication.rs:21:13 + | +21 | source3: String, + | ^^^^^^^ + +error: Cannot have 'backtrace' and 'backtrace(delegate)' fields in the same enum + --> $DIR/attribute-duplication.rs:24:13 + | +24 | backtrace1: String, + | ^^^^^^^^^^ + +error: Cannot have 'backtrace' and 'backtrace(delegate)' fields in the same enum + --> $DIR/attribute-duplication.rs:15:13 + | +15 | source: Box, + | ^^^^^^ + +error: Only one field can be designated as 'display'; found a duplicate here: + --> $DIR/attribute-duplication.rs:12:9 + | +12 | AVariant { + | ^^^^^^^^ + +error: Only one field can be designated as 'visibility'; found a duplicate here: + --> $DIR/attribute-duplication.rs:12:9 + | +12 | AVariant { + | ^^^^^^^^ + +error: Only one field can be designated as 'source(from)'; found a duplicate here: + --> $DIR/attribute-duplication.rs:41:25 + | +41 | #[snafu(source(from(UsableError, Box::new)))] + | ^^^^^^^^^^^ diff --git a/snafu-derive/src/lib.rs b/snafu-derive/src/lib.rs index 7504abff..f8e652d2 100644 --- a/snafu-derive/src/lib.rs +++ b/snafu-derive/src/lib.rs @@ -110,7 +110,7 @@ struct SyntaxErrors { impl SyntaxErrors { /// Adds a new syntax error. The given description will be used in the compile error pointing /// to the given span. Helper structs are available to format common descriptions, e.g. - /// OnlyValidOn. + /// OnlyValidOn and DuplicateAttribute. fn add(&mut self, span: proc_macro2::Span, description: D) where D: fmt::Display, @@ -118,6 +118,10 @@ impl SyntaxErrors { self.inner.push(syn::Error::new(span, description)); } + fn extend(&mut self, errors: Vec) { + self.inner.extend(errors); + } + /// Consume the SyntaxErrors, returning Ok if there were no syntax errors added, or Err(list) /// if there were syntax errors. fn finish(self) -> MultiSynResult<()> { @@ -150,6 +154,68 @@ impl fmt::Display for OnlyValidOn { } } +/// Helper structure to simplify parameters to SyntaxErrors.add, handling cases where an attribute +/// was incorrectly used multiple times on the same element. +struct DuplicateAttribute { + attribute: &'static str, +} + +impl fmt::Display for DuplicateAttribute { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!( + f, + "Only one field can be designated as '{}'; found a duplicate here:", + self.attribute + ) + } +} + +/// AtMostOne is a helper to track attributes seen during parsing. If more than one item is added, +/// it's added to a list of DuplicateAttribute errors, using the given `name` as a descriptor. +/// +/// When done parsing a structure, call `finish`; it gives you the first attribute found, if any, +/// and the list of errors. +struct AtMostOne { + name: &'static str, + value: Option, + errors: SyntaxErrors, +} + +impl AtMostOne { + /// Creates an AtMostOne to track an attribute with the given `name`. + fn new(name: &'static str) -> Self { + Self { + name, + value: None, + errors: SyntaxErrors::default(), + } + } + + /// Add an occurence of the attribute found at the given `span`. + fn add(&mut self, item: T, span: proc_macro2::Span) { + if let Some(_) = self.value { + self.errors.add( + span, + DuplicateAttribute { + attribute: self.name, + }, + ); + } else { + self.value = Some(item); + } + } + + /// Consumes the AtMostOne, returning the first item added, if any, and the list of errors + /// representing any items added beyond the first. + fn finish(self) -> (Option, Vec) { + let errors = match self.errors.finish() { + Ok(()) => Vec::new(), + Err(vec) => vec, + }; + (self.value, errors) + } +} + fn impl_snafu_macro(ty: syn::DeriveInput) -> TokenStream { match parse_snafu_information(ty) { Ok(info) => info.into(), @@ -198,12 +264,12 @@ fn parse_snafu_enum( let mut errors = SyntaxErrors::default(); - let mut default_visibility = private_visibility(); + let mut default_visibilities = AtMostOne::new("visibility"); for attr in attributes_from_syn(attrs)? { match attr { SnafuAttribute::Visibility(v) => { - default_visibility = v; + default_visibilities.add(v, name.span()); } SnafuAttribute::Display(..) => { errors.add( @@ -255,21 +321,25 @@ fn parse_snafu_enum( } } + let (maybe_default_visibility, errs) = default_visibilities.finish(); + let default_visibility = maybe_default_visibility.unwrap_or_else(private_visibility); + errors.extend(errs); + let variants: sponge::AllErrors<_, _> = enum_ .variants .into_iter() .map(|variant| { let name = variant.ident; - let mut display_format = None; - let mut visibility = None; + let mut display_formats = AtMostOne::new("display"); + let mut visibilities = AtMostOne::new("visibility"); let mut doc_comment = String::new(); let mut reached_end_of_doc_comment = false; for attr in attributes_from_syn(variant.attrs)? { match attr { - SnafuAttribute::Display(d) => display_format = Some(d), - SnafuAttribute::Visibility(v) => visibility = Some(v), + SnafuAttribute::Display(d) => display_formats.add(d, name.span()), + SnafuAttribute::Visibility(v) => visibilities.add(v, name.span()), SnafuAttribute::Source(..) => { errors.add( name.span(), @@ -321,9 +391,9 @@ fn parse_snafu_enum( }; let mut user_fields = Vec::new(); - let mut source_fields = Vec::new(); - let mut backtrace_fields = Vec::new(); - let mut backtrace_delegates = Vec::new(); + let mut source_fields = AtMostOne::new("source"); + let mut backtrace_fields = AtMostOne::new("backtrace"); + let mut backtrace_delegates = AtMostOne::new("backtrace(delegate)"); for syn_field in fields { let span = syn_field.span(); @@ -338,7 +408,7 @@ fn parse_snafu_enum( let mut has_backtrace = false; let mut is_source = None; let mut is_backtrace = None; - let mut transformation = None; + let mut transformations = AtMostOne::new("source(from)"); for attr in attributes_from_syn(syn_field.attrs)? { match attr { @@ -346,7 +416,7 @@ fn parse_snafu_enum( for s in ss { match s { Source::Flag(v) => is_source = Some(v), - Source::From(t, e) => transformation = Some((t, e)), + Source::From(t, e) => transformations.add((t, e), name.span()), } } } @@ -383,35 +453,56 @@ fn parse_snafu_enum( if is_source { if has_backtrace { - backtrace_delegates.push(field.clone()); + backtrace_delegates.add(field.clone(), field.name.span()); } let Field { name, ty } = field; - let transformation = match transformation { - Some((ty, expr)) => Transformation::Transform { ty, expr }, - None => Transformation::None { ty }, - }; - - source_fields.push(SourceField { + let (maybe_transformation, errs) = transformations.finish(); + let transformation = maybe_transformation + .map(|(ty, expr)| Transformation::Transform { ty, expr }) + .unwrap_or(Transformation::None { ty }); + errors.extend(errs); + + let span = name.span(); + source_fields.add(SourceField { name, transformation, - }); + }, span); } else if is_backtrace { - backtrace_fields.push(field); + let span = field.name.span(); + backtrace_fields.add(field, span); } else { user_fields.push(field); } } - let source_field = source_fields.pop(); - // Report a warning if there are multiple? + let (source_field, errs) = source_fields.finish(); + errors.extend(errs); - let backtrace_field = backtrace_fields.pop(); - // Report a warning if there are multiple? + let (backtrace_field, errs) = backtrace_fields.finish(); + errors.extend(errs); - let backtrace_delegate = backtrace_delegates.pop(); - // Report a warning if there are multiple? - // Report a warning if delegating and our own? + let (backtrace_delegate, errs) = backtrace_delegates.finish(); + errors.extend(errs); + + if let (Some(backtrace_field), Some(backtrace_delegate)) = + (backtrace_field.as_ref(), backtrace_delegate.as_ref()) + { + errors.add( + backtrace_field.name.span(), + "Cannot have 'backtrace' and 'backtrace(delegate)' fields in the same enum", + ); + errors.add( + backtrace_delegate.name.span(), + "Cannot have 'backtrace' and 'backtrace(delegate)' fields in the same enum", + ); + } + + let (display_format, errs) = display_formats.finish(); + errors.extend(errs); + + let (visibility, errs) = visibilities.finish(); + errors.extend(errs); Ok(VariantInfo { name, @@ -445,9 +536,10 @@ fn parse_snafu_struct( attrs: Vec, span: proc_macro2::Span, ) -> MultiSynResult { + use syn::spanned::Spanned; use syn::Fields; - let mut transformation = None; + let mut transformations = AtMostOne::new("source(from)"); let mut errors = SyntaxErrors::default(); @@ -486,7 +578,10 @@ fn parse_snafu_struct( }, ); } - Source::From(t, e) => transformation = Some((t, e)), + Source::From(t, e) => { + let span = t.span(); + transformations.add((t, e), span) + }, } } } @@ -529,12 +624,13 @@ fn parse_snafu_struct( return Err(vec![one_field_error(span)]); } - let transformation = match transformation { - Some((ty, expr)) => Transformation::Transform { ty, expr }, - None => Transformation::None { + let (maybe_transformation, errs) = transformations.finish(); + let transformation = maybe_transformation + .map(|(ty, expr)| Transformation::Transform { ty, expr }) + .unwrap_or(Transformation::None { ty: inner.into_value().ty, - }, - }; + }); + errors.extend(errs); errors.finish()?;