bevy_reflect: Type parameter bounds (#9046)

# Objective

Fixes #8965.

#### Background

For convenience and to ensure everything is setup properly, we
automatically add certain bounds to the derived types. The current
implementation does this by taking the types from all active fields and
adding them to the where-clause of the generated impls. I believe this
method was chosen because it won't add bounds to types that are
otherwise ignored.

```rust
#[derive(Reflect)]
struct Foo<T, U: SomeTrait, V> {
  t: T,
  u: U::Assoc,
  #[reflect(ignore)]
  v: [V; 2]
}

// Generates something like:
impl<T, U: SomeTrait, V> for Foo<T, U, V>
where
  // Active:
  T: Reflect,
  U::Assoc: Reflect,

  // Ignored:
  [V; 2]: Send + Sync + Any
{
  // ...
}
```

The self-referential type fails because it ends up using _itself_ as a
type bound due to being one of its own active fields.

```rust
#[derive(Reflect)]
struct Foo {
  foo: Vec<Foo>
}

// Foo where Vec<Foo>: Reflect -> Vec<T> where T: Reflect -> Foo where Vec<Foo>: Reflect -> ...
```

## Solution

We can't simply parse all field types for the name of our type. That
would be both complex and prone to errors and false-positives. And even
if it wasn't, what would we replace the bound with?

Instead, I opted to go for a solution that only adds the bounds to what
really needs it: the type parameters. While the bounds on concrete types
make errors a bit cleaner, they aren't strictly necessary. This means we
can change our generated where-clause to only add bounds to generic type
parameters.

Doing this, though, returns us back to the problem of over-bounding
parameters that don't need to be bounded. To solve this, I added a new
container attribute (based on
[this](dtolnay/syn#422 (comment))
comment and @nicopap's
[comment](#9046 (comment)))
that allows us to pass in a custom where clause to modify what bounds
are added to these type parameters.

This allows us to do stuff like:

```rust
trait Trait {
  type Assoc;
}

// We don't need `T` to be reflectable since we only care about `T::Assoc`.
#[derive(Reflect)]
#[reflect(where T::Assoc: FromReflect)]
struct Foo<T: Trait>(T::Assoc);

#[derive(TypePath)]
struct Bar;

impl Trait for Bar {
  type Assoc = usize;
}

#[derive(Reflect)]
struct Baz {
  a: Foo<Bar>,
}
```

> **Note**
> I also
[tried](dc139ea)
allowing `#[reflect(ignore)]` to be used on the type parameters
themselves, but that proved problematic since the derive macro does not
consume the attribute. This is why I went with the container attribute
approach.

### Alternatives

One alternative could possibly be to just not add reflection bounds
automatically (i.e. only add required bounds like `Send`, `Sync`, `Any`,
and `TypePath`).

The downside here is we add more friction to using reflection, which
already comes with its own set of considerations. This is a potentially
viable option, but we really need to consider whether or not the
ergonomics hit is worth it.

If we did decide to go the more manual route, we should at least
consider something like #5772 to make it easier for users to add the
right bounds (although, this could still become tricky with
`FromReflect` also being automatically derived).

### Open Questions

1. Should we go with this approach or the manual alternative?
2. ~~Should we add a `skip_params` attribute to avoid the `T: 'static`
trick?~~ ~~Decided to go with `custom_where()` as it's the simplest~~
Scratch that, went with a normal where clause
3. ~~`custom_where` bikeshedding?~~ No longer needed since we are using
a normal where clause

### TODO

- [x] Add compile-fail tests

---

## Changelog

- Fixed issue preventing recursive types from deriving `Reflect`
- Changed how where-clause bounds are generated by the `Reflect` derive
macro
- They are now only applied to the type parameters, not to all active
fields
- Added `#[reflect(where T: Trait, U::Assoc: Trait, ...)]` container
attribute

## Migration Guide

When deriving `Reflect`, generic type params that do not need the
automatic reflection bounds (such as `Reflect`) applied to them will
need to opt-out using a custom where clause like: `#[reflect(where T:
Trait, U::Assoc: Trait, ...)]`.

The attribute can define custom bounds only used by the reflection
impls. To simply opt-out all the type params, we can pass in an empty
where clause: `#[reflect(where)]`.

```rust
// BEFORE:
#[derive(Reflect)]
struct Foo<T>(#[reflect(ignore)] T);

// AFTER:
#[derive(Reflect)]
#[reflect(where)]
struct Foo<T>(#[reflect(ignore)] T);
```

---------

Co-authored-by: Nicola Papale <[email protected]>

Author: MrGVSV

crates/bevy_asset/src/handle.rs

@@ -122,6 +122,7 @@ impl std::fmt::Debug for StrongHandle {
 /// [`Handle::Strong`] also provides access to useful [`Asset`] metadata, such as the [`AssetPath`] (if it exists).
 #[derive(Component, Reflect)]
 #[reflect(Component)]
+#[reflect(where A: Asset)]
 pub enum Handle<A: Asset> {
     /// A "strong" reference to a live (or loading) [`Asset`]. If a [`Handle`] is [`Handle::Strong`], the [`Asset`] will be kept
     /// alive until the [`Handle`] is dropped. Strong handles also provide access to additional asset metadata.

crates/bevy_asset/src/id.rs

@@ -17,6 +17,7 @@ use thiserror::Error;
 ///
 /// For an "untyped" / "generic-less" id, see [`UntypedAssetId`].
 #[derive(Reflect)]
+#[reflect(where A: Asset)]
 pub enum AssetId<A: Asset> {
     /// A small / efficient runtime identifier that can be used to efficiently look up an asset stored in [`Assets`]. This is
     /// the "default" identifier used for assets. The alternative(s) (ex: [`AssetId::Uuid`]) will only be used if assets are

crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs

@@ -7,13 +7,13 @@
 
 use crate::utility;
 use bevy_macro_utils::fq_std::{FQAny, FQOption};
-use proc_macro2::{Ident, Span};
+use proc_macro2::{Ident, Span, TokenTree};
 use quote::quote_spanned;
 use syn::parse::{Parse, ParseStream};
 use syn::punctuated::Punctuated;
 use syn::spanned::Spanned;
 use syn::token::Comma;
-use syn::{Expr, LitBool, Meta, Path};
+use syn::{Expr, LitBool, Meta, MetaList, Path, WhereClause};
 
 // The "special" trait idents that are used internally for reflection.
 // Received via attributes like `#[reflect(PartialEq, Hash, ...)]`
@@ -211,24 +211,40 @@ pub(crate) struct ReflectTraits {
     partial_eq: TraitImpl,
     from_reflect_attrs: FromReflectAttrs,
     type_path_attrs: TypePathAttrs,
+    custom_where: Option<WhereClause>,
     idents: Vec<Ident>,
 }
 
 impl ReflectTraits {
-    pub fn from_metas(
+    pub fn from_meta_list(
+        meta: &MetaList,
+        is_from_reflect_derive: bool,
+    ) -> Result<Self, syn::Error> {
+        match meta.tokens.clone().into_iter().next() {
+            // Handles `#[reflect(where T: Trait, U::Assoc: Trait)]`
+            Some(TokenTree::Ident(ident)) if ident == "where" => Ok(Self {
+                custom_where: Some(meta.parse_args::<WhereClause>()?),
+                ..Self::default()
+            }),
+            _ => Self::from_metas(
+                meta.parse_args_with(Punctuated::<Meta, Comma>::parse_terminated)?,
+                is_from_reflect_derive,
+            ),
+        }
+    }
+
+    fn from_metas(
         metas: Punctuated<Meta, Comma>,
         is_from_reflect_derive: bool,
     ) -> Result<Self, syn::Error> {
         let mut traits = ReflectTraits::default();
         for meta in &metas {
             match meta {
-                // Handles `#[reflect( Hash, Default, ... )]`
+                // Handles `#[reflect( Debug, PartialEq, Hash, SomeTrait )]`
                 Meta::Path(path) => {
-                    // Get the first ident in the path (hopefully the path only contains one and not `std::hash::Hash`)
-                    let Some(segment) = path.segments.iter().next() else {
+                    let Some(ident) = path.get_ident() else {
                         continue;
                     };
-                    let ident = &segment.ident;
                     let ident_name = ident.to_string();
 
                     // Track the span where the trait is implemented for future errors
@@ -255,38 +271,38 @@ impl ReflectTraits {
                         }
                     }
                 }
+                // Handles `#[reflect( Debug(custom_debug_fn) )]`
+                Meta::List(list) if list.path.is_ident(DEBUG_ATTR) => {
+                    let ident = list.path.get_ident().unwrap();
+                    list.parse_nested_meta(|meta| {
+                        let trait_func_ident = TraitImpl::Custom(meta.path, ident.span());
+                        traits.debug.merge(trait_func_ident)
+                    })?;
+                }
+                // Handles `#[reflect( PartialEq(custom_partial_eq_fn) )]`
+                Meta::List(list) if list.path.is_ident(PARTIAL_EQ_ATTR) => {
+                    let ident = list.path.get_ident().unwrap();
+                    list.parse_nested_meta(|meta| {
+                        let trait_func_ident = TraitImpl::Custom(meta.path, ident.span());
+                        traits.partial_eq.merge(trait_func_ident)
+                    })?;
+                }
                 // Handles `#[reflect( Hash(custom_hash_fn) )]`
-                Meta::List(list) => {
-                    // Get the first ident in the path (hopefully the path only contains one and not `std::hash::Hash`)
-                    let Some(segment) = list.path.segments.iter().next() else {
-                        continue;
-                    };
-
-                    let ident = segment.ident.to_string();
-
-                    // Track the span where the trait is implemented for future errors
-                    let span = ident.span();
-
+                Meta::List(list) if list.path.is_ident(HASH_ATTR) => {
+                    let ident = list.path.get_ident().unwrap();
                     list.parse_nested_meta(|meta| {
-                        // This should be the path of the custom function
-                        let trait_func_ident = TraitImpl::Custom(meta.path, span);
-                        match ident.as_str() {
-                            DEBUG_ATTR => {
-                                traits.debug.merge(trait_func_ident)?;
-                            }
-                            PARTIAL_EQ_ATTR => {
-                                traits.partial_eq.merge(trait_func_ident)?;
-                            }
-                            HASH_ATTR => {
-                                traits.hash.merge(trait_func_ident)?;
-                            }
-                            _ => {
-                                return Err(syn::Error::new(span, "Can only use custom functions for special traits (i.e. `Hash`, `PartialEq`, `Debug`)"));
-                            }
-                        }
-                        Ok(())
+                        let trait_func_ident = TraitImpl::Custom(meta.path, ident.span());
+                        traits.hash.merge(trait_func_ident)
                     })?;
                 }
+                Meta::List(list) => {
+                    return Err(syn::Error::new_spanned(
+                        list,
+                        format!(
+                            "expected one of [{DEBUG_ATTR:?}, {PARTIAL_EQ_ATTR:?}, {HASH_ATTR:?}]"
+                        ),
+                    ));
+                }
                 Meta::NameValue(pair) => {
                     if pair.path.is_ident(FROM_REFLECT_ATTR) {
                         traits.from_reflect_attrs.auto_derive =
@@ -402,6 +418,10 @@ impl ReflectTraits {
         }
     }
 
+    pub fn custom_where(&self) -> Option<&WhereClause> {
+        self.custom_where.as_ref()
+    }
+
     /// Merges the trait implementations of this [`ReflectTraits`] with another one.
     ///
     /// An error is returned if the two [`ReflectTraits`] have conflicting implementations.
@@ -411,11 +431,26 @@ impl ReflectTraits {
         self.partial_eq.merge(other.partial_eq)?;
         self.from_reflect_attrs.merge(other.from_reflect_attrs)?;
         self.type_path_attrs.merge(other.type_path_attrs)?;
+
+        self.merge_custom_where(other.custom_where);
+
         for ident in other.idents {
             add_unique_ident(&mut self.idents, ident)?;
         }
         Ok(())
     }
+
+    fn merge_custom_where(&mut self, other: Option<WhereClause>) {
+        match (&mut self.custom_where, other) {
+            (Some(this), Some(other)) => {
+                this.predicates.extend(other.predicates);
+            }
+            (None, Some(other)) => {
+                self.custom_where = Some(other);
+            }
+            _ => {}
+        }
+    }
 }
 
 impl Parse for ReflectTraits {

crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs

@@ -167,10 +167,8 @@ impl<'a> ReflectDerive<'a> {
                     }
 
                     reflect_mode = Some(ReflectMode::Normal);
-                    let new_traits = ReflectTraits::from_metas(
-                        meta_list.parse_args_with(Punctuated::<Meta, Comma>::parse_terminated)?,
-                        is_from_reflect_derive,
-                    )?;
+                    let new_traits =
+                        ReflectTraits::from_meta_list(meta_list, is_from_reflect_derive)?;
                     traits.merge(new_traits)?;
                 }
                 Meta::List(meta_list) if meta_list.path.is_ident(REFLECT_VALUE_ATTRIBUTE_NAME) => {
@@ -182,10 +180,8 @@ impl<'a> ReflectDerive<'a> {
                     }
 
                     reflect_mode = Some(ReflectMode::Value);
-                    let new_traits = ReflectTraits::from_metas(
-                        meta_list.parse_args_with(Punctuated::<Meta, Comma>::parse_terminated)?,
-                        is_from_reflect_derive,
-                    )?;
+                    let new_traits =
+                        ReflectTraits::from_meta_list(meta_list, is_from_reflect_derive)?;
                     traits.merge(new_traits)?;
                 }
                 Meta::Path(path) if path.is_ident(REFLECT_VALUE_ATTRIBUTE_NAME) => {
@@ -484,7 +480,7 @@ impl<'a> ReflectStruct<'a> {
     }
 
     pub fn where_clause_options(&self) -> WhereClauseOptions {
-        WhereClauseOptions::new(self.meta(), self.active_fields(), self.ignored_fields())
+        WhereClauseOptions::new(self.meta())
     }
 }
 
@@ -507,22 +503,8 @@ impl<'a> ReflectEnum<'a> {
         &self.variants
     }
 
-    /// Get an iterator of fields which are exposed to the reflection API
-    pub fn active_fields(&self) -> impl Iterator<Item = &StructField<'a>> {
-        self.variants()
-            .iter()
-            .flat_map(|variant| variant.active_fields())
-    }
-
-    /// Get an iterator of fields which are ignored by the reflection API
-    pub fn ignored_fields(&self) -> impl Iterator<Item = &StructField<'a>> {
-        self.variants()
-            .iter()
-            .flat_map(|variant| variant.ignored_fields())
-    }
-
     pub fn where_clause_options(&self) -> WhereClauseOptions {
-        WhereClauseOptions::new(self.meta(), self.active_fields(), self.ignored_fields())
+        WhereClauseOptions::new(self.meta())
     }
 }
 
@@ -668,6 +650,7 @@ impl<'a> ReflectTypePath<'a> {
             where_clause: None,
             params: Punctuated::new(),
         };
+
         match self {
             Self::Internal { generics, .. } | Self::External { generics, .. } => generics,
             _ => EMPTY_GENERICS,

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs

@@ -2,7 +2,7 @@ use crate::container_attributes::REFLECT_DEFAULT;
 use crate::derive_data::ReflectEnum;
 use crate::enum_utility::{get_variant_constructors, EnumVariantConstructors};
 use crate::field_attributes::DefaultBehavior;
-use crate::utility::{extend_where_clause, ident_or_index, WhereClauseOptions};
+use crate::utility::{ident_or_index, WhereClauseOptions};
 use crate::{ReflectMeta, ReflectStruct};
 use bevy_macro_utils::fq_std::{FQAny, FQClone, FQDefault, FQOption};
 use proc_macro2::Span;
@@ -24,7 +24,7 @@ pub(crate) fn impl_value(meta: &ReflectMeta) -> proc_macro2::TokenStream {
     let bevy_reflect_path = meta.bevy_reflect_path();
     let (impl_generics, ty_generics, where_clause) = type_path.generics().split_for_impl();
     let where_from_reflect_clause =
-        extend_where_clause(where_clause, &WhereClauseOptions::new_value(meta));
+        WhereClauseOptions::new_type_path(meta).extend_where_clause(where_clause);
     quote! {
         impl #impl_generics #bevy_reflect_path::FromReflect for #type_path #ty_generics #where_from_reflect_clause  {
             fn from_reflect(reflect: &dyn #bevy_reflect_path::Reflect) -> #FQOption<Self> {
@@ -50,22 +50,8 @@ pub(crate) fn impl_enum(reflect_enum: &ReflectEnum) -> proc_macro2::TokenStream
     let (impl_generics, ty_generics, where_clause) = enum_path.generics().split_for_impl();
 
     // Add FromReflect bound for each active field
-    let where_from_reflect_clause = extend_where_clause(
-        where_clause,
-        &WhereClauseOptions::new_with_bounds(
-            reflect_enum.meta(),
-            reflect_enum.active_fields(),
-            reflect_enum.ignored_fields(),
-            |field| match &field.attrs.default {
-                DefaultBehavior::Default => Some(quote!(#FQDefault)),
-                _ => None,
-            },
-            |field| match &field.attrs.default {
-                DefaultBehavior::Func(_) => None,
-                _ => Some(quote!(#FQDefault)),
-            },
-        ),
-    );
+    let where_from_reflect_clause =
+        WhereClauseOptions::new(reflect_enum.meta()).extend_where_clause(where_clause);
 
     quote! {
         impl #impl_generics #bevy_reflect_path::FromReflect for #enum_path #ty_generics #where_from_reflect_clause  {
@@ -144,28 +130,8 @@ fn impl_struct_internal(
         .split_for_impl();
 
     // Add FromReflect bound for each active field
-    let where_from_reflect_clause = extend_where_clause(
-        where_clause,
-        &WhereClauseOptions::new_with_bounds(
-            reflect_struct.meta(),
-            reflect_struct.active_fields(),
-            reflect_struct.ignored_fields(),
-            |field| match &field.attrs.default {
-                DefaultBehavior::Default => Some(quote!(#FQDefault)),
-                _ => None,
-            },
-            |field| {
-                if is_defaultable {
-                    None
-                } else {
-                    match &field.attrs.default {
-                        DefaultBehavior::Func(_) => None,
-                        _ => Some(quote!(#FQDefault)),
-                    }
-                }
-            },
-        ),
-    );
+    let where_from_reflect_clause =
+        WhereClauseOptions::new(reflect_struct.meta()).extend_where_clause(where_clause);
 
     quote! {
         impl #impl_generics #bevy_reflect_path::FromReflect for #struct_path #ty_generics #where_from_reflect_clause {

crates/bevy_reflect/bevy_reflect_derive/src/impls/enums.rs

@@ -1,7 +1,6 @@
 use crate::derive_data::{EnumVariant, EnumVariantFields, ReflectEnum, StructField};
 use crate::enum_utility::{get_variant_constructors, EnumVariantConstructors};
 use crate::impls::{impl_type_path, impl_typed};
-use crate::utility::extend_where_clause;
 use bevy_macro_utils::fq_std::{FQAny, FQBox, FQOption, FQResult};
 use proc_macro2::{Ident, Span};
 use quote::quote;
@@ -92,7 +91,7 @@ pub(crate) fn impl_enum(reflect_enum: &ReflectEnum) -> proc_macro2::TokenStream
     let (impl_generics, ty_generics, where_clause) =
         reflect_enum.meta().type_path().generics().split_for_impl();
 
-    let where_reflect_clause = extend_where_clause(where_clause, &where_clause_options);
+    let where_reflect_clause = where_clause_options.extend_where_clause(where_clause);
 
     quote! {
         #get_type_registration_impl

crates/bevy_reflect/bevy_reflect_derive/src/impls/structs.rs

@@ -1,5 +1,5 @@
 use crate::impls::{impl_type_path, impl_typed};
-use crate::utility::{extend_where_clause, ident_or_index};
+use crate::utility::ident_or_index;
 use crate::ReflectStruct;
 use bevy_macro_utils::fq_std::{FQAny, FQBox, FQDefault, FQOption, FQResult};
 use quote::{quote, ToTokens};
@@ -99,7 +99,7 @@ pub(crate) fn impl_struct(reflect_struct: &ReflectStruct) -> proc_macro2::TokenS
         .generics()
         .split_for_impl();
 
-    let where_reflect_clause = extend_where_clause(where_clause, &where_clause_options);
+    let where_reflect_clause = where_clause_options.extend_where_clause(where_clause);
 
     quote! {
         #get_type_registration_impl

crates/bevy_reflect/bevy_reflect_derive/src/impls/tuple_structs.rs

@@ -1,5 +1,4 @@
 use crate::impls::{impl_type_path, impl_typed};
-use crate::utility::extend_where_clause;
 use crate::ReflectStruct;
 use bevy_macro_utils::fq_std::{FQAny, FQBox, FQDefault, FQOption, FQResult};
 use quote::{quote, ToTokens};
@@ -90,7 +89,7 @@ pub(crate) fn impl_tuple_struct(reflect_struct: &ReflectStruct) -> proc_macro2::
         .generics()
         .split_for_impl();
 
-    let where_reflect_clause = extend_where_clause(where_clause, &where_clause_options);
+    let where_reflect_clause = where_clause_options.extend_where_clause(where_clause);
 
     quote! {
         #get_type_registration_impl