mirror of https://github.com/bevyengine/bevy synced 2024-09-20 14:32:06 +00:00

History

Gino Valente 6e959db134 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](https://github.com/dtolnay/syn/issues/422#issuecomment-406882925) comment and @nicopap's [comment](https://github.com/bevyengine/bevy/pull/9046#issuecomment-1623593780)) 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](`dc139ea34c`) 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 <nicopap@users.noreply.github.com>		2024-01-28 16:24:03 +00:00
..
src	bevy_reflect: Add compile fail tests for bevy_reflect (#7041 )	2023-01-02 21:07:33 +00:00
tests	bevy_reflect: Type parameter bounds (#9046 )	2024-01-28 16:24:03 +00:00
Cargo.toml	bevy_reflect: Add compile fail tests for bevy_reflect (#7041 )	2023-01-02 21:07:33 +00:00
README.md	bevy_reflect: Add compile fail tests for bevy_reflect (#7041 )	2023-01-02 21:07:33 +00:00

README.md

Compile fail tests for bevy_reflect

This crate is separate from bevy_reflect and not part of the Bevy workspace in order to not fail crater tests for Bevy. The tests assert on the exact compiler errors and can easily fail for new Rust versions due to updated compiler errors (e.g. changes in spans).

The CI workflow executes these tests on the stable rust toolchain (see tools/ci).