diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs index 99513ed3acbd4..a6512e15d6e84 100644 --- 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 { + 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,7 +128,9 @@ 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) -> Self { + pub fn from_nested_metas( + nested_metas: &Punctuated, + ) -> Result { let mut traits = ReflectTraits::default(); for nested_meta in nested_metas.iter() { match nested_meta { @@ -115,17 +138,35 @@ impl ReflectTraits { 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,17 +178,24 @@ 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)?; + } + _ => {} } } } @@ -155,7 +203,7 @@ impl ReflectTraits { } } - 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 { match &self.hash { - TraitImpl::Implemented => Some(quote! { + &TraitImpl::Implemented(span) => Some(quote_spanned! {span=> fn reflect_hash(&self) -> Option { 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 { Some(#impl_fn(self)) } @@ -200,7 +248,7 @@ impl ReflectTraits { bevy_reflect_path: &Path, ) -> Option { 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 { let value = value.as_any(); if let Some(value) = value.downcast_ref::() { @@ -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 { Some(#impl_fn(self, value)) } @@ -224,12 +272,12 @@ impl ReflectTraits { /// If `Debug` was not registered, returns `None`. pub fn get_debug_impl(&self) -> Option { 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) } @@ -237,11 +285,42 @@ impl ReflectTraits { 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 { + 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 { let result = Punctuated::::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) -> 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 index 9fb4295ea7e70..7114613649f5f 100644 --- 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 { 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 index a9e57d3fbfac0..fb47aeb6c6f68 100644 --- 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::*;