-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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_derive 0.10.0 > 0.10.1 has a breaking change for recursion #8965
Comments
Note: While the above example does seem to break between 0.10.0 and 0.10.1, this example is broken on both: #[derive(Reflect, FromReflect)]
struct Foo {
foo: Vec<Foo>
} This indicates that the issue has possibly existed for longer. The fix would be to remove the generated self-referential where clause. However, this comes with difficulties due to generics and will require a thoughtful solution in order to capture all edge cases. |
I may have an idea to fix this. I'll see if I can get it working! Edit: Seems to be working. I'll run a few more tests then post the PR. |
# 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]>
# 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.
This should be fixed now with #9046 and #11597 merged. For recursive types, you need to opt-out of field bounds using #[derive(Reflect)]
#[reflect(no_field_bounds)]
struct Foo {
foo: Vec<Foo>,
} For generic recursive types, you may also need to use #[derive(Reflect)]
#[reflect(no_field_bounds)]
#[reflect(where T: FromReflect)]
struct Foo<T> {
foo: Vec<Foo<T>>,
t: T,
} |
# Objective Fixes bevyengine#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](bevyengine#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](bevyengine@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 bevyengine#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]>
# 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.
Bevy version
0.10.1
If you cannot get Bevy to build or run on your machine, please include:
What you did
In 0.10.0 version struct and enum that could be recursive with
#[cfg_attr(debug_assertions, derive(FromReflect, Reflect))]
working,but with 0.10.1 bevy_reflect_derive it does not even compile with a overflow evaluating the requirement
RecurEnum: FromReflect
What went wrong
I expected the automatic update from 0.10.0 to 0.10.1 to not have breaking change
Wonder if the issue is that 'Reflect' was never intended to be used this way to begin with
Additional information
Other information that can be used to further reproduce or isolate the problem.
Here is the simplified code that used to compile in 0.10.0 but doesn't in 0.10.1
also old lock that allows this code to compile
Cargo.lock.txt
P.S. adding cargo tolm that makes the example work by forcing bevy_reflect_derive to be 0.10.0
Cargo.toml.txt
The text was updated successfully, but these errors were encountered: