Skip to content

Commit

Permalink
bevy_reflect: Split #[reflect(where)] (#11597)
Browse files Browse the repository at this point in the history
# Objective

Revert the changes to type parameter bounds introduced in #9046,
improves the `#[reflect(where)]` attribute (also from #9046), and adds
the ability to opt out of field bounds.

This is based on suggestions by @soqb and discussion on
[Discord](https://discord.com/channels/691052431525675048/1002362493634629796/1201227833826103427).

## Solution

Reverts the changes to type parameter bounds when deriving `Reflect`,
introduced in #9046. This was originally done as a means of fixing a
recursion issue (#8965). However, as @soqb pointed out, we could achieve
the same result by simply making an opt-out attribute instead of messing
with the type parameter bounds.

This PR has four main changes:
1. Reverts the type parameter bounds from #9046
2. Includes `TypePath` as a default bound for active fields
3. Changes `#reflect(where)]` to be strictly additive
4. Adds `#reflect(no_field_bounds)]` to opt out of field bounds

Change 1 means that, like before, type parameters only receive at most
the `TypePath` bound (if `#[reflect(type_path = false)]` is not present)
and active fields receive the `Reflect` or `FromReflect` bound. And with
Change 2, they will also receive `TypePath` (since it's indirectly
required by `Typed` to construct `NamedField` and `UnnamedField`
instances).

Change 3 was made to make room for Change 4. By splitting out the
responsibility of `#reflect(where)]`, we can use it with or without
`#reflect(no_field_bounds)]` for various use cases.

For example, if we hadn't done this, the following would have failed:

```rust
// Since we're not using `#reflect(no_field_bounds)]`, 
// `T::Assoc` is automatically given the required bounds
// of `FromReflect + TypePath`
#[derive(Reflect)]
#[reflect(where T::Assoc: OtherTrait)]
struct Foo<T: MyTrait> {
  value: T::Assoc,
}
```

This provides more flexibility to the user while still letting them add
or remove most trait bounds.

And to solve the original recursion issue, we can do:

```rust
#[derive(Reflect)]
#[reflect(no_field_bounds)] // <-- Added
struct Foo {
  foo: Vec<Foo>
}
```

#### Bounds

All in all, we now have four sets of trait bounds:
- `Self` gets the bounds `Any + Send + Sync`
- Type parameters get the bound `TypePath`. This can be opted out of
with `#[reflect(type_path = false)]`
- Active fields get the bounds `TypePath` and `FromReflect`/`Reflect`
bounds. This can be opted out of with `#reflect(no_field_bounds)]`
- Custom bounds can be added with `#[reflect(where)]`

---

## Changelog

- Revert some changes #9046
- `#reflect(where)]` is now strictly additive
- Added `#reflect(no_field_bounds)]` attribute to opt out of automatic
field trait bounds when deriving `Reflect`
- Made the `TypePath` requirement on fields when deriving `Reflect` more
explicit

## Migration Guide

> [!important]
> This PR shouldn't be a breaking change relative to the current version
of Bevy (v0.12). And since it removes the breaking parts of #9046, that
PR also won't need a migration guide.
  • Loading branch information
MrGVSV committed Jan 29, 2024
1 parent ba2fffe commit 379b9e5
Show file tree
Hide file tree
Showing 10 changed files with 238 additions and 139 deletions.
1 change: 0 additions & 1 deletion crates/bevy_asset/src/handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ 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.
Expand Down
1 change: 0 additions & 1 deletion crates/bevy_asset/src/id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ const HASH_ATTR: &str = "Hash";
// but useful to know exist nonetheless
pub(crate) const REFLECT_DEFAULT: &str = "ReflectDefault";

// Attributes for `Reflect` implementation
const NO_FIELD_BOUNDS_ATTR: &str = "no_field_bounds";

// Attributes for `FromReflect` implementation
const FROM_REFLECT_ATTR: &str = "from_reflect";

Expand Down Expand Up @@ -212,6 +215,7 @@ pub(crate) struct ReflectTraits {
from_reflect_attrs: FromReflectAttrs,
type_path_attrs: TypePathAttrs,
custom_where: Option<WhereClause>,
no_field_bounds: bool,
idents: Vec<Ident>,
}

Expand Down Expand Up @@ -260,6 +264,9 @@ impl ReflectTraits {
HASH_ATTR => {
traits.hash.merge(TraitImpl::Implemented(span))?;
}
NO_FIELD_BOUNDS_ATTR => {
traits.no_field_bounds = true;
}
// We only track reflected idents for traits not considered special
_ => {
// Create the reflect ident
Expand Down Expand Up @@ -422,6 +429,10 @@ impl ReflectTraits {
self.custom_where.as_ref()
}

pub fn no_field_bounds(&self) -> bool {
self.no_field_bounds
}

/// Merges the trait implementations of this [`ReflectTraits`] with another one.
///
/// An error is returned if the two [`ReflectTraits`] have conflicting implementations.
Expand All @@ -434,6 +445,8 @@ impl ReflectTraits {

self.merge_custom_where(other.custom_where);

self.no_field_bounds |= other.no_field_bounds;

for ident in other.idents {
add_unique_ident(&mut self.idents, ident)?;
}
Expand Down
34 changes: 22 additions & 12 deletions crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::container_attributes::{FromReflectAttrs, ReflectTraits};
use crate::container_attributes::{FromReflectAttrs, ReflectTraits, TypePathAttrs};
use crate::field_attributes::{parse_field_attrs, ReflectFieldAttr};
use crate::type_path::parse_path_no_leading_colon;
use crate::utility::{StringExpr, WhereClauseOptions};
Expand Down Expand Up @@ -402,6 +402,11 @@ impl<'a> ReflectMeta<'a> {
self.traits.from_reflect_attrs()
}

/// The `TypePath` attributes on this type.
pub fn type_path_attrs(&self) -> &TypePathAttrs {
self.traits.type_path_attrs()
}

/// The path to this type.
pub fn type_path(&self) -> &ReflectTypePath<'a> {
&self.type_path
Expand Down Expand Up @@ -480,7 +485,7 @@ impl<'a> ReflectStruct<'a> {
}

pub fn where_clause_options(&self) -> WhereClauseOptions {
WhereClauseOptions::new(self.meta())
WhereClauseOptions::new_with_fields(self.meta(), self.active_types().into_boxed_slice())
}
}

Expand All @@ -503,28 +508,33 @@ impl<'a> ReflectEnum<'a> {
&self.variants
}

/// Get a collection of types which are exposed to the reflection API
pub fn active_types(&self) -> Vec<Type> {
self.active_fields()
.map(|field| field.data.ty.clone())
.collect()
}

/// 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())
}

pub fn where_clause_options(&self) -> WhereClauseOptions {
WhereClauseOptions::new(self.meta())
WhereClauseOptions::new_with_fields(self.meta(), self.active_types().into_boxed_slice())
}
}

impl<'a> EnumVariant<'a> {
/// Get an iterator of fields which are exposed to the reflection API
#[allow(dead_code)]
pub fn active_fields(&self) -> impl Iterator<Item = &StructField<'a>> {
self.fields()
.iter()
.filter(|field| field.attrs.ignore.is_active())
}

/// Get an iterator of fields which are ignored by the reflection API
#[allow(dead_code)]
pub fn ignored_fields(&self) -> impl Iterator<Item = &StructField<'a>> {
self.fields()
.iter()
.filter(|field| field.attrs.ignore.is_ignored())
}

/// The complete set of fields in this variant.
#[allow(dead_code)]
pub fn fields(&self) -> &[StructField<'a>] {
Expand Down
13 changes: 7 additions & 6 deletions crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ pub(crate) fn impl_value(meta: &ReflectMeta) -> proc_macro2::TokenStream {
let type_path = meta.type_path();
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 =
WhereClauseOptions::new_type_path(meta).extend_where_clause(where_clause);
let where_from_reflect_clause = WhereClauseOptions::new(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> {
Expand All @@ -50,8 +49,9 @@ 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 =
WhereClauseOptions::new(reflect_enum.meta()).extend_where_clause(where_clause);
let where_from_reflect_clause = reflect_enum
.where_clause_options()
.extend_where_clause(where_clause);

quote! {
impl #impl_generics #bevy_reflect_path::FromReflect for #enum_path #ty_generics #where_from_reflect_clause {
Expand Down Expand Up @@ -130,8 +130,9 @@ fn impl_struct_internal(
.split_for_impl();

// Add FromReflect bound for each active field
let where_from_reflect_clause =
WhereClauseOptions::new(reflect_struct.meta()).extend_where_clause(where_clause);
let where_from_reflect_clause = reflect_struct
.where_clause_options()
.extend_where_clause(where_clause);

quote! {
impl #impl_generics #bevy_reflect_path::FromReflect for #struct_path #ty_generics #where_from_reflect_clause {
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_reflect/bevy_reflect_derive/src/impls/typed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ pub(crate) enum TypedProperty {
}

pub(crate) fn impl_type_path(meta: &ReflectMeta) -> proc_macro2::TokenStream {
let where_clause_options = WhereClauseOptions::new_type_path(meta);
let where_clause_options = WhereClauseOptions::new(meta);

if !meta.traits().type_path_attrs().should_auto_derive() {
return proc_macro2::TokenStream::new();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ pub(crate) fn impl_value(meta: &ReflectMeta) -> proc_macro2::TokenStream {
#[cfg(not(feature = "documentation"))]
let with_docs: Option<proc_macro2::TokenStream> = None;

let where_clause_options = WhereClauseOptions::new_type_path(meta);
let where_clause_options = WhereClauseOptions::new(meta);
let typed_impl = impl_typed(
meta,
&where_clause_options,
Expand Down
87 changes: 59 additions & 28 deletions crates/bevy_reflect/bevy_reflect_derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,50 +131,85 @@ pub(crate) static TYPE_NAME_ATTRIBUTE_NAME: &str = "type_name";
/// This is useful for when a type can't or shouldn't implement `TypePath`,
/// or if a manual implementation is desired.
///
/// ## `#[reflect(where T: Trait, U::Assoc: Trait, ...)]`
/// ## `#[reflect(no_field_bounds)]`
///
/// This attribute will opt-out of the default trait bounds added to all field types
/// for the generated reflection trait impls.
///
/// Normally, all fields will have the bounds `TypePath`, and either `FromReflect` or `Reflect`
/// depending on if `#[reflect(from_reflect = false)]` is used.
/// However, this might not always be desirable, and so this attribute may be used to remove those bounds.
///
/// ### Example
///
/// If a type is recursive the default bounds will cause an overflow error when building:
///
/// ```ignore (bevy_reflect is not accessible from this crate)
/// #[derive(Reflect)] // ERROR: overflow evaluating the requirement `Foo: FromReflect`
/// struct Foo {
/// foo: Vec<Foo>,
/// }
///
/// // Generates a where clause like:
/// // impl bevy_reflect::Reflect for Foo
/// // where
/// // Self: Any + Send + Sync,
/// // Vec<Foo>: FromReflect + TypePath,
/// ```
///
/// By default, the derive macro will automatically add certain trait bounds to all generic type parameters
/// in order to make them compatible with reflection without the user needing to add them manually.
/// This includes traits like `Reflect` and `FromReflect`.
/// However, this may not always be desired, and some type paramaters can't or shouldn't require those bounds
/// (i.e. their usages in fields are ignored or they're only used for their associated types).
/// In this case, `Foo` is given the bounds `Vec<Foo>: FromReflect + TypePath`,
/// which requires that `Foo` implements `FromReflect`,
/// which requires that `Vec<Foo>` implements `FromReflect`,
/// and so on, resulting in the error.
///
/// With this attribute, you can specify a custom `where` clause to be used instead of the default.
/// If this attribute is present, none of the type parameters will receive the default bounds.
/// Only the bounds specified by the type itself and by this attribute will be used.
/// The only exceptions to this are the `Any`, `Send`, `Sync`, and `TypePath` bounds,
/// which will always be added regardless of this attribute due to their necessity for reflection
/// in general.
/// To fix this, we can add `#[reflect(no_field_bounds)]` to `Foo` to remove the bounds on `Vec<Foo>`:
///
/// This means that if you want to opt-out of the default bounds for _all_ type parameters,
/// you can add `#[reflect(where)]` to the container item to indicate
/// that an empty `where` clause should be used.
/// ```ignore (bevy_reflect is not accessible from this crate)
/// #[derive(Reflect)]
/// #[reflect(no_field_bounds)]
/// struct Foo {
/// foo: Vec<Foo>,
/// }
///
/// // Generates a where clause like:
/// // impl bevy_reflect::Reflect for Foo
/// // where
/// // Self: Any + Send + Sync,
/// ```
///
/// ## `#[reflect(where T: Trait, U::Assoc: Trait, ...)]`
///
/// This attribute can be used to add additional bounds to the generated reflection trait impls.
///
/// This is useful for when a type needs certain bounds only applied to the reflection impls
/// that are not otherwise automatically added by the derive macro.
///
/// ### Example
///
/// In the example below, we want to enforce that `T::Assoc: List` is required in order for
/// `Foo<T>` to be reflectable, but we don't want it to prevent `Foo<T>` from being used
/// in places where `T::Assoc: List` is not required.
///
/// ```ignore
/// trait Trait {
/// type Assoc;
/// }
///
/// #[derive(Reflect)]
/// #[reflect(where T::Assoc: FromReflect)]
/// #[reflect(where T::Assoc: List)]
/// struct Foo<T: Trait> where T::Assoc: Default {
/// value: T::Assoc,
/// }
///
/// // Generates a where clause like the following
/// // (notice that `T` does not have any `Reflect` or `FromReflect` bounds):
/// // Generates a where clause like:
/// //
/// // impl<T: Trait> bevy_reflect::Reflect for Foo<T>
/// // where
/// // Self: 'static,
/// // Self: Any + Send + Sync,
/// // T::Assoc: Default,
/// // T: bevy_reflect::TypePath
/// // + ::core::any::Any
/// // + ::core::marker::Send
/// // + ::core::marker::Sync,
/// // T::Assoc: FromReflect,
/// // T: TypePath,
/// // T::Assoc: FromReflect + TypePath,
/// // T::Assoc: List,
/// // {/* ... */}
/// ```
///
Expand All @@ -191,10 +226,6 @@ pub(crate) static TYPE_NAME_ATTRIBUTE_NAME: &str = "type_name";
/// which may be useful for maintaining invariants, keeping certain data private,
/// or allowing the use of types that do not implement `Reflect` within the container.
///
/// If the field contains a generic type parameter, you will likely need to add a
/// [`#[reflect(where)]`](#reflectwheret-trait-uassoc-trait-)
/// attribute to the container in order to avoid the default bounds being applied to the type parameter.
///
/// ## `#[reflect(skip_serializing)]`
///
/// This works similar to `#[reflect(ignore)]`, but rather than opting out of _all_ of reflection,
Expand Down
Loading

0 comments on commit 379b9e5

Please sign in to comment.