Repository: TedDriggs/darling (rust)

Issue:
Unknown fields are ignored in unit enum variants
The change in #260 allows non-existent fields to be added to an enum variant, and be silently ignored by `FromMeta::from_list`.

See the test I added in https://github.com/Sufflope/darling/blob/306_unknown_fields_are_ignored_in_unit_enum_variants/tests/unknown_fields.rs which passed up to `0.20.3` (adding parameters to the unit `Choice::Other` yields an error) and fails on the last example since `0.20.4` because the parameters are just ignored and I don't get an error.

The code is a bit convoluted because my use case is having options in an attribute macro, not a derive one, and I wanted to be able to write `#[my_macro(choices(one(params...), other))]`. I could not directly have a `Vec<Choice>` in `Opts`, it fails because `Vec<Choice>` doesn't implement `FromMeta` even though I derive `FromMeta` on `Choice`, that's why I added the `Choices` newtype.

Now it fails to emit an error on the last example because of this change in the `FromMeta` derived for `Choice`:

```diff
 impl ::darling::FromMeta for Choice {
     fn from_list(
         __outer: &[::darling::export::NestedMeta],
     ) -> ::darling::Result<Self> {
         match __outer.len() {
             0 => ::darling::export::Err(::darling::Error::too_few_items(1)),
             1 => {
                 if let ::darling::export::NestedMeta::Meta(ref __nested) = __outer[0]
                 {
                     match ::darling::util::path_to_string(__nested.path()).as_ref() {
                         "one" => {
                             ::darling::export::Ok(
                                 Choice::One(
                                     ::darling::FromMeta::from_meta(__nested)
                                         .map_err(|e| e.at("one"))?,
                                 ),
                             )
                         }
-                        "other" => {
-                            ::darling::export::Err(
-                                ::darling::Error::unsupported_format("list"),
-                            )
-                        }
+                        "other" => ::darling::export::Ok(Choice::Other),
                         __other => {
                             ::darling::export::Err(
                                 ::darling::Error::unknown_field_with_alts(
                                     __other,
                                     &["one", "other"],
                                 )
                                     .with_span(__nested),
                             )
                         }
```

Maybe I'm doing something completely wrong? I wrote this code initially on `darling` `0.14` and I'm trying to update to `0.20` actually.

Pull request:
Reject extra data passed to unit enum variants
- Remove ToTokens impl from Variant to force caller to choose right usage context (e.g. as_data_match_arm)
- Update DataMatchArm to allow unit variants if the value is just a path
- Add test written by @Sufflope to avoid regressions

Fixes #306

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;


Test patch:
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
+        )
+    }
+}