bevy_reflect_derive: Clean up attribute logic (bevyengine#11777)

# Objective

The code in `bevy_reflect_derive` could use some cleanup.

## Solution

Took some of the changes in bevyengine#11659 to create a dedicated PR for cleaning
up the field and container attribute logic.

#### Updated Naming

I renamed `ReflectTraits` and `ReflectFieldAttr` to
`ContainerAttributes` and `FieldAttributes`, respectively. I think these
are clearer.

#### Updated Parsing

##### Readability

The parsing logic wasn't too bad before, but it was getting difficult to
read. There was some duplicated logic between `Meta::List` and
`Meta::Path` attributes. Additionally, all the logic was kept inside a
large method.

To simply things, I replaced the nested meta parsing with `ParseStream`
parsing. In my opinion, this is easier to follow since it breaks up the
large match statement into a small set of single-line if statements,
where each if-block contains a single call to the appropriate attribute
parsing method.

##### Flexibility

On top of the added simplicity, this also makes our attribute parsing
much more flexible. It allows us to more elegantly handle custom where
clauses (i.e. `#[reflect(where T: Foo)]`) and it opens the door for more
non-standard attribute syntax (e.g. bevyengine#11659).

##### Errors

This also allows us to automatically provide certain errors when
parsing. For example, since we can use `stream.lookahead1()`, we get
errors like the following for free:

```
error: expected one of: `ignore`, `skip_serializing`, `default`
    --> crates/bevy_reflect/src/lib.rs:1988:23
     |
1988 |             #[reflect(foo)]
     |                       ^^^
```

---

## Changelog

> [!note]
> All changes are internal to `bevy_reflect_derive` and should not
affect the public API[^1].

- Renamed `ReflectTraits` to `ContainerAttributes`
  - Renamed `ReflectMeta::traits` to `ReflectMeta::attrs`
- Renamed `ReflectFieldAttr` to `FieldAttributes`
- Updated parsing logic for field/container attribute parsing
  - Now uses a `ParseStream` directly instead of nested meta parsing
- General code cleanup of the field/container attribute modules for
`bevy_reflect_derive`


[^1]: Does not include errors, which may look slightly different.

---------

Co-authored-by: Alice Cecile <[email protected]>

crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs

@@ -1,7 +1,7 @@
 use core::fmt;
 
-use crate::container_attributes::{FromReflectAttrs, ReflectTraits, TypePathAttrs};
-use crate::field_attributes::{parse_field_attrs, ReflectFieldAttr};
+use crate::container_attributes::{ContainerAttributes, FromReflectAttrs, TypePathAttrs};
+use crate::field_attributes::FieldAttributes;
 use crate::type_path::parse_path_no_leading_colon;
 use crate::utility::{StringExpr, WhereClauseOptions};
 use quote::{quote, ToTokens};
@@ -42,7 +42,7 @@ pub(crate) enum ReflectDerive<'a> {
 /// ```
 pub(crate) struct ReflectMeta<'a> {
     /// The registered traits for this type.
-    traits: ReflectTraits,
+    attrs: ContainerAttributes,
     /// The path to this type.
     type_path: ReflectTypePath<'a>,
     /// A cached instance of the path to the `bevy_reflect` crate.
@@ -95,7 +95,7 @@ pub(crate) struct StructField<'a> {
     /// The raw field.
     pub data: &'a Field,
     /// The reflection-based attributes on the field.
-    pub attrs: ReflectFieldAttr,
+    pub attrs: FieldAttributes,
     /// The index of this field within the struct.
     pub declaration_index: usize,
     /// The index of this field as seen by the reflection API.
@@ -118,7 +118,7 @@ pub(crate) struct EnumVariant<'a> {
     pub fields: EnumVariantFields<'a>,
     /// The reflection-based attributes on the variant.
     #[allow(dead_code)]
-    pub attrs: ReflectFieldAttr,
+    pub attrs: FieldAttributes,
     /// The index of this variant within the enum.
     #[allow(dead_code)]
     pub index: usize,
@@ -183,7 +183,7 @@ impl<'a> ReflectDerive<'a> {
         input: &'a DeriveInput,
         provenance: ReflectProvenance,
     ) -> Result<Self, syn::Error> {
-        let mut traits = ReflectTraits::default();
+        let mut traits = ContainerAttributes::default();
         // Should indicate whether `#[reflect_value]` was used.
         let mut reflect_mode = None;
         // Should indicate whether `#[type_path = "..."]` was used.
@@ -205,7 +205,8 @@ impl<'a> ReflectDerive<'a> {
                     }
 
                     reflect_mode = Some(ReflectMode::Normal);
-                    let new_traits = ReflectTraits::from_meta_list(meta_list, provenance.trait_)?;
+                    let new_traits =
+                        ContainerAttributes::parse_meta_list(meta_list, provenance.trait_)?;
                     traits.merge(new_traits)?;
                 }
                 Meta::List(meta_list) if meta_list.path.is_ident(REFLECT_VALUE_ATTRIBUTE_NAME) => {
@@ -217,7 +218,8 @@ impl<'a> ReflectDerive<'a> {
                     }
 
                     reflect_mode = Some(ReflectMode::Value);
-                    let new_traits = ReflectTraits::from_meta_list(meta_list, provenance.trait_)?;
+                    let new_traits =
+                        ContainerAttributes::parse_meta_list(meta_list, provenance.trait_)?;
                     traits.merge(new_traits)?;
                 }
                 Meta::Path(path) if path.is_ident(REFLECT_VALUE_ATTRIBUTE_NAME) => {
@@ -361,7 +363,7 @@ impl<'a> ReflectDerive<'a> {
             .enumerate()
             .map(
                 |(declaration_index, field)| -> Result<StructField, syn::Error> {
-                    let attrs = parse_field_attrs(&field.attrs)?;
+                    let attrs = FieldAttributes::parse_attributes(&field.attrs)?;
 
                     let reflection_index = if attrs.ignore.is_ignored() {
                         None
@@ -404,7 +406,7 @@ impl<'a> ReflectDerive<'a> {
                 };
                 Ok(EnumVariant {
                     fields,
-                    attrs: parse_field_attrs(&variant.attrs)?,
+                    attrs: FieldAttributes::parse_attributes(&variant.attrs)?,
                     data: variant,
                     index,
                     #[cfg(feature = "documentation")]
@@ -421,9 +423,9 @@ impl<'a> ReflectDerive<'a> {
 }
 
 impl<'a> ReflectMeta<'a> {
-    pub fn new(type_path: ReflectTypePath<'a>, traits: ReflectTraits) -> Self {
+    pub fn new(type_path: ReflectTypePath<'a>, attrs: ContainerAttributes) -> Self {
         Self {
-            traits,
+            attrs,
             type_path,
             bevy_reflect_path: utility::get_bevy_reflect_path(),
             #[cfg(feature = "documentation")]
@@ -438,19 +440,19 @@ impl<'a> ReflectMeta<'a> {
     }
 
     /// The registered reflect traits on this struct.
-    pub fn traits(&self) -> &ReflectTraits {
-        &self.traits
+    pub fn attrs(&self) -> &ContainerAttributes {
+        &self.attrs
     }
 
     /// The `FromReflect` attributes on this type.
     #[allow(clippy::wrong_self_convention)]
     pub fn from_reflect(&self) -> &FromReflectAttrs {
-        self.traits.from_reflect_attrs()
+        self.attrs.from_reflect_attrs()
     }
 
     /// The `TypePath` attributes on this type.
     pub fn type_path_attrs(&self) -> &TypePathAttrs {
-        self.traits.type_path_attrs()
+        self.attrs.type_path_attrs()
     }
 
     /// The path to this type.

crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs

@@ -4,14 +4,21 @@
 //! as opposed to an entire struct or enum. An example of such an attribute is
 //! the derive helper attribute for `Reflect`, which looks like: `#[reflect(ignore)]`.
 
+use crate::utility::terminated_parser;
 use crate::REFLECT_ATTRIBUTE_NAME;
-use syn::meta::ParseNestedMeta;
-use syn::{Attribute, LitStr, Token};
+use syn::parse::ParseStream;
+use syn::{Attribute, LitStr, Meta, Token};
 
-pub(crate) static IGNORE_SERIALIZATION_ATTR: &str = "skip_serializing";
-pub(crate) static IGNORE_ALL_ATTR: &str = "ignore";
+mod kw {
+    syn::custom_keyword!(ignore);
+    syn::custom_keyword!(skip_serializing);
+    syn::custom_keyword!(default);
+}
+
+pub(crate) const IGNORE_SERIALIZATION_ATTR: &str = "skip_serializing";
+pub(crate) const IGNORE_ALL_ATTR: &str = "ignore";
 
-pub(crate) static DEFAULT_ATTR: &str = "default";
+pub(crate) const DEFAULT_ATTR: &str = "default";
 
 /// Stores data about if the field should be visible via the Reflect and serialization interfaces
 ///
@@ -44,15 +51,6 @@ impl ReflectIgnoreBehavior {
     }
 }
 
-/// A container for attributes defined on a reflected type's field.
-#[derive(Default, Clone)]
-pub(crate) struct ReflectFieldAttr {
-    /// Determines how this field should be ignored if at all.
-    pub ignore: ReflectIgnoreBehavior,
-    /// Sets the default behavior of this field.
-    pub default: DefaultBehavior,
-}
-
 /// Controls how the default value is determined for a field.
 #[derive(Default, Clone)]
 pub(crate) enum DefaultBehavior {
@@ -68,79 +66,114 @@ pub(crate) enum DefaultBehavior {
     Func(syn::ExprPath),
 }
 
-/// Parse all field attributes marked "reflect" (such as `#[reflect(ignore)]`).
-pub(crate) fn parse_field_attrs(attrs: &[Attribute]) -> Result<ReflectFieldAttr, syn::Error> {
-    let mut args = ReflectFieldAttr::default();
-    let mut errors: Option<syn::Error> = None;
-
-    let attrs = attrs
-        .iter()
-        .filter(|a| a.path().is_ident(REFLECT_ATTRIBUTE_NAME));
-    for attr in attrs {
-        let result = attr.parse_nested_meta(|meta| parse_meta(&mut args, meta));
-        if let Err(err) = result {
-            if let Some(ref mut error) = errors {
-                error.combine(err);
-            } else {
-                errors = Some(err);
-            }
-        }
-    }
-
-    if let Some(error) = errors {
-        Err(error)
-    } else {
-        Ok(args)
-    }
+/// A container for attributes defined on a reflected type's field.
+#[derive(Default, Clone)]
+pub(crate) struct FieldAttributes {
+    /// Determines how this field should be ignored if at all.
+    pub ignore: ReflectIgnoreBehavior,
+    /// Sets the default behavior of this field.
+    pub default: DefaultBehavior,
 }
 
-fn parse_meta(args: &mut ReflectFieldAttr, meta: ParseNestedMeta) -> Result<(), syn::Error> {
-    if meta.path.is_ident(DEFAULT_ATTR) {
-        // Allow:
-        // - `#[reflect(default)]`
-        // - `#[reflect(default = "path::to::func")]`
-        if !matches!(args.default, DefaultBehavior::Required) {
-            return Err(meta.error(format!("only one of [{:?}] is allowed", [DEFAULT_ATTR])));
-        }
+impl FieldAttributes {
+    /// Parse all field attributes marked "reflect" (such as `#[reflect(ignore)]`).
+    pub fn parse_attributes(attrs: &[Attribute]) -> syn::Result<Self> {
+        let mut args = FieldAttributes::default();
+
+        attrs
+            .iter()
+            .filter_map(|attr| {
+                if !attr.path().is_ident(REFLECT_ATTRIBUTE_NAME) {
+                    // Not a reflect attribute -> skip
+                    return None;
+                }
+
+                let Meta::List(meta) = &attr.meta else {
+                    return Some(syn::Error::new_spanned(attr, "expected meta list"));
+                };
+
+                // Parse all attributes inside the list, collecting any errors
+                meta.parse_args_with(terminated_parser(Token![,], |stream| {
+                    args.parse_field_attribute(stream)
+                }))
+                .err()
+            })
+            .reduce(|mut acc, err| {
+                acc.combine(err);
+                acc
+            })
+            .map_or(Ok(args), Err)
+    }
 
-        if meta.input.peek(Token![=]) {
-            let lit = meta.value()?.parse::<LitStr>()?;
-            args.default = DefaultBehavior::Func(lit.parse()?);
+    /// Parses a single field attribute.
+    fn parse_field_attribute(&mut self, input: ParseStream) -> syn::Result<()> {
+        let lookahead = input.lookahead1();
+        if lookahead.peek(kw::ignore) {
+            self.parse_ignore(input)
+        } else if lookahead.peek(kw::skip_serializing) {
+            self.parse_skip_serializing(input)
+        } else if lookahead.peek(kw::default) {
+            self.parse_default(input)
         } else {
-            args.default = DefaultBehavior::Default;
+            Err(lookahead.error())
         }
+    }
 
-        Ok(())
-    } else if meta.path.is_ident(IGNORE_ALL_ATTR) {
-        // Allow:
-        // - `#[reflect(ignore)]`
-        if args.ignore != ReflectIgnoreBehavior::None {
-            return Err(meta.error(format!(
-                "only one of [{:?}] is allowed",
+    /// Parse `ignore` attribute.
+    ///
+    /// Examples:
+    /// - `#[reflect(ignore)]`
+    fn parse_ignore(&mut self, input: ParseStream) -> syn::Result<()> {
+        if self.ignore != ReflectIgnoreBehavior::None {
+            return Err(input.error(format!(
+                "only one of {:?} is allowed",
                 [IGNORE_ALL_ATTR, IGNORE_SERIALIZATION_ATTR]
             )));
         }
 
-        args.ignore = ReflectIgnoreBehavior::IgnoreAlways;
-
+        input.parse::<kw::ignore>()?;
+        self.ignore = ReflectIgnoreBehavior::IgnoreAlways;
         Ok(())
-    } else if meta.path.is_ident(IGNORE_SERIALIZATION_ATTR) {
-        // Allow:
-        // - `#[reflect(skip_serializing)]`
-        if args.ignore != ReflectIgnoreBehavior::None {
-            return Err(meta.error(format!(
-                "only one of [{:?}] is allowed",
+    }
+
+    /// Parse `skip_serializing` attribute.
+    ///
+    /// Examples:
+    /// - `#[reflect(skip_serializing)]`
+    fn parse_skip_serializing(&mut self, input: ParseStream) -> syn::Result<()> {
+        if self.ignore != ReflectIgnoreBehavior::None {
+            return Err(input.error(format!(
+                "only one of {:?} is allowed",
                 [IGNORE_ALL_ATTR, IGNORE_SERIALIZATION_ATTR]
             )));
         }
 
-        args.ignore = ReflectIgnoreBehavior::IgnoreSerialization;
+        input.parse::<kw::skip_serializing>()?;
+        self.ignore = ReflectIgnoreBehavior::IgnoreSerialization;
+        Ok(())
+    }
+
+    /// Parse `default` attribute.
+    ///
+    /// Examples:
+    /// - `#[reflect(default)]`
+    /// - `#[reflect(default = "path::to::func")]`
+    fn parse_default(&mut self, input: ParseStream) -> syn::Result<()> {
+        if !matches!(self.default, DefaultBehavior::Required) {
+            return Err(input.error(format!("only one of {:?} is allowed", [DEFAULT_ATTR])));
+        }
+
+        input.parse::<kw::default>()?;
+
+        if input.peek(Token![=]) {
+            input.parse::<Token![=]>()?;
+
+            let lit = input.parse::<LitStr>()?;
+            self.default = DefaultBehavior::Func(lit.parse()?);
+        } else {
+            self.default = DefaultBehavior::Default;
+        }
 
         Ok(())
-    } else {
-        Err(meta.error(format!(
-            "unknown attribute, expected {:?}",
-            [DEFAULT_ATTR, IGNORE_ALL_ATTR, IGNORE_SERIALIZATION_ATTR]
-        )))
     }
 }

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs

@@ -98,7 +98,7 @@ fn impl_struct_internal(
     let MemberValuePair(active_members, active_values) =
         get_active_fields(reflect_struct, &ref_struct, &ref_struct_type, is_tuple);
 
-    let is_defaultable = reflect_struct.meta().traits().contains(REFLECT_DEFAULT);
+    let is_defaultable = reflect_struct.meta().attrs().contains(REFLECT_DEFAULT);
     let constructor = if is_defaultable {
         quote!(
             let mut __this: Self = #FQDefault::default();

crates/bevy_reflect/bevy_reflect_derive/src/impls/enums.rs

@@ -35,7 +35,7 @@ pub(crate) fn impl_enum(reflect_enum: &ReflectEnum) -> proc_macro2::TokenStream
 
     let hash_fn = reflect_enum
         .meta()
-        .traits()
+        .attrs()
         .get_hash_impl(bevy_reflect_path)
         .unwrap_or_else(|| {
             quote! {
@@ -44,10 +44,10 @@ pub(crate) fn impl_enum(reflect_enum: &ReflectEnum) -> proc_macro2::TokenStream
                 }
             }
         });
-    let debug_fn = reflect_enum.meta().traits().get_debug_impl();
+    let debug_fn = reflect_enum.meta().attrs().get_debug_impl();
     let partial_eq_fn = reflect_enum
         .meta()
-        .traits()
+        .attrs()
         .get_partial_eq_impl(bevy_reflect_path)
         .unwrap_or_else(|| {
             quote! {

crates/bevy_reflect/bevy_reflect_derive/src/impls/structs.rs

@@ -32,11 +32,11 @@ pub(crate) fn impl_struct(reflect_struct: &ReflectStruct) -> proc_macro2::TokenS
 
     let hash_fn = reflect_struct
         .meta()
-        .traits()
+        .attrs()
         .get_hash_impl(bevy_reflect_path);
-    let debug_fn = reflect_struct.meta().traits().get_debug_impl();
+    let debug_fn = reflect_struct.meta().attrs().get_debug_impl();
     let partial_eq_fn = reflect_struct.meta()
-        .traits()
+        .attrs()
         .get_partial_eq_impl(bevy_reflect_path)
         .unwrap_or_else(|| {
             quote! {

crates/bevy_reflect/bevy_reflect_derive/src/impls/tuple_structs.rs

@@ -24,12 +24,12 @@ pub(crate) fn impl_tuple_struct(reflect_struct: &ReflectStruct) -> proc_macro2::
 
     let hash_fn = reflect_struct
         .meta()
-        .traits()
+        .attrs()
         .get_hash_impl(bevy_reflect_path);
-    let debug_fn = reflect_struct.meta().traits().get_debug_impl();
+    let debug_fn = reflect_struct.meta().attrs().get_debug_impl();
     let partial_eq_fn = reflect_struct
         .meta()
-        .traits()
+        .attrs()
         .get_partial_eq_impl(bevy_reflect_path)
         .unwrap_or_else(|| {
             quote! {