Support multiple #[reflect]/#[reflect_value] + improve error messages (#6237)

TehPers · TehPers · commit 132e8fb3821c · 2022-10-17T14:38:56.000Z
# Objective

Currently, surprising behavior happens when specifying `#[reflect(...)]` or `#[reflect_value(...)]` multiple times. Rather than merging the traits lists from all attributes, only the trait list from the last attribute is used. For example, in the following code, only the `Debug` and `Hash` traits are reflected and not `Default` or `PartialEq`:

```rs
#[derive(Debug, PartialEq, Hash, Default, Reflect)]
#[reflect(PartialEq, Default)]
#[reflect(Debug, Hash)]
struct Foo;
```

This is especially important when some traits should only be reflected under certain circumstances. For example, this previously had surprisingly behavior when the "serialize" feature is enabled:

```rs
#[derive(Debug, Hash, Reflect)]
#[reflect(Debug, Hash)]
#[cfg_attr(
    feature = "serialize",
    derive(Serialize, Deserialize),
    reflect(Serialize, Deserialize)
]
struct Foo;
```

In addition, compile error messages generated from using the derive macro often point to the `#[derive(Reflect)]` rather than to the source of the error. It would be a lot more helpful if the compiler errors pointed to what specifically caused the error rather than just to the derive macro itself.

## Solution

Merge the trait lists in all `#[reflect(...)]` and `#[reflect_value(...)]` attributes. Additionally, make `#[reflect]` and `#[reflect_value]` mutually exclusive.

Additionally, span information is carried throughout some parts of the code now to ensure that error messages point to more useful places and better indicate what caused those errors. For example, `#[reflect(Hash, Hash)]` points to the second `Hash` as the source of an error. Also, in the following example, the compiler error now points to the `Hash` in `#[reflect(Hash)]` rather than to the derive macro:

```rs
#[derive(Reflect)]
#[reflect(Hash)] // &lt;-- compiler error points to `Hash` for lack of a `Hash` implementation
struct Foo;
```

---

## Changelog

Changed
- Using multiple `#[reflect(...)]` or `#[reflect_value(...)]` attributes now merges the trait lists. For example, `#[reflect(Debug, Hash)] #[reflect(PartialEq, Default)]` is equivalent to `#[reflect(Debug, Hash, PartialEq, Default)]`.
  - Multiple `#[reflect(...)]` and `#[reflect_value(...)]` attributes were previously accepted, but only the last attribute was respected.
  - Using both `#[reflect(...)]` and `#[reflect_value(...)]` was previously accepted, but had surprising behavior. This is no longer accepted.
- Improved error messages for `#[derive(Reflect)]` by propagating useful span information. Many errors should now point to the source of those errors rather than to the derive macro.
diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs b/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,39 @@ 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/type is specified multiple times
+const CONFLICTING_TYPE_DATA_MESSAGE: &str = "conflicting type data registration";
+
 /// 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(Path, Span),
+}
+
+impl TraitImpl {
+    /// Merges this [`TraitImpl`] with another.
+    ///
+    /// Returns whichever value is not [`TraitImpl::NotImplemented`].
+    /// If both values are [`TraitImpl::NotImplemented`], then that is returned.
+    /// Otherwise, 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_TYPE_DATA_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 +128,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();
+
+                    // Track the span where the trait is implemented for future errors
+                    let span = ident.span();
 
-                    match ident.as_str() {
-                        DEBUG_ATTR => traits.debug = TraitImpl::Implemented,
-                        PARTIAL_EQ_ATTR => traits.partial_eq = TraitImpl::Implemented,
-                        HASH_ATTR => traits.hash = TraitImpl::Implemented,
+                    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,25 +178,32 @@ 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());
-                            match ident.as_str() {
-                                DEBUG_ATTR => traits.debug = trait_func_ident,
-                                PARTIAL_EQ_ATTR => traits.partial_eq = trait_func_ident,
-                                HASH_ATTR => traits.hash = trait_func_ident,
-                                _ => {}
+                        // This should be the path of the custom function
+                        let trait_func_ident = TraitImpl::Custom(path.clone(), span);
+                        match ident.as_str() {
+                            DEBUG_ATTR => {
+                                traits.debug = traits.debug.merge(trait_func_ident)?;
+                            }
+                            PARTIAL_EQ_ATTR => {
+                                traits.partial_eq = traits.partial_eq.merge(trait_func_ident)?;
                             }
+                            HASH_ATTR => {
+                                traits.hash = traits.hash.merge(trait_func_ident)?;
+                            }
+                            _ => {}
                         }
                     }
                 }
                 _ => {}
             }
         }
 
-        traits
+        Ok(traits)
     }
 
     /// Returns true if the given reflected trait name (i.e. `ReflectDefault` for `Default`)
@@ -174,7 +222,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 +231,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 +248,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 +258,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 +272,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_TYPE_DATA_MESSAGE));
+    }
+
+    idents.push(ident);
+    Ok(())
+}
diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs b/crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs
@@ -108,28 +108,65 @@ pub(crate) enum EnumVariantFields<'a> {
     Unit,
 }
 
+/// The method in which the type should be reflected.
+#[derive(PartialEq, Eq)]
+enum ReflectMode {
+    /// Reflect the type normally, providing information about all fields/variants.
+    Normal,
+    /// Reflect the type as a value.
+    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_mode = 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_mode, None | Some(ReflectMode::Normal)) {
+                        return Err(syn::Error::new(
+                            meta_list.span(),
+                            format_args!("cannot use both `#[{REFLECT_ATTRIBUTE_NAME}]` and `#[{REFLECT_VALUE_ATTRIBUTE_NAME}]`"),
+                        ));
+                    }
+
+                    reflect_mode = Some(ReflectMode::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_mode, None | Some(ReflectMode::Value)) {
+                        return Err(syn::Error::new(
+                            meta_list.span(),
+                            format_args!("cannot use both `#[{REFLECT_ATTRIBUTE_NAME}]` and `#[{REFLECT_VALUE_ATTRIBUTE_NAME}]`"),
+                        ));
+                    }
+
+                    reflect_mode = Some(ReflectMode::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_mode, None | Some(ReflectMode::Value)) {
+                        return Err(syn::Error::new(
+                            path.span(),
+                            format_args!("cannot use both `#[{REFLECT_ATTRIBUTE_NAME}]` and `#[{REFLECT_VALUE_ATTRIBUTE_NAME}]`"),
+                        ));
+                    }
+
+                    reflect_mode = Some(ReflectMode::Value);
                 }
                 _ => continue,
             }
         }
-        if force_reflect_value {
+
+        // Use normal reflection if unspecified
+        let reflect_mode = reflect_mode.unwrap_or(ReflectMode::Normal);
+
+        if reflect_mode == ReflectMode::Value {
             return Ok(Self::Value(ReflectMeta::new(
                 &input.ident,
                 &input.generics,
diff --git a/crates/bevy_reflect/src/lib.rs b/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::*;
