[Merged by Bors] - Support multiple #[reflect]/#[reflect_value] + improve error messages

crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs

@@ -6,10 +6,11 @@
 //! `#[reflect(PartialEq, Default, ...)]` and `#[reflect_value(PartialEq, Default, ...)]`.
 
 use crate::utility;
-use proc_macro2::Ident;
-use quote::quote;
+use proc_macro2::{Ident, Span};
+use quote::quote_spanned;
 use syn::parse::{Parse, ParseStream};
 use syn::punctuated::Punctuated;
+use syn::spanned::Spanned;
 use syn::token::Comma;
 use syn::{Meta, NestedMeta, Path};
 
@@ -23,19 +24,38 @@ const HASH_ATTR: &str = "Hash";
 // but useful to know exist nonetheless
 pub(crate) const REFLECT_DEFAULT: &str = "ReflectDefault";
 
+// The error message to show when a trait is specified multiple times
+const CONFLICTING_IMPL_MESSAGE: &str = "conflicting reflected trait implementation";
+
 /// A marker for trait implementations registered via the `Reflect` derive macro.
 #[derive(Clone, Default)]
 pub(crate) enum TraitImpl {
     /// The trait is not registered as implemented.
     #[default]
     NotImplemented,
+
     /// The trait is registered as implemented.
-    Implemented,
+    Implemented(Span),
 
     // TODO: This can be made to use `ExprPath` instead of `Ident`, allowing for fully qualified paths to be used
     /// The trait is registered with a custom function rather than an actual implementation.
-    Custom(Ident),
+    Custom(Ident, Span),
 }
+
+impl TraitImpl {
+    /// Merges this [`TraitImpl`] with another.
+    ///
+    /// An error is returned if neither value is [`TraitImpl::NotImplemented`].
+    pub fn merge(self, other: TraitImpl) -> Result<TraitImpl, syn::Error> {
+        match (self, other) {
+            (TraitImpl::NotImplemented, value) | (value, TraitImpl::NotImplemented) => Ok(value),
+            (_, TraitImpl::Implemented(span) | TraitImpl::Custom(_, span)) => {
+                Err(syn::Error::new(span, CONFLICTING_IMPL_MESSAGE))
+            }
+        }
+    }
+}
+
 /// A collection of traits that have been registered for a reflected type.
 ///
 /// This keeps track of a few traits that are utilized internally for reflection
@@ -107,25 +127,45 @@ pub(crate) struct ReflectTraits {
 
 impl ReflectTraits {
     /// Create a new [`ReflectTraits`] instance from a set of nested metas.
-    pub fn from_nested_metas(nested_metas: &Punctuated<NestedMeta, Comma>) -> Self {
+    pub fn from_nested_metas(
+        nested_metas: &Punctuated<NestedMeta, Comma>,
+    ) -> Result<Self, syn::Error> {
         let mut traits = ReflectTraits::default();
         for nested_meta in nested_metas.iter() {
             match nested_meta {
                 // Handles `#[reflect( Hash, Default, ... )]`
                 NestedMeta::Meta(Meta::Path(path)) => {
                     // Get the first ident in the path (hopefully the path only contains one and not `std::hash::Hash`)
                     let ident = if let Some(segment) = path.segments.iter().next() {
-                        segment.ident.to_string()
+                        &segment.ident
                     } else {
                         continue;
                     };
+                    let ident_name = ident.to_string();
 
-                    match ident.as_str() {
-                        DEBUG_ATTR => traits.debug = TraitImpl::Implemented,
-                        PARTIAL_EQ_ATTR => traits.partial_eq = TraitImpl::Implemented,
-                        HASH_ATTR => traits.hash = TraitImpl::Implemented,
+                    // Track the span where the trait is implemented for future errors
+                    let span = ident.span();
+
+                    match ident_name.as_str() {
+                        DEBUG_ATTR => {
+                            traits.debug = traits.debug.merge(TraitImpl::Implemented(span))?;
+                        }
+                        PARTIAL_EQ_ATTR => {
+                            traits.partial_eq =
+                                traits.partial_eq.merge(TraitImpl::Implemented(span))?;
+                        }
+                        HASH_ATTR => {
+                            traits.hash = traits.hash.merge(TraitImpl::Implemented(span))?;
+                        }
                         // We only track reflected idents for traits not considered special
-                        _ => traits.idents.push(utility::get_reflect_ident(&ident)),
+                        _ => {
+                            // Create the reflect ident
+                            // We set the span to the old ident so any compile errors point to that ident instead
+                            let mut reflect_ident = utility::get_reflect_ident(&ident_name);
+                            reflect_ident.set_span(span);
+
+                            add_unique_ident(&mut traits.idents, reflect_ident)?;
+                        }
                     }
                 }
                 // Handles `#[reflect( Hash(custom_hash_fn) )]`
@@ -137,11 +177,14 @@ impl ReflectTraits {
                         continue;
                     };
 
+                    // Track the span where the trait is implemented for future errors
+                    let span = ident.span();
+
                     let list_meta = list.nested.iter().next();
                     if let Some(NestedMeta::Meta(Meta::Path(path))) = list_meta {
                         if let Some(segment) = path.segments.iter().next() {
                             // This should be the ident of the custom function
-                            let trait_func_ident = TraitImpl::Custom(segment.ident.clone());
+                            let trait_func_ident = TraitImpl::Custom(segment.ident.clone(), span);
                             match ident.as_str() {
                                 DEBUG_ATTR => traits.debug = trait_func_ident,
                                 PARTIAL_EQ_ATTR => traits.partial_eq = trait_func_ident,
@@ -155,7 +198,7 @@ impl ReflectTraits {
             }
         }
 
-        traits
+        Ok(traits)
     }
 
     /// Returns true if the given reflected trait name (i.e. `ReflectDefault` for `Default`)
@@ -174,7 +217,7 @@ impl ReflectTraits {
     /// If `Hash` was not registered, returns `None`.
     pub fn get_hash_impl(&self, bevy_reflect_path: &Path) -> Option<proc_macro2::TokenStream> {
         match &self.hash {
-            TraitImpl::Implemented => Some(quote! {
+            &TraitImpl::Implemented(span) => Some(quote_spanned! {span=>
                 fn reflect_hash(&self) -> Option<u64> {
                     use std::hash::{Hash, Hasher};
                     let mut hasher = #bevy_reflect_path::ReflectHasher::default();
@@ -183,7 +226,7 @@ impl ReflectTraits {
                     Some(hasher.finish())
                 }
             }),
-            TraitImpl::Custom(impl_fn) => Some(quote! {
+            &TraitImpl::Custom(ref impl_fn, span) => Some(quote_spanned! {span=>
                 fn reflect_hash(&self) -> Option<u64> {
                     Some(#impl_fn(self))
                 }
@@ -200,7 +243,7 @@ impl ReflectTraits {
         bevy_reflect_path: &Path,
     ) -> Option<proc_macro2::TokenStream> {
         match &self.partial_eq {
-            TraitImpl::Implemented => Some(quote! {
+            &TraitImpl::Implemented(span) => Some(quote_spanned! {span=>
                 fn reflect_partial_eq(&self, value: &dyn #bevy_reflect_path::Reflect) -> Option<bool> {
                     let value = value.as_any();
                     if let Some(value) = value.downcast_ref::<Self>() {
@@ -210,7 +253,7 @@ impl ReflectTraits {
                     }
                 }
             }),
-            TraitImpl::Custom(impl_fn) => Some(quote! {
+            &TraitImpl::Custom(ref impl_fn, span) => Some(quote_spanned! {span=>
                 fn reflect_partial_eq(&self, value: &dyn #bevy_reflect_path::Reflect) -> Option<bool> {
                     Some(#impl_fn(self, value))
                 }
@@ -224,24 +267,55 @@ impl ReflectTraits {
     /// If `Debug` was not registered, returns `None`.
     pub fn get_debug_impl(&self) -> Option<proc_macro2::TokenStream> {
         match &self.debug {
-            TraitImpl::Implemented => Some(quote! {
+            &TraitImpl::Implemented(span) => Some(quote_spanned! {span=>
                 fn debug(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
                     std::fmt::Debug::fmt(self, f)
                 }
             }),
-            TraitImpl::Custom(impl_fn) => Some(quote! {
+            &TraitImpl::Custom(ref impl_fn, span) => Some(quote_spanned! {span=>
                 fn debug(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
                     #impl_fn(self, f)
                 }
             }),
             TraitImpl::NotImplemented => None,
         }
     }
+
+    /// Merges the trait implementations of this [`ReflectTraits`] with another one.
+    ///
+    /// An error is returned if the two [`ReflectTraits`] have conflicting implementations.
+    pub fn merge(self, other: ReflectTraits) -> Result<Self, syn::Error> {
+        Ok(ReflectTraits {
+            debug: self.debug.merge(other.debug)?,
+            hash: self.hash.merge(other.hash)?,
+            partial_eq: self.partial_eq.merge(other.partial_eq)?,
+            idents: {
+                let mut idents = self.idents;
+                for ident in other.idents {
+                    add_unique_ident(&mut idents, ident)?;
+                }
+                idents
+            },
+        })
+    }
 }
 
 impl Parse for ReflectTraits {
     fn parse(input: ParseStream) -> syn::Result<Self> {
         let result = Punctuated::<NestedMeta, Comma>::parse_terminated(input)?;
-        Ok(ReflectTraits::from_nested_metas(&result))
+        ReflectTraits::from_nested_metas(&result)
+    }
+}
+
+/// Adds an identifier to a vector of identifiers if it is not already present.
+///
+/// Returns an error if the identifier already exists in the list.
+fn add_unique_ident(idents: &mut Vec<Ident>, ident: Ident) -> Result<(), syn::Error> {
+    let ident_name = ident.to_string();
+    if idents.iter().any(|i| i == ident_name.as_str()) {
+        return Err(syn::Error::new(ident.span(), CONFLICTING_IMPL_MESSAGE));
     }
+
+    idents.push(ident);
+    Ok(())
 }

crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs

@@ -108,28 +108,62 @@ pub(crate) enum EnumVariantFields<'a> {
     Unit,
 }
 
+#[derive(Clone, Copy, PartialEq, Eq)]
+enum ReflectType {
+    Normal,
+    Value,
+}
+
 impl<'a> ReflectDerive<'a> {
     pub fn from_input(input: &'a DeriveInput) -> Result<Self, syn::Error> {
         let mut traits = ReflectTraits::default();
         // Should indicate whether `#[reflect_value]` was used
-        let mut force_reflect_value = false;
+        let mut reflect_type = None;
 
         for attribute in input.attrs.iter().filter_map(|attr| attr.parse_meta().ok()) {
             match attribute {
                 Meta::List(meta_list) if meta_list.path.is_ident(REFLECT_ATTRIBUTE_NAME) => {
-                    traits = ReflectTraits::from_nested_metas(&meta_list.nested);
+                    if !matches!(reflect_type, None | Some(ReflectType::Normal)) {
+                        return Err(syn::Error::new(
+                            meta_list.span(),
+                            format_args!("cannot use both `#[{REFLECT_ATTRIBUTE_NAME}]` and `#[{REFLECT_VALUE_ATTRIBUTE_NAME}]`"),
+                        ));
+                    }
+
+                    reflect_type = Some(ReflectType::Normal);
+                    let new_traits = ReflectTraits::from_nested_metas(&meta_list.nested)?;
+                    traits = traits.merge(new_traits)?;
                 }
                 Meta::List(meta_list) if meta_list.path.is_ident(REFLECT_VALUE_ATTRIBUTE_NAME) => {
-                    force_reflect_value = true;
-                    traits = ReflectTraits::from_nested_metas(&meta_list.nested);
+                    if !matches!(reflect_type, None | Some(ReflectType::Value)) {
+                        return Err(syn::Error::new(
+                            meta_list.span(),
+                            format_args!("cannot use both `#[{REFLECT_ATTRIBUTE_NAME}]` and `#[{REFLECT_VALUE_ATTRIBUTE_NAME}]`"),
+                        ));
+                    }
+
+                    reflect_type = Some(ReflectType::Value);
+                    let new_traits = ReflectTraits::from_nested_metas(&meta_list.nested)?;
+                    traits = traits.merge(new_traits)?;
                 }
                 Meta::Path(path) if path.is_ident(REFLECT_VALUE_ATTRIBUTE_NAME) => {
-                    force_reflect_value = true;
+                    if !matches!(reflect_type, None | Some(ReflectType::Value)) {
+                        return Err(syn::Error::new(
+                            path.span(),
+                            format_args!("cannot use both `#[{REFLECT_ATTRIBUTE_NAME}]` and `#[{REFLECT_VALUE_ATTRIBUTE_NAME}]`"),
+                        ));
+                    }
+
+                    reflect_type = Some(ReflectType::Value);
                 }
                 _ => continue,
             }
         }
-        if force_reflect_value {
+
+        // Use normal reflection if unspecified
+        let reflect_type = reflect_type.unwrap_or(ReflectType::Normal);
+
+        if reflect_type == ReflectType::Value {
             return Ok(Self::Value(ReflectMeta::new(
                 &input.ident,
                 &input.generics,

crates/bevy_reflect/src/lib.rs

@@ -948,6 +948,48 @@ bevy_reflect::tests::should_reflect_debug::Test {
         assert_eq!(expected, format!("\n{:#?}", reflected));
     }
 
+    #[test]
+    fn multiple_reflect_lists() {
+        #[derive(Hash, PartialEq, Reflect)]
+        #[reflect(Debug, Hash)]
+        #[reflect(PartialEq)]
+        struct Foo(i32);
+
+        impl Debug for Foo {
+            fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+                write!(f, "Foo")
+            }
+        }
+
+        let foo = Foo(123);
+        let foo: &dyn Reflect = &foo;
+
+        assert!(foo.reflect_hash().is_some());
+        assert_eq!(Some(true), foo.reflect_partial_eq(foo));
+        assert_eq!("Foo".to_string(), format!("{foo:?}"));
+    }
+
+    #[test]
+    fn multiple_reflect_value_lists() {
+        #[derive(Clone, Hash, PartialEq, Reflect)]
+        #[reflect_value(Debug, Hash)]
+        #[reflect_value(PartialEq)]
+        struct Foo(i32);
+
+        impl Debug for Foo {
+            fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+                write!(f, "Foo")
+            }
+        }
+
+        let foo = Foo(123);
+        let foo: &dyn Reflect = &foo;
+
+        assert!(foo.reflect_hash().is_some());
+        assert_eq!(Some(true), foo.reflect_partial_eq(foo));
+        assert_eq!("Foo".to_string(), format!("{foo:?}"));
+    }
+
     #[cfg(feature = "glam")]
     mod glam {
         use super::*;