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: Better proxies #6971

Merged
merged 11 commits into from
Apr 26, 2023

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Dec 16, 2022

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.

// 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:

#[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:

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

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.

View

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

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:

    #[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):

    // 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:

    // BEFORE:
    if matches!(value.get_type_info(), TypeInfo::Dynamic) {
      // ...
    }
    // AFTER:
    if value.is_dynamic() {
      // ...
    }

@MrGVSV MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Dec 16, 2022
@alice-i-cecile
Copy link
Member

@soqb can I get your review here?

@MrGVSV
Copy link
Member Author

MrGVSV commented Dec 26, 2022

@soqb can I get your review here?

Keep in mind I still need to run some benchmarks 😅

@soqb
Copy link
Contributor

soqb commented Jan 7, 2023

im still not convinced this is better than returning the proxied TypeInfo from get_type_info on Reflect and adding some sort of is_dynamic(&self) -> bool method.

this would avoid polluting the trait space with too many reflect-adjacent traits (limited to Reflect, the type traits Struct, List etc. and perhaps PartialReflect in the future) and would allow us to return more precise information if its available without the loss of current behavior.

frankly, imo, DynamicInfo is an API wart that doesn't really provide us with any great benefits. it feels a lot like ValueInfo with the added semantics that the type is dynamic.
it's especially annoying considering TypeInfo is not one-to-one with the variants of Reflect{Ref,Mut,Owned} (e.g. a ReflectRef::Struct is not necessarily a TypeInfo::Struct).

i like the idea of changing the signature to return Cow<'static, TypeInfo>, so that the dynamics can return runtime-information on their values (DynamicStruct::get_type_info returning Cow::Owned(TypeInfo::Struct(...))) although we should carefully monitor the performance implications.

i definitely won't block approving this PR on the "dynamic TypeInfo generation" behavior since it could be implemented before or after this PR, but i'm not convinced that the Proxy trait is the right API to expose.

@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 8, 2023

Adding Reflect::is_dynamic

im still not convinced this is better than returning the proxied TypeInfo from get_type_info on Reflect and adding some sort of is_dynamic(&self) -> bool method.

this would avoid polluting the trait space with too many reflect-adjacent traits (limited to Reflect, the type traits Struct, List etc. and perhaps PartialReflect in the future) and would allow us to return more precise information if its available without the loss of current behavior.

Hm, that's not a bad idea actually, Having gone back and forth on it a fair amount in the past, I think the issues presented in #6601 (and other discussions) show that it would be pretty beneficial to add a Reflect::is_dynamic method. It removes the need for Proxy and is probably more efficient (consumers don't need to then always get the TypeInfo, which can be expensive for non-Dynamic types where a read lock might be required).

In defense of Proxy, though, I don't think it's strictly "polluting the trait space" so to speak. It's a bit out of place right now, but I see it more as a precursor to PartialReflect. PartialReflect is meant for proxy types like the Dynamic structs. It's likely it will need to provide methods that allow those types to work alongside concrete types, with one possibly being something like Proxy::represents.

However, given the options of keeping Proxy or following your suggestion, I think I'm more in favor of the latter.

Removing DynamicInfo

frankly, imo, DynamicInfo is an API wart that doesn't really provide us with any great benefits. it feels a lot like ValueInfo with the added semantics that the type is dynamic.
it's especially annoying considering TypeInfo is not one-to-one with the variants of Reflect{Ref,Mut,Owned} (e.g. a ReflectRef::Struct is not necessarily a TypeInfo::Struct).

Personally, I'd love to remove DynamicInfo for the same reasons you mentioned. However, there are two points that need to be addressed:

  1. Reflect::get_type_info is covered, but what should Typed::type_info return?
  2. How can we statically know whether or not a type is dynamic? As mentioned in this comment, it can be useful to special case Dynamics for certain operations.

Point 1

For Point 1, I propose we make the Dynamics' Typed::type_info return ValueInfo. Doing this means that Reflect::get_type_info and Typed::type_info could possibly differ. This might be confusing and footgun-y for users.

So perhaps we could better differentiate the two while providing more context to the former, by renaming Reflect::get_type_info to something like Reflect::represented_type_info.

This new method name would signal to users that the returned TypeInfo may not exactly match the dyn Reflect it comes from, but is a representation of that value. And this allows us to differ from Typed::type_info without incurring that mental penalty.

(There may be better names than represented_type_info, but to me it's clear, allows for similar naming conventions (i.e. represented_type_path) in the future, and aligns with the existing <dyn Reflect>::represents method.)

Point 2

Whether or not we go with ValueInfo or StructInfo or whatever, we should probably add an is_dynamic field that can be accessed statically. In conjunction with Reflect::is_dynamic, we can effectively continue to special case Dynamics in certain use-cases (e.g. de/serialization).

Cow-ing TypeInfo

i like the idea of changing the signature to return Cow<'static, TypeInfo>, so that the dynamics can return runtime-information on their values (DynamicStruct::get_type_info returning Cow::Owned(TypeInfo::Struct(...))) although we should carefully monitor the performance implications.

Yeah this is the idea. I haven't done much modding for Bevy (or in general really haha), but I have a feeling it will be useful to modders. And would probably be better than forcing them to use globals or intern their data.

I wonder if that should be done in this PR or a separate one? 🤔

Final Thoughts

i definitely won't block approving this PR on the "dynamic TypeInfo generation" behavior since it could be implemented before or after this PR, but i'm not convinced that the Proxy trait is the right API to expose.

I don't think it could be done before this PR since the Dynamics don't contain the represented TypeInfo to return (unless you're referring to something else). Either way, it is starting to feel like the right move.

For now, I think I'll do the following (unless there are objections):

  • Remove Proxy and replace it with the Reflect::is_dynamic/Reflect::get_type_info suggestion
  • Rename Reflect::get_type_info to Reflect::represented_type_info
  • Remove DynamicInfo (using ValueInfo + ***Info::is_dynamic in its place)

And I'll try to do so in individual commits so that we can revert if we need to.

@soqb
Copy link
Contributor

soqb commented Jan 8, 2023

other options i can see for Typed::type_info() != Reflect::get_type_info:

  1. add is_dynamic to Typed (this doesn't feel like the right location though).
  2. change DynamicInfo to something along the lines of struct DynamicInfo(Box<TypeInfo>) (a thin wrapper around another TypeInfo
  3. simply remove the Typed impl for dynamics. this may seem like a hack, but i would argue that the Dynamic types by virtue of their dynamism are not "typed". (do we rely on Typed for dynamics anywhere?) i think this would allow us to keep the signature of Typed::type_info() to returning &'static TypeInfo.

@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 8, 2023

other options i can see for Typed::type_info() != Reflect::get_type_info:

  1. add is_dynamic to Typed (this doesn't feel like the right location though).
  2. change DynamicInfo to something along the lines of struct DynamicInfo(Box<TypeInfo>) (a thin wrapper around another TypeInfo
  3. simply remove the Typed impl for dynamics. this may seem like a hack, but i would argue that the Dynamic types by virtue of their dynamism are not "typed". (do we rely on Typed for dynamics anywhere?) i think this would allow us to keep the signature of Typed::type_info() to returning &'static TypeInfo.

I don't think 2 is feasible (again, due to the static-ness of Typed::type_info. 1 is okay but prevents us from accessing that information dynamically from the registry.

And 3 actually sounds pretty reasonable. We don't have GetTypeRegistration implemented for the Dynamics, why should we implement Typed? However, we'd no longer be able to access TypeInfo for the Dynamic dynamically for stuff like de/serialization.

Edit: Actually just looked at the code. Because we don't have GetTypeRegistration, I don't think we register any of the Dynamics. This means the checks in place in serde/de.rs aren't actually doing anything anyway 😅.

I’d like to go with 3 and remove Typed for Dynamics altogether, but how do we check if a type is dynamic statically in those cases?

@soqb
Copy link
Contributor

soqb commented Jan 8, 2023

but how do we check if a type is dynamic statically in those cases?

is there a good use case for this?

@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 8, 2023

but how do we check if a type is dynamic statically in those cases?

is there a good use case for this?

For example, preventing Dynamics from being serialized and deserialized as mentioned in this comment (serialization is actually preventable if we add Reflect::is_dynamic).

@soqb
Copy link
Contributor

soqb commented Jan 8, 2023

im confused as to what you mean.
given:

struct MyStruct {
    dynamic_struct: DynamicStruct,
}

why would you want to prevent (de)serialization?

{
    "my_crate::MyStruct": (
        dynamic_struct: (
            foo: 123,
        ),
    ),
}

seems perfectly deserializable.

@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 8, 2023

im confused as to what you mean.

given:

struct MyStruct {

    dynamic_struct: DynamicStruct,

}

why would you want to prevent (de)serialization?

{

    "my_crate::MyStruct": (

        dynamic_struct: (

            foo: 123,

        ),

    ),

}

seems perfectly deserializable.

In that example, what type does foo deserialize to? i32? u32? How can we ensure it stays consistent across builds? Dynamics don't contain the type information necessary to consistently deserialize data. So we either need to prohibit it or control how that data is structured for Dynamic types.

@soqb
Copy link
Contributor

soqb commented Jan 9, 2023

good point. i wonder if we should choose a canonical representation for each of serde's primitives e.g. all numbers are i64 all strings are String etc. and then FromReflect can convert between them (i64 -> u32).

for now i think adding an DynamicType or similar marker to the type registry will suffice.

@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 10, 2023

good point. i wonder if we should choose a canonical representation for each of serde's primitives e.g. all numbers are i64 all strings are String etc. and then FromReflect can convert between them (i64 -> u32).

Yeah that could be an option. It might be a little confusing or too implicit and would need proper documentation. I'm also not sure how it would work for non-primitive types. But I think that should be explored in a separate ticket.

for now i think adding an DynamicType or similar marker to the type registry will suffice.

Since the Dynamic types aren't themselves registered, I'm not sure how we'd store/access data like DynamicType.

Maybe we just make deserialization error out if there's no registration? If not that, we either need to add a new mechanism to store DynamicType or just add GetTypeRegistration impls and register the Dynamics in TypeRegistry::new.

@soqb
Copy link
Contributor

soqb commented Jan 10, 2023

Since the Dynamic types aren't themselves registered, I'm not sure how we'd store/access data like DynamicType.

if they aren't register how are they deserialized? surely they go through visit_tuple or similar which error if they can't find a registration.

or just add GetTypeRegistration impls and register the Dynamics in TypeRegistry::new.

this is not possible using TypeRegistration::of since it requires Typed which we're getting rid of for dynamics.

@soqb
Copy link
Contributor

soqb commented Jan 10, 2023

we currently cannot directly serialize a DynamicStruct for example:

#[derive(Reflect)]
struct ContainsDyn(DynamicStruct);

let mut registry = TypeRegistry::new();
registry.register::<ContainsDyn>();
let registration = registry.get(TypeId::of::<ContainsDyn>()).unwrap();

let contains_dyn = ContainsDyn(DynamicStruct::default());

let reflect_ser = ReflectSerializer::new(&contains_dyn, &registry);
let ron = ron::ser::to_string(&reflect_ser); // Err(Message("no registration found for dynamic type with name "))

Comment on lines 383 to 385
self.represented_type
.map(|info| info.type_name())
.unwrap_or_default()
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be something like:

Suggested change
self.represented_type
.map(|info| info.type_name())
.unwrap_or_default()
self.represented_type
.map(|info| info.type_name())
.unwrap_or_else(|| std::any::type_name::<Self>())

to aid with debugging and error messages? (likewise for other dynamics).

Copy link
Contributor

Choose a reason for hiding this comment

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

oops! i see that i accidentally approved somehow. ignore that 😅.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I love this solution, but I do agree it may be better than simply returning an empty string. I'll make the change!

@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 11, 2023

we currently cannot directly serialize a DynamicStruct for example:

Yeah, as I mentioned that code in the deserializer isn't doing anything because we don't (and can't) register those types. And with that being the one place we need to statically access whether a type is dynamic or not, we should be okay to remove DynamicInfo altogether!

@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 11, 2023

Here's another question: what does a Dynamic type's Reflect::represented_type_info return when it hasn't yet been given a TypeInfo (i.e. it's only partially constructed) or if it has no Rust type to represent (i.e. for types that exist only in scripts/mods)?

Should Reflect::represented_type_info return Option<&'static TypeInfo> instead of just &'static TypeInfo?

I don't think doing so will have any major consequences apart from needing to unwrap or guard against the Option. Any other thoughts on this?

@alice-i-cecile
Copy link
Member

I would absolutely use an Option here.

@soqb
Copy link
Contributor

soqb commented Jan 11, 2023

three options i can think of for opaques (for scripting):

  • None.
  • Some(TypeInfo::Value). already semantically represents an opaque Rust type. Values all have a TypeId though so we might want to consider something else.
  • a new Some(TypeInfo::{Opaque,External}). has some sort of non-static type identifier/name so it can be reflected but is not necessarily a Rust type.

@soqb
Copy link
Contributor

soqb commented Jan 11, 2023

can you provide an example of a partially initialised dynamic - this sounds like an antipattern so maybe it should be removed if Bevy has this capability.

@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 11, 2023

can you provide an example of a partially initialised dynamic - this sounds like an antipattern so maybe it should be removed if Bevy has this capability.

A good example is actually the reflection example. Besides being proxies, the Dynamics were also intended to be used to apply "patches" to other values (including other Dynamics).

We also partially construct them when deserializing reflected data, only finalizing them after all data is deserialized. And the deserialized data itself may be missing fields, especially if they're marked with #[reflect(default)], which is intended to allow types to be "partially constructed" until they get filled in using FromReflect.

I don't think this is behavior we want to remove. If we want to create separate Proxy*** types that are always valid type proxies, we can do that. But I’m not sure if that should be done in this PR (or maybe it should since this is about proxies haha). Otherwise, we should probably stick to the dual-responsibility types.

@soqb
Copy link
Contributor

soqb commented Jan 11, 2023

i wouldn't call those partially constructed - hence my confusion but i wouldn't say they had a defined represented type either; they are "partially representative". unless we want a new TypeInfo::Partial, which i'd advice against, i'd say they should return Cow::Owned(TypeInfo::***) from represented_type(); that behaviour isn't planned on being implemented in this PR so unless you're willing to put it in i think they should either return TypeInfo::Value or we should leave in TypeInfo::Dynamic until the second PR comes around. the churn from wrapping the return type in Option just to undo it soon after doesn't seem worth it. there may be other cases i haven't considered for returning None although i'm not sure it makes sense - surely DynamicInfo::Value is currently always applicable for any impl Reflect, granted there are some gotchas around TypeId but those are wider issues.

@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 12, 2023

wouldn't call those partially constructed - hence my confusion but i wouldn't say they had a defined represented type either; they are "partially representative".

Sorry, my terminology was a bit confusing 😅

i'd say they should return Cow::Owned(TypeInfo::***) from represented_type(); that behaviour isn't planned on being implemented in this PR so unless you're willing to put it in i think they should either return TypeInfo::Value or we should leave in TypeInfo::Dynamic until the second PR comes around. the churn from wrapping the return type in Option just to undo it soon after doesn't seem worth it. there may be other cases i haven't considered for returning None although i'm not sure it makes sense - surely DynamicInfo::Value is currently always applicable for any impl Reflect, granted there are some gotchas around TypeId but those are wider issues.

Yeah, I'd like to avoid doing the whole Cow<'static, TypeInfo> thing in this PR. But I'm not sure I see how that removes the need for making Reflect::represented_type_info return an Option. We still need to deal with the Dynamics that don't have a TypeInfo to return, right?

@soqb
Copy link
Contributor

soqb commented Jan 12, 2023

i'm not sure how a dynamic can not have a TypeInfo to return. surely DynamicStruct::default() should return a TypeInfo::Struct with no fields?

@MrGVSV
Copy link
Member Author

MrGVSV commented Jan 12, 2023

i'm not sure how a dynamic can not have a TypeInfo to return.

Again, this is in the case where a user does something like:

let mut dyn_struct = DynamicStruct::default();
dyn_struct.insert("foo", 123_u32);

let type_info = dyn_struct.represented_type_info();
assert_eq!(???, type_info.type_id());

surely DynamicStruct::default() should return a TypeInfo::Struct with no fields?

Hm, that's a possibility. The next question then would be: how do we handle the required data in StructInfo? For example, would StructInfo::type_name just be an empty string? What about StructInfo::type_id?

But I can see now why we would need Cow<'static, TypeInfo> in order for that idea to work in the first place.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 25, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 25, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 25, 2023
@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Apr 25, 2023
github-merge-queue bot pushed a commit 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>
@alice-i-cecile
Copy link
Member

@MrGVSV can you take a look at why this is failing to compile in CI and rebase if needed?

@MrGVSV
Copy link
Member Author

MrGVSV commented Apr 26, 2023

@alice-i-cecile should be rebased and ready to go now!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 26, 2023
Merged via the queue into bevyengine:main with commit 75130bd Apr 26, 2023
@MrGVSV MrGVSV deleted the reflect-better-proxies branch April 26, 2023 15:00
@Selene-Amanita Selene-Amanita added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 10, 2023
github-merge-queue bot pushed a commit that referenced this pull request Jul 15, 2024
# Objective

Right now, `TypeInfo` can be accessed directly from a type using either
`Typed::type_info` or `Reflect::get_represented_type_info`.

However, once that `TypeInfo` is accessed, any nested types must be
accessed via the `TypeRegistry`.

```rust
#[derive(Reflect)]
struct Foo {
  bar: usize
}

let registry = TypeRegistry::default();

let TypeInfo::Struct(type_info) = Foo::type_info() else {
  panic!("expected struct info");
};

let field = type_info.field("bar").unwrap();

let field_info = registry.get_type_info(field.type_id()).unwrap();
assert!(field_info.is::<usize>());;
```

## Solution

Enable nested types within a `TypeInfo` to be retrieved directly.

```rust
#[derive(Reflect)]
struct Foo {
  bar: usize
}

let TypeInfo::Struct(type_info) = Foo::type_info() else {
  panic!("expected struct info");
};

let field = type_info.field("bar").unwrap();

let field_info = field.type_info().unwrap();
assert!(field_info.is::<usize>());;
```

The particular implementation was chosen for two reasons.

Firstly, we can't just store `TypeInfo` inside another `TypeInfo`
directly. This is because some types are recursive and would result in a
deadlock when trying to create the `TypeInfo` (i.e. it has to create the
`TypeInfo` before it can use it, but it also needs the `TypeInfo` before
it can create it). Therefore, we must instead store the function so it
can be retrieved lazily.

I had considered also using a `OnceLock` or something to lazily cache
the info, but I figured we can look into optimizations later. The API
should remain the same with or without the `OnceLock`.

Secondly, a new wrapper trait had to be introduced: `MaybeTyped`. Like
`RegisterForReflection`, this trait is `#[doc(hidden)]` and only exists
so that we can properly handle dynamic type fields without requiring
them to implement `Typed`. We don't want dynamic types to implement
`Typed` due to the fact that it would make the return type
`Option<&'static TypeInfo>` for all types even though only the dynamic
types ever need to return `None` (see #6971 for details).

Users should never have to interact with this trait as it has a blanket
impl for all `Typed` types. And `Typed` is automatically implemented
when deriving `Reflect` (as it is required).

The one downside is we do need to return `Option<&'static TypeInfo>`
from all these new methods so that we can handle the dynamic cases. If
we didn't have to, we'd be able to get rid of the `Option` entirely. But
I think that's an okay tradeoff for this one part of the API, and keeps
the other APIs intact.

## Testing

This PR contains tests to verify everything works as expected. You can
test locally by running:

```
cargo test --package bevy_reflect
```

---

## Changelog

### Public Changes

- Added `ArrayInfo::item_info` method
- Added `NamedField::type_info` method
- Added `UnnamedField::type_info` method
- Added `ListInfo::item_info` method
- Added `MapInfo::key_info` method
- Added `MapInfo::value_info` method
- All active fields now have a `Typed` bound (remember that this is
automatically satisfied for all types that derive `Reflect`)

### Internal Changes

- Added `MaybeTyped` trait

## Migration Guide

All active fields for reflected types (including lists, maps, tuples,
etc.), must implement `Typed`. For the majority of users this won't have
any visible impact.

However, users implementing `Reflect` manually may need to update their
types to implement `Typed` if they weren't already.

Additionally, custom dynamic types will need to implement the new hidden
`MaybeTyped` trait.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants