From e3fa0d7dc41f726d92f66d54d963d844715445fb Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Sat, 2 Jul 2022 12:41:12 -0700 Subject: [PATCH 1/9] Add from_reflect_ergonomics RFC --- rfcs/from_reflect_ergonomics.md | 284 ++++++++++++++++++++++++++++++++ 1 file changed, 284 insertions(+) create mode 100644 rfcs/from_reflect_ergonomics.md diff --git a/rfcs/from_reflect_ergonomics.md b/rfcs/from_reflect_ergonomics.md new file mode 100644 index 00000000..0452f33b --- /dev/null +++ b/rfcs/from_reflect_ergonomics.md @@ -0,0 +1,284 @@ +# Feature Name: `from_reflect_ergonomics` + +## Summary + +Derive `FromReflect` by default when deriving `Reflect`, add `ReflectFromReflect` type data, and make `ReflectDeserializer` easier to use. + +## Terminology + +#### *Dynamic Types* + +To make this document easier to read, the term "**Dynamic**" (with this casing) will be used to refer to any of the `Dynamic***` structs. This includes `DynamicStruct`, `DynamicTupleStruct`, `DynamicList`, etc. + +Other forms might include "Dynamics," "Dynamic structs," or "Dynamic types" depending on context. + +In example code, variables containing these types will be denoted as `dyn_X`, where `X` corresponds to their data structure. For example, `dyn_tuple_struct` would represent a `DynamicTupleStruct`. + +#### *Real Types* + +The term "**real type**" will be used to distinguish Dynamics and other concrete types. We can define it as any concrete type known at compile time that is *not* one of the Dynamic structs. + +In example code, variables containing real types will be denoted as `real_Y`, where `Y` corresponds to their data structure. For example, `real_list` would represent a list-like type, such as `Vec`. + +## Background + +### What is `FromReflect`? + +For those unfamiliar, here's a quick refresher on `FromReflect`. + +The `FromReflect` trait is meant to allow types to be constructed dynamically (well, not completely, but we'll get into that later). Specifically, it enables a real type to be generated from a Dynamic one. As an example, we can convert a `DynamicTuple` to a real tuple like so: + +```rust +let mut dyn_tuple = DynamicTuple::default(); +dyn_tuple.insert(123usize); +dyn_tuple.insert(321usize); + +let real_tuple = <(usize, usize)>::from_reflect(&dyn_tuple).unwrap(); +``` + +A Dynamic value to a real one. Neat! + +### How does `FromReflect` work? + +Basically it's `FromReflect` all the way down. + +Given a struct like: + +```rust +struct Foo { + value: (f32, f32) +} +``` + +Calling `Foo::from_reflect` will kick off a chain of `FromReflect`: + +`Foo::from_reflect` calls `<(f32, f32)>::from_reflect` which itself calls `f32::from_reflect`. + +This is why, when deriving `FromReflect`, all fields *must* also implement `FromReflect`[^1]. Without this, the trait itself won't just "not work"— it won't even compile. + +### Why is `FromReflect` useful? + +So why does this matter? If we need the real type we can just avoid using Dynamics, right? + +Well for any users who have tried working with serialized reflection data, they've likely come across the biggest use case for `FromReflect`: deserialization. + +When reflected data is deserialized, all of it is placed in a Dynamic type[^2]. This means that, even if we know the real type ahead of time, we don't quite have it yet. This is where `FromReflect` shines, as it allows us to convert that Dynamic into our desired real type. + +## Motivation + +So where's the issue? + +The main problem is in the ergonomics of this whole system for the typical Bevy developer as well as the consistency of it across the Bevy ecosystem. To be specific, there are three areas that could be improved: + +1. **Derive Complexity** - A way to cut back on repetitive or redundant code. +2. **Dynamic Conversions** - A way to use `FromReflect` in a purely dynamic way, where no real types are known. +3. **Deserialization Clarity** - A way to make it clear to the user exactly what they are getting back from the deserializer. + +Please note that these are not listed in order of importance but, rather, order of understanding. + +## User-facing explanation + +### Derive Complexity + +> Removing redundancy and providing sane defaults + +Bevy's reflection model is powerful because it not only allows type metadata to be easily traversed, but it also allows for (de)serialization without the need for any `serde` derives. + +This is great because nearly every reflectable type likely wants to be readily serializable as well. This is so they can be supported by Bevy's scene format or a maybe custom config format. This can be used for sending serialized data over the network or to an editor. + +Chances are, if a type implements `Reflect`, it should also be implementing `FromReflect`. Because deserialization is so common in these situations, `FromReflect` is now automatically derived alongside `Reflect`. + +```rust +// Before: +#[derive(Reflect, FromReflect)] +struct Foo; + +// After: +#[derive(Reflect)] +struct Foo; +``` + +Not only does this cut back on the amount of code a user must write, but it helps establish some amount of consistency across the Bevy ecosystem. Many plugin authors, and even Bevy itself (here's one [example](https://github.com/bevyengine/bevy/blob/4c5e30a9f83b4d856e58d23a4e89d662abf476ad/crates/bevy_sprite/src/rect.rs#L7-L7)), can sometimes forget to consider users who might want to use their type with `FromReflect`. + +#### Opting Out + +Again, in most cases, a type that implements `Reflect` is probably fine implementing `FromReflect` as well— if not for themselves, then for another user/plugin/editor/etc. However, there are a few cases where this is not always true— often for runtime identifier types, or other types never meant to be serialized or manually constructed. + +Take Bevy's [`Entity`](https://github.com/bevyengine/bevy/blob/4c5e30a9f83b4d856e58d23a4e89d662abf476ad/crates/bevy_ecs/src/entity/mod.rs#L101-L101) type as an example: + +```rust +#[derive(Clone, Copy, Hash, Eq, Ord, PartialEq, PartialOrd)] +pub struct Entity { + pub(crate) generation: u32, + pub(crate) id: u32, +} +``` + +The `generation` and `id` of each entity is generated at runtime (not good for general-purpose serialization). Additionally, the fields are only `pub(crate)` because this struct should almost never be manually generated outside of the crate its defined (not good for `FromReflect`). + +But say we wanted to make it so that we could reflect an entity to get its current `id`. To do this without implementing `FromReflect` requires us to *opt-out* of the automatic derive: + +```rust +#[derive(Reflect)] +#[from_reflect(auto_derive = false)] +pub struct Entity { + pub(crate) generation: u32, + pub(crate) id: u32, +} +``` + +By marking the container with `#[from_reflect(auto_derive = false)]`, we signal to the derive macro that this type should not be constructible via `FromReflect` and that it should not be automatically derived. + +### Dynamic Conversions + +> Adding `ReflectFromReflect` type data + +`FromReflect` is great in that you can easily convert a Dynamic type to a real one. Oftentimes, however, you might find yourself in a situation where the real type is not known. You need to call `<_>::from_reflect`, but have no idea what should go in place of `<_>`. + +In these cases, the `ReflectFromReflect` type data may be used. This can be retrieved from the type registry using the `TypeID` or the type name of the real type. For example: + +```rust +let rfr = registry + .get_type_data::(type_id) + .unwrap(); + +let real_struct: Box = rfr.from_reflect(&dyn_struct).unwrap(); +``` + +Calling `rfr.from_reflect` will return an `Option>`, where the `Box` contains the real type registered with `type_id`. + +#### More Derive Complexity + +This feature also pairs nicely with the changes listed in **[Derive Complexity](#derive-complexity)**, since otherwise this would require the following macros at minimum: + +```rust +#[derive(Reflect, FromReflect)] +#[reflect(FromReflect)] +struct Foo; +``` + +Again, listing all of that out on almost every reflectable type just adds noise. + +### Deserialization Clarity + +> Making the output of deserialization less confusing + +This RFC also seeks to improve the understanding of `ReflectDeserializer`. For newer devs or those not familiar with Bevy's reflection have likely been confused why they can't do something like this: + +```rust +let some_data: Box = reflect_deserializer.deserialize(&mut deserializer).unwrap(); +let real_struct: Foo = some_data.take().unwrap(); // PANIC! +``` + +Again, this is because `ReflectDeserializer` always returns a Dynamic[^2]. A user must then use `FromReflect` themselves to get the real value. + +Instead, `ReflectDeserializer::deserialize` now performs the conversion automatically before returning. Not only does this cut back on code needed to be written by the user, but it also cuts back on the need for them to understand the complexity and internals of deserializing reflected data. This means that the above code should no longer panic (assuming the real type *actually is* `Foo`). + +If any type in the serialized data did not register `ReflectFromReflect`, this should result in an error being returned indicating that `ReflectFromReflect` was not registered for the given type. With the changes listed in **[Derive Complexity](#derive-complexity)**, this should be less likely to happen, since all types should register it by default. If it does happen, the user can choose to either register said type data, or forgo the automatic conversion with **dynamic deserialization**. + +#### Dynamic Deserialization + +There may be some cases where a user has data that doesn't conform to `FromReflect` but is still serializable. For example, we might not want to make `Entity` constructible, but we could still pass along data that *represents* an `Entity` so we it can be constructed in some Bevy-controlled way. + +These types require manual reconstruction of the real type using the deserialized data. This is what the previous behavior of `ReflectDeserializer::deserialize`. We can re-enable this behavior on the deserializer be disabling automatic conversion: + +```rust +// Disable automatic conversions +reflect_deserializer.set_auto_convert(false); + +// Use the deserializer as normal +let some_data: Box = reflect_deserializer.deserialize(&mut deserializer).unwrap(); +let dyn_struct: DynamicStruct = some_data.take().unwrap(); // OK! +``` + +## Implementation strategy + +### Implementing: Derive Complexity + +Implementation for **[Derive Complexity](#derive-complexity)** is rather straightforward: call the `FromReflect` derive function from the `Reflect` derive function. They both take `&ReflectDeriveData`, which should make doing this relatively painless. + +Note that we are not removing the `FromReflect` derive. There may be cases where a manual implementation of `Reflect` is needed, such as for other proxy types akin to the Dynamics. To make things easier, we can still expose the standard `FromReflect` derive on its own. + +The biggest change will be the inclusion of the opt-out strategy. This means we need to be able to parse `#[from_reflect(auto_derive = false)]` and use it to control whether we call the `FromReflect` derive function or not. This attribute is a `syn::MetaNameValue` where the path is `auto_derive` and the literal is a `Lit::Bool`. + +Essentially we can do something like: + +```rust +pub fn derive_reflect(input: TokenStream) -> TokenStream { + // ... + let from_reflect_impl = if is_auto_derive { + match derive_data.derive_type() { + DeriveType::Struct | DeriveType::UnitStruct => Some(from_reflect::impl_struct(&derive_data)), + // ... + } + } else { + None + }; + + return quote! { + #reflect_impl + + #from_reflect_impl + }; +} +``` + +> As an added benefit, we only need to parse the `ReflectDeriveData` once, as opposed to twice for both reflect derives. + +### Implementing: Dynamic Conversions + +Implementing **[Dynamic Conversions](#dynamic-conversions)** mainly involves the creation of a `ReflectFromReflect` type. This was previously explored in https://github.com/bevyengine/bevy/pull/4147, however, it was abandoned because of the issues this RFC aim to fix. + +This type data should automatically be registered if `FromReflect` is automatically derived. + +### Implementing: Deserialization Clarity + +**[Deserialization Clarity](#deserialization-clarity)** can readily be implemented by adding a private field to `ReflectDeserializer`: + +```rust +pub struct ReflectDeserializer<'a> { + registry: &'a TypeRegistry, + auto_convert: bool // <- New +} +``` + +This should default to `true` and be changeable using a new method: + +```rust +impl<'a> ReflectDeserializer<'a> { + pub fn set_auto_convert(&mut self, value: bool) { + self.auto_convert = value; + } +} +``` + +Once the Dynamic is deserialized by the visitor, it will check this value to see whether it needs to perform the conversion. This is done using the same method as shown in **[Dynamic Conversions](#dynamic-conversions)**. However, rather than unwrapping, we will return a `Result` that gives better error messages that point to `FromReflect` and/or `ReflectFromReflect`. + +## Drawbacks + +* Unidiomatic. One of the biggest drawbacks of this change is that it is not very standard to have opt-out mechanisms for derives. They mainly act in an additive fashion. Thus, it could be confusing. + +* Increased code generation. By automatically deriving `FromReflect` we increase the amount of code that needs to be generated per type— namely for reflectable types that *really* don't need `FromReflect` (such as for a single binary that doesn't need serialization and is not meant to be a library). + +
+ Rebuttal + + In most cases, however, this is code that *should* be generated as explained in **[Derive Complexity](#derive-complexity)**. Additionally, the code generation is rather small with each field in a struct only generating around 1–4 lines of code. + +
+ +## Rationale and alternatives + +The biggest alternative is doing nothing. Again, most of these are ergonomics and ecosystem wins. The engine will work pretty much the same with or without these changes. The hope is that we can make reflection less intimidating, confusing, and frustrating, while making it simpler, more intuitive, and more consistent across crates. + +One concern about integrating the `FromReflect` derive into the `Reflect` derive is that we might want to support something like a `FromWorldReflect`. However, using the opt-out method this should still be possible. + +## Unresolved questions + +- Should `ReflectFromReflect` be replaced by a method on `TypeRegistration`, such as `TypeRegistration::construct`? +- Any other pros/cons to these changes? + + + +[^1]: This only applies to *active* fields, or fields not marked as `#[reflect(ignore)]`. For *inactive* fields, it instead requires a `Default` impl (unless given a specialized default value using `#[reflect(default = "some_function")]`) +[^2]: Actually, not *all* data is deserialized into a Dynamic. Primitive types, like `u8`, are deserialized into their real type. This is a confusing disparity addressed by https://github.com/bevyengine/bevy/pull/4561. \ No newline at end of file From f3347b05b095233f37df8b9fe6575ae4d0bd9dbc Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Sat, 2 Jul 2022 12:46:08 -0700 Subject: [PATCH 2/9] Renamed file --- .../{from_reflect_ergonomics.md => 59-from_reflect_ergonomics.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename rfcs/{from_reflect_ergonomics.md => 59-from_reflect_ergonomics.md} (100%) diff --git a/rfcs/from_reflect_ergonomics.md b/rfcs/59-from_reflect_ergonomics.md similarity index 100% rename from rfcs/from_reflect_ergonomics.md rename to rfcs/59-from_reflect_ergonomics.md From a9d23382d8646cf4500157b8e57a6efcfca082d5 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Mon, 4 Jul 2022 12:33:48 -0700 Subject: [PATCH 3/9] Address PR comments 1 --- rfcs/59-from_reflect_ergonomics.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/59-from_reflect_ergonomics.md b/rfcs/59-from_reflect_ergonomics.md index 0452f33b..7ba590bc 100644 --- a/rfcs/59-from_reflect_ergonomics.md +++ b/rfcs/59-from_reflect_ergonomics.md @@ -180,7 +180,7 @@ If any type in the serialized data did not register `ReflectFromReflect`, this s There may be some cases where a user has data that doesn't conform to `FromReflect` but is still serializable. For example, we might not want to make `Entity` constructible, but we could still pass along data that *represents* an `Entity` so we it can be constructed in some Bevy-controlled way. -These types require manual reconstruction of the real type using the deserialized data. This is what the previous behavior of `ReflectDeserializer::deserialize`. We can re-enable this behavior on the deserializer be disabling automatic conversion: +These types require manual reconstruction of the real type using the deserialized data. As such, they should just deserialize to a Dynamic and leave the construction part up to the user. The `ReflectDeserializer::deserialize` method, before this RFC, would do just that and return a Dynamic for *all* types. We can re-enable this behavior on the deserializer be disabling automatic conversion: ```rust // Disable automatic conversions From fbc3926fca93767e006020447234d5d1035d2a7b Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Mon, 4 Jul 2022 13:00:55 -0700 Subject: [PATCH 4/9] Update terminology section --- rfcs/59-from_reflect_ergonomics.md | 39 ++++++++++++++---------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/rfcs/59-from_reflect_ergonomics.md b/rfcs/59-from_reflect_ergonomics.md index 7ba590bc..eae0a699 100644 --- a/rfcs/59-from_reflect_ergonomics.md +++ b/rfcs/59-from_reflect_ergonomics.md @@ -6,19 +6,16 @@ Derive `FromReflect` by default when deriving `Reflect`, add `ReflectFromReflect ## Terminology -#### *Dynamic Types* +### *Dynamic Types* vs *Real Types* -To make this document easier to read, the term "**Dynamic**" (with this casing) will be used to refer to any of the `Dynamic***` structs. This includes `DynamicStruct`, `DynamicTupleStruct`, `DynamicList`, etc. +A **Dynamic** type refers to any of the `Dynamic***` structs used by `bevy_reflect` to represent a basic Rust data structure. For example, a `DynamicTupleStruct` is a Dynamic that represents a tuple struct in Rust. -Other forms might include "Dynamics," "Dynamic structs," or "Dynamic types" depending on context. +A **Real** type, on the other hand, refers to any type that is *not* a Dynamic. For example, `i32` is a Real type, and so is `Vec`. -In example code, variables containing these types will be denoted as `dyn_X`, where `X` corresponds to their data structure. For example, `dyn_tuple_struct` would represent a `DynamicTupleStruct`. +Since both may be represented as a `dyn Reflect`, examples in this RFC will try to use the following conventions: -#### *Real Types* - -The term "**real type**" will be used to distinguish Dynamics and other concrete types. We can define it as any concrete type known at compile time that is *not* one of the Dynamic structs. - -In example code, variables containing real types will be denoted as `real_Y`, where `Y` corresponds to their data structure. For example, `real_list` would represent a list-like type, such as `Vec`. +* Variables named `dyn_***` contain a Dynamic type. The full name should match the name of the corresponding Dynamic type. For example, a `dyn_tuple` represents a `DynamicTuple`. +* Variables named `real_***` contain a Real type. The full name should match the corresponding data structure. For example, a `real_tuple` represents any Rust tuple, such as `(i32, f32, usize)`. ## Background @@ -26,7 +23,7 @@ In example code, variables containing real types will be denoted as `real_Y`, wh For those unfamiliar, here's a quick refresher on `FromReflect`. -The `FromReflect` trait is meant to allow types to be constructed dynamically (well, not completely, but we'll get into that later). Specifically, it enables a real type to be generated from a Dynamic one. As an example, we can convert a `DynamicTuple` to a real tuple like so: +The `FromReflect` trait is meant to allow types to be constructed dynamically (well, not completely, but we'll get into that later). Specifically, it enables a Real type to be generated from a Dynamic one. As an example, we can convert a `DynamicTuple` to a Real tuple like so: ```rust let mut dyn_tuple = DynamicTuple::default(); @@ -36,7 +33,7 @@ dyn_tuple.insert(321usize); let real_tuple = <(usize, usize)>::from_reflect(&dyn_tuple).unwrap(); ``` -A Dynamic value to a real one. Neat! +A Dynamic value to a Real one. Neat! ### How does `FromReflect` work? @@ -58,11 +55,11 @@ This is why, when deriving `FromReflect`, all fields *must* also implement `From ### Why is `FromReflect` useful? -So why does this matter? If we need the real type we can just avoid using Dynamics, right? +So why does this matter? If we need the Real type we can just avoid using Dynamics, right? Well for any users who have tried working with serialized reflection data, they've likely come across the biggest use case for `FromReflect`: deserialization. -When reflected data is deserialized, all of it is placed in a Dynamic type[^2]. This means that, even if we know the real type ahead of time, we don't quite have it yet. This is where `FromReflect` shines, as it allows us to convert that Dynamic into our desired real type. +When reflected data is deserialized, all of it is placed in a Dynamic type[^2]. This means that, even if we know the Real type ahead of time, we don't quite have it yet. This is where `FromReflect` shines, as it allows us to convert that Dynamic into our desired Real type. ## Motivation @@ -71,7 +68,7 @@ So where's the issue? The main problem is in the ergonomics of this whole system for the typical Bevy developer as well as the consistency of it across the Bevy ecosystem. To be specific, there are three areas that could be improved: 1. **Derive Complexity** - A way to cut back on repetitive or redundant code. -2. **Dynamic Conversions** - A way to use `FromReflect` in a purely dynamic way, where no real types are known. +2. **Dynamic Conversions** - A way to use `FromReflect` in a purely dynamic way, where no Real types are known. 3. **Deserialization Clarity** - A way to make it clear to the user exactly what they are getting back from the deserializer. Please note that these are not listed in order of importance but, rather, order of understanding. @@ -133,9 +130,9 @@ By marking the container with `#[from_reflect(auto_derive = false)]`, we signal > Adding `ReflectFromReflect` type data -`FromReflect` is great in that you can easily convert a Dynamic type to a real one. Oftentimes, however, you might find yourself in a situation where the real type is not known. You need to call `<_>::from_reflect`, but have no idea what should go in place of `<_>`. +`FromReflect` is great in that you can easily convert a Dynamic type to a Real one. Oftentimes, however, you might find yourself in a situation where the Real type is not known. You need to call `<_>::from_reflect`, but have no idea what should go in place of `<_>`. -In these cases, the `ReflectFromReflect` type data may be used. This can be retrieved from the type registry using the `TypeID` or the type name of the real type. For example: +In these cases, the `ReflectFromReflect` type data may be used. This can be retrieved from the type registry using the `TypeID` or the type name of the Real type. For example: ```rust let rfr = registry @@ -145,7 +142,7 @@ let rfr = registry let real_struct: Box = rfr.from_reflect(&dyn_struct).unwrap(); ``` -Calling `rfr.from_reflect` will return an `Option>`, where the `Box` contains the real type registered with `type_id`. +Calling `rfr.from_reflect` will return an `Option>`, where the `Box` contains the Real type registered with `type_id`. #### More Derive Complexity @@ -170,9 +167,9 @@ let some_data: Box = reflect_deserializer.deserialize(&mut deserial let real_struct: Foo = some_data.take().unwrap(); // PANIC! ``` -Again, this is because `ReflectDeserializer` always returns a Dynamic[^2]. A user must then use `FromReflect` themselves to get the real value. +Again, this is because `ReflectDeserializer` always returns a Dynamic[^2]. A user must then use `FromReflect` themselves to get the Real value. -Instead, `ReflectDeserializer::deserialize` now performs the conversion automatically before returning. Not only does this cut back on code needed to be written by the user, but it also cuts back on the need for them to understand the complexity and internals of deserializing reflected data. This means that the above code should no longer panic (assuming the real type *actually is* `Foo`). +Instead, `ReflectDeserializer::deserialize` now performs the conversion automatically before returning. Not only does this cut back on code needed to be written by the user, but it also cuts back on the need for them to understand the complexity and internals of deserializing reflected data. This means that the above code should no longer panic (assuming the Real type *actually is* `Foo`). If any type in the serialized data did not register `ReflectFromReflect`, this should result in an error being returned indicating that `ReflectFromReflect` was not registered for the given type. With the changes listed in **[Derive Complexity](#derive-complexity)**, this should be less likely to happen, since all types should register it by default. If it does happen, the user can choose to either register said type data, or forgo the automatic conversion with **dynamic deserialization**. @@ -180,7 +177,7 @@ If any type in the serialized data did not register `ReflectFromReflect`, this s There may be some cases where a user has data that doesn't conform to `FromReflect` but is still serializable. For example, we might not want to make `Entity` constructible, but we could still pass along data that *represents* an `Entity` so we it can be constructed in some Bevy-controlled way. -These types require manual reconstruction of the real type using the deserialized data. As such, they should just deserialize to a Dynamic and leave the construction part up to the user. The `ReflectDeserializer::deserialize` method, before this RFC, would do just that and return a Dynamic for *all* types. We can re-enable this behavior on the deserializer be disabling automatic conversion: +These types require manual reconstruction of the Real type using the deserialized data. As such, they should just deserialize to a Dynamic and leave the construction part up to the user. The `ReflectDeserializer::deserialize` method, before this RFC, would do just that and return a Dynamic for *all* types. We can re-enable this behavior on the deserializer be disabling automatic conversion: ```rust // Disable automatic conversions @@ -281,4 +278,4 @@ One concern about integrating the `FromReflect` derive into the `Reflect` derive [^1]: This only applies to *active* fields, or fields not marked as `#[reflect(ignore)]`. For *inactive* fields, it instead requires a `Default` impl (unless given a specialized default value using `#[reflect(default = "some_function")]`) -[^2]: Actually, not *all* data is deserialized into a Dynamic. Primitive types, like `u8`, are deserialized into their real type. This is a confusing disparity addressed by https://github.com/bevyengine/bevy/pull/4561. \ No newline at end of file +[^2]: Actually, not *all* data is deserialized into a Dynamic. Primitive types, like `u8`, are deserialized into their Real type. This is a confusing disparity addressed by https://github.com/bevyengine/bevy/pull/4561. \ No newline at end of file From 3b150bebc14ae7cd5f73092ba5cbb873423c7aab Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Mon, 4 Jul 2022 13:22:30 -0700 Subject: [PATCH 5/9] Add background on Reflect itself --- rfcs/59-from_reflect_ergonomics.md | 47 ++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/rfcs/59-from_reflect_ergonomics.md b/rfcs/59-from_reflect_ergonomics.md index eae0a699..4483765a 100644 --- a/rfcs/59-from_reflect_ergonomics.md +++ b/rfcs/59-from_reflect_ergonomics.md @@ -19,11 +19,52 @@ Since both may be represented as a `dyn Reflect`, examples in this RFC will try ## Background -### What is `FromReflect`? +### What is `Reflect`? + +Before diving into this RFC and the world of Bevy reflection, let's take a quick step back and recall what `Reflect` is and why we need it. + +Reflection in programming gives developers the power to analyze the code itself, and use that information to perform dynamic operations. This can be as simple as getting the name of a struct's field. Or it can be more complex, such as iterating over all elements of a tuple and comparing them with values in a list. + +Rust does not come with this functionality by default. Bevy's `Reflect` instead powers this *meta-programming* mechanism. `Reflect` can be derived on any struct using `#[derive(Reflect)]` and allows that struct to be reflected. This derive will also implement a number of other necessary traits. For structs, one such trait is `Struct` (or `TupleStruct` for tuple structs). + +If a type implements `Reflect`, you'll often see it used in trait object form: `dyn Reflect`. + +This whole system is very useful, especially for dynamically operating on a value based on the data we get from reflecting it. -For those unfamiliar, here's a quick refresher on `FromReflect`. +### Why do we need Dynamic types? + +Reflection works great when you already have a value to reflect. However, there are many times when you might not have a value (or the full value). For example, take the following struct: + +```rust +struct Foo { + x: i32, + y: i32 +} +``` + +If we need to deserialize the following JSON: + +```json +{ + "type": "my_crate::Foo", + "struct": { + "x": { + "type": "i32", + "value": 123 + } + } +} +``` + +How can we do this? Notice that we're missing the `y` field! + +This is where Dynamic types come in— specifically a `DynamicStruct`. Dynamic types allow us to mimic *any* type. We can use this Dynamic to apply its data atop an existing value where applicable. Or, in the case above, we can use it to store partial information of a type. + +The problem is that when we deserialize this and get that `DynamicStruct` back, how do we get to the Real type, `Foo`? Enter `FromReflect`. + +### What is `FromReflect`? -The `FromReflect` trait is meant to allow types to be constructed dynamically (well, not completely, but we'll get into that later). Specifically, it enables a Real type to be generated from a Dynamic one. As an example, we can convert a `DynamicTuple` to a Real tuple like so: +The `FromReflect` trait is meant to allow types to be constructed dynamically, often using Dynamic types. Specifically, it enables a Real type to be generated from a Dynamic one. As an example, we can convert a `DynamicTuple` to a Real tuple like so: ```rust let mut dyn_tuple = DynamicTuple::default(); From 43365bd081ed78799cf96440f34a402cf71df537 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Mon, 4 Jul 2022 13:30:55 -0700 Subject: [PATCH 6/9] Clarify FromReflect usefulness section --- rfcs/59-from_reflect_ergonomics.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rfcs/59-from_reflect_ergonomics.md b/rfcs/59-from_reflect_ergonomics.md index 4483765a..43522f0f 100644 --- a/rfcs/59-from_reflect_ergonomics.md +++ b/rfcs/59-from_reflect_ergonomics.md @@ -100,7 +100,9 @@ So why does this matter? If we need the Real type we can just avoid using Dynami Well for any users who have tried working with serialized reflection data, they've likely come across the biggest use case for `FromReflect`: deserialization. -When reflected data is deserialized, all of it is placed in a Dynamic type[^2]. This means that, even if we know the Real type ahead of time, we don't quite have it yet. This is where `FromReflect` shines, as it allows us to convert that Dynamic into our desired Real type. +When reflected data is deserialized, all of it is placed in a Dynamic type[^2]. This means that, even if we know the Real type ahead of time, we're still stuck with the Dynamic type instead. Like in the example above, we might know we want `Foo`, but deserializing it using `ReflectDeserializer` will give us a `DynamicStruct`. + +This is why `FromReflect` is so useful: it allows us to convert that Dynamic into our desired Real type. So with it, we can convert the returned `DynamicStruct` into a Real instance of `Foo`. ## Motivation From 93a61aa4d4ddf2751410b40b7aa9d43208f6d7ac Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Mon, 4 Jul 2022 14:08:24 -0700 Subject: [PATCH 7/9] Fix typo --- rfcs/59-from_reflect_ergonomics.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/59-from_reflect_ergonomics.md b/rfcs/59-from_reflect_ergonomics.md index 43522f0f..e360cef5 100644 --- a/rfcs/59-from_reflect_ergonomics.md +++ b/rfcs/59-from_reflect_ergonomics.md @@ -220,7 +220,7 @@ If any type in the serialized data did not register `ReflectFromReflect`, this s There may be some cases where a user has data that doesn't conform to `FromReflect` but is still serializable. For example, we might not want to make `Entity` constructible, but we could still pass along data that *represents* an `Entity` so we it can be constructed in some Bevy-controlled way. -These types require manual reconstruction of the Real type using the deserialized data. As such, they should just deserialize to a Dynamic and leave the construction part up to the user. The `ReflectDeserializer::deserialize` method, before this RFC, would do just that and return a Dynamic for *all* types. We can re-enable this behavior on the deserializer be disabling automatic conversion: +These types require manual reconstruction of the Real type using the deserialized data. As such, they should just deserialize to a Dynamic and leave the construction part up to the user. The `ReflectDeserializer::deserialize` method, before this RFC, would do just that and return a Dynamic for *all* types. We can re-enable this behavior on the deserializer by disabling automatic conversion: ```rust // Disable automatic conversions From 9101c86edecdd0ddf3fe6b855750508d3bf45eb4 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Mon, 4 Jul 2022 14:10:51 -0700 Subject: [PATCH 8/9] Break up lines for easier review --- rfcs/59-from_reflect_ergonomics.md | 264 ++++++++++++++++++++--------- 1 file changed, 183 insertions(+), 81 deletions(-) diff --git a/rfcs/59-from_reflect_ergonomics.md b/rfcs/59-from_reflect_ergonomics.md index e360cef5..51e04cf9 100644 --- a/rfcs/59-from_reflect_ergonomics.md +++ b/rfcs/59-from_reflect_ergonomics.md @@ -2,43 +2,61 @@ ## Summary -Derive `FromReflect` by default when deriving `Reflect`, add `ReflectFromReflect` type data, and make `ReflectDeserializer` easier to use. +Derive `FromReflect` by default when deriving `Reflect`, add `ReflectFromReflect` type data, and +make `ReflectDeserializer` easier to use. ## Terminology ### *Dynamic Types* vs *Real Types* -A **Dynamic** type refers to any of the `Dynamic***` structs used by `bevy_reflect` to represent a basic Rust data structure. For example, a `DynamicTupleStruct` is a Dynamic that represents a tuple struct in Rust. +A **Dynamic** type refers to any of the `Dynamic***` structs used by `bevy_reflect` to represent a basic Rust data +structure. +For example, a `DynamicTupleStruct` is a Dynamic that represents a tuple struct in Rust. -A **Real** type, on the other hand, refers to any type that is *not* a Dynamic. For example, `i32` is a Real type, and so is `Vec`. +A **Real** type, on the other hand, refers to any type that is *not* a Dynamic. +For example, `i32` is a Real type, and so is `Vec`. Since both may be represented as a `dyn Reflect`, examples in this RFC will try to use the following conventions: -* Variables named `dyn_***` contain a Dynamic type. The full name should match the name of the corresponding Dynamic type. For example, a `dyn_tuple` represents a `DynamicTuple`. -* Variables named `real_***` contain a Real type. The full name should match the corresponding data structure. For example, a `real_tuple` represents any Rust tuple, such as `(i32, f32, usize)`. +* Variables named `dyn_***` contain a Dynamic type. The full name should match the name of the corresponding Dynamic + type. For example, a `dyn_tuple` represents a `DynamicTuple`. +* Variables named `real_***` contain a Real type. The full name should match the corresponding data structure. For + example, a `real_tuple` represents any Rust tuple, such as `(i32, f32, usize)`. ## Background ### What is `Reflect`? -Before diving into this RFC and the world of Bevy reflection, let's take a quick step back and recall what `Reflect` is and why we need it. +Before diving into this RFC and the world of Bevy reflection, let's take a quick step back and recall what `Reflect` is +and why we need it. -Reflection in programming gives developers the power to analyze the code itself, and use that information to perform dynamic operations. This can be as simple as getting the name of a struct's field. Or it can be more complex, such as iterating over all elements of a tuple and comparing them with values in a list. +Reflection in programming gives developers the power to analyze the code itself, and use that information to perform +dynamic operations. +This can be as simple as getting the name of a struct's field. +Or it can be more complex, such as iterating over all elements of a tuple and comparing them with values in a list. -Rust does not come with this functionality by default. Bevy's `Reflect` instead powers this *meta-programming* mechanism. `Reflect` can be derived on any struct using `#[derive(Reflect)]` and allows that struct to be reflected. This derive will also implement a number of other necessary traits. For structs, one such trait is `Struct` (or `TupleStruct` for tuple structs). +Rust does not come with this functionality by default. +Bevy's `Reflect` instead powers this *meta-programming* mechanism. +`Reflect` can be derived on any struct using `#[derive(Reflect)]` and allows that struct to be reflected. +This derive will also implement a number of other necessary traits. +For structs, one such trait is `Struct` (or `TupleStruct` for tuple structs). If a type implements `Reflect`, you'll often see it used in trait object form: `dyn Reflect`. -This whole system is very useful, especially for dynamically operating on a value based on the data we get from reflecting it. +This whole system is very useful, especially for dynamically operating on a value based on the data we get from +reflecting it. ### Why do we need Dynamic types? -Reflection works great when you already have a value to reflect. However, there are many times when you might not have a value (or the full value). For example, take the following struct: +Reflection works great when you already have a value to reflect. +However, there are many times when you might not have a value (or the full value). + +For example, take the following struct: ```rust struct Foo { - x: i32, - y: i32 + x: i32, + y: i32 } ``` @@ -58,33 +76,40 @@ If we need to deserialize the following JSON: How can we do this? Notice that we're missing the `y` field! -This is where Dynamic types come in— specifically a `DynamicStruct`. Dynamic types allow us to mimic *any* type. We can use this Dynamic to apply its data atop an existing value where applicable. Or, in the case above, we can use it to store partial information of a type. +This is where Dynamic types come in— specifically a `DynamicStruct`. +Dynamic types allow us to mimic *any* type. +We can use this Dynamic to apply its data atop an existing value where applicable. +Or, in the case above, we can use it to store partial information of a type. -The problem is that when we deserialize this and get that `DynamicStruct` back, how do we get to the Real type, `Foo`? Enter `FromReflect`. +The problem is that when we deserialize this and get that `DynamicStruct` back, how do we get to the Real type, `Foo`? +Enter `FromReflect`. ### What is `FromReflect`? -The `FromReflect` trait is meant to allow types to be constructed dynamically, often using Dynamic types. Specifically, it enables a Real type to be generated from a Dynamic one. As an example, we can convert a `DynamicTuple` to a Real tuple like so: +The `FromReflect` trait is meant to allow types to be constructed dynamically, often using Dynamic types. +Specifically, it enables a Real type to be generated from a Dynamic one. + +As an example, we can convert a `DynamicTuple` to a Real tuple like so: ```rust -let mut dyn_tuple = DynamicTuple::default(); +let mut dyn_tuple = DynamicTuple::default (); dyn_tuple.insert(123usize); dyn_tuple.insert(321usize); -let real_tuple = <(usize, usize)>::from_reflect(&dyn_tuple).unwrap(); +let real_tuple = < (usize, usize) >::from_reflect( & dyn_tuple).unwrap(); ``` A Dynamic value to a Real one. Neat! ### How does `FromReflect` work? -Basically it's `FromReflect` all the way down. +Basically it's `FromReflect` all the way down. Given a struct like: ```rust struct Foo { - value: (f32, f32) + value: (f32, f32) } ``` @@ -92,27 +117,37 @@ Calling `Foo::from_reflect` will kick off a chain of `FromReflect`: `Foo::from_reflect` calls `<(f32, f32)>::from_reflect` which itself calls `f32::from_reflect`. -This is why, when deriving `FromReflect`, all fields *must* also implement `FromReflect`[^1]. Without this, the trait itself won't just "not work"— it won't even compile. +This is why, when deriving `FromReflect`, all fields *must* also implement `FromReflect`[^1]. +Without this, the trait itself won't just "not work"— it won't even compile. ### Why is `FromReflect` useful? So why does this matter? If we need the Real type we can just avoid using Dynamics, right? -Well for any users who have tried working with serialized reflection data, they've likely come across the biggest use case for `FromReflect`: deserialization. +Well for any users who have tried working with serialized reflection data, they've likely come across the biggest use +case for `FromReflect`: deserialization. -When reflected data is deserialized, all of it is placed in a Dynamic type[^2]. This means that, even if we know the Real type ahead of time, we're still stuck with the Dynamic type instead. Like in the example above, we might know we want `Foo`, but deserializing it using `ReflectDeserializer` will give us a `DynamicStruct`. +When reflected data is deserialized, all of it is placed in a Dynamic type[^2]. +This means that, even if we know the Real type ahead of time, we're still stuck with the Dynamic type instead. +Like in the example above, we might know we want `Foo`, but deserializing it using `ReflectDeserializer` will +give us a `DynamicStruct`. -This is why `FromReflect` is so useful: it allows us to convert that Dynamic into our desired Real type. So with it, we can convert the returned `DynamicStruct` into a Real instance of `Foo`. +This is why `FromReflect` is so useful: it allows us to convert that Dynamic into our desired Real type. +So with it, we can convert the returned `DynamicStruct` into a Real instance of `Foo`. ## Motivation So where's the issue? -The main problem is in the ergonomics of this whole system for the typical Bevy developer as well as the consistency of it across the Bevy ecosystem. To be specific, there are three areas that could be improved: +The main problem is in the ergonomics of this whole system for the typical Bevy developer as well as the consistency of +it across the Bevy ecosystem. + +To be specific, there are three areas that could be improved: 1. **Derive Complexity** - A way to cut back on repetitive or redundant code. 2. **Dynamic Conversions** - A way to use `FromReflect` in a purely dynamic way, where no Real types are known. -3. **Deserialization Clarity** - A way to make it clear to the user exactly what they are getting back from the deserializer. +3. **Deserialization Clarity** - A way to make it clear to the user exactly what they are getting back from the + deserializer. Please note that these are not listed in order of importance but, rather, order of understanding. @@ -122,11 +157,16 @@ Please note that these are not listed in order of importance but, rather, order > Removing redundancy and providing sane defaults -Bevy's reflection model is powerful because it not only allows type metadata to be easily traversed, but it also allows for (de)serialization without the need for any `serde` derives. +Bevy's reflection model is powerful because it not only allows type metadata to be easily traversed, but it also allows +for (de)serialization without the need for any `serde` derives. -This is great because nearly every reflectable type likely wants to be readily serializable as well. This is so they can be supported by Bevy's scene format or a maybe custom config format. This can be used for sending serialized data over the network or to an editor. +This is great because nearly every reflectable type likely wants to be readily serializable as well. +This is so they can be supported by Bevy's scene format or a maybe custom config format. +This can be used for sending serialized data over the network or to an editor. -Chances are, if a type implements `Reflect`, it should also be implementing `FromReflect`. Because deserialization is so common in these situations, `FromReflect` is now automatically derived alongside `Reflect`. +Chances are, if a type implements `Reflect`, it should also be implementing `FromReflect`. +Because deserialization is so common in these situations, `FromReflect` is now automatically derived alongside +`Reflect`. ```rust // Before: @@ -138,58 +178,76 @@ struct Foo; struct Foo; ``` -Not only does this cut back on the amount of code a user must write, but it helps establish some amount of consistency across the Bevy ecosystem. Many plugin authors, and even Bevy itself (here's one [example](https://github.com/bevyengine/bevy/blob/4c5e30a9f83b4d856e58d23a4e89d662abf476ad/crates/bevy_sprite/src/rect.rs#L7-L7)), can sometimes forget to consider users who might want to use their type with `FromReflect`. +Not only does this cut back on the amount of code a user must write, but it helps establish some amount of consistency +across the Bevy ecosystem. +Many plugin authors, and even Bevy itself (here's one [example](https://github.com/bevyengine/bevy/blob/4c5e30a9f83b4d856e58d23a4e89d662abf476ad/crates/bevy_sprite/src/rect.rs#L7-L7)), +can sometimes forget to consider users who might want to use their type with `FromReflect`. #### Opting Out -Again, in most cases, a type that implements `Reflect` is probably fine implementing `FromReflect` as well— if not for themselves, then for another user/plugin/editor/etc. However, there are a few cases where this is not always true— often for runtime identifier types, or other types never meant to be serialized or manually constructed. +Again, in most cases, a type that implements `Reflect` is probably fine implementing `FromReflect` as well— if not for +themselves, then for another user/plugin/editor/etc. +However, there are a few cases where this is not always true— often for runtime identifier types, or other types never +meant to be serialized or manually constructed. -Take Bevy's [`Entity`](https://github.com/bevyengine/bevy/blob/4c5e30a9f83b4d856e58d23a4e89d662abf476ad/crates/bevy_ecs/src/entity/mod.rs#L101-L101) type as an example: +Take Bevy's [`Entity`](https://github.com/bevyengine/bevy/blob/4c5e30a9f83b4d856e58d23a4e89d662abf476ad/crates/bevy_ecs/src/entity/mod.rs#L101-L101) +type as an example: ```rust #[derive(Clone, Copy, Hash, Eq, Ord, PartialEq, PartialOrd)] pub struct Entity { - pub(crate) generation: u32, - pub(crate) id: u32, + pub(crate) generation: u32, + pub(crate) id: u32, } ``` -The `generation` and `id` of each entity is generated at runtime (not good for general-purpose serialization). Additionally, the fields are only `pub(crate)` because this struct should almost never be manually generated outside of the crate its defined (not good for `FromReflect`). +The `generation` and `id` of each entity is generated at runtime (not good for general-purpose serialization). +Additionally, the fields are only `pub(crate)` because this struct should almost never be manually generated outside +the crate it's defined (not good for `FromReflect`). -But say we wanted to make it so that we could reflect an entity to get its current `id`. To do this without implementing `FromReflect` requires us to *opt-out* of the automatic derive: +But say we wanted to make it so that we could reflect an entity to get its current `id`. +To do this without implementing `FromReflect` requires us to *opt-out* of the automatic derive: ```rust #[derive(Reflect)] #[from_reflect(auto_derive = false)] pub struct Entity { - pub(crate) generation: u32, - pub(crate) id: u32, + pub(crate) generation: u32, + pub(crate) id: u32, } ``` -By marking the container with `#[from_reflect(auto_derive = false)]`, we signal to the derive macro that this type should not be constructible via `FromReflect` and that it should not be automatically derived. +By marking the container with `#[from_reflect(auto_derive = false)]`, we signal to the derive macro that this type +should not be constructible via `FromReflect` and that it should not be automatically derived. ### Dynamic Conversions > Adding `ReflectFromReflect` type data -`FromReflect` is great in that you can easily convert a Dynamic type to a Real one. Oftentimes, however, you might find yourself in a situation where the Real type is not known. You need to call `<_>::from_reflect`, but have no idea what should go in place of `<_>`. +`FromReflect` is great in that you can easily convert a Dynamic type to a Real one. +Oftentimes, however, you might find yourself in a situation where the Real type is not known. +You need to call `<_>::from_reflect`, but have no idea what should go in place of `<_>`. -In these cases, the `ReflectFromReflect` type data may be used. This can be retrieved from the type registry using the `TypeID` or the type name of the Real type. For example: +In these cases, the `ReflectFromReflect` type data may be used. +This can be retrieved from the type registry using the `TypeID` or the type name of the Real type. + +For example: ```rust let rfr = registry - .get_type_data::(type_id) - .unwrap(); +.get_type_data::(type_id) +.unwrap(); -let real_struct: Box = rfr.from_reflect(&dyn_struct).unwrap(); +let real_struct: Box = rfr.from_reflect( & dyn_struct).unwrap(); ``` -Calling `rfr.from_reflect` will return an `Option>`, where the `Box` contains the Real type registered with `type_id`. +Calling `rfr.from_reflect` will return an `Option>`, where the `Box` contains the Real +type registered with `type_id`. #### More Derive Complexity -This feature also pairs nicely with the changes listed in **[Derive Complexity](#derive-complexity)**, since otherwise this would require the following macros at minimum: +This feature also pairs nicely with the changes listed in **[Derive Complexity](#derive-complexity)**, since otherwise +this would require the following macros at minimum: ```rust #[derive(Reflect, FromReflect)] @@ -203,31 +261,48 @@ Again, listing all of that out on almost every reflectable type just adds noise. > Making the output of deserialization less confusing -This RFC also seeks to improve the understanding of `ReflectDeserializer`. For newer devs or those not familiar with Bevy's reflection have likely been confused why they can't do something like this: +This RFC also seeks to improve the understanding of `ReflectDeserializer`. +For newer devs or those not familiar with Bevy's reflection have likely been confused why they can't do something like +this: ```rust -let some_data: Box = reflect_deserializer.deserialize(&mut deserializer).unwrap(); +let some_data: Box = reflect_deserializer.deserialize( & mut deserializer).unwrap(); let real_struct: Foo = some_data.take().unwrap(); // PANIC! ``` -Again, this is because `ReflectDeserializer` always returns a Dynamic[^2]. A user must then use `FromReflect` themselves to get the Real value. +Again, this is because `ReflectDeserializer` always returns a Dynamic[^2]. +A user must then use `FromReflect` themselves to get the Real value. -Instead, `ReflectDeserializer::deserialize` now performs the conversion automatically before returning. Not only does this cut back on code needed to be written by the user, but it also cuts back on the need for them to understand the complexity and internals of deserializing reflected data. This means that the above code should no longer panic (assuming the Real type *actually is* `Foo`). +Instead, `ReflectDeserializer::deserialize` now performs the conversion automatically before returning. +Not only does this cut back on code needed to be written by the user, but it also cuts back on the need for them to +understand the complexity and internals of deserializing reflected data. +This means that the above code should no longer panic (assuming the Real type *actually is* `Foo`). -If any type in the serialized data did not register `ReflectFromReflect`, this should result in an error being returned indicating that `ReflectFromReflect` was not registered for the given type. With the changes listed in **[Derive Complexity](#derive-complexity)**, this should be less likely to happen, since all types should register it by default. If it does happen, the user can choose to either register said type data, or forgo the automatic conversion with **dynamic deserialization**. +If any type in the serialized data did not register `ReflectFromReflect`, this should result in an error being returned +indicating that `ReflectFromReflect` was not registered for the given type. +With the changes listed in **[Derive Complexity](#derive-complexity)**, this should be less likely to happen, since all +types should register it by default. +If it does happen, the user can choose to either register said type data, or forgo the automatic conversion with +**dynamic deserialization**. #### Dynamic Deserialization -There may be some cases where a user has data that doesn't conform to `FromReflect` but is still serializable. For example, we might not want to make `Entity` constructible, but we could still pass along data that *represents* an `Entity` so we it can be constructed in some Bevy-controlled way. +There may be some cases where a user has data that doesn't conform to `FromReflect` but is still serializable. +For example, we might not want to make `Entity` constructible, but we could still pass along data that *represents* +an `Entity` so it can be constructed in some Bevy-controlled way. + +These types require manual reconstruction of the Real type using the deserialized data. +As such, they should just deserialize to a Dynamic and leave the construction part up to the user. +The `ReflectDeserializer::deserialize` method, before this RFC, would do just that and return a Dynamic for *all* types. -These types require manual reconstruction of the Real type using the deserialized data. As such, they should just deserialize to a Dynamic and leave the construction part up to the user. The `ReflectDeserializer::deserialize` method, before this RFC, would do just that and return a Dynamic for *all* types. We can re-enable this behavior on the deserializer by disabling automatic conversion: +We can re-enable this behavior on the deserializer by disabling automatic conversion: ```rust // Disable automatic conversions reflect_deserializer.set_auto_convert(false); // Use the deserializer as normal -let some_data: Box = reflect_deserializer.deserialize(&mut deserializer).unwrap(); +let some_data: Box = reflect_deserializer.deserialize( & mut deserializer).unwrap(); let dyn_struct: DynamicStruct = some_data.take().unwrap(); // OK! ``` @@ -235,27 +310,35 @@ let dyn_struct: DynamicStruct = some_data.take().unwrap(); // OK! ### Implementing: Derive Complexity -Implementation for **[Derive Complexity](#derive-complexity)** is rather straightforward: call the `FromReflect` derive function from the `Reflect` derive function. They both take `&ReflectDeriveData`, which should make doing this relatively painless. +Implementation for **[Derive Complexity](#derive-complexity)** is rather straightforward: call the `FromReflect` derive +function from the `Reflect` derive function. +They both take `&ReflectDeriveData`, which should make doing this relatively painless. -Note that we are not removing the `FromReflect` derive. There may be cases where a manual implementation of `Reflect` is needed, such as for other proxy types akin to the Dynamics. To make things easier, we can still expose the standard `FromReflect` derive on its own. +Note that we are not removing the `FromReflect` derive. +There may be cases where a manual implementation of `Reflect` is needed, such as for other proxy types akin to the +Dynamics. +To make things easier, we can still expose the standard `FromReflect` derive on its own. -The biggest change will be the inclusion of the opt-out strategy. This means we need to be able to parse `#[from_reflect(auto_derive = false)]` and use it to control whether we call the `FromReflect` derive function or not. This attribute is a `syn::MetaNameValue` where the path is `auto_derive` and the literal is a `Lit::Bool`. +The biggest change will be the inclusion of the opt-out strategy. +This means we need to be able to parse `#[from_reflect(auto_derive = false)]` and use it to control whether we call the +`FromReflect` derive function or not. +This attribute is a `syn::MetaNameValue` where the path is `auto_derive` and the literal is a `Lit::Bool`. Essentially we can do something like: ```rust pub fn derive_reflect(input: TokenStream) -> TokenStream { - // ... - let from_reflect_impl = if is_auto_derive { - match derive_data.derive_type() { - DeriveType::Struct | DeriveType::UnitStruct => Some(from_reflect::impl_struct(&derive_data)), - // ... - } - } else { - None - }; - - return quote! { + // ... + let from_reflect_impl = if is_auto_derive { + match derive_data.derive_type() { + DeriveType::Struct | DeriveType::UnitStruct => Some(from_reflect::impl_struct(&derive_data)), + // ... + } + } else { + None + }; + + return quote! { #reflect_impl #from_reflect_impl @@ -267,18 +350,22 @@ pub fn derive_reflect(input: TokenStream) -> TokenStream { ### Implementing: Dynamic Conversions -Implementing **[Dynamic Conversions](#dynamic-conversions)** mainly involves the creation of a `ReflectFromReflect` type. This was previously explored in https://github.com/bevyengine/bevy/pull/4147, however, it was abandoned because of the issues this RFC aim to fix. +Implementing **[Dynamic Conversions](#dynamic-conversions)** mainly involves the creation of a `ReflectFromReflect` +type. +This was previously explored in https://github.com/bevyengine/bevy/pull/4147, however, it was abandoned because of +the issues this RFC aim to fix. This type data should automatically be registered if `FromReflect` is automatically derived. ### Implementing: Deserialization Clarity -**[Deserialization Clarity](#deserialization-clarity)** can readily be implemented by adding a private field to `ReflectDeserializer`: +**[Deserialization Clarity](#deserialization-clarity)** can readily be implemented by adding a private field +to `ReflectDeserializer`: ```rust pub struct ReflectDeserializer<'a> { - registry: &'a TypeRegistry, - auto_convert: bool // <- New + registry: &'a TypeRegistry, + auto_convert: bool // <- New } ``` @@ -286,39 +373,54 @@ This should default to `true` and be changeable using a new method: ```rust impl<'a> ReflectDeserializer<'a> { - pub fn set_auto_convert(&mut self, value: bool) { - self.auto_convert = value; - } + pub fn set_auto_convert(&mut self, value: bool) { + self.auto_convert = value; + } } ``` -Once the Dynamic is deserialized by the visitor, it will check this value to see whether it needs to perform the conversion. This is done using the same method as shown in **[Dynamic Conversions](#dynamic-conversions)**. However, rather than unwrapping, we will return a `Result` that gives better error messages that point to `FromReflect` and/or `ReflectFromReflect`. +Once the Dynamic is deserialized by the visitor, it will check this value to see whether it needs to perform the +conversion. +This is done using the same method as shown in **[Dynamic Conversions](#dynamic-conversions)**. +However, rather than unwrapping, we will return a `Result` that gives better error messages that point to `FromReflect` +and/or `ReflectFromReflect`. ## Drawbacks -* Unidiomatic. One of the biggest drawbacks of this change is that it is not very standard to have opt-out mechanisms for derives. They mainly act in an additive fashion. Thus, it could be confusing. +* Unidiomatic. One of the biggest drawbacks of this change is that it is not very standard to have opt-out mechanisms + for derives. They mainly act in an additive fashion. Thus, it could be confusing. -* Increased code generation. By automatically deriving `FromReflect` we increase the amount of code that needs to be generated per type— namely for reflectable types that *really* don't need `FromReflect` (such as for a single binary that doesn't need serialization and is not meant to be a library). +* Increased code generation. By automatically deriving `FromReflect` we increase the amount of code that needs to be + generated per type— namely for reflectable types that *really* don't need `FromReflect` (such as for a single binary + that doesn't need serialization and is not meant to be a library).
Rebuttal - In most cases, however, this is code that *should* be generated as explained in **[Derive Complexity](#derive-complexity)**. Additionally, the code generation is rather small with each field in a struct only generating around 1–4 lines of code. + In most cases, however, this is code that *should* be generated as explained in + **[Derive Complexity](#derive-complexity)**. + Additionally, the code generation is rather small with each field in a struct only generating around 1–4 lines of + code.
## Rationale and alternatives -The biggest alternative is doing nothing. Again, most of these are ergonomics and ecosystem wins. The engine will work pretty much the same with or without these changes. The hope is that we can make reflection less intimidating, confusing, and frustrating, while making it simpler, more intuitive, and more consistent across crates. +The biggest alternative is doing nothing. Again, most of these are ergonomics and ecosystem wins. +The engine will work pretty much the same with or without these changes. +The hope is that we can make reflection less intimidating, confusing, and frustrating, while making it simpler, +more intuitive, and more consistent across crates. -One concern about integrating the `FromReflect` derive into the `Reflect` derive is that we might want to support something like a `FromWorldReflect`. However, using the opt-out method this should still be possible. +One concern about integrating the `FromReflect` derive into the `Reflect` derive is that we might want to support +something like a `FromWorldReflect`. +However, using the opt-out method this should still be possible. ## Unresolved questions - Should `ReflectFromReflect` be replaced by a method on `TypeRegistration`, such as `TypeRegistration::construct`? - Any other pros/cons to these changes? - +
[^1]: This only applies to *active* fields, or fields not marked as `#[reflect(ignore)]`. For *inactive* fields, it instead requires a `Default` impl (unless given a specialized default value using `#[reflect(default = "some_function")]`) [^2]: Actually, not *all* data is deserialized into a Dynamic. Primitive types, like `u8`, are deserialized into their Real type. This is a confusing disparity addressed by https://github.com/bevyengine/bevy/pull/4561. \ No newline at end of file From a2a990cd2fc960d520ba89440931dc309e039a0e Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Tue, 5 Jul 2022 22:21:29 -0700 Subject: [PATCH 9/9] Address PR comments --- rfcs/59-from_reflect_ergonomics.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/59-from_reflect_ergonomics.md b/rfcs/59-from_reflect_ergonomics.md index 51e04cf9..a652781c 100644 --- a/rfcs/59-from_reflect_ergonomics.md +++ b/rfcs/59-from_reflect_ergonomics.md @@ -103,7 +103,7 @@ A Dynamic value to a Real one. Neat! ### How does `FromReflect` work? -Basically it's `FromReflect` all the way down. +Basically, `FromReflect` methods are recursively called for each field. Given a struct like: @@ -118,7 +118,7 @@ Calling `Foo::from_reflect` will kick off a chain of `FromReflect`: `Foo::from_reflect` calls `<(f32, f32)>::from_reflect` which itself calls `f32::from_reflect`. This is why, when deriving `FromReflect`, all fields *must* also implement `FromReflect`[^1]. -Without this, the trait itself won't just "not work"— it won't even compile. +Without this, the trait implementation won't compile. ### Why is `FromReflect` useful?