Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bevy_reflect: Split #[reflect(where)] #11597

Merged

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Jan 29, 2024

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.

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 bevy_reflect: Type parameter bounds #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:

// 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:

#[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 bevy_reflect: Type parameter bounds #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.

@MrGVSV MrGVSV requested a review from soqb January 29, 2024 06:12
@MrGVSV MrGVSV added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Jan 29, 2024
Copy link
Contributor

@soqb soqb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow! that was quick. i'm really glad with how the new WhereClauseOptions looks.

@MrGVSV MrGVSV added this to the 0.13 milestone Jan 29, 2024
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 29, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 29, 2024
Merged via the queue into bevyengine:main with commit 379b9e5 Jan 29, 2024
26 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

Revert the changes to type parameter bounds introduced in bevyengine#9046,
improves the `#[reflect(where)]` attribute (also from bevyengine#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 bevyengine#9046. This was originally done as a means of fixing a
recursion issue (bevyengine#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 bevyengine#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 bevyengine#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 bevyengine#9046, that
PR also won't need a migration guide.
TrialDragon added a commit to TrialDragon/bevy-website that referenced this pull request Feb 18, 2024
The breaking changes were removed in a follow-up PR: bevyengine/bevy#11597 . As such these changes, and their migration guide, were old and irrelevant to the released version of 0.13.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants