From 02f5777500f2b54997af5e68c0a86410c80a7287 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Fri, 20 May 2022 08:30:05 +0200 Subject: [PATCH 1/7] Add Reflect API changes proposition --- rfcs/56-better-reflect.md | 261 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 261 insertions(+) create mode 100644 rfcs/56-better-reflect.md diff --git a/rfcs/56-better-reflect.md b/rfcs/56-better-reflect.md new file mode 100644 index 00000000..b708dec6 --- /dev/null +++ b/rfcs/56-better-reflect.md @@ -0,0 +1,261 @@ +# bevy_reflect: Unique `Reflect` + +This builds on https://github.com/bevyengine/bevy/pull/4042 and the `TypeInfo` +API. + +## Summary + +Add a `ReflectProxy` enum that generalizes the `Dynamic***` structs from +`bevy_reflect` to all types, and remove `Reflect` implementation for +`Dynamic***` structs. + +I also suggest renaming all `Dynamic***` into `***Proxy` and the reflect +traits `clone_dynamic` methods to `proxy_clone`. + +## Lexicon/Terminology/Nomenclature/Jargon + +#### pass for + +We say that `T: Reflect` **passes for** `U: Reflect` when `T`'s `type_name` is +equal to `U`'s, and it is possible to convert from a `T` to a `U` using +`apply`, `set` or `FromReflect::from_reflect`. + +#### underlying + +The **underlying** value of a dynamic object (eg: `dyn Reflect`) is the +concrete type backing it. In the following code, the **underlying type** of +`reflect_a` is `MysteryType`, while the **underlying value** is the value of +`a`. + +```rust +let a = MysteryType::new(); +let reflect_a: Box = Box::new(a.clone()) as Box; +``` + +When using `clone_value`, the underlying type changes: if `MysteryType` is a +struct, the underlying type of `reflect_clone` will be `DynamicStruct`: + +```rust +let reflect_clone: Box = reflect_a.clone_value(); +``` + +#### Underlying-sensible method + +Those are methods of `Reflect` that act differently between `reflect_clone` and +`reflect_a` from the previous examples. + + +#### `Dynamic***` + +The structures holding generic structural information about some proxied type. +* `DynamicMap` +* `DynamicList` +* `DynamicTuple` +* `DynamicStruct` +* `DynamicTupleStruct` + +#### Reflect subtraits + +The top level traits in `bevy_reflect`: `Struct`, `TupleStruct`, `Tuple`, +`List` and `Map`. They are all subtraits of `Reflect`. + +## Motivation + +The `Reflect` trait is a trap, it doesn't work as expected due to `Dynamic***` +mixing up with the "regular" types under the `dyn Reflect` trait object. + +To solve this, we do not allow more than a single underlying type to "pass for" +a concrete type in `Reflect`. This implies that the various `Dynamic***` types +shouldn't implement `Reflect`. + +When the underlying type of a `Box` is a `Dynamic***`, a lot of +the `Reflect` methods become invalid, such as `any`, `reflect_hash`, +`reflect_partial_eq`, `serializable`, `downcast`, `is` or `take`. However, +those methods are provided regardless. + +### What's under the `dyn Reflect`? + +Currently, `Reflect` has very a confusing behavior. Most of the methods on +`Reflect` are highly dependent on the underlying type, regardless of whether +they are supposed to stand for the requested type or not. + +For example + +```rust +let a = MysteryType::new(); // suppose MysteryType implements Reflect +let reflect_a: Box = Box::new(a.clone()) as Box; +let dynamic_a: Box = a.clone_value(); +// This more or less always panic +assert_eq!((&*reflect_a).type_id(), (&*dynamic_a).type_id()) +``` + +This is because multiple different things can pretend to be the same thing. In +itself this shouldn't be an issue, but the current `Reflect` API is leaky, and +you need to be aware of both this limitation and the underlying type to be able +to use the `Reflect` API correctly. + + +### `reflect_hash` and co. with `Dynamic***` + +The problem is not limited to `type_id`, but also extends to the `reflect_*` +methods. + +```rust +let a = MysteryType::new(); // suppose MysteryType implements Reflect +let reflect_a: Box = Box::new(a.clone()) as Box; +let dynamic_a: Box = a.clone_value(); +let reflect_hash = reflect_a.reflect_hash(); +let dynamic_hash = dynamic_a.reflect_hash(); +// This more or less always panic if `MysteryType` implements `Hash` +assert_eq!(reflect_hash, dynamic_hash) +``` + + +## Proposition + +The problem stems from the duplicity of `dyn Reflect`. We want to rely on the +underlying methods of the same thing to be unique. + +To solve this, we do not allow more than a single underlying type to "pass for" +a concrete type in `Reflect`. This implies that the various `Dynamic***` types +shouldn't implement `Reflect`. + +## User-facing explanation + +* `Reflect` maps 1-to-1 with an underlying type meaning that if a + `Box` cannot be downcasted to `T`, you cannot build a `T` from + it. +* There is a `ReflectProxy` enum you can use to construct any `Reflect` type + based on some dynamic structure. This can be used, for example, for + deserialization of types that implement `Reflect` but not `Deserialize`. + * You can "clone reflectively" a `Box` by using + `Reflect::proxy_clone`. It returns a `ReflectProxy`. + * You can use `ReflectProxy::sidecast` to create a `Box` from + a `ReflectProxy`. The created trait object's underlying type is a concrete + type you can downcast to. +* `FromReflect` doesn't exist anymore. Instead, `Reflect::downcast` provides + now the same guarentees that `FromReflect::from_reflect` had. If you need to + get a `T` from a `ReflectProxy`, use a combination of + `ReflectProxy::sidecast_type` and `downcast`. + + +## Implementation strategy + +The `Reflect` API stays identical to the one we have today, at the exception of +`clone_value`. + +We remove the `Dynamic***: Reflect` implementations and we introduce a +`ReflectProxy` enum. + +```rust +pub enum ReflectProxy { + Struct(DynamicStruct), + TupleStruct(DynamicTupleStruct), + Tuple(DynamicTuple), + List(DynamicList), + Map(DynamicMap), + AlreadyReflect(Box), +} +``` + +`ReflectProxy` mirrors the `ReflectRef` and `TypeInfo` enums. + +`ReflectProxy` has a method to convert the underlying value into the +value it is proxying. This allows you to get a `Reflect` from a `ReflectProxy` +while upholding the `Reflect` uniqueness rule. + +```rust +impl ReflectProxy { + pub fn type_name(&self) -> &str { + match self { /* etc. */ } + } + pub fn can_sidecast(&self, registry: &TypeRegistry) -> Result<(), SidecastError> {} + pub fn sidecast(self, registry: &TypeRegistry) + -> Result, SidecastError> { + let type_name = self.type_name(); + let registration = registry.get_with_name(type_name).ok_or(SidecastError::Unregistered)?; + Ok(registration.constructor(self)?) + } + pub fn sidecast_type(self) -> Result, SidecastError> { + let registration = TypeRegistration::of::(); + Ok(registration.constructor(self)?) + } +} +``` + +`sidecast` requires introducing a new field to `TypeRegistration`: +```rust +pub struct TypeRegistration { + short_name: String, + // new: constructor + constructor: fn(ReflectProxy) -> Result, ConstructorError>, + data: HashMap>, + type_info: TypeInfo, +} +impl TypeRegistration { + pub fn constructor(&self, proxy: ReflectProxy) -> Result, ConstructorError> { + (self.constructor)(proxy) + } + // ... +} +``` + +`constructor` would simply construct the `T` and wrap it into a `Box` before +returning it. + +`constructor` can be derived similarly to `TypeInfo`. It will however +require additional consuming methods on `Dynamic***` structs in order to be +able to move their `Box` fields into the constructed data +structure. + +## Drawbacks + +- Users cannot define their own proxy type, they must use the `Dynamic***` + types. +- `Dynamic***` do not implement the `Reflect` subtraits anymore. +- There is some added complexity (although it fixes what I believe to be a + major limitation of the `bevy_reflect` API) +- `Reflect::apply` now only accepts a `ReflectProxy` (see next section) + +## Rationale and alternatives + +- Divide `Reflect` in two distinct traits, one with the underlying-sensible + method, and one with the other methods. (see next points) +- An earlier version of this RFC proposed `ReflectProxy` as a **trait**, + however, I reformulated it as an enum when considering how to implement it. + (How would you define the `TypeRegistration::constructor` field if passed a + `Box`? You'd need something like `ReflectProxyRef`?) + +I believe this change is needed, since `Reflect` duplicity is a large footgun. + +I don't believe that having `Dynamic***` implement `Reflect` makes sense, I +think it introduces much more issues than it solves. The only use-case for +`Dynamic***` being `Reflect` are the `reflect_mut`, `reflect_ref` and `apply` +methods. However, those could as well be implemented outside of the `Reflect` +trait, as mentioned in the "Future possibilities" section. + +## Unresolved questions + +- How does this interact with `#[reflect(ignore)]`? +- Is a `trait ReflectProxy` more desirable than an `enum` and how to implement + it? +- Should `Dynamic***` inner values be `Box` or `Box`? + With `Box`: + * You must have a `TypeRegistry` and you must call `sidecast` both when + _building_ the `Dynamic***` and when converting from `ReflectProxy` to + `Reflect` (note that you most often already need the `TypeRegistry` when + building the `Dynamic***` if you want to access `TypeInfo`) + * The heavy lifting is mostly done when constructing the `ReflectProxy`, + `sidecast` is only a shallow conversion, calling `downcast` on the top + level data structure fields. + * There is less change to do if we keep it as it is today. + * With `Box`, the opposite is true. +- Should we try to implement the reflect subtraits for the `Dynamic***`? + (this would require removing the `Subtrait: Reflect` bounds) + +## Future possibilities + +We might be able to move `reflect_ref` and `reflect_mut` out of `Reflect` into +its own trait. (`Overlay`), make `Reflect: Overlay` and implement `Overlay` for +the `Dynamic***` types and `ReflectProxy`. `apply` could then accept a +`T: Overlay` instead of a `ReflectProxy`. From e45293ee12f8b6d6d1edc11bb0b9059f98ccf1c3 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Fri, 17 Jun 2022 21:33:41 +0200 Subject: [PATCH 2/7] Start drafting CanonReflect --- rfcs/56-better-reflect.md | 354 ++++++++++++++++++++++---------------- 1 file changed, 202 insertions(+), 152 deletions(-) diff --git a/rfcs/56-better-reflect.md b/rfcs/56-better-reflect.md index b708dec6..ebd9252b 100644 --- a/rfcs/56-better-reflect.md +++ b/rfcs/56-better-reflect.md @@ -1,25 +1,14 @@ -# bevy_reflect: Unique `Reflect` - -This builds on https://github.com/bevyengine/bevy/pull/4042 and the `TypeInfo` -API. +# bevy_reflect: `CanonReflect` as a unique `Reflect` ## Summary -Add a `ReflectProxy` enum that generalizes the `Dynamic***` structs from -`bevy_reflect` to all types, and remove `Reflect` implementation for -`Dynamic***` structs. - -I also suggest renaming all `Dynamic***` into `***Proxy` and the reflect -traits `clone_dynamic` methods to `proxy_clone`. +- Add a `CanonReflect: Reflect` trait that replaces `Reflect` +- Remove from `Reflect` all methods that assume uniqueness of underlying type. +- Add a `from_reflect` method to `Typed` that takes a `Box` and + returns a `Self` ## Lexicon/Terminology/Nomenclature/Jargon -#### pass for - -We say that `T: Reflect` **passes for** `U: Reflect` when `T`'s `type_name` is -equal to `U`'s, and it is possible to convert from a `T` to a `U` using -`apply`, `set` or `FromReflect::from_reflect`. - #### underlying The **underlying** value of a dynamic object (eg: `dyn Reflect`) is the @@ -39,39 +28,16 @@ struct, the underlying type of `reflect_clone` will be `DynamicStruct`: let reflect_clone: Box = reflect_a.clone_value(); ``` -#### Underlying-sensible method - -Those are methods of `Reflect` that act differently between `reflect_clone` and -`reflect_a` from the previous examples. - - -#### `Dynamic***` - -The structures holding generic structural information about some proxied type. -* `DynamicMap` -* `DynamicList` -* `DynamicTuple` -* `DynamicStruct` -* `DynamicTupleStruct` - -#### Reflect subtraits - -The top level traits in `bevy_reflect`: `Struct`, `TupleStruct`, `Tuple`, -`List` and `Map`. They are all subtraits of `Reflect`. - ## Motivation The `Reflect` trait is a trap, it doesn't work as expected due to `Dynamic***` mixing up with the "regular" types under the `dyn Reflect` trait object. -To solve this, we do not allow more than a single underlying type to "pass for" -a concrete type in `Reflect`. This implies that the various `Dynamic***` types -shouldn't implement `Reflect`. - When the underlying type of a `Box` is a `Dynamic***`, a lot of the `Reflect` methods become invalid, such as `any`, `reflect_hash`, -`reflect_partial_eq`, `serializable`, `downcast`, `is` or `take`. However, -those methods are provided regardless. +`reflect_partial_eq`, `serializable`, `downcast`, `is` or `take`. Yet, the user +can still call those methods, despite the fact that they are invalid in this +condition. ### What's under the `dyn Reflect`? @@ -86,7 +52,7 @@ let a = MysteryType::new(); // suppose MysteryType implements Reflect let reflect_a: Box = Box::new(a.clone()) as Box; let dynamic_a: Box = a.clone_value(); // This more or less always panic -assert_eq!((&*reflect_a).type_id(), (&*dynamic_a).type_id()) +assert!(dynamic_a.is::()) ``` This is because multiple different things can pretend to be the same thing. In @@ -97,7 +63,7 @@ to use the `Reflect` API correctly. ### `reflect_hash` and co. with `Dynamic***` -The problem is not limited to `type_id`, but also extends to the `reflect_*` +The problem is not limited to `is`, but also extends to the `reflect_*` methods. ```rust @@ -110,96 +76,186 @@ let dynamic_hash = dynamic_a.reflect_hash(); assert_eq!(reflect_hash, dynamic_hash) ``` +### Problem -## Proposition +The problem stems from some methods of `dyn Reflect` assuming that the +underlying type is always the same. -The problem stems from the duplicity of `dyn Reflect`. We want to rely on the -underlying methods of the same thing to be unique. +The following methods are the one that assumes uniqueness: +- `any` +- `any_mut` +- `downcast` +- `take` +- `is` +- `downcast_ref` +- `downcast_mut` +- `set` -To solve this, we do not allow more than a single underlying type to "pass for" -a concrete type in `Reflect`. This implies that the various `Dynamic***` types -shouldn't implement `Reflect`. +The `Any` trait bound on `Reflect` is similarly problematic, as it introduces +the same issues as those methods. -## User-facing explanation +Note that the following methods are also problematic, and require discussion, +but to keep to scope of this RFC short, we will keep them in `Reflect`. +- `reflect_hash` +- `reflect_partial_eq` +- `serializable` -* `Reflect` maps 1-to-1 with an underlying type meaning that if a - `Box` cannot be downcasted to `T`, you cannot build a `T` from - it. -* There is a `ReflectProxy` enum you can use to construct any `Reflect` type - based on some dynamic structure. This can be used, for example, for - deserialization of types that implement `Reflect` but not `Deserialize`. - * You can "clone reflectively" a `Box` by using - `Reflect::proxy_clone`. It returns a `ReflectProxy`. - * You can use `ReflectProxy::sidecast` to create a `Box` from - a `ReflectProxy`. The created trait object's underlying type is a concrete - type you can downcast to. -* `FromReflect` doesn't exist anymore. Instead, `Reflect::downcast` provides - now the same guarentees that `FromReflect::from_reflect` had. If you need to - get a `T` from a `ReflectProxy`, use a combination of - `ReflectProxy::sidecast_type` and `downcast`. +## Proposition +- Create a `CanonReflect` trait +- Remove all methods assuming uniqueness and `Any` trait bound from `Reflect`, + add them to `CanonReflect`. +- Remove `set` method from `Reflect` +- Implement a way to convert from `Reflect` to `CanonReflect`: + - New method to `Typed` trait: `from_reflect` + - Remove `FromReflect` trait + - New `constructor` field to `TypeRegistration` to convert a `Box` + into a `Box` dynamically. + - New `make_canon` method to `TypeRegistry` + +### `CanonReflect` trait + +We remove all methods methods assuming uniqueness and `Any` trait bound from +`Reflect` and only implement them on "canonical" types: ie: the ones implemented +by the `#[derive(Reflect)]` macros. + +We introduce a new trait `CanonReflect`, that must be explicitly implemented +to mark a type as being the canonical type. +```rust +pub trait CanonReflect: Reflect + Any { + fn any_mut(&mut self) -> &mut dyn Any { + // implementation + } + // etc. +} +``` -## Implementation strategy +This trait is automatically implemented by the `#[derive(Reflect)]` macro. -The `Reflect` API stays identical to the one we have today, at the exception of -`clone_value`. +This prevents any surprises caused by the underlying type of a reflect not being +the one expected. -We remove the `Dynamic***: Reflect` implementations and we introduce a -`ReflectProxy` enum. +Note that when [trait upcasting] is implemented, the `any_mut` and other +`Any`-related methods should be moved into a `impl dyn CanonReflect` block. -```rust -pub enum ReflectProxy { - Struct(DynamicStruct), - TupleStruct(DynamicTupleStruct), - Tuple(DynamicTuple), - List(DynamicList), - Map(DynamicMap), - AlreadyReflect(Box), -} -``` +### `Reflect` to `CanonReflect` conversion + +We still want to be able to use the `Any` methods on our `Reflect`, so we want +a way to get a `CanonReflect` from a `Reflect`. This is only possible by +checking if the underlying type is the one we expect OR converting into the +underlying type described by the `Reflect::type_name()` method. + +To do so, it's necessary to either provide an expected type or access the type +registry to invoke the constructor associated with teh `Reflect::type_name()`. + +To define a constructor We move the `from_reflect` method from `FromReflect` to `Typed`. For no +particular reasons but to reduce how many traits we export from `bevy_reflect`. -`ReflectProxy` mirrors the `ReflectRef` and `TypeInfo` enums. +**Danger**: Using `type_name` from `Reflect` is error-prone. For the conversion +to pick up the right type, we need `type_name()` to match exactly the one +returned by `std::any::type_name`. Ideally we have an alternative that defines +the underlying type without ambiguity, such as `target_type_id` or +`full_type_name`. -`ReflectProxy` has a method to convert the underlying value into the -value it is proxying. This allows you to get a `Reflect` from a `ReflectProxy` -while upholding the `Reflect` uniqueness rule. +We introduce a new field to `TypeRegistration`: `constructor`. It allows +converting from a `Reflect` to a `CanonReflect`, so that it's possible to +use its underlying-sensible methods. ```rust -impl ReflectProxy { - pub fn type_name(&self) -> &str { - match self { /* etc. */ } - } - pub fn can_sidecast(&self, registry: &TypeRegistry) -> Result<(), SidecastError> {} - pub fn sidecast(self, registry: &TypeRegistry) - -> Result, SidecastError> { - let type_name = self.type_name(); - let registration = registry.get_with_name(type_name).ok_or(SidecastError::Unregistered)?; - Ok(registration.constructor(self)?) - } - pub fn sidecast_type(self) -> Result, SidecastError> { - let registration = TypeRegistration::of::(); - Ok(registration.constructor(self)?) - } +trait Typed { + fn from_reflect(reflect: Box) -> Result>; } -``` - -`sidecast` requires introducing a new field to `TypeRegistration`: -```rust pub struct TypeRegistration { short_name: String, // new: constructor - constructor: fn(ReflectProxy) -> Result, ConstructorError>, + constructor: fn(Box) + -> Result, Box>, data: HashMap>, type_info: TypeInfo, } +``` + +We add a `make_canon` method to `TypeRegistry` and `TypeRegistration` to use +that new field. + +```rust impl TypeRegistration { - pub fn constructor(&self, proxy: ReflectProxy) -> Result, ConstructorError> { - (self.constructor)(proxy) - } - // ... + pub fn new() -> Self { + Self { + //... + constructor: T::from_reflect(from).map(|t| Box::new(t)), + } + } + pub fn make_canon(&self, reflect: Box) + -> Result, Box> + { + (self.constructor)(proxy) + } + // ... +} +impl TypeRegistry { + fn make_canon(&self, reflect: Box) + -> Result, Box> + { + if let Some(registration) = self.get_with_name(reflect.type_name()) { + registration.make_canon(reflect) + } else { + Err(reflect) + } + } +} +``` + +The `Reflect` trait now only has the following methods: +```rust +pub trait Reflect: Send + Sync { + fn type_name(&self) -> &str; + + fn as_reflect(&self) -> &dyn Reflect; + fn as_reflect_mut(&mut self) -> &mut dyn Reflect; + fn reflect_ref(&self) -> ReflectRef; + fn reflect_mut(&mut self) -> ReflectMut; + + fn apply(&mut self, value: &dyn Reflect); + // Change name of `clone_value` + // Ideally add documentation explaining that the underlying type changes. + fn clone_dynamically(&self) -> Box; + + fn debug(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {} + + // Questionable, but let's leave those for later. + fn reflect_hash(&self) -> Option {} + fn reflect_partial_eq(&self, _value: &dyn Reflect) -> Option {} + fn serializable(&self) -> Option {} +} +impl dyn Reflect { + pub fn make_canon(self: Box) + -> Result, Box> + { + T::from_reflect(from).map(|t| Box::new(t)) + } } ``` +Note that [trait upcasting] would allow us to remove the `as_reflect` and +`as_reflect_mut` methods. In fact, I don't think they are necessary at all. + +Note that the various `Dynamic***` types shouldn't implement `CanonReflect`, +only `Reflect`. + +## User-facing explanation + +- `CanonReflect` maps 1-to-1 with an underlying type meaning that if a + `Box` cannot be downcasted to `T`, you cannot build a `T` from + it. +- `FromReflect` doesn't exist anymore. Instead, `Reflect::downcast` provides + now the same guarantees that `FromReflect::from_reflect` had. If you need to + get a `T` from a `ReflectProxy`, use a combination of + `ReflectProxy::sidecast_type` and `downcast`. + + +`sidecast` requires introducing a new field to `TypeRegistration`: + `constructor` would simply construct the `T` and wrap it into a `Box` before returning it. @@ -210,52 +266,46 @@ structure. ## Drawbacks -- Users cannot define their own proxy type, they must use the `Dynamic***` - types. -- `Dynamic***` do not implement the `Reflect` subtraits anymore. -- There is some added complexity (although it fixes what I believe to be a - major limitation of the `bevy_reflect` API) -- `Reflect::apply` now only accepts a `ReflectProxy` (see next section) - -## Rationale and alternatives - -- Divide `Reflect` in two distinct traits, one with the underlying-sensible - method, and one with the other methods. (see next points) -- An earlier version of this RFC proposed `ReflectProxy` as a **trait**, - however, I reformulated it as an enum when considering how to implement it. - (How would you define the `TypeRegistration::constructor` field if passed a - `Box`? You'd need something like `ReflectProxyRef`?) - -I believe this change is needed, since `Reflect` duplicity is a large footgun. - -I don't believe that having `Dynamic***` implement `Reflect` makes sense, I -think it introduces much more issues than it solves. The only use-case for -`Dynamic***` being `Reflect` are the `reflect_mut`, `reflect_ref` and `apply` -methods. However, those could as well be implemented outside of the `Reflect` -trait, as mentioned in the "Future possibilities" section. +- Users must be aware of the difference between `CanonReflect` and `Reflect`, + and it must be explained. +- You cannot directly convert a `Reflect` into a `Any` or `T`, you must first + make sure it is conforms a canonical representation by using either + `TypeRegistry::make_canon` or `::make_canon(self: + Box)`. +- The addition of `constructor` to `Typed` will increase the size of generated + code. +- `constructor` makes `Typed` not object safe. But it should be fine + since `Typed` is not intended to be used with dynamic trait objects. ## Unresolved questions -- How does this interact with `#[reflect(ignore)]`? -- Is a `trait ReflectProxy` more desirable than an `enum` and how to implement - it? -- Should `Dynamic***` inner values be `Box` or `Box`? - With `Box`: - * You must have a `TypeRegistry` and you must call `sidecast` both when - _building_ the `Dynamic***` and when converting from `ReflectProxy` to - `Reflect` (note that you most often already need the `TypeRegistry` when - building the `Dynamic***` if you want to access `TypeInfo`) - * The heavy lifting is mostly done when constructing the `ReflectProxy`, - `sidecast` is only a shallow conversion, calling `downcast` on the top - level data structure fields. - * There is less change to do if we keep it as it is today. - * With `Box`, the opposite is true. -- Should we try to implement the reflect subtraits for the `Dynamic***`? - (this would require removing the `Subtrait: Reflect` bounds) +- `CanonReflect` name? My first instinct was `ReflectInstance`, but I changed it + to `ReflectType`, then `CanonicalReflect` and finally `CanonReflect` thinking + it was too long. + \ + Technically, `Canon` doesn't mean "normal representation", as "canonical" + does, but it's short and close enough to be understood. I thought about + using `canonize` instead of `make_canon`, but it felt too religious. ## Future possibilities -We might be able to move `reflect_ref` and `reflect_mut` out of `Reflect` into -its own trait. (`Overlay`), make `Reflect: Overlay` and implement `Overlay` for -the `Dynamic***` types and `ReflectProxy`. `apply` could then accept a -`T: Overlay` instead of a `ReflectProxy`. +- Add a `target_type(&self) -> TypeId` or `full_type_name` method to `Reflect` + such that it's easier to check if we are converting into the proper canonical + type. +- Performance: currently we force-convert from `CanonReflect` to `Reflect`, + then deeply traverse the data structure for all our reflect methods. It could + be greatly optimized if we add a way to tell if the underlying type is + already canon. (For example `make_canon` could be a no-op if the underlying + type is already the canonical form) + \ + This also interacts interestingly with `ReflectRef` and `ReflectMut`. Should + the `Value` variant hold a `dyn Reflect` or a `dyn CanonReflect`? +- `impl Reflect for T {}` and other subtraits rather than + `Struct: Reflect`. +- Redesign `reflect_hash` and `reflect_partial_eq`, since this RFC doesn't fix + the issue with them. +- Add a `reflect_owned` that returns a `Dynamic` equivalent of `ReflectRef` + (As an earlier version of this RFC called `ReflectProxy`) +- Make use of [trait upcasting]. + +[trait upcasting]: https://github.com/rust-lang/rust/issues/65991 From 757173fa0fa74dd8ae6c473a7214ef2c30312691 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Sat, 18 Jun 2022 10:31:39 +0200 Subject: [PATCH 3/7] Finish CanonReflect rework --- rfcs/56-better-reflect.md | 55 +++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/rfcs/56-better-reflect.md b/rfcs/56-better-reflect.md index ebd9252b..2cc31f5b 100644 --- a/rfcs/56-better-reflect.md +++ b/rfcs/56-better-reflect.md @@ -5,7 +5,7 @@ - Add a `CanonReflect: Reflect` trait that replaces `Reflect` - Remove from `Reflect` all methods that assume uniqueness of underlying type. - Add a `from_reflect` method to `Typed` that takes a `Box` and - returns a `Self` + returns a `Self` to convert a `Reflect` into a `CanonReflect`. ## Lexicon/Terminology/Nomenclature/Jargon @@ -115,7 +115,7 @@ but to keep to scope of this RFC short, we will keep them in `Reflect`. ### `CanonReflect` trait -We remove all methods methods assuming uniqueness and `Any` trait bound from +We remove all methods assuming uniqueness and `Any` trait bound from `Reflect` and only implement them on "canonical" types: ie: the ones implemented by the `#[derive(Reflect)]` macros. @@ -146,10 +146,11 @@ checking if the underlying type is the one we expect OR converting into the underlying type described by the `Reflect::type_name()` method. To do so, it's necessary to either provide an expected type or access the type -registry to invoke the constructor associated with teh `Reflect::type_name()`. +registry to invoke the constructor associated with the `Reflect::type_name()`. -To define a constructor We move the `from_reflect` method from `FromReflect` to `Typed`. For no -particular reasons but to reduce how many traits we export from `bevy_reflect`. +To define a constructor We move the `from_reflect` method from `FromReflect` to +`Typed`. For no particular reasons but to reduce how many traits we export from +`bevy_reflect`. **Danger**: Using `type_name` from `Reflect` is error-prone. For the conversion to pick up the right type, we need `type_name()` to match exactly the one @@ -167,11 +168,11 @@ trait Typed { } pub struct TypeRegistration { short_name: String, + data: HashMap>, + type_info: TypeInfo, // new: constructor constructor: fn(Box) -> Result, Box>, - data: HashMap>, - type_info: TypeInfo, } ``` @@ -189,7 +190,7 @@ impl TypeRegistration { pub fn make_canon(&self, reflect: Box) -> Result, Box> { - (self.constructor)(proxy) + (self.constructor)(reflect) } // ... } @@ -232,7 +233,7 @@ impl dyn Reflect { pub fn make_canon(self: Box) -> Result, Box> { - T::from_reflect(from).map(|t| Box::new(t)) + T::from_reflect(self).map(|t| Box::new(t)) } } ``` @@ -245,24 +246,16 @@ only `Reflect`. ## User-facing explanation -- `CanonReflect` maps 1-to-1 with an underlying type meaning that if a - `Box` cannot be downcasted to `T`, you cannot build a `T` from - it. -- `FromReflect` doesn't exist anymore. Instead, `Reflect::downcast` provides - now the same guarantees that `FromReflect::from_reflect` had. If you need to - get a `T` from a `ReflectProxy`, use a combination of - `ReflectProxy::sidecast_type` and `downcast`. - - -`sidecast` requires introducing a new field to `TypeRegistration`: - -`constructor` would simply construct the `T` and wrap it into a `Box` before -returning it. +`Reflect` let you navigate any type regardless independently of their shape +with the `reflect_ref` and `reflect_mut` methods. `CanonReflect` is a special +case of `Reflect` that also let you convert into a concrete type using the +`Any` methods. To get a `CanonReflect` from a `Reflect`, you should use any +of the `make_canon` methods on `dyn Reflect`, `TypeRegistry` or +`TypeRegistration`. -`constructor` can be derived similarly to `TypeInfo`. It will however -require additional consuming methods on `Dynamic***` structs in order to be -able to move their `Box` fields into the constructed data -structure. +Any type derived with `#[derive(Reflect)]` implements `CanonReflect`. The only +types that implements `Reflect` but not `CanonReflect` are "proxy" types such +as `DynamicStruct`, or any third party equivalent. ## Drawbacks @@ -275,7 +268,7 @@ structure. - The addition of `constructor` to `Typed` will increase the size of generated code. - `constructor` makes `Typed` not object safe. But it should be fine - since `Typed` is not intended to be used with dynamic trait objects. + since `Typed` is not intended to be used as a dynamic trait objects. ## Unresolved questions @@ -286,12 +279,18 @@ structure. Technically, `Canon` doesn't mean "normal representation", as "canonical" does, but it's short and close enough to be understood. I thought about using `canonize` instead of `make_canon`, but it felt too religious. +- An earlier version of this RFC had the exact opposite approach to fix the + uniqueness issue: Instead of removing all underlying-dependent methods from + `Reflect` and adding them to a new trait, we kept the underlying-dependent + methods but moved all the non-dependent methods to a new trait. Which one to + go with? ## Future possibilities - Add a `target_type(&self) -> TypeId` or `full_type_name` method to `Reflect` such that it's easier to check if we are converting into the proper canonical - type. + type. We might also benefit from a method that is capable of taking two + type paths and comparing them accounting for shortcuts. - Performance: currently we force-convert from `CanonReflect` to `Reflect`, then deeply traverse the data structure for all our reflect methods. It could be greatly optimized if we add a way to tell if the underlying type is From 6dc651972b5f92c95f35bc158527908d640bb6b9 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Sun, 19 Jun 2022 09:39:38 +0200 Subject: [PATCH 4/7] Replace `make_canon` with `into_full` This allows us to remove the hard-requirement on `FromReflect` for everything, and also makes the conversion from `Reflect` to `CanonReflect` less compute-intensive. --- rfcs/56-better-reflect.md | 129 ++++++++++---------------------------- 1 file changed, 32 insertions(+), 97 deletions(-) diff --git a/rfcs/56-better-reflect.md b/rfcs/56-better-reflect.md index 2cc31f5b..492b0918 100644 --- a/rfcs/56-better-reflect.md +++ b/rfcs/56-better-reflect.md @@ -4,8 +4,8 @@ - Add a `CanonReflect: Reflect` trait that replaces `Reflect` - Remove from `Reflect` all methods that assume uniqueness of underlying type. -- Add a `from_reflect` method to `Typed` that takes a `Box` and - returns a `Self` to convert a `Reflect` into a `CanonReflect`. +- Add a `into_full` method to `Reflect` to be able to convert a + `Box` into a `Box` when possible. ## Lexicon/Terminology/Nomenclature/Jargon @@ -105,13 +105,10 @@ but to keep to scope of this RFC short, we will keep them in `Reflect`. - Create a `CanonReflect` trait - Remove all methods assuming uniqueness and `Any` trait bound from `Reflect`, add them to `CanonReflect`. -- Remove `set` method from `Reflect` - Implement a way to convert from `Reflect` to `CanonReflect`: - - New method to `Typed` trait: `from_reflect` - - Remove `FromReflect` trait - - New `constructor` field to `TypeRegistration` to convert a `Box` - into a `Box` dynamically. - - New `make_canon` method to `TypeRegistry` + - Add a new method to `Reflect`: `into_full` that returns a `CanonReflect` + when the underlying type is canonical. + ### `CanonReflect` trait @@ -145,71 +142,23 @@ a way to get a `CanonReflect` from a `Reflect`. This is only possible by checking if the underlying type is the one we expect OR converting into the underlying type described by the `Reflect::type_name()` method. -To do so, it's necessary to either provide an expected type or access the type -registry to invoke the constructor associated with the `Reflect::type_name()`. - -To define a constructor We move the `from_reflect` method from `FromReflect` to -`Typed`. For no particular reasons but to reduce how many traits we export from -`bevy_reflect`. - -**Danger**: Using `type_name` from `Reflect` is error-prone. For the conversion -to pick up the right type, we need `type_name()` to match exactly the one -returned by `std::any::type_name`. Ideally we have an alternative that defines -the underlying type without ambiguity, such as `target_type_id` or -`full_type_name`. - -We introduce a new field to `TypeRegistration`: `constructor`. It allows -converting from a `Reflect` to a `CanonReflect`, so that it's possible to -use its underlying-sensible methods. - -```rust -trait Typed { - fn from_reflect(reflect: Box) -> Result>; -} -pub struct TypeRegistration { - short_name: String, - data: HashMap>, - type_info: TypeInfo, - // new: constructor - constructor: fn(Box) - -> Result, Box>, -} -``` - -We add a `make_canon` method to `TypeRegistry` and `TypeRegistration` to use -that new field. +To do so, we have two options: +1. Provide an expected type or access a constructor stored in type registry + and convert with `FromReflect` from any `Reflect` to the canonical + representation of a `Reflect`. +2. Add a `into_full(self: Box) -> Option>` method to + `Reflect` that returns `Some` only for canonical types. -```rust -impl TypeRegistration { - pub fn new() -> Self { - Self { - //... - constructor: T::from_reflect(from).map(|t| Box::new(t)), - } - } - pub fn make_canon(&self, reflect: Box) - -> Result, Box> - { - (self.constructor)(reflect) - } - // ... -} -impl TypeRegistry { - fn make_canon(&self, reflect: Box) - -> Result, Box> - { - if let Some(registration) = self.get_with_name(reflect.type_name()) { - registration.make_canon(reflect) - } else { - Err(reflect) - } - } -} -``` +(1) Is complex and requires a lot more design, so we'll stick with (2) for this +RFC. -The `Reflect` trait now only has the following methods: +The `Reflect` trait now has the following methods: ```rust pub trait Reflect: Send + Sync { + // new + fn as_full(&self) -> Option<&dyn CanonReflect>; + fn into_full(self: Box) -> Option>; + fn type_name(&self) -> &str; fn as_reflect(&self) -> &dyn Reflect; @@ -229,15 +178,12 @@ pub trait Reflect: Send + Sync { fn reflect_partial_eq(&self, _value: &dyn Reflect) -> Option {} fn serializable(&self) -> Option {} } -impl dyn Reflect { - pub fn make_canon(self: Box) - -> Result, Box> - { - T::from_reflect(self).map(|t| Box::new(t)) - } -} ``` +`into_full` will return `None` by default, but in the `#[derive(Reflect)]` +macro will return `Some(self)`. Converting from a proxy non-canonical `Reflect` +to a `CanonReflect` is currently impossible. + Note that [trait upcasting] would allow us to remove the `as_reflect` and `as_reflect_mut` methods. In fact, I don't think they are necessary at all. @@ -249,9 +195,8 @@ only `Reflect`. `Reflect` let you navigate any type regardless independently of their shape with the `reflect_ref` and `reflect_mut` methods. `CanonReflect` is a special case of `Reflect` that also let you convert into a concrete type using the -`Any` methods. To get a `CanonReflect` from a `Reflect`, you should use any -of the `make_canon` methods on `dyn Reflect`, `TypeRegistry` or -`TypeRegistration`. +`Any` methods. To get a `CanonReflect` from a `Reflect`, you should use +`Reflect::into_full`. Any type derived with `#[derive(Reflect)]` implements `CanonReflect`. The only types that implements `Reflect` but not `CanonReflect` are "proxy" types such @@ -261,14 +206,8 @@ as `DynamicStruct`, or any third party equivalent. - Users must be aware of the difference between `CanonReflect` and `Reflect`, and it must be explained. -- You cannot directly convert a `Reflect` into a `Any` or `T`, you must first - make sure it is conforms a canonical representation by using either - `TypeRegistry::make_canon` or `::make_canon(self: - Box)`. -- The addition of `constructor` to `Typed` will increase the size of generated - code. -- `constructor` makes `Typed` not object safe. But it should be fine - since `Typed` is not intended to be used as a dynamic trait objects. +- It is impossible to make a proxy `Reflect` into a `CanonReflect` without + knowing the underlying type. ## Unresolved questions @@ -277,8 +216,7 @@ as `DynamicStruct`, or any third party equivalent. it was too long. \ Technically, `Canon` doesn't mean "normal representation", as "canonical" - does, but it's short and close enough to be understood. I thought about - using `canonize` instead of `make_canon`, but it felt too religious. + does, but it's short and close enough to be understood. - An earlier version of this RFC had the exact opposite approach to fix the uniqueness issue: Instead of removing all underlying-dependent methods from `Reflect` and adding them to a new trait, we kept the underlying-dependent @@ -291,14 +229,8 @@ as `DynamicStruct`, or any third party equivalent. such that it's easier to check if we are converting into the proper canonical type. We might also benefit from a method that is capable of taking two type paths and comparing them accounting for shortcuts. -- Performance: currently we force-convert from `CanonReflect` to `Reflect`, - then deeply traverse the data structure for all our reflect methods. It could - be greatly optimized if we add a way to tell if the underlying type is - already canon. (For example `make_canon` could be a no-op if the underlying - type is already the canonical form) - \ - This also interacts interestingly with `ReflectRef` and `ReflectMut`. Should - the `Value` variant hold a `dyn Reflect` or a `dyn CanonReflect`? +- Could we modify the `ReflectRef` and `ReflectMut` enums to enable a sort of + "partial" evaluation of structures by adding a `&dyn CanonReflect` variant? - `impl Reflect for T {}` and other subtraits rather than `Struct: Reflect`. - Redesign `reflect_hash` and `reflect_partial_eq`, since this RFC doesn't fix @@ -306,5 +238,8 @@ as `DynamicStruct`, or any third party equivalent. - Add a `reflect_owned` that returns a `Dynamic` equivalent of `ReflectRef` (As an earlier version of this RFC called `ReflectProxy`) - Make use of [trait upcasting]. +- An earlier version of this RFC changed heavily the `FromReflect` trait to + make use of it in the construction of `Reflect` from `PartialReflect`, this + was only needed because `into_full` didn't exist. [trait upcasting]: https://github.com/rust-lang/rust/issues/65991 From ab3c1146ee0b1416b774ed50cf153c8da4145e8b Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Sun, 19 Jun 2022 11:30:43 +0200 Subject: [PATCH 5/7] Rename `CanonReflect` and reformulate the problem By community consensus, `CanonReflect` is now `Reflect` and "old" `Reflect` is now `PartialReflect`. Since we "keep" `Reflect`, I reformulate the change to conceptually fit with the current reference frame. I also explain in more details the concepts `Reflect` represent and make clear what the true underlying source of the awkwardness of `Reflect` is. --- rfcs/56-better-reflect.md | 303 +++++++++++++++++++++++--------------- 1 file changed, 188 insertions(+), 115 deletions(-) diff --git a/rfcs/56-better-reflect.md b/rfcs/56-better-reflect.md index 492b0918..20c412f7 100644 --- a/rfcs/56-better-reflect.md +++ b/rfcs/56-better-reflect.md @@ -1,13 +1,22 @@ -# bevy_reflect: `CanonReflect` as a unique `Reflect` +# bevy_reflect: `Reflect` as a unique `Reflect` ## Summary -- Add a `CanonReflect: Reflect` trait that replaces `Reflect` -- Remove from `Reflect` all methods that assume uniqueness of underlying type. -- Add a `into_full` method to `Reflect` to be able to convert a - `Box` into a `Box` when possible. +- create a new trait: `PartialReflect` +- `Reflect: PartialReflect` +- All methods on `Reflect` that **does not** depend on the uniqueness of the + underlying type are moved to `PartialReflect` +- Most things should now accept a `dyn PartialReflect` rather than + `dyn Reflect`. +- It is possible to convert a `PartialReflect` into a `Reflect` using a + `into_full` method. +- `FromReflect` becomes `FromPartialReflect`. -## Lexicon/Terminology/Nomenclature/Jargon +## Jargon + +For clarity, we will always call "new Reflect" the new version of Reflect, +with guaranteed uniqueness, and "old Reflect" the current implementation of +Reflect. #### underlying @@ -28,21 +37,43 @@ struct, the underlying type of `reflect_clone` will be `DynamicStruct`: let reflect_clone: Box = reflect_a.clone_value(); ``` +#### canonical type + +A `Box` has a _canonical underlying type_ when the +underlying type is the actual struct it is supposed to reflect. All types +derived with `#[derive(Reflect)]` are canonical types. All new `Reflect`s are +canonical types. + +#### proxies + +A `PartialReflect` (or old `Reflect`) implementor is said to be "proxy" when it +doesn't have a canonical representation. The `Dynamic***` types are all proxies. +Something that implements `PartialReflect` but not new `Reflect` is a proxy by +definition. + +#### Language from previous versions of this RFC + +This RFC went through a few name changes, in order to understand previous +discussion, here is a translation table: +* `CanonReflect`: What is now the new `Reflect` +* `ReflectProxy`: A version of what is now `PartialReflect`, but as a concrete + enum rather than a trait + ## Motivation -The `Reflect` trait is a trap, it doesn't work as expected due to `Dynamic***` +The old `Reflect` trait is a trap, it doesn't work as expected due to proxies mixing up with the "regular" types under the `dyn Reflect` trait object. -When the underlying type of a `Box` is a `Dynamic***`, a lot of -the `Reflect` methods become invalid, such as `any`, `reflect_hash`, +When the underlying type of an old `Box` is a proxy, a lot of +the old `Reflect` methods become invalid, such as `any`, `reflect_hash`, `reflect_partial_eq`, `serializable`, `downcast`, `is` or `take`. Yet, the user can still call those methods, despite the fact that they are invalid in this condition. -### What's under the `dyn Reflect`? +### What's under the old `dyn Reflect`? Currently, `Reflect` has very a confusing behavior. Most of the methods on -`Reflect` are highly dependent on the underlying type, regardless of whether +old `Reflect` are highly dependent on the underlying type, regardless of whether they are supposed to stand for the requested type or not. For example @@ -56,12 +87,12 @@ assert!(dynamic_a.is::()) ``` This is because multiple different things can pretend to be the same thing. In -itself this shouldn't be an issue, but the current `Reflect` API is leaky, and +itself this shouldn't be an issue, but the old `Reflect` API is leaky, and you need to be aware of both this limitation and the underlying type to be able -to use the `Reflect` API correctly. +to use the old `Reflect` API correctly. -### `reflect_hash` and co. with `Dynamic***` +### `reflect_hash` and co. with proxies The problem is not limited to `is`, but also extends to the `reflect_*` methods. @@ -78,7 +109,20 @@ assert_eq!(reflect_hash, dynamic_hash) ### Problem -The problem stems from some methods of `dyn Reflect` assuming that the +Reflect has multiple distinct roles that require different assumptions on the +underlying type: +1. A trait for structural exploration of data structures independent from the + data structure itself. Exposed by methods `reflect_ref`, `reflect_mut` and + `apply` +2. An extension to `Any` that allows generic construction from structural + descriptions (automatic deserialization) through registry. +3. "trait reflection" through registry. + +Role (1) and (2, 3) require different assumptions. As noted in the earlier +sections, `Any` requires assumptions on the underlying types, it is the case of +trait reflection as well. + +The problem stems from some methods of old `dyn Reflect` assuming that the underlying type is always the same. The following methods are the one that assumes uniqueness: @@ -91,155 +135,184 @@ The following methods are the one that assumes uniqueness: - `downcast_mut` - `set` -The `Any` trait bound on `Reflect` is similarly problematic, as it introduces +The `Any` trait bound on old `Reflect` is similarly problematic, as it introduces the same issues as those methods. Note that the following methods are also problematic, and require discussion, -but to keep to scope of this RFC short, we will keep them in `Reflect`. +but to keep to scope of this RFC short, we will keep them in `PartialReflect`. - `reflect_hash` - `reflect_partial_eq` - `serializable` ## Proposition -- Create a `CanonReflect` trait -- Remove all methods assuming uniqueness and `Any` trait bound from `Reflect`, - add them to `CanonReflect`. -- Implement a way to convert from `Reflect` to `CanonReflect`: - - Add a new method to `Reflect`: `into_full` that returns a `CanonReflect` - when the underlying type is canonical. +This RFC separates the bits of `Reflect` needed for (1) and the bits needed for +(2) and (3). + +We introduce `PartialReflect` to isolate the methods of `Reflect` for (1). This +also means that the old `Reflect` subtraits now are bound to `PartialReflect` +rather than `Reflect`. Proxy types such as `DynamicStruct` will also now only +implement `PartialReflect` instead of full `Reflect`, because they do not make +sense in the context of (2) and (3). + +This makes `Reflect` "canonical", in that the underlying type is the one being +described all the time. And now the `Any` bound and any-related methods are all +"safe" to use, because the assumptions requires to be able to use them are +backed into the type system. + +We still probably want a way to convert from a `PartialReflect` to a `Reflect`, +so we add a `into_full` method to `PartialReflect` to convert them into +`Reflect` in cases where the underlying type is canonical. + +The `FromReflect` trait will be renamed `FromPartialReflect`, a future +implementation might also add the ability to go from any _proxy_ partial +reflect into a full reflect using a combination of `FromPartialReflect` and the +type registry. + +In short: +- create a new trait: `PartialReflect` +- `Reflect: PartialReflect` +- All methods on `Reflect` that **does not** depend on the uniqueness of the + underlying type are moved to `PartialReflect` +- Most things should now accept a `dyn PartialReflect` rather than + `dyn Reflect`. +- It is possible to convert a `PartialReflect` into a `Reflect` using a + `into_full` method. +- `FromReflect` becomes `FromPartialReflect`. + + +### `PartialReflect` trait + +All methods that do not assume underlying uniqueness should be moved to a new +trait: `PartialReflect`. `PartialReflect` also has a `as_full` and `into_full` +to convert them into "full" `Reflect`. We also keep the "trait reflection" +methods, because changing the trait reflection design is complex and we want to +keep the scope of this RFC minimal. + +```rust +pub trait PartialReflect: Send + Sync { + // new + fn as_full(&self) -> Option<&dyn Reflect>; + fn into_full(self: Box) -> Option>; + + fn type_name(&self) -> &str; + fn as_partial_reflect(&self) -> &dyn PartialReflect; + fn as_partial_reflect_mut(&mut self) -> &mut dyn PartialReflect; + fn reflect_ref(&self) -> ReflectRef; + fn reflect_mut(&mut self) -> ReflectMut; + + fn apply(&mut self, value: &dyn PartialReflect); + // Change name of `clone_value` + // Ideally add documentation explaining that the underlying type changes. + fn clone_partial(&self) -> Box; + + fn debug(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {} + + // Questionable, but let's leave those for later. + fn reflect_hash(&self) -> Option {} + fn reflect_partial_eq(&self, _value: &dyn PartialReflect) -> Option {} + fn serializable(&self) -> Option {} +} +``` -### `CanonReflect` trait +### New `Reflect` trait -We remove all methods assuming uniqueness and `Any` trait bound from -`Reflect` and only implement them on "canonical" types: ie: the ones implemented -by the `#[derive(Reflect)]` macros. +`Reflect` now, instead of implementing those methods, has `PartialReflect` as +trait bound: -We introduce a new trait `CanonReflect`, that must be explicitly implemented -to mark a type as being the canonical type. ```rust -pub trait CanonReflect: Reflect + Any { - fn any_mut(&mut self) -> &mut dyn Any { - // implementation - } - // etc. +pub trait Reflect: PartialReflect + Any { + fn as_reflect(&self) -> &dyn Reflect; + fn as_reflect_mut(&mut self) -> &mut dyn Reflect; + + fn any_mut(&mut self) -> &mut dyn Any { + // implementation + } + // etc. + fn any + fn any_mut + fn downcast + fn take + fn is + fn downcast_ref + fn downcast_mut + fn set } ``` This trait is automatically implemented by the `#[derive(Reflect)]` macro. -This prevents any surprises caused by the underlying type of a reflect not being -the one expected. - Note that when [trait upcasting] is implemented, the `any_mut` and other -`Any`-related methods should be moved into a `impl dyn CanonReflect` block. +`Any`-related methods should be moved into a `impl dyn Reflect` block, +`as_reflect` and `as_partial_reflect` can also be dropped with trait +upcasting. -### `Reflect` to `CanonReflect` conversion +### `PartialReflect` to `Reflect` conversion -We still want to be able to use the `Any` methods on our `Reflect`, so we want -a way to get a `CanonReflect` from a `Reflect`. This is only possible by -checking if the underlying type is the one we expect OR converting into the -underlying type described by the `Reflect::type_name()` method. +We still want to be able to use the `Any` methods on our `PartialReflect`, so we +want a way to get a new `Reflect` from a `PartialReflect`. This is only possible +by checking if the underlying type is the one we expect OR converting into the +underlying type described by the `PartialReflect::type_name()` method. To do so, we have two options: 1. Provide an expected type or access a constructor stored in type registry and convert with `FromReflect` from any `Reflect` to the canonical representation of a `Reflect`. -2. Add a `into_full(self: Box) -> Option>` method to - `Reflect` that returns `Some` only for canonical types. +2. Add a `into_full(self: Box) -> Option>` method to + `PartialReflect` that returns `Some` only for canonical types. (1) Is complex and requires a lot more design, so we'll stick with (2) for this RFC. -The `Reflect` trait now has the following methods: -```rust -pub trait Reflect: Send + Sync { - // new - fn as_full(&self) -> Option<&dyn CanonReflect>; - fn into_full(self: Box) -> Option>; - - fn type_name(&self) -> &str; - - fn as_reflect(&self) -> &dyn Reflect; - fn as_reflect_mut(&mut self) -> &mut dyn Reflect; - fn reflect_ref(&self) -> ReflectRef; - fn reflect_mut(&mut self) -> ReflectMut; - - fn apply(&mut self, value: &dyn Reflect); - // Change name of `clone_value` - // Ideally add documentation explaining that the underlying type changes. - fn clone_dynamically(&self) -> Box; - - fn debug(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {} - - // Questionable, but let's leave those for later. - fn reflect_hash(&self) -> Option {} - fn reflect_partial_eq(&self, _value: &dyn Reflect) -> Option {} - fn serializable(&self) -> Option {} -} -``` - `into_full` will return `None` by default, but in the `#[derive(Reflect)]` -macro will return `Some(self)`. Converting from a proxy non-canonical `Reflect` -to a `CanonReflect` is currently impossible. - -Note that [trait upcasting] would allow us to remove the `as_reflect` and -`as_reflect_mut` methods. In fact, I don't think they are necessary at all. - -Note that the various `Dynamic***` types shouldn't implement `CanonReflect`, -only `Reflect`. +macro will return `Some(self)`. Converting from a proxy `PartialReflect` +to a `Reflect` is currently impossible. ## User-facing explanation -`Reflect` let you navigate any type regardless independently of their shape -with the `reflect_ref` and `reflect_mut` methods. `CanonReflect` is a special -case of `Reflect` that also let you convert into a concrete type using the -`Any` methods. To get a `CanonReflect` from a `Reflect`, you should use -`Reflect::into_full`. +`PartialReflect` let you navigate any type independently of their shape +with the `reflect_ref` and `reflect_mut` methods. `Reflect` is a special +case of `PartialReflect` that also let you convert into a concrete type using the +`Any` methods. To get a `Reflect` from a `PartialReflect`, you should use +`PartialReflect::into_full`. -Any type derived with `#[derive(Reflect)]` implements `CanonReflect`. The only -types that implements `Reflect` but not `CanonReflect` are "proxy" types such -as `DynamicStruct`, or any third party equivalent. +Any type derived with `#[derive(Reflect)]` implements both `Reflect` and +`PartialReflect`. The only types that implements `PartialReflect` but not +`Reflect` are "proxy" types such as `DynamicStruct`, or any third party +equivalent. ## Drawbacks -- Users must be aware of the difference between `CanonReflect` and `Reflect`, - and it must be explained. -- It is impossible to make a proxy `Reflect` into a `CanonReflect` without +- It is impossible to make a proxy `PartialReflect` into a `Reflect` without knowing the underlying type. +- This splits the reflect API, that used to be neatly uniform. It will be + more difficult to combine various use cases of `bevy_reflect`, requiring + explicit conversions. (Note that however, the previous state of things would + result in things getting silently broken) ## Unresolved questions -- `CanonReflect` name? My first instinct was `ReflectInstance`, but I changed it - to `ReflectType`, then `CanonicalReflect` and finally `CanonReflect` thinking - it was too long. - \ - Technically, `Canon` doesn't mean "normal representation", as "canonical" - does, but it's short and close enough to be understood. -- An earlier version of this RFC had the exact opposite approach to fix the - uniqueness issue: Instead of removing all underlying-dependent methods from - `Reflect` and adding them to a new trait, we kept the underlying-dependent - methods but moved all the non-dependent methods to a new trait. Which one to - go with? +- Should `Reflect` still be named `Reflect`, does it need to be a subtrait of + `PartialReflect`? +- Is the ability to convert from a proxy `PartialReflect` to `Reflect` a + hard-requirement, ie: blocker? (I think only a first attempt at implementation + can answer this) ## Future possibilities -- Add a `target_type(&self) -> TypeId` or `full_type_name` method to `Reflect` - such that it's easier to check if we are converting into the proper canonical - type. We might also benefit from a method that is capable of taking two - type paths and comparing them accounting for shortcuts. - Could we modify the `ReflectRef` and `ReflectMut` enums to enable a sort of - "partial" evaluation of structures by adding a `&dyn CanonReflect` variant? -- `impl Reflect for T {}` and other subtraits rather than - `Struct: Reflect`. -- Redesign `reflect_hash` and `reflect_partial_eq`, since this RFC doesn't fix - the issue with them. + "partial" evaluation of structures by adding a `&dyn Reflect` variant? +- `impl PartialReflect for T {}` and other subtraits rather than + `Struct: PartialReflect`. - Add a `reflect_owned` that returns a `Dynamic` equivalent of `ReflectRef` (As an earlier version of this RFC called `ReflectProxy`) - Make use of [trait upcasting]. -- An earlier version of this RFC changed heavily the `FromReflect` trait to - make use of it in the construction of `Reflect` from `PartialReflect`, this - was only needed because `into_full` didn't exist. +- An earlier version of this RFC combined the `FromReflect` trait with the + `TypeRegistry` to enable conversion from any proxy `PartialReflect` to + concrete `Reflect`, this method is probably still needed. +- (2) and (3) are still worth discriminating, this relates to the `reflect_hash` + and `reflect_partial_eq` methods, which are explicitly left "broken as before" + in this RFC. [trait upcasting]: https://github.com/rust-lang/rust/issues/65991 From cd5e84a8970d51520a982a5baf07753e3bcb6aa2 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Wed, 22 Jun 2022 19:12:27 +0200 Subject: [PATCH 6/7] Add back 'pass for' jargo entry --- rfcs/56-better-reflect.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/rfcs/56-better-reflect.md b/rfcs/56-better-reflect.md index 20c412f7..180b7ae1 100644 --- a/rfcs/56-better-reflect.md +++ b/rfcs/56-better-reflect.md @@ -18,6 +18,12 @@ For clarity, we will always call "new Reflect" the new version of Reflect, with guaranteed uniqueness, and "old Reflect" the current implementation of Reflect. +#### pass for + +We say that old `T: Reflect` **passes for** old `U: Reflect` when `T`'s +`type_name` is equal to `U`'s, and it is possible to convert from a `T` to a `U` +using `apply`, `set` or `FromReflect::from_reflect`. + #### underlying The **underlying** value of a dynamic object (eg: `dyn Reflect`) is the From bc7d6db96c999348f3a4a7dc89e9141f5725810e Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Thu, 19 Jan 2023 11:08:57 +0100 Subject: [PATCH 7/7] Integrate bjorn3 suggestions --- rfcs/56-better-reflect.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/56-better-reflect.md b/rfcs/56-better-reflect.md index 180b7ae1..38071256 100644 --- a/rfcs/56-better-reflect.md +++ b/rfcs/56-better-reflect.md @@ -8,6 +8,7 @@ underlying type are moved to `PartialReflect` - Most things should now accept a `dyn PartialReflect` rather than `dyn Reflect`. +- When possible, things should return a `dyn Reflect`. - It is possible to convert a `PartialReflect` into a `Reflect` using a `into_full` method. - `FromReflect` becomes `FromPartialReflect`. @@ -199,7 +200,7 @@ keep the scope of this RFC minimal. pub trait PartialReflect: Send + Sync { // new fn as_full(&self) -> Option<&dyn Reflect>; - fn into_full(self: Box) -> Option>; + fn into_full(self: Box) -> Result, Box>; fn type_name(&self) -> &str; @@ -237,7 +238,6 @@ pub trait Reflect: PartialReflect + Any { } // etc. fn any - fn any_mut fn downcast fn take fn is