From f96cd758cd40efefe29e9931589118247228b07d Mon Sep 17 00:00:00 2001 From: Gino Valente <49806985+MrGVSV@users.noreply.github.com> Date: Wed, 9 Aug 2023 17:37:56 -0700 Subject: [PATCH] bevy_reflect: Opt-out attribute for `TypePath` (#9140) # Objective Fixes #9094 ## Solution Takes a bit from [this](https://github.com/bevyengine/bevy/issues/9094#issuecomment-1629333851) comment as well as a [comment](https://discord.com/channels/691052431525675048/1002362493634629796/1128024873260810271) from @soqb. This allows users to opt-out of the `TypePath` implementation that is automatically generated by the `Reflect` derive macro, allowing custom `TypePath` implementations. ```rust #[derive(Reflect)] #[reflect(type_path = false)] struct Foo { #[reflect(ignore)] _marker: PhantomData, } struct NotTypePath; impl TypePath for Foo { fn type_path() -> &'static str { std::any::type_name::() } fn short_type_path() -> &'static str { static CELL: GenericTypePathCell = GenericTypePathCell::new(); CELL.get_or_insert::(|| { bevy_utils::get_short_name(std::any::type_name::()) }) } fn crate_name() -> Option<&'static str> { Some("my_crate") } fn module_path() -> Option<&'static str> { Some("my_crate::foo") } fn type_ident() -> Option<&'static str> { Some("Foo") } } // Can use `TypePath` let _ = as TypePath>::type_path(); // Can register the type let mut registry = TypeRegistry::default(); registry.register::>(); ``` #### Type Path Stability The stability of type paths mainly come into play during serialization. If a type is moved between builds, an unstable type path may become invalid. Users that opt-out of `TypePath` and rely on something like `std::any::type_name` as in the example above, should be aware that this solution removes the stability guarantees. Deserialization thus expects that type to never move. If it does, then the serialized type paths will need to be updated accordingly. If a user depends on stability, they will need to implement that stability logic manually (probably by looking at the expanded output of a typical `Reflect`/`TypePath` derive). This could be difficult for type parameters that don't/can't implement `TypePath`, and will need to make heavy use of string parsing and manipulation to achieve the same effect (alternatively, they can choose to simply exclude any type parameter that doesn't implement `TypePath`). --- ## Changelog - Added the `#[reflect(type_path = false)]` attribute to opt out of the `TypePath` impl when deriving `Reflect` --------- Co-authored-by: Carter Anderson --- .../src/container_attributes.rs | 103 ++++++++++++++---- .../bevy_reflect_derive/src/derive_data.rs | 2 +- .../bevy_reflect_derive/src/impls/typed.rs | 4 + .../bevy_reflect_derive/src/lib.rs | 7 ++ .../bevy_reflect_derive/src/utility.rs | 26 +++-- crates/bevy_reflect/src/lib.rs | 49 +++++++++ 6 files changed, 159 insertions(+), 32 deletions(-) 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 d0fb321e3d..ad2067e222 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs @@ -13,7 +13,7 @@ use syn::parse::{Parse, ParseStream}; use syn::punctuated::Punctuated; use syn::spanned::Spanned; use syn::token::Comma; -use syn::{LitBool, Meta, Path}; +use syn::{Expr, LitBool, Meta, Path}; // The "special" trait idents that are used internally for reflection. // Received via attributes like `#[reflect(PartialEq, Hash, ...)]` @@ -28,6 +28,9 @@ pub(crate) const REFLECT_DEFAULT: &str = "ReflectDefault"; // Attributes for `FromReflect` implementation const FROM_REFLECT_ATTR: &str = "from_reflect"; +// Attributes for `TypePath` implementation +const TYPE_PATH_ATTR: &str = "type_path"; + // The error message to show when a trait/type is specified multiple times const CONFLICTING_TYPE_DATA_MESSAGE: &str = "conflicting type data registration"; @@ -87,7 +90,48 @@ impl FromReflectAttrs { if existing.value() != new.value() { return Err(syn::Error::new( new.span(), - format!("`from_reflect` already set to {}", existing.value()), + format!("`{FROM_REFLECT_ATTR}` already set to {}", existing.value()), + )); + } + } else { + self.auto_derive = Some(new); + } + } + + Ok(()) + } +} + +/// A collection of attributes used for deriving `TypePath` via the `Reflect` derive. +/// +/// Note that this differs from the attributes used by the `TypePath` derive itself, +/// which look like `[type_path = "my_crate::foo"]`. +/// The attributes used by reflection take the form `#[reflect(type_path = false)]`. +/// +/// These attributes should only be used for `TypePath` configuration specific to +/// deriving `Reflect`. +#[derive(Clone, Default)] +pub(crate) struct TypePathAttrs { + auto_derive: Option, +} + +impl TypePathAttrs { + /// Returns true if `TypePath` should be automatically derived as part of the `Reflect` derive. + pub fn should_auto_derive(&self) -> bool { + self.auto_derive + .as_ref() + .map(|lit| lit.value()) + .unwrap_or(true) + } + + /// Merges this [`TypePathAttrs`] with another. + pub fn merge(&mut self, other: TypePathAttrs) -> Result<(), syn::Error> { + if let Some(new) = other.auto_derive { + if let Some(existing) = &self.auto_derive { + if existing.value() != new.value() { + return Err(syn::Error::new( + new.span(), + format!("`{TYPE_PATH_ATTR}` already set to {}", existing.value()), )); } } else { @@ -165,7 +209,8 @@ pub(crate) struct ReflectTraits { debug: TraitImpl, hash: TraitImpl, partial_eq: TraitImpl, - from_reflect: FromReflectAttrs, + from_reflect_attrs: FromReflectAttrs, + type_path_attrs: TypePathAttrs, idents: Vec, } @@ -244,25 +289,18 @@ impl ReflectTraits { } Meta::NameValue(pair) => { if pair.path.is_ident(FROM_REFLECT_ATTR) { - traits.from_reflect.auto_derive = match &pair.value { - syn::Expr::Lit(syn::ExprLit { - lit: syn::Lit::Bool(lit), - .. - }) => { + traits.from_reflect_attrs.auto_derive = + Some(extract_bool(&pair.value, |lit| { // Override `lit` if this is a `FromReflect` derive. // This typically means a user is opting out of the default implementation // from the `Reflect` derive and using the `FromReflect` derive directly instead. is_from_reflect_derive .then(|| LitBool::new(true, Span::call_site())) - .or_else(|| Some(lit.clone())) - } - _ => { - return Err(syn::Error::new( - pair.value.span(), - "Expected a boolean value", - )) - } - }; + .unwrap_or_else(|| lit.clone()) + })?); + } else if pair.path.is_ident(TYPE_PATH_ATTR) { + traits.type_path_attrs.auto_derive = + Some(extract_bool(&pair.value, Clone::clone)?); } else { return Err(syn::Error::new(pair.path.span(), "Unknown attribute")); } @@ -284,10 +322,15 @@ impl ReflectTraits { &self.idents } - /// The `FromReflect` attributes on this type. + /// The `FromReflect` configuration found within `#[reflect(...)]` attributes on this type. #[allow(clippy::wrong_self_convention)] - pub fn from_reflect(&self) -> &FromReflectAttrs { - &self.from_reflect + pub fn from_reflect_attrs(&self) -> &FromReflectAttrs { + &self.from_reflect_attrs + } + + /// The `TypePath` configuration found within `#[reflect(...)]` attributes on this type. + pub fn type_path_attrs(&self) -> &TypePathAttrs { + &self.type_path_attrs } /// Returns the implementation of `Reflect::reflect_hash` as a `TokenStream`. @@ -366,7 +409,8 @@ impl ReflectTraits { self.debug.merge(other.debug)?; self.hash.merge(other.hash)?; self.partial_eq.merge(other.partial_eq)?; - self.from_reflect.merge(other.from_reflect)?; + self.from_reflect_attrs.merge(other.from_reflect_attrs)?; + self.type_path_attrs.merge(other.type_path_attrs)?; for ident in other.idents { add_unique_ident(&mut self.idents, ident)?; } @@ -392,3 +436,20 @@ fn add_unique_ident(idents: &mut Vec, ident: Ident) -> Result<(), syn::Er idents.push(ident); Ok(()) } + +/// Extract a boolean value from an expression. +/// +/// The mapper exists so that the caller can conditionally choose to use the given +/// value or supply their own. +fn extract_bool( + value: &Expr, + mut mapper: impl FnMut(&LitBool) -> LitBool, +) -> Result { + match value { + syn::Expr::Lit(syn::ExprLit { + lit: syn::Lit::Bool(lit), + .. + }) => Ok(mapper(lit)), + _ => Err(syn::Error::new(value.span(), "Expected a boolean value")), + } +} 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 7a6d015434..ada309aacf 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs @@ -384,7 +384,7 @@ impl<'a> ReflectMeta<'a> { /// The `FromReflect` attributes on this type. #[allow(clippy::wrong_self_convention)] pub fn from_reflect(&self) -> &FromReflectAttrs { - self.traits.from_reflect() + self.traits.from_reflect_attrs() } /// The name of this struct. diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/impls/typed.rs b/crates/bevy_reflect/bevy_reflect_derive/src/impls/typed.rs index 290154692c..5894ef82e5 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/impls/typed.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/impls/typed.rs @@ -52,6 +52,10 @@ pub(crate) fn impl_type_path( meta: &ReflectMeta, where_clause_options: &WhereClauseOptions, ) -> proc_macro2::TokenStream { + if !meta.traits().type_path_attrs().should_auto_derive() { + return proc_macro2::TokenStream::new(); + } + let type_path = meta.type_path(); let bevy_reflect_path = meta.bevy_reflect_path(); diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs b/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs index 031395a465..11c1b88801 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs @@ -128,6 +128,13 @@ pub(crate) static TYPE_NAME_ATTRIBUTE_NAME: &str = "type_name"; /// /// Note that in the latter case, `ReflectFromReflect` will no longer be automatically registered. /// +/// ## `#[reflect(type_path = false)]` +/// +/// This attribute will opt-out of the default `TypePath` implementation. +/// +/// This is useful for when a type can't or shouldn't implement `TypePath`, +/// or if a manual implementation is desired. +/// /// # Field Attributes /// /// Along with the container attributes, this macro comes with some attributes that may be applied diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/utility.rs b/crates/bevy_reflect/bevy_reflect_derive/src/utility.rs index 0e43b9b4e6..925e678f29 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/utility.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/utility.rs @@ -152,16 +152,22 @@ impl WhereClauseOptions { }) .unzip(); - let (parameter_types, parameter_trait_bounds): (Vec<_>, Vec<_>) = meta - .type_path() - .generics() - .type_params() - .map(|param| { - let ident = param.ident.clone(); - let bounds = quote!(#bevy_reflect_path::TypePath); - (ident, bounds) - }) - .unzip(); + let (parameter_types, parameter_trait_bounds): (Vec<_>, Vec<_>) = + if meta.traits().type_path_attrs().should_auto_derive() { + meta.type_path() + .generics() + .type_params() + .map(|param| { + let ident = param.ident.clone(); + let bounds = quote!(#bevy_reflect_path::TypePath); + (ident, bounds) + }) + .unzip() + } else { + // If we don't need to derive `TypePath` for the type parameters, + // we can skip adding its bound to the `where` clause. + (Vec::new(), Vec::new()) + }; Self { active_types: active_types.into_boxed_slice(), diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index 76821257ca..1290ebf48c 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -592,6 +592,7 @@ mod tests { use super::*; use crate as bevy_reflect; use crate::serde::{ReflectSerializer, UntypedReflectDeserializer}; + use crate::utility::GenericTypePathCell; #[test] fn reflect_struct() { @@ -1894,6 +1895,54 @@ bevy_reflect::tests::should_reflect_debug::Test { let _ = > as TypePath>::type_path(); } + #[test] + fn can_opt_out_type_path() { + #[derive(Reflect)] + #[reflect(type_path = false)] + struct Foo { + #[reflect(ignore)] + _marker: PhantomData, + } + + struct NotTypePath; + + impl TypePath for Foo { + fn type_path() -> &'static str { + std::any::type_name::() + } + + fn short_type_path() -> &'static str { + static CELL: GenericTypePathCell = GenericTypePathCell::new(); + CELL.get_or_insert::(|| { + bevy_utils::get_short_name(std::any::type_name::()) + }) + } + + fn crate_name() -> Option<&'static str> { + Some("bevy_reflect") + } + + fn module_path() -> Option<&'static str> { + Some("bevy_reflect::tests") + } + + fn type_ident() -> Option<&'static str> { + Some("Foo") + } + } + + // Can use `TypePath` + let path = as TypePath>::type_path(); + assert_eq!("bevy_reflect::tests::can_opt_out_type_path::Foo", path); + + // Can register the type + let mut registry = TypeRegistry::default(); + registry.register::>(); + + let registration = registry.get(TypeId::of::>()).unwrap(); + assert_eq!("Foo", registration.short_name()); + } + #[cfg(feature = "glam")] mod glam { use super::*;