bevy_reflect: Opt-out attribute for TypePath (#9140)

Fixes #9094

Takes a bit from
[this](#9094 (comment))
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
struct Foo<T> {
    #[reflect(ignore)]
    _marker: PhantomData<T>,
}

struct NotTypePath;

impl<T: 'static> TypePath for Foo<T> {
    fn type_path() -> &'static str {
        std::any::type_name::<Self>()
    }

    fn short_type_path() -> &'static str {
        static CELL: GenericTypePathCell = GenericTypePathCell::new();
        CELL.get_or_insert::<Self, _>(|| {
            bevy_utils::get_short_name(std::any::type_name::<Self>())
        })
    }

    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 _ = <Foo<NotTypePath> as TypePath>::type_path();

// Can register the type
let mut registry = TypeRegistry::default();
registry.register::<Foo<NotTypePath>>();
```

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`).

---

- Added the `#[reflect(type_path = false)]` attribute to opt out of the
`TypePath` impl when deriving `Reflect`

---------

Co-authored-by: Carter Anderson <[email protected]>

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<LitBool>,
+}
+
+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<Ident>,
 }
 
@@ -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),
-                                ..
-                            }) => {
-                                // Overrride `lit` if this is a `FromReflect` derive.
+                        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: 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<LitBool, syn::Error> {
+    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")),
+    }
+}

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.

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();
 

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

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(),

crates/bevy_reflect/src/lib.rs

@@ -569,6 +569,7 @@ mod tests {
     use super::*;
     use crate as bevy_reflect;
     use crate::serde::{ReflectSerializer, UntypedReflectDeserializer};
+    use crate::utility::GenericTypePathCell;
 
     #[test]
     fn reflect_struct() {
@@ -1871,6 +1872,54 @@ bevy_reflect::tests::should_reflect_debug::Test {
         let _ = <Recurse<Recurse<()>> as TypePath>::type_path();
     }
 
+    #[test]
+    fn can_opt_out_type_path() {
+        #[derive(Reflect)]
+        #[reflect(type_path = false)]
+        struct Foo<T> {
+            #[reflect(ignore)]
+            _marker: PhantomData<T>,
+        }
+
+        struct NotTypePath;
+
+        impl<T: 'static> TypePath for Foo<T> {
+            fn type_path() -> &'static str {
+                std::any::type_name::<Self>()
+            }
+
+            fn short_type_path() -> &'static str {
+                static CELL: GenericTypePathCell = GenericTypePathCell::new();
+                CELL.get_or_insert::<Self, _>(|| {
+                    bevy_utils::get_short_name(std::any::type_name::<Self>())
+                })
+            }
+
+            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 = <Foo<NotTypePath> as TypePath>::type_path();
+        assert_eq!("bevy_reflect::tests::can_opt_out_type_path::Foo<bevy_reflect::tests::can_opt_out_type_path::NotTypePath>", path);
+
+        // Can register the type
+        let mut registry = TypeRegistry::default();
+        registry.register::<Foo<NotTypePath>>();
+
+        let registration = registry.get(TypeId::of::<Foo<NotTypePath>>()).unwrap();
+        assert_eq!("Foo<NotTypePath>", registration.short_name());
+    }
+
     #[cfg(feature = "glam")]
     mod glam {
         use super::*;