Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bevy_reflect: Unique Reflect #56

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nicopap
Copy link

@nicopap nicopap commented May 20, 2022

RENDERED

  • 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.

Unresolved questions

  • CanonReflect name? ⇒ Went with Reflect and PartialReflect
  • CanonReflect or ReflectProxy? Remove all underlying-dependent methods from
    Reflect and adding them to a new trait, or keep the underlying-dependent
    methods but moved all the non-dependent methods to a new trait. Which one to
    go with? ⇒ Went with a bit of both.
  • Implementation (partial: reflect: implement the unique reflect rfc bevy#7207)
  • Consider making more things Reflect.
  • Is a TypeRegistry entry for FromPartialReflect necessary? (no, as per partial implementation)

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great! I think you hit the nail on the head with one of reflection's biggest footguns right now: guarantees that a dyn Reflect is actually the type you think it is.

Using something like ReflectProxy, I think the API will just be a lot safer and easier to use. This involves a bit more work on the users' end (more passing around of the TypeRegistry), but overall I think it's pretty worth it.

Good work!

Comment on lines 206 to 209
`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<dyn Reflect>` fields into the constructed data
structure.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this work? For example, if I have the following code:

#[derive(Reflect)]
struct MyReflectStruct {
  foo: i32,

  #[reflect(ignore)]
  bar: MyNonReflectStruct,
}

Is it doing the same thing as FromReflect does currently where we require ignored fields be Default?

rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
Comment on lines 232 to 234
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`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that the actual biggest use-case for them is deserialization. Since we can't guarantee that all types implement FromReflect we're forced to return a Dynamic***.


## Unresolved questions

- How does this interact with `#[reflect(ignore)]`?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I gather, it really just encapsulates what FromReflect did. I imagine we'll have to do a similar thing. The tricky part is handling stuff beyond just Default, such as FromWorld.

rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
Comment on lines 125 to 127
* `Reflect` maps 1-to-1 with an underlying type meaning that if a
`Box<dyn Reflect>` cannot be downcasted to `T`, you cannot build a `T` from
it.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't that prevent Reflect from working for non-rust types like in case of scripting?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate. How do you use a non-rust type in rust? Wouldn't it need a conversion at one point?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea would be that you have for example a PyObject which is how any python object is internally represented and then it's Reflect implementation would present a different type depending on the type the PyObject has on the python side. For example for

class Foo(object):
    __slots__ = ('a', 'b')

class Bar(object):
    __slots__ = ('c', 'd')

a PyObject holding a Foo would present the fields a and b, while a PyObject holding a Bar would present the fields c and d and a PyObject holding an array would present itself as an array through Reflect.

Copy link
Member

@MrGVSV MrGVSV May 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this only exists on the Python side, right? What would this map to in Rust currently if there's no matching struct? A DynamicStruct?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need to map to a rust type I think. downcast could return None for any type you attempt to downcast to. type_name would give the python name. It shouldn't map to DynamicStruct as that would lose the python type.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work, but it would have the exact same issue as what the Dynamic* types have right now in that the downcasted type doesn't match the semantic type that determines the layout which .reflect() gives.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think I get the issue here. What about the proposition of moving reflect_ref and reflect_mut to their own trait independent from Reflect (call it the Bavy trait) and also move all the Reflect subtraits to Bavy? You could then implement Bavy for PyObject (or a wrapper type, per orphan rule). Maybe also move the sidecast proposition and implement Bavy for ReflectProxy?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather go the opposite way and move all methods except reflect_ref and reflect_mut into separate traits as not every type may have an implementation for them and reflect_ref and reflect_mut are the core reflection api.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjorn3 I think this is a good idea. Maybe I could revise this RFC to have a UniqueReflect vs Reflect?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would you want to reuse Reflect as a trait for this as opposed to writing specific code for Python? what's the use case? I thought Reflect as a trait is supposed to model how you can represent data in Rust, not in any language ever.

@nicopap nicopap marked this pull request as draft June 1, 2022 14:01
@colepoirier
Copy link

palliate

@nicopap could you clarify this by using a simpler word than “palliate”?

rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
- 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this adds complexity so much as points it out; which makes it easier to deal with that complexity by modelling it more accurately.

rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
@nicopap nicopap marked this pull request as ready for review June 18, 2022 08:31
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
Comment on lines 244 to 245
Note that the various `Dynamic***` types shouldn't implement `CanonReflect`,
only `Reflect`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fact should probably be a lot more prominent, since it's the basis for this RFC.

rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
@nicopap
Copy link
Author

nicopap commented Jun 19, 2022

Next revision:

  • CanonReflect, ReflectReflect, PartialReflect
  • add into_canon to PartialReflect
  • Remove FromReflect change (though it will be renamed FromPartialReflect)

This allows us to remove the hard-requirement on `FromReflect` for
everything, and also makes the conversion from `Reflect` to
`CanonReflect` less compute-intensive.
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.
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really liking the direction this is heading :D

rfcs/56-better-reflect.md Show resolved Hide resolved
Comment on lines +296 to +297
- Should `Reflect` still be named `Reflect`, does it need to be a subtrait of
`PartialReflect`?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it should definitely be a subtrait. Because then easily have access to all PartialReflect methods. Plus, it forces manual implementors to implement both, which might be necessary for certain reflect-based functionality (e.g. third-party crate assumptions).

rfcs/56-better-reflect.md Show resolved Hide resolved
rfcs/56-better-reflect.md Show resolved Hide resolved
@afonsolage
Copy link

Nice! Just wanted to note that another advantage of using PartialReflect is that it will be possible to optimize the serde format by not serializing default values.

This will be very useful for scenes and networking

Copy link

@jakobhellermann jakobhellermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a big fan of this RFC, it nicely separates these different concerns of Reflect and makes it much less error-prone to work with.

Comment on lines +169 to +171
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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I am sure, into_full on a proxy type will always return None, right?
It only returns Some if the dyn PartialReflect has an underlying type T: Reflect.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading futher confirms that this is true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that's the idea. To convert a proxy, you'll still need to use FromPartialReflect like you do now with FromReflect.

fn as_full(&self) -> Option<&dyn Reflect>;
fn into_full(self: Box<Self>) -> Option<Box<dyn Reflect>>;

fn type_name(&self) -> &str;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still want to tie PartialReflect to a type name?
I can think of a few situations where this isn't necessary/wanted.

  1. someone wants to bulk-edit a bunch of values, possible of different types. These types share fields a and b.
    The person can then create a DynamicStruct with the shared types and apply it to every value.
  2. implementing PartialReflect for non-rust values, e.g. a scripting language integration can implement PartialReflect for LuaValue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mentioned this on Discord, but essentially type_name still has a use for deserialization.

However, for Point 2 I think you could intentionally use a non-real type name and be fine. As long as you aren't hoping to deserialize it (unless you #[reflect(Deserialize)]), I think it should be okay.

rfcs/56-better-reflect.md Show resolved Hide resolved
Comment on lines +208 to +209
fn reflect_ref(&self) -> ReflectRef;
fn reflect_mut(&mut self) -> ReflectMut;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One piece of information that this restructuring loses is that reflect_ref on a &dyn Reflect will only contain &dyn Reflect itself, no &dyn PartialReflect.

So you may have to call into_full().unwrap() in a few places where you know the value originates from a &dyn Reflect.

let value: &dyn Reflect = &Time::default();
match value.reflect_ref() {
  ReflectRef::Struct(strukt) => {
    let field = strukt.field("last_update");
    // field.downcast() doesn't exist
    // infallible unwrap
    field.into_full().unwrap().downcast::<Instant>();
  },
  _ => unreachable!(),
}

The alternative is duplicating the ReflectRef, ReflectMut, Struct, List, Enum etc. for variants that preserve &dyn Reflect which is IMO not worth it.

Just pointing it out.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding this, bevyengine/bevy#7207 does store &dyn PartialReflect in proxies and leaves PartialReflect: Any. I think it will require a different vision on ReflectRef and proxies to complete the RFC and have a proper separation between proxy and canonical types.

rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
pub trait Reflect: PartialReflect + Any {
fn as_reflect(&self) -> &dyn Reflect;
fn as_reflect_mut(&mut self) -> &mut dyn Reflect;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs methods to turn it back into PartialReflect. At least until trait upcasting is stabilized.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As trait upcasting is not merged, it means I could give those methods the same name as the ones on PartialReflect?

Such as:

  • fn as_partial_reflect(&self) -> &dyn PartialReflect
  • fn as_partial_reflect_mut(&mut self) -> &mut dyn PartialReflect

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hang on, it might actually be possible to call those methods of PartialReflect on an Box<dyn Reflect> already.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is possible. bevyengine/bevy#7207 uses this.

rfcs/56-better-reflect.md Outdated Show resolved Hide resolved
github-merge-queue bot pushed a commit to bevyengine/bevy that referenced this pull request Apr 25, 2023
# Objective

> This PR is based on discussion from #6601

The Dynamic types (e.g. `DynamicStruct`, `DynamicList`, etc.) act as
both:
1. Dynamic containers which may hold any arbitrary data
2. Proxy types which may represent any other type

Currently, the only way we can represent the proxy-ness of a Dynamic is
by giving it a name.

```rust
// This is just a dynamic container
let mut data = DynamicStruct::default();

// This is a "proxy"
data.set_name(std::any::type_name::<Foo>());
```

This type name is the only way we check that the given Dynamic is a
proxy of some other type. When we need to "assert the type" of a `dyn
Reflect`, we call `Reflect::type_name` on it. However, because we're
only using a string to denote the type, we run into a few gotchas and
limitations.

For example, hashing a Dynamic proxy may work differently than the type
it proxies:

```rust
#[derive(Reflect, Hash)]
#[reflect(Hash)]
struct Foo(i32);

let concrete = Foo(123);
let dynamic = concrete.clone_dynamic();

let concrete_hash = concrete.reflect_hash();
let dynamic_hash = dynamic.reflect_hash();

// The hashes are not equal because `concrete` uses its own `Hash` impl
// while `dynamic` uses a reflection-based hashing algorithm
assert_ne!(concrete_hash, dynamic_hash);
```

Because the Dynamic proxy only knows about the name of the type, it's
unaware of any other information about it. This means it also differs on
`Reflect::reflect_partial_eq`, and may include ignored or skipped fields
in places the concrete type wouldn't.

## Solution

Rather than having Dynamics pass along just the type name of proxied
types, we can instead have them pass around the `TypeInfo`.

Now all Dynamic types contain an `Option<&'static TypeInfo>` rather than
a `String`:

```diff
pub struct DynamicTupleStruct {
-    type_name: String,
+    represented_type: Option<&'static TypeInfo>,
    fields: Vec<Box<dyn Reflect>>,
}
```

By changing `Reflect::get_type_info` to
`Reflect::represented_type_info`, hopefully we make this behavior a
little clearer. And to account for `None` values on these dynamic types,
`Reflect::represented_type_info` now returns `Option<&'static
TypeInfo>`.

```rust
let mut data = DynamicTupleStruct::default();

// Not proxying any specific type
assert!(dyn_tuple_struct.represented_type_info().is_none());

let type_info = <Foo as Typed>::type_info();
dyn_tuple_struct.set_represented_type(Some(type_info));
// Alternatively:
// let dyn_tuple_struct = foo.clone_dynamic();

// Now we're proxying `Foo`
assert!(dyn_tuple_struct.represented_type_info().is_some());
```

This means that we can have full access to all the static type
information for the proxied type. Future work would include
transitioning more static type information (trait impls, attributes,
etc.) over to the `TypeInfo` so it can actually be utilized by Dynamic
proxies.

### Alternatives & Rationale

> **Note** 
> These alternatives were written when this PR was first made using a
`Proxy` trait. This trait has since been removed.

<details>
<summary>View</summary>

#### Alternative: The `Proxy<T>` Approach

I had considered adding something like a `Proxy<T>` type where `T` would
be the Dynamic and would contain the proxied type information.

This was nice in that it allows us to explicitly determine whether
something is a proxy or not at a type level. `Proxy<DynamicStruct>`
proxies a struct. Makes sense.

The reason I didn't go with this approach is because (1) tuples, (2)
complexity, and (3) `PartialReflect`.

The `DynamicTuple` struct allows us to represent tuples at runtime. It
also allows us to do something you normally can't with tuples: add new
fields. Because of this, adding a field immediately invalidates the
proxy (e.g. our info for `(i32, i32)` doesn't apply to `(i32, i32,
NewField)`). By going with this PR's approach, we can just remove the
type info on `DynamicTuple` when that happens. However, with the
`Proxy<T>` approach, it becomes difficult to represent this behavior—
we'd have to completely control how we access data for `T` for each `T`.

Secondly, it introduces some added complexities (aside from the manual
impls for each `T`). Does `Proxy<T>` impl `Reflect`? Likely yes, if we
want to represent it as `dyn Reflect`. What `TypeInfo` do we give it?
How would we forward reflection methods to the inner type (remember, we
don't have specialization)? How do we separate this from Dynamic types?
And finally, how do all this in a way that's both logical and intuitive
for users?

Lastly, introducing a `Proxy` trait rather than a `Proxy<T>` struct is
actually more inline with the [Unique Reflect
RFC](bevyengine/rfcs#56). In a way, the `Proxy`
trait is really one part of the `PartialReflect` trait introduced in
that RFC (it's technically not in that RFC but it fits well with it),
where the `PartialReflect` serves as a way for proxies to work _like_
concrete types without having full access to everything a concrete
`Reflect` type can do. This would help bridge the gap between the
current state of the crate and the implementation of that RFC.

All that said, this is still a viable solution. If the community
believes this is the better path forward, then we can do that instead.
These were just my reasons for not initially going with it in this PR.

#### Alternative: The Type Registry Approach

The `Proxy` trait is great and all, but how does it solve the original
problem? Well, it doesn't— yet!

The goal would be to start moving information from the derive macro and
its attributes to the generated `TypeInfo` since these are known
statically and shouldn't change. For example, adding `ignored: bool` to
`[Un]NamedField` or a list of impls.

However, there is another way of storing this information. This is, of
course, one of the uses of the `TypeRegistry`. If we're worried about
Dynamic proxies not aligning with their concrete counterparts, we could
move more type information to the registry and require its usage.

For example, we could replace `Reflect::reflect_hash(&self)` with
`Reflect::reflect_hash(&self, registry: &TypeRegistry)`.

That's not the _worst_ thing in the world, but it is an ergonomics loss.

Additionally, other attributes may have their own requirements, further
restricting what's possible without the registry. The `Reflect::apply`
method will require the registry as well now. Why? Well because the
`map_apply` function used for the `Reflect::apply` impls on `Map` types
depends on `Map::insert_boxed`, which (at least for `DynamicMap`)
requires `Reflect::reflect_hash`. The same would apply when adding
support for reflection-based diffing, which will require
`Reflect::reflect_partial_eq`.

Again, this is a totally viable alternative. I just chose not to go with
it for the reasons above. If we want to go with it, then we can close
this PR and we can pursue this alternative instead.

#### Downsides

Just to highlight a quick potential downside (likely needs more
investigation): retrieving the `TypeInfo` requires acquiring a lock on
the `GenericTypeInfoCell` used by the `Typed` impls for generic types
(non-generic types use a `OnceBox which should be faster). I am not sure
how much of a performance hit that is and will need to run some
benchmarks to compare against.

</details>

### Open Questions

1. Should we use `Cow<'static, TypeInfo>` instead? I think that might be
easier for modding? Perhaps, in that case, we need to update
`Typed::type_info` and friends as well?
2. Are the alternatives better than the approach this PR takes? Are
there other alternatives?

---

## Changelog

### Changed

- `Reflect::get_type_info` has been renamed to
`Reflect::represented_type_info`
- This method now returns `Option<&'static TypeInfo>` rather than just
`&'static TypeInfo`

### Added

- Added `Reflect::is_dynamic` method to indicate when a type is dynamic
- Added a `set_represented_type` method on all dynamic types

### Removed

- Removed `TypeInfo::Dynamic` (use `Reflect::is_dynamic` instead)
- Removed `Typed` impls for all dynamic types

## Migration Guide

- The Dynamic types no longer take a string type name. Instead, they
require a static reference to `TypeInfo`:

    ```rust
    #[derive(Reflect)]
    struct MyTupleStruct(f32, f32);
    
    let mut dyn_tuple_struct = DynamicTupleStruct::default();
    dyn_tuple_struct.insert(1.23_f32);
    dyn_tuple_struct.insert(3.21_f32);
    
    // BEFORE:
    let type_name = std::any::type_name::<MyTupleStruct>();
    dyn_tuple_struct.set_name(type_name);
    
    // AFTER:
    let type_info = <MyTupleStruct as Typed>::type_info();
    dyn_tuple_struct.set_represented_type(Some(type_info));
    ```

- `Reflect::get_type_info` has been renamed to
`Reflect::represented_type_info` and now also returns an
`Option<&'static TypeInfo>` (instead of just `&'static TypeInfo`):

    ```rust
    // BEFORE:
    let info: &'static TypeInfo = value.get_type_info();
    // AFTER:
let info: &'static TypeInfo = value.represented_type_info().unwrap();
    ```

- `TypeInfo::Dynamic` and `DynamicInfo` has been removed. Use
`Reflect::is_dynamic` instead:
   
    ```rust
    // BEFORE:
    if matches!(value.get_type_info(), TypeInfo::Dynamic) {
      // ...
    }
    // AFTER:
    if value.is_dynamic() {
      // ...
    }
    ```

---------

Co-authored-by: radiish <cb.setho@gmail.com>
github-merge-queue bot pushed a commit to bevyengine/bevy that referenced this pull request Apr 25, 2023
# Objective

> This PR is based on discussion from #6601

The Dynamic types (e.g. `DynamicStruct`, `DynamicList`, etc.) act as
both:
1. Dynamic containers which may hold any arbitrary data
2. Proxy types which may represent any other type

Currently, the only way we can represent the proxy-ness of a Dynamic is
by giving it a name.

```rust
// This is just a dynamic container
let mut data = DynamicStruct::default();

// This is a "proxy"
data.set_name(std::any::type_name::<Foo>());
```

This type name is the only way we check that the given Dynamic is a
proxy of some other type. When we need to "assert the type" of a `dyn
Reflect`, we call `Reflect::type_name` on it. However, because we're
only using a string to denote the type, we run into a few gotchas and
limitations.

For example, hashing a Dynamic proxy may work differently than the type
it proxies:

```rust
#[derive(Reflect, Hash)]
#[reflect(Hash)]
struct Foo(i32);

let concrete = Foo(123);
let dynamic = concrete.clone_dynamic();

let concrete_hash = concrete.reflect_hash();
let dynamic_hash = dynamic.reflect_hash();

// The hashes are not equal because `concrete` uses its own `Hash` impl
// while `dynamic` uses a reflection-based hashing algorithm
assert_ne!(concrete_hash, dynamic_hash);
```

Because the Dynamic proxy only knows about the name of the type, it's
unaware of any other information about it. This means it also differs on
`Reflect::reflect_partial_eq`, and may include ignored or skipped fields
in places the concrete type wouldn't.

## Solution

Rather than having Dynamics pass along just the type name of proxied
types, we can instead have them pass around the `TypeInfo`.

Now all Dynamic types contain an `Option<&'static TypeInfo>` rather than
a `String`:

```diff
pub struct DynamicTupleStruct {
-    type_name: String,
+    represented_type: Option<&'static TypeInfo>,
    fields: Vec<Box<dyn Reflect>>,
}
```

By changing `Reflect::get_type_info` to
`Reflect::represented_type_info`, hopefully we make this behavior a
little clearer. And to account for `None` values on these dynamic types,
`Reflect::represented_type_info` now returns `Option<&'static
TypeInfo>`.

```rust
let mut data = DynamicTupleStruct::default();

// Not proxying any specific type
assert!(dyn_tuple_struct.represented_type_info().is_none());

let type_info = <Foo as Typed>::type_info();
dyn_tuple_struct.set_represented_type(Some(type_info));
// Alternatively:
// let dyn_tuple_struct = foo.clone_dynamic();

// Now we're proxying `Foo`
assert!(dyn_tuple_struct.represented_type_info().is_some());
```

This means that we can have full access to all the static type
information for the proxied type. Future work would include
transitioning more static type information (trait impls, attributes,
etc.) over to the `TypeInfo` so it can actually be utilized by Dynamic
proxies.

### Alternatives & Rationale

> **Note** 
> These alternatives were written when this PR was first made using a
`Proxy` trait. This trait has since been removed.

<details>
<summary>View</summary>

#### Alternative: The `Proxy<T>` Approach

I had considered adding something like a `Proxy<T>` type where `T` would
be the Dynamic and would contain the proxied type information.

This was nice in that it allows us to explicitly determine whether
something is a proxy or not at a type level. `Proxy<DynamicStruct>`
proxies a struct. Makes sense.

The reason I didn't go with this approach is because (1) tuples, (2)
complexity, and (3) `PartialReflect`.

The `DynamicTuple` struct allows us to represent tuples at runtime. It
also allows us to do something you normally can't with tuples: add new
fields. Because of this, adding a field immediately invalidates the
proxy (e.g. our info for `(i32, i32)` doesn't apply to `(i32, i32,
NewField)`). By going with this PR's approach, we can just remove the
type info on `DynamicTuple` when that happens. However, with the
`Proxy<T>` approach, it becomes difficult to represent this behavior—
we'd have to completely control how we access data for `T` for each `T`.

Secondly, it introduces some added complexities (aside from the manual
impls for each `T`). Does `Proxy<T>` impl `Reflect`? Likely yes, if we
want to represent it as `dyn Reflect`. What `TypeInfo` do we give it?
How would we forward reflection methods to the inner type (remember, we
don't have specialization)? How do we separate this from Dynamic types?
And finally, how do all this in a way that's both logical and intuitive
for users?

Lastly, introducing a `Proxy` trait rather than a `Proxy<T>` struct is
actually more inline with the [Unique Reflect
RFC](bevyengine/rfcs#56). In a way, the `Proxy`
trait is really one part of the `PartialReflect` trait introduced in
that RFC (it's technically not in that RFC but it fits well with it),
where the `PartialReflect` serves as a way for proxies to work _like_
concrete types without having full access to everything a concrete
`Reflect` type can do. This would help bridge the gap between the
current state of the crate and the implementation of that RFC.

All that said, this is still a viable solution. If the community
believes this is the better path forward, then we can do that instead.
These were just my reasons for not initially going with it in this PR.

#### Alternative: The Type Registry Approach

The `Proxy` trait is great and all, but how does it solve the original
problem? Well, it doesn't— yet!

The goal would be to start moving information from the derive macro and
its attributes to the generated `TypeInfo` since these are known
statically and shouldn't change. For example, adding `ignored: bool` to
`[Un]NamedField` or a list of impls.

However, there is another way of storing this information. This is, of
course, one of the uses of the `TypeRegistry`. If we're worried about
Dynamic proxies not aligning with their concrete counterparts, we could
move more type information to the registry and require its usage.

For example, we could replace `Reflect::reflect_hash(&self)` with
`Reflect::reflect_hash(&self, registry: &TypeRegistry)`.

That's not the _worst_ thing in the world, but it is an ergonomics loss.

Additionally, other attributes may have their own requirements, further
restricting what's possible without the registry. The `Reflect::apply`
method will require the registry as well now. Why? Well because the
`map_apply` function used for the `Reflect::apply` impls on `Map` types
depends on `Map::insert_boxed`, which (at least for `DynamicMap`)
requires `Reflect::reflect_hash`. The same would apply when adding
support for reflection-based diffing, which will require
`Reflect::reflect_partial_eq`.

Again, this is a totally viable alternative. I just chose not to go with
it for the reasons above. If we want to go with it, then we can close
this PR and we can pursue this alternative instead.

#### Downsides

Just to highlight a quick potential downside (likely needs more
investigation): retrieving the `TypeInfo` requires acquiring a lock on
the `GenericTypeInfoCell` used by the `Typed` impls for generic types
(non-generic types use a `OnceBox which should be faster). I am not sure
how much of a performance hit that is and will need to run some
benchmarks to compare against.

</details>

### Open Questions

1. Should we use `Cow<'static, TypeInfo>` instead? I think that might be
easier for modding? Perhaps, in that case, we need to update
`Typed::type_info` and friends as well?
2. Are the alternatives better than the approach this PR takes? Are
there other alternatives?

---

## Changelog

### Changed

- `Reflect::get_type_info` has been renamed to
`Reflect::represented_type_info`
- This method now returns `Option<&'static TypeInfo>` rather than just
`&'static TypeInfo`

### Added

- Added `Reflect::is_dynamic` method to indicate when a type is dynamic
- Added a `set_represented_type` method on all dynamic types

### Removed

- Removed `TypeInfo::Dynamic` (use `Reflect::is_dynamic` instead)
- Removed `Typed` impls for all dynamic types

## Migration Guide

- The Dynamic types no longer take a string type name. Instead, they
require a static reference to `TypeInfo`:

    ```rust
    #[derive(Reflect)]
    struct MyTupleStruct(f32, f32);
    
    let mut dyn_tuple_struct = DynamicTupleStruct::default();
    dyn_tuple_struct.insert(1.23_f32);
    dyn_tuple_struct.insert(3.21_f32);
    
    // BEFORE:
    let type_name = std::any::type_name::<MyTupleStruct>();
    dyn_tuple_struct.set_name(type_name);
    
    // AFTER:
    let type_info = <MyTupleStruct as Typed>::type_info();
    dyn_tuple_struct.set_represented_type(Some(type_info));
    ```

- `Reflect::get_type_info` has been renamed to
`Reflect::represented_type_info` and now also returns an
`Option<&'static TypeInfo>` (instead of just `&'static TypeInfo`):

    ```rust
    // BEFORE:
    let info: &'static TypeInfo = value.get_type_info();
    // AFTER:
let info: &'static TypeInfo = value.represented_type_info().unwrap();
    ```

- `TypeInfo::Dynamic` and `DynamicInfo` has been removed. Use
`Reflect::is_dynamic` instead:
   
    ```rust
    // BEFORE:
    if matches!(value.get_type_info(), TypeInfo::Dynamic) {
      // ...
    }
    // AFTER:
    if value.is_dynamic() {
      // ...
    }
    ```

---------

Co-authored-by: radiish <cb.setho@gmail.com>
github-merge-queue bot pushed a commit to bevyengine/bevy that referenced this pull request Apr 26, 2023
# Objective

> This PR is based on discussion from #6601

The Dynamic types (e.g. `DynamicStruct`, `DynamicList`, etc.) act as
both:
1. Dynamic containers which may hold any arbitrary data
2. Proxy types which may represent any other type

Currently, the only way we can represent the proxy-ness of a Dynamic is
by giving it a name.

```rust
// This is just a dynamic container
let mut data = DynamicStruct::default();

// This is a "proxy"
data.set_name(std::any::type_name::<Foo>());
```

This type name is the only way we check that the given Dynamic is a
proxy of some other type. When we need to "assert the type" of a `dyn
Reflect`, we call `Reflect::type_name` on it. However, because we're
only using a string to denote the type, we run into a few gotchas and
limitations.

For example, hashing a Dynamic proxy may work differently than the type
it proxies:

```rust
#[derive(Reflect, Hash)]
#[reflect(Hash)]
struct Foo(i32);

let concrete = Foo(123);
let dynamic = concrete.clone_dynamic();

let concrete_hash = concrete.reflect_hash();
let dynamic_hash = dynamic.reflect_hash();

// The hashes are not equal because `concrete` uses its own `Hash` impl
// while `dynamic` uses a reflection-based hashing algorithm
assert_ne!(concrete_hash, dynamic_hash);
```

Because the Dynamic proxy only knows about the name of the type, it's
unaware of any other information about it. This means it also differs on
`Reflect::reflect_partial_eq`, and may include ignored or skipped fields
in places the concrete type wouldn't.

## Solution

Rather than having Dynamics pass along just the type name of proxied
types, we can instead have them pass around the `TypeInfo`.

Now all Dynamic types contain an `Option<&'static TypeInfo>` rather than
a `String`:

```diff
pub struct DynamicTupleStruct {
-    type_name: String,
+    represented_type: Option<&'static TypeInfo>,
    fields: Vec<Box<dyn Reflect>>,
}
```

By changing `Reflect::get_type_info` to
`Reflect::represented_type_info`, hopefully we make this behavior a
little clearer. And to account for `None` values on these dynamic types,
`Reflect::represented_type_info` now returns `Option<&'static
TypeInfo>`.

```rust
let mut data = DynamicTupleStruct::default();

// Not proxying any specific type
assert!(dyn_tuple_struct.represented_type_info().is_none());

let type_info = <Foo as Typed>::type_info();
dyn_tuple_struct.set_represented_type(Some(type_info));
// Alternatively:
// let dyn_tuple_struct = foo.clone_dynamic();

// Now we're proxying `Foo`
assert!(dyn_tuple_struct.represented_type_info().is_some());
```

This means that we can have full access to all the static type
information for the proxied type. Future work would include
transitioning more static type information (trait impls, attributes,
etc.) over to the `TypeInfo` so it can actually be utilized by Dynamic
proxies.

### Alternatives & Rationale

> **Note** 
> These alternatives were written when this PR was first made using a
`Proxy` trait. This trait has since been removed.

<details>
<summary>View</summary>

#### Alternative: The `Proxy<T>` Approach

I had considered adding something like a `Proxy<T>` type where `T` would
be the Dynamic and would contain the proxied type information.

This was nice in that it allows us to explicitly determine whether
something is a proxy or not at a type level. `Proxy<DynamicStruct>`
proxies a struct. Makes sense.

The reason I didn't go with this approach is because (1) tuples, (2)
complexity, and (3) `PartialReflect`.

The `DynamicTuple` struct allows us to represent tuples at runtime. It
also allows us to do something you normally can't with tuples: add new
fields. Because of this, adding a field immediately invalidates the
proxy (e.g. our info for `(i32, i32)` doesn't apply to `(i32, i32,
NewField)`). By going with this PR's approach, we can just remove the
type info on `DynamicTuple` when that happens. However, with the
`Proxy<T>` approach, it becomes difficult to represent this behavior—
we'd have to completely control how we access data for `T` for each `T`.

Secondly, it introduces some added complexities (aside from the manual
impls for each `T`). Does `Proxy<T>` impl `Reflect`? Likely yes, if we
want to represent it as `dyn Reflect`. What `TypeInfo` do we give it?
How would we forward reflection methods to the inner type (remember, we
don't have specialization)? How do we separate this from Dynamic types?
And finally, how do all this in a way that's both logical and intuitive
for users?

Lastly, introducing a `Proxy` trait rather than a `Proxy<T>` struct is
actually more inline with the [Unique Reflect
RFC](bevyengine/rfcs#56). In a way, the `Proxy`
trait is really one part of the `PartialReflect` trait introduced in
that RFC (it's technically not in that RFC but it fits well with it),
where the `PartialReflect` serves as a way for proxies to work _like_
concrete types without having full access to everything a concrete
`Reflect` type can do. This would help bridge the gap between the
current state of the crate and the implementation of that RFC.

All that said, this is still a viable solution. If the community
believes this is the better path forward, then we can do that instead.
These were just my reasons for not initially going with it in this PR.

#### Alternative: The Type Registry Approach

The `Proxy` trait is great and all, but how does it solve the original
problem? Well, it doesn't— yet!

The goal would be to start moving information from the derive macro and
its attributes to the generated `TypeInfo` since these are known
statically and shouldn't change. For example, adding `ignored: bool` to
`[Un]NamedField` or a list of impls.

However, there is another way of storing this information. This is, of
course, one of the uses of the `TypeRegistry`. If we're worried about
Dynamic proxies not aligning with their concrete counterparts, we could
move more type information to the registry and require its usage.

For example, we could replace `Reflect::reflect_hash(&self)` with
`Reflect::reflect_hash(&self, registry: &TypeRegistry)`.

That's not the _worst_ thing in the world, but it is an ergonomics loss.

Additionally, other attributes may have their own requirements, further
restricting what's possible without the registry. The `Reflect::apply`
method will require the registry as well now. Why? Well because the
`map_apply` function used for the `Reflect::apply` impls on `Map` types
depends on `Map::insert_boxed`, which (at least for `DynamicMap`)
requires `Reflect::reflect_hash`. The same would apply when adding
support for reflection-based diffing, which will require
`Reflect::reflect_partial_eq`.

Again, this is a totally viable alternative. I just chose not to go with
it for the reasons above. If we want to go with it, then we can close
this PR and we can pursue this alternative instead.

#### Downsides

Just to highlight a quick potential downside (likely needs more
investigation): retrieving the `TypeInfo` requires acquiring a lock on
the `GenericTypeInfoCell` used by the `Typed` impls for generic types
(non-generic types use a `OnceBox which should be faster). I am not sure
how much of a performance hit that is and will need to run some
benchmarks to compare against.

</details>

### Open Questions

1. Should we use `Cow<'static, TypeInfo>` instead? I think that might be
easier for modding? Perhaps, in that case, we need to update
`Typed::type_info` and friends as well?
2. Are the alternatives better than the approach this PR takes? Are
there other alternatives?

---

## Changelog

### Changed

- `Reflect::get_type_info` has been renamed to
`Reflect::represented_type_info`
- This method now returns `Option<&'static TypeInfo>` rather than just
`&'static TypeInfo`

### Added

- Added `Reflect::is_dynamic` method to indicate when a type is dynamic
- Added a `set_represented_type` method on all dynamic types

### Removed

- Removed `TypeInfo::Dynamic` (use `Reflect::is_dynamic` instead)
- Removed `Typed` impls for all dynamic types

## Migration Guide

- The Dynamic types no longer take a string type name. Instead, they
require a static reference to `TypeInfo`:

    ```rust
    #[derive(Reflect)]
    struct MyTupleStruct(f32, f32);
    
    let mut dyn_tuple_struct = DynamicTupleStruct::default();
    dyn_tuple_struct.insert(1.23_f32);
    dyn_tuple_struct.insert(3.21_f32);
    
    // BEFORE:
    let type_name = std::any::type_name::<MyTupleStruct>();
    dyn_tuple_struct.set_name(type_name);
    
    // AFTER:
    let type_info = <MyTupleStruct as Typed>::type_info();
    dyn_tuple_struct.set_represented_type(Some(type_info));
    ```

- `Reflect::get_type_info` has been renamed to
`Reflect::represented_type_info` and now also returns an
`Option<&'static TypeInfo>` (instead of just `&'static TypeInfo`):

    ```rust
    // BEFORE:
    let info: &'static TypeInfo = value.get_type_info();
    // AFTER:
let info: &'static TypeInfo = value.represented_type_info().unwrap();
    ```

- `TypeInfo::Dynamic` and `DynamicInfo` has been removed. Use
`Reflect::is_dynamic` instead:
   
    ```rust
    // BEFORE:
    if matches!(value.get_type_info(), TypeInfo::Dynamic) {
      // ...
    }
    // AFTER:
    if value.is_dynamic() {
      // ...
    }
    ```

---------

Co-authored-by: radiish <cb.setho@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

9 participants