reflect: improve container attributes#7317
Conversation
alice-i-cecile
added
C-Examples
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Information about the special trait-like attributes needs to be prominent and clear in the docs for the derive macro, or the syntax needs to be more distinct.
I agree with the change, but the UX is quite confusing with some traits being capitalized and others seemingly not.
|
Still need to give a proper review, but I'm wondering if— while we're changing this up anyways— we could separate out the type data registrations from everything else. As we start to add more container attributes (e.g. It might be nice to break it up like:
Thoughts? |
|
Hmm this might make sense from an implementation perspective, but from a user perspective I would find this pretty confusing. We train people to think "when I want reflected TraitX behaviors, write What is the specific real world confusion we're solving here? Ex: is it that people are reaching for |
Hm, true. I think the difficulty comes when we want to provide customization for these attributes. All of these allow users to specify custom implementations like We could optimize for users not needing to specify custom trait functions for these types and keep things as is.
Additionally, these aren't strictly "internal implementations" in the sense that the user doesn't ever need to be fully aware of them. This is required knowledge if one wishes to use the affected At this point in time, I'm not too sure which way is the best to move forward. As much as I'd like to change these attributes for consistency with the field attributes, the user-facing argument is a good one. I'd be curious to hear thoughts from others on the matter, especially those unfamiliar with reflection. Is it unintuitive to have a few traits that control |
Not yet. But on that note, maybe we should build a mechanism for passing in parameters to constructors of "true" type data? Then this could still "feel" unified.
Agreed, but I would argue these are still "reflect configuration/behaviors associated with a specific trait (Hash, PartialEq, etc)".
I feel pretty strongly that we should use "reflect trait syntax" for these cases (while trying to unify behaviors, syntax, and code paths to the maximum extent that makes sense). My gut reaction to seeing the new syntax is "this is weird and confusing" (both as a user of Bevy and as an informed developer of it). I think ideologically the current trait syntax makes sense. We are still reflecting a specific trait/behavior, it just manifests slightly differently. In the context of trait "type data" (which adds additional behaviors and data on top of exposing the trait itself), I think something like a "Hash behavior override" fits in perfectly. Changing the syntax has multiple negative impacts:
|
BenjaminBrienen
added
S-Waiting-on-Author
# Objective Using `Reflect::clone_value` can be somewhat confusing to those unfamiliar with how Bevy's reflection crate works. For example take the following code: ```rust let value: usize = 123; let clone: Box<dyn Reflect> = value.clone_value(); ``` What can we expect to be the underlying type of `clone`? If you guessed `usize`, then you're correct! Let's try another: ```rust #[derive(Reflect, Clone)] struct Foo(usize); let value: Foo = Foo(123); let clone: Box<dyn Reflect> = value.clone_value(); ``` What about this code? What is the underlying type of `clone`? If you guessed `Foo`, unfortunately you'd be wrong. It's actually `DynamicStruct`. It's not obvious that the generated `Reflect` impl actually calls `Struct::clone_dynamic` under the hood, which always returns `DynamicStruct`. There are already some efforts to make this a bit more apparent to the end-user: #7207 changes the signature of `Reflect::clone_value` to instead return `Box<dyn PartialReflect>`, signaling that we're potentially returning a dynamic type. But why _can't_ we return `Foo`? `Foo` can obviously be cloned— in fact, we already derived `Clone` on it. But even without the derive, this seems like something `Reflect` should be able to handle. Almost all types that implement `Reflect` either contain no data (trivially clonable), they contain a `#[reflect_value]` type (which, by definition, must implement `Clone`), or they contain another `Reflect` type (which recursively fall into one of these three categories). This PR aims to enable true reflection-based cloning where you get back exactly the type that you think you do. ## Solution Add a `Reflect::reflect_clone` method which returns `Result<Box<dyn Reflect>, ReflectCloneError>`, where the `Box<dyn Reflect>` is guaranteed to be the same type as `Self`. ```rust #[derive(Reflect)] struct Foo(usize); let value: Foo = Foo(123); let clone: Box<dyn Reflect> = value.reflect_clone().unwrap(); assert!(clone.is::<Foo>()); ``` Notice that we didn't even need to derive `Clone` for this to work: it's entirely powered via reflection! Under the hood, the macro generates something like this: ```rust fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> { Ok(Box::new(Self { // The `reflect_clone` impl for `usize` just makes use of its `Clone` impl 0: Reflect::reflect_clone(&self.0)?.take().map_err(/* ... */)?, })) } ``` If we did derive `Clone`, we can tell `Reflect` to rely on that instead: ```rust #[derive(Reflect, Clone)] #[reflect(Clone)] struct Foo(usize); ``` <details> <summary>Generated Code</summary> ```rust fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> { Ok(Box::new(Clone::clone(self))) } ``` </details> Or, we can specify our own cloning function: ```rust #[derive(Reflect)] #[reflect(Clone(incremental_clone))] struct Foo(usize); fn incremental_clone(value: &usize) -> usize { *value + 1 } ``` <details> <summary>Generated Code</summary> ```rust fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> { Ok(Box::new(incremental_clone(self))) } ``` </details> Similarly, we can specify how fields should be cloned. This is important for fields that are `#[reflect(ignore)]`'d as we otherwise have no way to know how they should be cloned. ```rust #[derive(Reflect)] struct Foo { #[reflect(ignore, clone)] bar: usize, #[reflect(ignore, clone = "incremental_clone")] baz: usize, } fn incremental_clone(value: &usize) -> usize { *value + 1 } ``` <details> <summary>Generated Code</summary> ```rust fn reflect_clone(&self) -> Result<Box<dyn Reflect>, ReflectCloneError> { Ok(Box::new(Self { bar: Clone::clone(&self.bar), baz: incremental_clone(&self.baz), })) } ``` </details> If we don't supply a `clone` attribute for an ignored field, then the method will automatically return `Err(ReflectCloneError::FieldNotClonable {/* ... */})`. `Err` values "bubble up" to the caller. So if `Foo` contains `Bar` and the `reflect_clone` method for `Bar` returns `Err`, then the `reflect_clone` method for `Foo` also returns `Err`. ### Attribute Syntax You might have noticed the differing syntax between the container attribute and the field attribute. This was purely done for consistency with the current attributes. There are PRs aimed at improving this. #7317 aims at making the "special-cased" attributes more in line with the field attributes syntactically. And #9323 aims at moving away from the stringified paths in favor of just raw function paths. ### Compatibility with Unique Reflect This PR was designed with Unique Reflect (#7207) in mind. This method actually wouldn't change that much (if at all) under Unique Reflect. It would still exist on `Reflect` and it would still `Option<Box<dyn Reflect>>`. In fact, Unique Reflect would only _improve_ the user's understanding of what this method returns. We may consider moving what's currently `Reflect::clone_value` to `PartialReflect` and possibly renaming it to `partial_reflect_clone` or `clone_dynamic` to better indicate how it differs from `reflect_clone`. ## Testing You can test locally by running the following command: ``` cargo test --package bevy_reflect ``` --- ## Changelog - Added `Reflect::reflect_clone` method - Added `ReflectCloneError` error enum - Added `#[reflect(Clone)]` container attribute - Added `#[reflect(clone)]` field attribute
|
Has conflicts for a few months now. Let us know if you would like to put the PR up for adoption or if you intend to solve the merge conflicts. |
|
This PR appears to have gone stale. Nominating to close. |
|
Yeah I'll close this. The proposed syntax changes are deal breakers. It is worth exploring ways to improve the flexibility of the system and remove special casing, provided we aren't making the user-facing experience weirder. |
…13723) # Objective Addresses [comments](bevyengine#7317 (comment)) regarding bevyengine#7317 (note that this doesn't replace bevyengine#7317, there are still some great improvements there besides this syntactical problem). There currently exist some "special" type data registrations that can be registered like other type data (e.g. `#[reflect(Hash)]`) or can use a "special" syntax to allow specifying custom implementations (e.g. `#[reflect(Hash(custom_hash_fn))]`). And there may be more to follow (bevyengine#13432). What's interesting is that most of these special cased registrations don't actually come with any type data type. Instead, they simply modify methods on `Reflect` (e.g. `Reflect::reflect_hash`). bevyengine#7317 sought to distinguish between these "special" registrations by making them lowercase and use a more conventional attribute style: `#[reflect(hash = "custom_hash_fn")]`. However, while this did help distinguish these registrations and make them a bit prettier, they now require the user to actually know which traits are "special" and which are not (as pointed out [here](bevyengine#7317 (comment))). Ideally, users shouldn't have to know which traits are "special" until they need to. For most users, they should just know that they need to register their trait in order for certain things to work. And the special-casing may be easier to follow if we open up the configuration abilities to _all_ type data. ## Solution This PR introduces `CreateTypeData` which replaces `FromType`. This was done for two reasons. Firstly, `FromType` isn't very descriptive as to what it should be used for. We are creating type data from a type, but it's not immediately clear this is even for type data. Renaming to `CreateTypeData` should hopefully make this much clearer. Secondly, in order to support type data with parameters like the `custom_hash_fn` in `reflect(Hash(custom_hash_fn))`, an additional `Input` type parameter had to be added. This makes the new signature `CreateTypeData<T, Input = ()>`. We can now create type data that accepts input! ```rust trait Combine { fn combine(a: f32, b: f32) -> f32; } #[derive(Clone)] struct ReflectCombine { multiplier: f32, additional: f32, combine: fn(f32, f32) -> f32, } impl ReflectCombine { pub fn combine(&self, a: f32, b: f32) -> f32 { let combined = (self.combine)(a, b); let multiplied = self.multiplier * combined; multiplied + self.additional } } impl<T: Combine + Reflect> CreateTypeData<T, (f32, f32)> for ReflectCombine { fn create_type_data(input: (f32, f32)) -> Self { Self { multiplier: input.0, additional: input.1, combine: T::combine, } } } ``` And then register them with the special function-like syntax: ```rust #[derive(Reflect)] #[reflect(Combine(2.0, 4.0))] struct Foo; ``` The above code will compile into the following registration: ```rust registration.insert(<ReflectCombine as CreateTypeData<Self, _>>::create_type_data((2.0, 4.0))) ``` Notice how the macro automatically generates the tuple for us, so we don't have to add an additional layer of parentheses. ### Multiple Input Types You might be wondering why we're using a type parameter instead of an associated type to specify the input type. An associated type would limit us to a single implementation. This means that if we want to support the type data with optional parameters (e.g. support both `Hash` and `Hash(custom_hash_fn)`), then all type data must take in `Option<Self::Input>`, regardless of whether or not a `None` case is supported. This is important because the macro has to be pass in _something_, whether that be `()` or `None`. By using a type parameter we open the door to type data with required input: ```rust // `ReflectMyTrait` must be registered with input impl<T> CreateTypeData<T, u32> for ReflectMyTrait { fn create_type_data(input: u32) -> Self { Self { value: input, } } } // And we can support all different input types impl<T> CreateTypeData<T, i32> for ReflectMyTrait { fn create_type_data(input: i32) -> Self { Self { value: input.abs() as u32, } } } ``` However, this may be something we don't necessarily care about since users could also get away with this using custom input enums. And the required-input case could be deferred until runtime (i.e. maybe a panic in the `None` case). ### Adding `ReflectPartialEq` and `ReflectHash` I had originally considered adding `ReflectPartialEq` and `ReflectHash` type data to further decrease the differences between the "special" registrations and the regular ones. However, I chose not to do that to (1) reduce the complexity of this PR and (2) we may end up removing these entirely due to bevyengine#8695. ### What else is this good for? Another question you might have is what else this is good for beyond just making things a bit more consistent. I'm not sure exactly how the community will use it, but I can see it being used for things like feature gating certain functionality: ```rust #[derive(Reflect)] #[cfg_attr(feature = "debug", reflect(MyTrait(true)))] #[cfg_attr(not(feature = "debug"), reflect(MyTrait(false)))] struct Foo; ``` Or to emulate specialization via reflection: ```rust impl<T> DoSomething for T { fn do_something(&self) { println!("Doing the same old stuff."); } } #[derive(Reflect)] #[reflect(ReflectDoSomething(|_| { println!("Doing something special!"); }))] struct Foo; ``` Note that all of the above could always be done with manual registration. However, due to them requiring input, some cases could _only_ be done with manual registration. This PR mainly opens the door to doing more of this interesting stuff with type data via the macro registration. It not only unifies "special" and regular registrations, but also manual and automatic registrations. ## Testing The tests for this feature are split into doctests (for the docs on `CreateTypeData`) and in the compile-fail tests. These will both be verified automatically by CI. --- ## Changelog - Replaced `FromType<T>` with `CreateTypeData<T, Input = ()>` - Type data may now opt-in to accepting input during creation using the `#[reflect(MyTrait(...))]` syntax - Added `TypeRegistry::register_type_data_with` method ## Migration Guide `FromType<T>` has been replaced by `CreateTypeData<T, Input = ()>`. Implementors of `FromType<T>` will need to update their implementation: ```rust // BEFORE impl<T> FromType<T> for ReflectMyTrait { fn from_type() -> Self { // ... } } // AFTER impl<T> CreateTypeData<T> for ReflectMyTrait { fn create_type_data(input: ()) -> Self { // ... } } ``` Additionally, any calls made to `FromType::from_type` will need to be updated as well: ```rust // BEFORE <ReflectMyTrait as FromType<Foo>>::from_type() // AFTER <ReflectMyTrait as CreateTypeData<Foo>>::create_type_data(()) ```
6 participants
Objective
#[reflect(Debug, PartialEq, Hash, Default)].#[reflect(my_crate::foo::MyTrait)].Solution
#[reflect(Default)].#[reflect(debug, partial_eq, hash)]to make obvious the distinction between normal and "special" idents.#[reflect(Hash(my_hash_fn))]is now#[reflect(hash = "my_hash_fn")].#[reflect]attributes.Changelog
#[reflect(Debug, PartialEq, Hash, Default)]syntax with#[reflect(debug, partial_eq, hash)]syntax.Migration Guide
#[reflect(Debug, PartialEq, Hash, Default)]syntax with#[reflect(debug, partial_eq, hash)]syntax.FromReflect::from_reflectimplementations no longer can create a value from a partial set of fields if it implements default. All fields are required.