Skip to content

Commit a4470e8

Browse files
MrGVSVsoqb
andauthored
bevy_reflect: Better proxies (#6971)
# 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 <[email protected]>
1 parent 6df65a2 commit a4470e8

22 files changed

+355
-374
lines changed

crates/bevy_reflect/bevy_reflect_derive/src/impls/enums.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,8 +192,8 @@ pub(crate) fn impl_enum(reflect_enum: &ReflectEnum) -> TokenStream {
192192
}
193193

194194
#[inline]
195-
fn get_type_info(&self) -> &'static #bevy_reflect_path::TypeInfo {
196-
<Self as #bevy_reflect_path::Typed>::type_info()
195+
fn get_represented_type_info(&self) -> #FQOption<&'static #bevy_reflect_path::TypeInfo> {
196+
#FQOption::Some(<Self as #bevy_reflect_path::Typed>::type_info())
197197
}
198198

199199
#[inline]

crates/bevy_reflect/bevy_reflect_derive/src/impls/structs.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ pub(crate) fn impl_struct(reflect_struct: &ReflectStruct) -> TokenStream {
151151

152152
fn clone_dynamic(&self) -> #bevy_reflect_path::DynamicStruct {
153153
let mut dynamic: #bevy_reflect_path::DynamicStruct = #FQDefault::default();
154-
dynamic.set_name(::std::string::ToString::to_string(#bevy_reflect_path::Reflect::type_name(self)));
154+
dynamic.set_represented_type(#bevy_reflect_path::Reflect::get_represented_type_info(self));
155155
#(dynamic.insert_boxed(#field_names, #bevy_reflect_path::Reflect::clone_value(&self.#field_idents));)*
156156
dynamic
157157
}
@@ -164,8 +164,8 @@ pub(crate) fn impl_struct(reflect_struct: &ReflectStruct) -> TokenStream {
164164
}
165165

166166
#[inline]
167-
fn get_type_info(&self) -> &'static #bevy_reflect_path::TypeInfo {
168-
<Self as #bevy_reflect_path::Typed>::type_info()
167+
fn get_represented_type_info(&self) -> #FQOption<&'static #bevy_reflect_path::TypeInfo> {
168+
#FQOption::Some(<Self as #bevy_reflect_path::Typed>::type_info())
169169
}
170170

171171
#[inline]

crates/bevy_reflect/bevy_reflect_derive/src/impls/tuple_structs.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ pub(crate) fn impl_tuple_struct(reflect_struct: &ReflectStruct) -> TokenStream {
122122

123123
fn clone_dynamic(&self) -> #bevy_reflect_path::DynamicTupleStruct {
124124
let mut dynamic: #bevy_reflect_path::DynamicTupleStruct = #FQDefault::default();
125-
dynamic.set_name(::std::string::ToString::to_string(#bevy_reflect_path::Reflect::type_name(self)));
125+
dynamic.set_represented_type(#bevy_reflect_path::Reflect::get_represented_type_info(self));
126126
#(dynamic.insert_boxed(#bevy_reflect_path::Reflect::clone_value(&self.#field_idents));)*
127127
dynamic
128128
}
@@ -135,8 +135,8 @@ pub(crate) fn impl_tuple_struct(reflect_struct: &ReflectStruct) -> TokenStream {
135135
}
136136

137137
#[inline]
138-
fn get_type_info(&self) -> &'static #bevy_reflect_path::TypeInfo {
139-
<Self as #bevy_reflect_path::Typed>::type_info()
138+
fn get_represented_type_info(&self) -> #FQOption<&'static #bevy_reflect_path::TypeInfo> {
139+
#FQOption::Some(<Self as #bevy_reflect_path::Typed>::type_info())
140140
}
141141

142142
#[inline]

crates/bevy_reflect/bevy_reflect_derive/src/impls/values.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ pub(crate) fn impl_value(meta: &ReflectMeta) -> TokenStream {
4949
}
5050

5151
#[inline]
52-
fn get_type_info(&self) -> &'static #bevy_reflect_path::TypeInfo {
53-
<Self as #bevy_reflect_path::Typed>::type_info()
52+
fn get_represented_type_info(&self) -> #FQOption<&'static #bevy_reflect_path::TypeInfo> {
53+
#FQOption::Some(<Self as #bevy_reflect_path::Typed>::type_info())
5454
}
5555

5656
#[inline]

crates/bevy_reflect/src/array.rs

Lines changed: 32 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,4 @@
1-
use crate::{
2-
utility::{reflect_hasher, NonGenericTypeInfoCell},
3-
DynamicInfo, Reflect, ReflectMut, ReflectOwned, ReflectRef, TypeInfo, Typed,
4-
};
1+
use crate::{utility::reflect_hasher, Reflect, ReflectMut, ReflectOwned, ReflectRef, TypeInfo};
52
use std::{
63
any::{Any, TypeId},
74
fmt::Debug,
@@ -67,7 +64,7 @@ pub trait Array: Reflect {
6764
/// Clones the list, producing a [`DynamicArray`].
6865
fn clone_dynamic(&self) -> DynamicArray {
6966
DynamicArray {
70-
name: self.type_name().to_string(),
67+
represented_type: self.get_represented_type_info(),
7168
values: self.iter().map(|value| value.clone_value()).collect(),
7269
}
7370
}
@@ -167,22 +164,22 @@ impl ArrayInfo {
167164
/// [`DynamicList`]: crate::DynamicList
168165
#[derive(Debug)]
169166
pub struct DynamicArray {
170-
pub(crate) name: String,
167+
pub(crate) represented_type: Option<&'static TypeInfo>,
171168
pub(crate) values: Box<[Box<dyn Reflect>]>,
172169
}
173170

174171
impl DynamicArray {
175172
#[inline]
176173
pub fn new(values: Box<[Box<dyn Reflect>]>) -> Self {
177174
Self {
178-
name: String::default(),
175+
represented_type: None,
179176
values,
180177
}
181178
}
182179

183180
pub fn from_vec<T: Reflect>(values: Vec<T>) -> Self {
184181
Self {
185-
name: String::default(),
182+
represented_type: None,
186183
values: values
187184
.into_iter()
188185
.map(|field| Box::new(field) as Box<dyn Reflect>)
@@ -191,26 +188,37 @@ impl DynamicArray {
191188
}
192189
}
193190

194-
#[inline]
195-
pub fn name(&self) -> &str {
196-
&self.name
197-
}
191+
/// Sets the [type] to be represented by this `DynamicArray`.
192+
///
193+
/// # Panics
194+
///
195+
/// Panics if the given [type] is not a [`TypeInfo::Array`].
196+
///
197+
/// [type]: TypeInfo
198+
pub fn set_represented_type(&mut self, represented_type: Option<&'static TypeInfo>) {
199+
if let Some(represented_type) = represented_type {
200+
assert!(
201+
matches!(represented_type, TypeInfo::Array(_)),
202+
"expected TypeInfo::Array but received: {:?}",
203+
represented_type
204+
);
205+
}
198206

199-
#[inline]
200-
pub fn set_name(&mut self, name: String) {
201-
self.name = name;
207+
self.represented_type = represented_type;
202208
}
203209
}
204210

205211
impl Reflect for DynamicArray {
206212
#[inline]
207213
fn type_name(&self) -> &str {
208-
self.name.as_str()
214+
self.represented_type
215+
.map(|info| info.type_name())
216+
.unwrap_or_else(|| std::any::type_name::<Self>())
209217
}
210218

211219
#[inline]
212-
fn get_type_info(&self) -> &'static TypeInfo {
213-
<Self as Typed>::type_info()
220+
fn get_represented_type_info(&self) -> Option<&'static TypeInfo> {
221+
self.represented_type
214222
}
215223

216224
#[inline]
@@ -281,6 +289,11 @@ impl Reflect for DynamicArray {
281289
fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option<bool> {
282290
array_partial_eq(self, value)
283291
}
292+
293+
#[inline]
294+
fn is_dynamic(&self) -> bool {
295+
true
296+
}
284297
}
285298

286299
impl Array for DynamicArray {
@@ -312,7 +325,7 @@ impl Array for DynamicArray {
312325
#[inline]
313326
fn clone_dynamic(&self) -> DynamicArray {
314327
DynamicArray {
315-
name: self.name.clone(),
328+
represented_type: self.represented_type,
316329
values: self
317330
.values
318331
.iter()
@@ -322,13 +335,6 @@ impl Array for DynamicArray {
322335
}
323336
}
324337

325-
impl Typed for DynamicArray {
326-
fn type_info() -> &'static TypeInfo {
327-
static CELL: NonGenericTypeInfoCell = NonGenericTypeInfoCell::new();
328-
CELL.get_or_set(|| TypeInfo::Dynamic(DynamicInfo::new::<Self>()))
329-
}
330-
}
331-
332338
/// An iterator over an [`Array`].
333339
pub struct ArrayIter<'a> {
334340
array: &'a dyn Array,

crates/bevy_reflect/src/enums/dynamic_enum.rs

Lines changed: 36 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
1-
use crate::utility::NonGenericTypeInfoCell;
21
use crate::{
3-
enum_debug, enum_hash, enum_partial_eq, DynamicInfo, DynamicStruct, DynamicTuple, Enum,
4-
Reflect, ReflectMut, ReflectOwned, ReflectRef, Struct, Tuple, TypeInfo, Typed,
5-
VariantFieldIter, VariantType,
2+
enum_debug, enum_hash, enum_partial_eq, DynamicStruct, DynamicTuple, Enum, Reflect, ReflectMut,
3+
ReflectOwned, ReflectRef, Struct, Tuple, TypeInfo, VariantFieldIter, VariantType,
64
};
75
use std::any::Any;
86
use std::fmt::Formatter;
@@ -58,7 +56,6 @@ impl From<()> for DynamicVariant {
5856
///
5957
/// // Create a DynamicEnum to represent the new value
6058
/// let mut dyn_enum = DynamicEnum::new(
61-
/// Reflect::type_name(&value),
6259
/// "None",
6360
/// DynamicVariant::Unit
6461
/// );
@@ -71,7 +68,7 @@ impl From<()> for DynamicVariant {
7168
/// ```
7269
#[derive(Default, Debug)]
7370
pub struct DynamicEnum {
74-
name: String,
71+
represented_type: Option<&'static TypeInfo>,
7572
variant_name: String,
7673
variant_index: usize,
7774
variant: DynamicVariant,
@@ -82,17 +79,12 @@ impl DynamicEnum {
8279
///
8380
/// # Arguments
8481
///
85-
/// * `name`: The type name of the enum
8682
/// * `variant_name`: The name of the variant to set
8783
/// * `variant`: The variant data
8884
///
89-
pub fn new<I: Into<String>, V: Into<DynamicVariant>>(
90-
name: I,
91-
variant_name: I,
92-
variant: V,
93-
) -> Self {
85+
pub fn new<I: Into<String>, V: Into<DynamicVariant>>(variant_name: I, variant: V) -> Self {
9486
Self {
95-
name: name.into(),
87+
represented_type: None,
9688
variant_index: 0,
9789
variant_name: variant_name.into(),
9890
variant: variant.into(),
@@ -103,33 +95,40 @@ impl DynamicEnum {
10395
///
10496
/// # Arguments
10597
///
106-
/// * `name`: The type name of the enum
10798
/// * `variant_index`: The index of the variant to set
10899
/// * `variant_name`: The name of the variant to set
109100
/// * `variant`: The variant data
110101
///
111102
pub fn new_with_index<I: Into<String>, V: Into<DynamicVariant>>(
112-
name: I,
113103
variant_index: usize,
114104
variant_name: I,
115105
variant: V,
116106
) -> Self {
117107
Self {
118-
name: name.into(),
108+
represented_type: None,
119109
variant_index,
120110
variant_name: variant_name.into(),
121111
variant: variant.into(),
122112
}
123113
}
124114

125-
/// Returns the type name of the enum.
126-
pub fn name(&self) -> &str {
127-
&self.name
128-
}
115+
/// Sets the [type] to be represented by this `DynamicEnum`.
116+
///
117+
/// # Panics
118+
///
119+
/// Panics if the given [type] is not a [`TypeInfo::Enum`].
120+
///
121+
/// [type]: TypeInfo
122+
pub fn set_represented_type(&mut self, represented_type: Option<&'static TypeInfo>) {
123+
if let Some(represented_type) = represented_type {
124+
assert!(
125+
matches!(represented_type, TypeInfo::Enum(_)),
126+
"expected TypeInfo::Enum but received: {:?}",
127+
represented_type
128+
);
129+
}
129130

130-
/// Sets the type name of the enum.
131-
pub fn set_name(&mut self, name: String) {
132-
self.name = name;
131+
self.represented_type = represented_type;
133132
}
134133

135134
/// Set the current enum variant represented by this struct.
@@ -142,11 +141,11 @@ impl DynamicEnum {
142141
pub fn set_variant_with_index<I: Into<String>, V: Into<DynamicVariant>>(
143142
&mut self,
144143
variant_index: usize,
145-
name: I,
144+
variant_name: I,
146145
variant: V,
147146
) {
148147
self.variant_index = variant_index;
149-
self.variant_name = name.into();
148+
self.variant_name = variant_name.into();
150149
self.variant = variant.into();
151150
}
152151

@@ -161,9 +160,9 @@ impl DynamicEnum {
161160
///
162161
/// This is functionally the same as [`DynamicEnum::from`] except it takes a reference.
163162
pub fn from_ref<TEnum: Enum>(value: &TEnum) -> Self {
164-
match value.variant_type() {
163+
let type_info = value.get_represented_type_info();
164+
let mut dyn_enum = match value.variant_type() {
165165
VariantType::Unit => DynamicEnum::new_with_index(
166-
value.type_name(),
167166
value.variant_index(),
168167
value.variant_name(),
169168
DynamicVariant::Unit,
@@ -174,7 +173,6 @@ impl DynamicEnum {
174173
data.insert_boxed(field.value().clone_value());
175174
}
176175
DynamicEnum::new_with_index(
177-
value.type_name(),
178176
value.variant_index(),
179177
value.variant_name(),
180178
DynamicVariant::Tuple(data),
@@ -187,13 +185,15 @@ impl DynamicEnum {
187185
data.insert_boxed(name, field.value().clone_value());
188186
}
189187
DynamicEnum::new_with_index(
190-
value.type_name(),
191188
value.variant_index(),
192189
value.variant_name(),
193190
DynamicVariant::Struct(data),
194191
)
195192
}
196-
}
193+
};
194+
195+
dyn_enum.set_represented_type(type_info);
196+
dyn_enum
197197
}
198198
}
199199

@@ -276,7 +276,7 @@ impl Enum for DynamicEnum {
276276

277277
fn clone_dynamic(&self) -> DynamicEnum {
278278
Self {
279-
name: self.name.clone(),
279+
represented_type: self.represented_type,
280280
variant_index: self.variant_index,
281281
variant_name: self.variant_name.clone(),
282282
variant: self.variant.clone(),
@@ -287,12 +287,14 @@ impl Enum for DynamicEnum {
287287
impl Reflect for DynamicEnum {
288288
#[inline]
289289
fn type_name(&self) -> &str {
290-
&self.name
290+
self.represented_type
291+
.map(|info| info.type_name())
292+
.unwrap_or_default()
291293
}
292294

293295
#[inline]
294-
fn get_type_info(&self) -> &'static TypeInfo {
295-
<Self as Typed>::type_info()
296+
fn get_represented_type_info(&self) -> Option<&'static TypeInfo> {
297+
self.represented_type
296298
}
297299

298300
#[inline]
@@ -418,10 +420,3 @@ impl Reflect for DynamicEnum {
418420
write!(f, ")")
419421
}
420422
}
421-
422-
impl Typed for DynamicEnum {
423-
fn type_info() -> &'static TypeInfo {
424-
static CELL: NonGenericTypeInfoCell = NonGenericTypeInfoCell::new();
425-
CELL.get_or_set(|| TypeInfo::Dynamic(DynamicInfo::new::<Self>()))
426-
}
427-
}

0 commit comments

Comments
 (0)