Skip to content

Commit

Permalink
reflect: TypePath part 2 (bevyengine#8768)
Browse files Browse the repository at this point in the history
# Objective

- Followup to bevyengine#7184.
- ~Deprecate `TypeUuid` and remove its internal references.~ No longer
part of this PR.
- Use `TypePath` for the type registry, and (de)serialisation instead of
`std::any::type_name`.
- Allow accessing type path information behind proxies.

## Solution
- Introduce methods on `TypeInfo` and friends for dynamically querying
type path. These methods supersede the old `type_name` methods.
- Remove `Reflect::type_name` in favor of `DynamicTypePath::type_path`
and `TypeInfo::type_path_table`.
- Switch all uses of `std::any::type_name` in reflection, non-debugging
contexts to use `TypePath`.

---

## Changelog

- Added `TypePathTable` for dynamically accessing methods on `TypePath`
through `TypeInfo` and the type registry.
- Removed `type_name` from all `TypeInfo`-like structs.
- Added `type_path` and `type_path_table` methods to all `TypeInfo`-like
structs.
- Removed `Reflect::type_name` in favor of
`DynamicTypePath::reflect_type_path` and `TypeInfo::type_path`.
- Changed the signature of all `DynamicTypePath` methods to return
strings with a static lifetime.

## Migration Guide

- Rely on `TypePath` instead of `std::any::type_name` for all stability
guarantees and for use in all reflection contexts, this is used through
with one of the following APIs:
  - `TypePath::type_path` if you have a concrete type and not a value.
- `DynamicTypePath::reflect_type_path` if you have an `dyn Reflect`
value without a concrete type.
- `TypeInfo::type_path` for use through the registry or if you want to
work with the represented type of a `DynamicFoo`.
  
- Remove `type_name` from manual `Reflect` implementations.
- Use `type_path` and `type_path_table` in place of `type_name` on
`TypeInfo`-like structs.
- Use `get_with_type_path(_mut)` over `get_with_type_name(_mut)`.

## Note to reviewers

I think if anything we were a little overzealous in merging bevyengine#7184 and we
should take that extra care here.

In my mind, this is the "point of no return" for `TypePath` and while I
think we all agree on the design, we should carefully consider if the
finer details and current implementations are actually how we want them
moving forward.

For example [this incorrect `TypePath` implementation for
`String`](https://github.com/soqb/bevy/blob/3fea3c6c0b5719dfbd3d4230f5282ec80d82556a/crates/bevy_reflect/src/impls/std.rs#L90)
(note that `String` is in the default Rust prelude) snuck in completely
under the radar.
  • Loading branch information
soqb authored and Ray Redondo committed Jan 9, 2024
1 parent b2e8088 commit c4abc05
Show file tree
Hide file tree
Showing 36 changed files with 816 additions and 787 deletions.
2 changes: 1 addition & 1 deletion crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,7 @@ impl App {
/// See [`bevy_reflect::TypeRegistry::register_type_data`].
#[cfg(feature = "bevy_reflect")]
pub fn register_type_data<
T: bevy_reflect::Reflect + 'static,
T: bevy_reflect::Reflect + bevy_reflect::TypePath,
D: bevy_reflect::TypeData + bevy_reflect::FromType<T>,
>(
&mut self,
Expand Down
14 changes: 8 additions & 6 deletions crates/bevy_ecs/src/reflect/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ impl<B: Bundle + Reflect + FromWorld> FromType<B> for ReflectBundle {
.for_each(|field| insert_field::<B>(entity, field, registry)),
_ => panic!(
"expected bundle `{}` to be named struct or tuple",
std::any::type_name::<B>()
// FIXME: once we have unique reflect, use `TypePath`.
std::any::type_name::<B>(),
),
}
},
Expand All @@ -163,7 +164,8 @@ impl<B: Bundle + Reflect + FromWorld> FromType<B> for ReflectBundle {
.for_each(|field| apply_or_insert_field::<B>(entity, field, registry)),
_ => panic!(
"expected bundle `{}` to be named struct or tuple",
std::any::type_name::<B>()
// FIXME: once we have unique reflect, use `TypePath`.
std::any::type_name::<B>(),
),
}
},
Expand All @@ -188,14 +190,14 @@ fn insert_field<B: 'static>(
if world.components().get_id(TypeId::of::<B>()).is_some() {
panic!(
"no `ReflectComponent` registration found for `{}`",
field.type_name()
field.reflect_type_path(),
);
};
});

panic!(
"no `ReflectBundle` registration found for `{}`",
field.type_name()
field.reflect_type_path(),
)
}
}
Expand All @@ -214,14 +216,14 @@ fn apply_or_insert_field<B: 'static>(
if world.components().get_id(TypeId::of::<B>()).is_some() {
panic!(
"no `ReflectComponent` registration found for `{}`",
field.type_name()
field.reflect_type_path(),
);
};
});

panic!(
"no `ReflectBundle` registration found for `{}`",
field.type_name()
field.reflect_type_path(),
)
}
}
26 changes: 13 additions & 13 deletions crates/bevy_ecs/src/reflect/entity_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,10 @@ impl<'w, 's, 'a> ReflectCommandExt for EntityCommands<'w, 's, 'a> {
self
}

fn remove_reflect(&mut self, component_type_name: impl Into<Cow<'static, str>>) -> &mut Self {
fn remove_reflect(&mut self, component_type_path: impl Into<Cow<'static, str>>) -> &mut Self {
self.commands.add(RemoveReflect {
entity: self.entity,
component_type_name: component_type_name.into(),
component_type_path: component_type_path.into(),
});
self
}
Expand All @@ -189,15 +189,15 @@ fn insert_reflect(
type_registry: &TypeRegistry,
component: Box<dyn Reflect>,
) {
let type_info = component.type_name();
let type_info = component.reflect_type_path();
let Some(mut entity) = world.get_entity_mut(entity) else {
panic!("error[B0003]: Could not insert a reflected component (of type {}) for entity {entity:?} because it doesn't exist in this World.", component.type_name());
panic!("error[B0003]: Could not insert a reflected component (of type {}) for entity {entity:?} because it doesn't exist in this World.", component.reflect_type_path());
};
let Some(type_registration) = type_registry.get_with_name(type_info) else {
panic!("Could not get type registration (for component type {}) because it doesn't exist in the TypeRegistry.", component.type_name());
let Some(type_registration) = type_registry.get_with_type_path(type_info) else {
panic!("Could not get type registration (for component type {}) because it doesn't exist in the TypeRegistry.", component.reflect_type_path());
};
let Some(reflect_component) = type_registration.data::<ReflectComponent>() else {
panic!("Could not get ReflectComponent data (for component type {}) because it doesn't exist in this TypeRegistration.", component.type_name());
panic!("Could not get ReflectComponent data (for component type {}) because it doesn't exist in this TypeRegistration.", component.reflect_type_path());
};
reflect_component.insert(&mut entity, &*component);
}
Expand Down Expand Up @@ -246,12 +246,12 @@ fn remove_reflect(
world: &mut World,
entity: Entity,
type_registry: &TypeRegistry,
component_type_name: Cow<'static, str>,
component_type_path: Cow<'static, str>,
) {
let Some(mut entity) = world.get_entity_mut(entity) else {
return;
};
let Some(type_registration) = type_registry.get_with_name(&component_type_name) else {
let Some(type_registration) = type_registry.get_with_type_path(&component_type_path) else {
return;
};
let Some(reflect_component) = type_registration.data::<ReflectComponent>() else {
Expand All @@ -269,7 +269,7 @@ pub struct RemoveReflect {
pub entity: Entity,
/// The [`Component`](crate::component::Component) type name that will be used to remove a component
/// of the same type from the entity.
pub component_type_name: Cow<'static, str>,
pub component_type_path: Cow<'static, str>,
}

impl Command for RemoveReflect {
Expand All @@ -279,7 +279,7 @@ impl Command for RemoveReflect {
world,
self.entity,
&registry.read(),
self.component_type_name,
self.component_type_path,
);
}
}
Expand Down Expand Up @@ -413,7 +413,7 @@ mod tests {

commands
.entity(entity)
.remove_reflect(boxed_reflect_component_a.type_name().to_owned());
.remove_reflect(boxed_reflect_component_a.reflect_type_path().to_owned());
system_state.apply(&mut world);

assert_eq!(world.entity(entity).get::<ComponentA>(), None);
Expand Down Expand Up @@ -443,7 +443,7 @@ mod tests {
commands
.entity(entity)
.remove_reflect_with_registry::<TypeRegistryResource>(
boxed_reflect_component_a.type_name().to_owned(),
boxed_reflect_component_a.reflect_type_path().to_owned(),
);
system_state.apply(&mut world);

Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ pub(crate) enum ReflectDerive<'a> {
/// // traits
/// // |----------------------------------------|
/// #[reflect(PartialEq, Serialize, Deserialize, Default)]
/// // type_name generics
/// // type_path generics
/// // |-------------------||----------|
/// struct ThingThatImReflecting<T1, T2, T3> {/* ... */}
/// ```
pub(crate) struct ReflectMeta<'a> {
/// The registered traits for this type.
traits: ReflectTraits,
/// The name of this type.
/// The path to this type.
type_path: ReflectTypePath<'a>,
/// A cached instance of the path to the `bevy_reflect` crate.
bevy_reflect_path: Path,
Expand Down Expand Up @@ -389,7 +389,7 @@ impl<'a> ReflectMeta<'a> {
self.traits.from_reflect_attrs()
}

/// The name of this struct.
/// The path to this type.
pub fn type_path(&self) -> &ReflectTypePath<'a> {
&self.type_path
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ pub(crate) fn impl_enum(reflect_enum: &ReflectEnum) -> proc_macro2::TokenStream
if let #bevy_reflect_path::ReflectRef::Enum(#ref_value) = #bevy_reflect_path::Reflect::reflect_ref(#ref_value) {
match #bevy_reflect_path::Enum::variant_name(#ref_value) {
#(#variant_names => #fqoption::Some(#variant_constructors),)*
name => panic!("variant with name `{}` does not exist on enum `{}`", name, ::core::any::type_name::<Self>()),
name => panic!("variant with name `{}` does not exist on enum `{}`", name, <Self as #bevy_reflect_path::TypePath>::type_path()),
}
} else {
#FQOption::None
Expand Down
15 changes: 4 additions & 11 deletions crates/bevy_reflect/bevy_reflect_derive/src/impls/enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,20 +58,18 @@ pub(crate) fn impl_enum(reflect_enum: &ReflectEnum) -> proc_macro2::TokenStream
}
});

let string_name = enum_path.get_ident().unwrap().to_string();

#[cfg(feature = "documentation")]
let info_generator = {
let doc = reflect_enum.meta().doc();
quote! {
#bevy_reflect_path::EnumInfo::new::<Self>(#string_name, &variants).with_docs(#doc)
#bevy_reflect_path::EnumInfo::new::<Self>(&variants).with_docs(#doc)
}
};

#[cfg(not(feature = "documentation"))]
let info_generator = {
quote! {
#bevy_reflect_path::EnumInfo::new::<Self>(#string_name, &variants)
#bevy_reflect_path::EnumInfo::new::<Self>(&variants)
}
};

Expand Down Expand Up @@ -188,11 +186,6 @@ pub(crate) fn impl_enum(reflect_enum: &ReflectEnum) -> proc_macro2::TokenStream
}

impl #impl_generics #bevy_reflect_path::Reflect for #enum_path #ty_generics #where_reflect_clause {
#[inline]
fn type_name(&self) -> &str {
::core::any::type_name::<Self>()
}

#[inline]
fn get_represented_type_info(&self) -> #FQOption<&'static #bevy_reflect_path::TypeInfo> {
#FQOption::Some(<Self as #bevy_reflect_path::Typed>::type_info())
Expand Down Expand Up @@ -264,11 +257,11 @@ pub(crate) fn impl_enum(reflect_enum: &ReflectEnum) -> proc_macro2::TokenStream
#(#variant_names => {
*self = #variant_constructors
})*
name => panic!("variant with name `{}` does not exist on enum `{}`", name, ::core::any::type_name::<Self>()),
name => panic!("variant with name `{}` does not exist on enum `{}`", name, <Self as #bevy_reflect_path::TypePath>::type_path()),
}
}
} else {
panic!("`{}` is not an enum", #bevy_reflect_path::Reflect::type_name(#ref_value));
panic!("`{}` is not an enum", #bevy_reflect_path::DynamicTypePath::reflect_type_path(#ref_value));
}
}

Expand Down
11 changes: 2 additions & 9 deletions crates/bevy_reflect/bevy_reflect_derive/src/impls/structs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,20 +63,18 @@ pub(crate) fn impl_struct(reflect_struct: &ReflectStruct) -> proc_macro2::TokenS
}
};

let string_name = struct_path.get_ident().unwrap().to_string();

#[cfg(feature = "documentation")]
let info_generator = {
let doc = reflect_struct.meta().doc();
quote! {
#bevy_reflect_path::StructInfo::new::<Self>(#string_name, &fields).with_docs(#doc)
#bevy_reflect_path::StructInfo::new::<Self>(&fields).with_docs(#doc)
}
};

#[cfg(not(feature = "documentation"))]
let info_generator = {
quote! {
#bevy_reflect_path::StructInfo::new::<Self>(#string_name, &fields)
#bevy_reflect_path::StructInfo::new::<Self>(&fields)
}
};

Expand Down Expand Up @@ -163,11 +161,6 @@ pub(crate) fn impl_struct(reflect_struct: &ReflectStruct) -> proc_macro2::TokenS
}

impl #impl_generics #bevy_reflect_path::Reflect for #struct_path #ty_generics #where_reflect_clause {
#[inline]
fn type_name(&self) -> &str {
::core::any::type_name::<Self>()
}

#[inline]
fn get_represented_type_info(&self) -> #FQOption<&'static #bevy_reflect_path::TypeInfo> {
#FQOption::Some(<Self as #bevy_reflect_path::Typed>::type_info())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,20 +57,18 @@ pub(crate) fn impl_tuple_struct(reflect_struct: &ReflectStruct) -> proc_macro2::
}
};

let string_name = struct_path.get_ident().unwrap().to_string();

#[cfg(feature = "documentation")]
let info_generator = {
let doc = reflect_struct.meta().doc();
quote! {
#bevy_reflect_path::TupleStructInfo::new::<Self>(#string_name, &fields).with_docs(#doc)
#bevy_reflect_path::TupleStructInfo::new::<Self>(&fields).with_docs(#doc)
}
};

#[cfg(not(feature = "documentation"))]
let info_generator = {
quote! {
#bevy_reflect_path::TupleStructInfo::new::<Self>(#string_name, &fields)
#bevy_reflect_path::TupleStructInfo::new::<Self>(&fields)
}
};

Expand Down Expand Up @@ -133,11 +131,6 @@ pub(crate) fn impl_tuple_struct(reflect_struct: &ReflectStruct) -> proc_macro2::
}

impl #impl_generics #bevy_reflect_path::Reflect for #struct_path #ty_generics #where_reflect_clause {
#[inline]
fn type_name(&self) -> &str {
::core::any::type_name::<Self>()
}

#[inline]
fn get_represented_type_info(&self) -> #FQOption<&'static #bevy_reflect_path::TypeInfo> {
#FQOption::Some(<Self as #bevy_reflect_path::Typed>::type_info())
Expand Down
7 changes: 1 addition & 6 deletions crates/bevy_reflect/bevy_reflect_derive/src/impls/values.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,6 @@ pub(crate) fn impl_value(meta: &ReflectMeta) -> proc_macro2::TokenStream {
#typed_impl

impl #impl_generics #bevy_reflect_path::Reflect for #type_path #ty_generics #where_reflect_clause {
#[inline]
fn type_name(&self) -> &str {
::core::any::type_name::<Self>()
}

#[inline]
fn get_represented_type_info(&self) -> #FQOption<&'static #bevy_reflect_path::TypeInfo> {
#FQOption::Some(<Self as #bevy_reflect_path::Typed>::type_info())
Expand Down Expand Up @@ -96,7 +91,7 @@ pub(crate) fn impl_value(meta: &ReflectMeta) -> proc_macro2::TokenStream {
if let #FQOption::Some(value) = <dyn #FQAny>::downcast_ref::<Self>(value) {
*self = #FQClone::clone(value);
} else {
panic!("Value is not {}.", ::core::any::type_name::<Self>());
panic!("Value is not {}.", <Self as #bevy_reflect_path::TypePath>::type_path());
}
}

Expand Down
Loading

0 comments on commit c4abc05

Please sign in to comment.