Add error checks for duplicate attributes

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<EnumError>,
+            #[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<UsableError>);
+}
+
+fn main() {}

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<EnumError>,
+   |             ^^^^^^
+
+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)))]
+   |                         ^^^^^^^^^^^

snafu-derive/src/lib.rs

@@ -110,14 +110,18 @@ 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<D>(&mut self, span: proc_macro2::Span, description: D)
     where
         D: fmt::Display,
     {
         self.inner.push(syn::Error::new(span, description));
     }
 
+    fn extend(&mut self, errors: Vec<syn::Error>) {
+        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<T> {
+    name: &'static str,
+    value: Option<T>,
+    errors: SyntaxErrors,
+}
+
+impl<T> AtMostOne<T> {
+    /// 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<T>, Vec<syn::Error>) {
+        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,15 +408,15 @@ 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 {
                         SnafuAttribute::Source(ss) => {
                             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<syn::Attribute>,
     span: proc_macro2::Span,
 ) -> MultiSynResult<StructInfo> {
+    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()?;