Repository: TedDriggs/darling (rust)

Issue:
Emit useful error when attrs field is used and forward_attrs is missing
I ran across an error ("Errors were already checked") using FromDeriveInput that seems a little difficult to understand and doesn't seem to have a good explanation anywhere. I narrowed it down to when args is included in the struct. Full repro:

```rust
use darling::FromDeriveInput;
use proc_macro::TokenStream;
use syn::{parse_macro_input, Attribute, Ident};

#[proc_macro_derive(Hello)]
pub fn derive_hello(input: TokenStream) -> TokenStream {
    let input = parse_macro_input!(input as syn::DeriveInput);
    let _args = HelloArgs::from_derive_input(&input);
    TokenStream::new()
}

#[derive(FromDeriveInput)]
struct HelloArgs {
    ident: Ident,
    attrs: Vec<Attribute>, // This is the problem
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_derive_hello() {
        let input = r#"
            #[derive(Hello)]
            struct World;
        "#;
        let input = syn::parse_str(input).unwrap();
        let args = HelloArgs::from_derive_input(&input).unwrap();
        assert_eq!(args.ident, "World");
    }
}
```

```
---- tests::test_derive_hello stdout ----
thread 'tests::test_derive_hello' panicked at src/lib.rs:12:10:
Errors were already checked
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::option::expect_failed
   3: core::option::Option<T>::expect
             at /Users/joshka/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs:933:21
   4: <darling_error::HelloArgs as darling_core::from_derive_input::FromDeriveInput>::from_derive_input
             at ./src/lib.rs:12:10
   5: darling_error::tests::test_derive_hello
             at ./src/lib.rs:30:20
   6: darling_error::tests::test_derive_hello::{{closure}}
             at ./src/lib.rs:24:27
   7: core::ops::function::FnOnce::call_once
             at /Users/joshka/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   8: core::ops::function::FnOnce::call_once
             at /rustc/9fc6b43126469e3858e2fe86cafb4f0fd5068869/library/core/src/ops/function.rs:250:5
```

I'm not sure if this is a bug in darling, I'm assuming that I'm probably using this wrong, but the error message doesn't make it easy to understand what do next. It's also problematic as it doesn't really point back to the `attrs` field as being the problem. Consider adding extra info about what this means and what is missing from the struct / config to make it correct, or documenting this in the docs (the only hit for this error message is the line of code where it's omitted).

Pull request:
Error if `attrs` field is declared and `forward_attrs` is missing.
Previously, this would result in a cryptic error due to an expect(...) failure if neither `attributes` nor `forward_attrs` were declared.

This change is technically breaking, as it was previously possible to declare this field without using `forward_attrs`; if `attributes` was used the `attrs` field would be initialized to an empty array. That behavior was more likely to mask issues than be useful, but it's possible someone declared the field in order to manually populate it later.

Fixes #317

Patch:
diff --git a/core/src/codegen/from_meta_impl.rs b/core/src/codegen/from_meta_impl.rs
index ec34fbe..d96e639 100644
--- a/core/src/codegen/from_meta_impl.rs
+++ b/core/src/codegen/from_meta_impl.rs
@@ -106,6 +106,8 @@ impl ToTokens for FromMetaImpl<'_> {
                         ))
                     });
 
+                let data_variants = variants.iter().map(Variant::as_data_match_arm);
+
                 quote!(
                     fn from_list(__outer: &[::darling::export::NestedMeta]) -> ::darling::Result<Self> {
                         // An enum must have exactly one value inside the parentheses if it's not a unit
@@ -115,7 +117,7 @@ impl ToTokens for FromMetaImpl<'_> {
                             1 => {
                                 if let ::darling::export::NestedMeta::Meta(ref __nested) = __outer[0] {
                                     match ::darling::util::path_to_string(__nested.path()).as_ref() {
-                                        #(#variants)*
+                                        #(#data_variants)*
                                         __other => ::darling::export::Err(::darling::Error::#unknown_variant_err.with_span(__nested))
                                     }
                                 } else {
diff --git a/core/src/codegen/variant.rs b/core/src/codegen/variant.rs
index 5c97d87..8fd742a 100644
--- a/core/src/codegen/variant.rs
+++ b/core/src/codegen/variant.rs
@@ -57,16 +57,6 @@ impl UsesTypeParams for Variant<'_> {
     }
 }
 
-impl ToTokens for Variant<'_> {
-    fn to_tokens(&self, tokens: &mut TokenStream) {
-        if self.data.is_unit() {
-            self.as_unit_match_arm().to_tokens(tokens);
-        } else {
-            self.as_data_match_arm().to_tokens(tokens)
-        }
-    }
-}
-
 /// Code generator for an enum variant in a unit match position.
 /// This is placed in generated `from_string` calls for the parent enum.
 /// Value-carrying variants wrapped in this type will emit code to produce an "unsupported format" error.
@@ -141,8 +131,16 @@ impl ToTokens for DataMatchArm<'_> {
         let ty_ident = val.ty_ident;
 
         if val.data.is_unit() {
+            // Allow unit variants to match a list item if it's just a path with no associated
+            // value, e.g. `volume(shout)` is allowed.
             tokens.append_all(quote!(
-                #name_in_attr => ::darling::export::Err(::darling::Error::unsupported_format("list")),
+                #name_in_attr => {
+                    if let ::darling::export::syn::Meta::Path(_) = *__nested {
+                        ::darling::export::Ok(#ty_ident::#variant_ident)
+                    } else {
+                        ::darling::export::Err(::darling::Error::unsupported_format("non-path"))
+                    }
+                },
             ));
 
             return;
diff --git a/core/src/options/outer_from.rs b/core/src/options/outer_from.rs
index b56d419..5a02a1a 100644
--- a/core/src/options/outer_from.rs
+++ b/core/src/options/outer_from.rs
@@ -1,12 +1,13 @@
 use syn::spanned::Spanned;
 use syn::{Field, Ident, Meta};
 
+use crate::ast::Data;
 use crate::codegen::ForwardAttrs;
 use crate::options::{
     Core, DefaultExpression, ForwardAttrsFilter, ForwardedField, ParseAttribute, ParseData,
 };
 use crate::util::PathList;
-use crate::{FromField, FromMeta, Result};
+use crate::{Error, FromField, FromMeta, Result};
 
 /// Reusable base for `FromDeriveInput`, `FromVariant`, `FromField`, and other top-level
 /// `From*` traits.
@@ -91,5 +92,20 @@ impl ParseData for OuterFrom {
 
     fn validate_body(&self, errors: &mut crate::error::Accumulator) {
         self.container.validate_body(errors);
+        if let Some(attrs) = &self.attrs {
+            if self.forward_attrs.is_none() {
+                let container_name = match &self.container.data {
+                    Data::Enum(_) => "enum",
+                    Data::Struct(_) => "struct",
+                };
+                errors.push(
+                    Error::custom(format!(
+                        "field will not be populated because `forward_attrs` is not set on the {}",
+                        container_name
+                    ))
+                    .with_span(&attrs.ident),
+                );
+            }
+        }
     }
 }


Test patch:
diff --git a/tests/compile-fail/attrs_without_forward_attrs.rs b/tests/compile-fail/attrs_without_forward_attrs.rs
new file mode 100644
index 0000000..b466c59
--- /dev/null
+++ b/tests/compile-fail/attrs_without_forward_attrs.rs
@@ -0,0 +1,10 @@
+use darling::FromDeriveInput;
+use syn::{Attribute, Ident};
+
+#[derive(FromDeriveInput)]
+struct HelloArgs {
+    ident: Ident,
+    attrs: Vec<Attribute>,
+}
+
+fn main() {}
diff --git a/tests/compile-fail/attrs_without_forward_attrs.stderr b/tests/compile-fail/attrs_without_forward_attrs.stderr
new file mode 100644
index 0000000..ea8d36f
--- /dev/null
+++ b/tests/compile-fail/attrs_without_forward_attrs.stderr
@@ -0,0 +1,5 @@
+error: field will not be populated because `forward_attrs` is not set on the struct
+ --> tests/compile-fail/attrs_without_forward_attrs.rs:7:5
+  |
+7 |     attrs: Vec<Attribute>,
+  |     ^^^^^
diff --git a/tests/enums_unit.rs b/tests/enums_unit.rs
index 34e0135..278686f 100644
--- a/tests/enums_unit.rs
+++ b/tests/enums_unit.rs
@@ -1,6 +1,7 @@
 //! Test expansion of enum variants which have no associated data.
 
-use darling::FromMeta;
+use darling::{ast::NestedMeta, FromMeta};
+use syn::{parse_quote, spanned::Spanned, Meta};
 
 #[derive(Debug, FromMeta)]
 #[darling(rename_all = "snake_case")]
@@ -12,3 +13,111 @@ enum Pattern {
 
 #[test]
 fn expansion() {}
+
+#[test]
+fn rejected_in_unit_enum_variants() {
+    #[derive(Debug, FromMeta, PartialEq)]
+    struct Opts {
+        choices: Choices,
+    }
+
+    #[derive(Debug, PartialEq)]
+    struct Choices {
+        values: Vec<Choice>,
+    }
+
+    impl FromMeta for Choices {
+        fn from_list(items: &[NestedMeta]) -> darling::Result<Self> {
+            let values = items
+                .iter()
+                .map(|item| match item {
+                    NestedMeta::Meta(meta) => match meta {
+                        Meta::Path(path) => Choice::from_string(
+                            &path
+                                .get_ident()
+                                .ok_or(
+                                    darling::Error::custom("choice must be an ident (no colon)")
+                                        .with_span(path),
+                                )?
+                                .to_string(),
+                        )
+                        .map_err(|e| e.with_span(path)),
+                        Meta::List(list) => Choice::from_list(&[item.clone()])
+                            .map_err(|e| e.with_span(&list.span())),
+                        Meta::NameValue(n) => Err(darling::Error::custom(
+                            "choice options are not set as name-value, use parentheses",
+                        )
+                        .with_span(&n.eq_token)),
+                    },
+                    _ => {
+                        Err(darling::Error::custom("literal is not a valid choice").with_span(item))
+                    }
+                })
+                .collect::<Result<_, _>>()?;
+            Ok(Self { values })
+        }
+    }
+
+    #[derive(Debug, FromMeta, PartialEq)]
+    enum Choice {
+        One(One),
+        Other,
+    }
+
+    #[derive(Debug, FromMeta, PartialEq)]
+    struct One {
+        foo: String,
+    }
+
+    for (tokens, expected) in [
+        (
+            parse_quote! {
+                choices(one(foo = "bar"))
+            },
+            Ok(Opts {
+                choices: Choices {
+                    values: vec![Choice::One(One {
+                        foo: "bar".to_string(),
+                    })],
+                },
+            }),
+        ),
+        (
+            parse_quote! {
+                choices(other)
+            },
+            Ok(Opts {
+                choices: Choices {
+                    values: vec![Choice::Other],
+                },
+            }),
+        ),
+        (
+            parse_quote! {
+                choices(other, one(foo = "bar"))
+            },
+            Ok(Opts {
+                choices: Choices {
+                    values: vec![
+                        Choice::Other,
+                        Choice::One(One {
+                            foo: "bar".to_string(),
+                        }),
+                    ],
+                },
+            }),
+        ),
+        (
+            parse_quote! {
+                choices(other(foo = "bar"))
+            },
+            Err("Unexpected meta-item format `non-path` at choices".to_string()),
+        ),
+    ] {
+        assert_eq!(
+            Opts::from_list(&NestedMeta::parse_meta_list(tokens).unwrap())
+                .map_err(|e| e.to_string()),
+            expected
+        )
+    }
+}