reflect: `TypePath` part 2 #8768

soqb · 2023-06-06T19:37:36Z

Objective

Followup to reflect: stable type path v2 #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 #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 (note that String is in the default Rust prelude) snuck in completely under the radar.

soqb · 2023-06-23T15:58:03Z

i've stripped down this PR (TypeUuid is no longer deprecated), in the hopes we can squeeze this in before 0.11!

MrGVSV

Looking great! And yeah removing the TypeUuid changes definitely helped make this review a bit nicer.

Just some comments, questions, and nitpicks!

crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs

crates/bevy_reflect/src/array.rs

crates/bevy_reflect/src/serde/de.rs

crates/bevy_reflect/src/reflect.rs

crates/bevy_reflect/src/type_registry.rs

crates/bevy_reflect/src/impls/std.rs

crates/bevy_reflect/src/lib.rs

crates/bevy_scene/src/serde.rs

soqb · 2023-06-26T09:21:52Z

failure in CI fixed by #8957, so that is a prerequisite for this.

github-actions · 2023-08-19T14:24:49Z

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions · 2023-08-19T14:44:34Z

Example alien_cake_addict failed to run, please try running it locally and check the result.

MrGVSV

Looks good to me!

crates/bevy_reflect/src/lib.rs

crates/bevy_reflect/src/impls/std.rs

crates/bevy_scene/src/dynamic_scene.rs

crates/bevy_scene/src/scene_spawner.rs

kristoff3r

I read through everything and it looks very reasonable to me (although I'm completely new to bevy_reflect so take that with a grain of salt). All the changed code looks the same or better.

Also the link to the "incorrect type path implementation" is dead, it would've been nice to know what that was so I could have looked for similar stuff or missing tests.

There were some names in the changelog that hasn't been updated to match the code:

TypePathVtable -> TypePathTable
type_path_vtable -> type_path_table

crates/bevy_reflect/src/serde/ser.rs

alice-i-cecile · 2023-09-18T19:38:11Z

@MrGVSV, can you go through and resolve conversations that are no longer relevant? @soqb, can you rebase? I think we can get this done for 0.12, so let's get the final steps rolling.

MrGVSV · 2023-09-18T20:19:52Z

@MrGVSV, can you go through and resolve conversations that are no longer relevant?

I think this would have to be done by the author or the PR (@soqb).

soqb · 2023-09-18T21:10:19Z

i'll get round to updating everything this week when i get a spot

…t-type-path-part-2

MrGVSV · 2023-09-24T20:59:49Z

crates/bevy_ecs/src/reflect/bundle.rs

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


Nit: Could you add an issue link to these FIXME comments?

MrGVSV · 2023-09-24T21:05:59Z

crates/bevy_reflect/src/lib.rs

        );
    }

+    macro_rules! assert_type_paths {
+        ($($ty:ty => $long:literal, $short:literal,)*) => {


Total nitpick that definitely is not required: I think wrapping the literals in parentheses (like a tuple) might be easier to read, but that's definitely just my opinion haha

MrGVSV · 2023-09-24T21:09:01Z

crates/bevy_reflect/src/lib.rs

-    a_struct: bevy_reflect::tests::should_reflect_debug::SomeStruct {
+    a_struct: bevy_reflect::tests::SomeStruct {


Hm, does TypePath mention this quirk (due to module_path!)? If not, that might be worth adding as part of this PR so users who are defining structs inside functions aren't wholly caught off guard.

crates/bevy_reflect/src/reflect.rs

alice-i-cecile · 2023-10-02T12:10:40Z

@soqb @MrGVSV It looks like there's a bit of cleanup left (and merge conflicts). Do you feel like we should block on that, or just merge this and move on?

MrGVSV · 2023-10-02T15:41:06Z

@soqb @MrGVSV It looks like there's a bit of cleanup left (and merge conflicts). Do you feel like we should block on that, or just merge this and move on?

Not sure about the merge conflicts, but I believe my remaining comments are more nits than anything we should block on.

…t-type-path-part-2

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

alice-i-cecile added this to the 0.11 milestone Jun 6, 2023

alice-i-cecile added the A-Assets Load files from disk to use for things like images, models, and sounds label Jun 6, 2023

MrGVSV self-requested a review June 6, 2023 22:03

soqb added 13 commits June 23, 2023 13:47

removed all reflect-scoped std::any::type_name usages

10bd53e

regular tests

5a9a5ff

doctests!

0fd5e36

docs and collision resistance

6792bb1

Merge remote-tracking branch 'bevy/main' into HEAD

e6c6e3f

merge fixes

fa544af

remove extra println

0e94e7e

docs and tests

8cb80ea

maximally relax bounds for all TypePath impls

6768e05

Typed: TypePath and where clause lifetime bounds

4a9e902

various correctness fixes

4e8dfa7

Merge branch 'main' of https://github.com/bevyengine/bevy into HEAD

469c927

merge fixes

bd0b407

soqb force-pushed the reflect-type-path-part-2 branch from 66d6322 to bd0b407 Compare June 23, 2023 15:30

soqb added 3 commits June 23, 2023 16:42

documentation fixes

edc0d43

cargo fmt --all

a6b3b1c

fix doc test

6a5ed35

soqb marked this pull request as ready for review June 23, 2023 15:58

MrGVSV reviewed Jun 26, 2023

View reviewed changes

fix doc slips

5e2ac16

cart added this to the 0.12 milestone Jul 7, 2023

soqb added 3 commits August 15, 2023 01:23

merge with main

c2689a3

vtable -> table

6057a4f

address feedback

bce5c9d

remove unused import

2a4442f

appease clippy

c1bc5cc

MrGVSV approved these changes Sep 5, 2023

View reviewed changes

kristoff3r approved these changes Sep 18, 2023

View reviewed changes

crates/bevy_reflect/src/serde/ser.rs Show resolved Hide resolved

soqb added 5 commits September 22, 2023 19:36

readded via deprecation Reflect::type_name

a3dbbd5

aaagh!

b6b9af8

TypePath for &mut

9d49b3a

Merge branch 'main' of https://github.com/bevyengine/bevy into reflec…

eca4e38

…t-type-path-part-2

add missing doc link

aae3ca3

MrGVSV reviewed Sep 24, 2023

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 29, 2023

Merge branch 'main' of https://github.com/bevyengine/bevy into reflec…

0b533c0

…t-type-path-part-2

alice-i-cecile added this pull request to the merge queue Oct 9, 2023

Merged via the queue into bevyengine:main with commit 262846e Oct 9, 2023
21 checks passed

cart mentioned this pull request Oct 13, 2023

News: Release 0.12 bevyengine/bevy-website#754

Merged

43 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reflect: `TypePath` part 2 #8768

reflect: `TypePath` part 2 #8768

soqb commented Jun 6, 2023 •

edited

Loading

soqb commented Jun 23, 2023 •

edited

Loading

MrGVSV left a comment

soqb commented Jun 26, 2023 •

edited

Loading

github-actions bot commented Aug 19, 2023

github-actions bot commented Aug 19, 2023

MrGVSV left a comment

kristoff3r left a comment

alice-i-cecile commented Sep 18, 2023

MrGVSV commented Sep 18, 2023

soqb commented Sep 18, 2023

MrGVSV Sep 24, 2023

MrGVSV Sep 24, 2023

MrGVSV Sep 24, 2023

alice-i-cecile commented Oct 2, 2023

MrGVSV commented Oct 2, 2023

		a_struct: bevy_reflect::tests::should_reflect_debug::SomeStruct {
		a_struct: bevy_reflect::tests::SomeStruct {

reflect: TypePath part 2 #8768

reflect: TypePath part 2 #8768

Conversation

soqb commented Jun 6, 2023 • edited Loading

Objective

Solution

Changelog

Migration Guide

Note to reviewers

soqb commented Jun 23, 2023 • edited Loading

MrGVSV left a comment

Choose a reason for hiding this comment

soqb commented Jun 26, 2023 • edited Loading

github-actions bot commented Aug 19, 2023

github-actions bot commented Aug 19, 2023

MrGVSV left a comment

Choose a reason for hiding this comment

kristoff3r left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Sep 18, 2023

MrGVSV commented Sep 18, 2023

soqb commented Sep 18, 2023

MrGVSV Sep 24, 2023

Choose a reason for hiding this comment

MrGVSV Sep 24, 2023

Choose a reason for hiding this comment

MrGVSV Sep 24, 2023

Choose a reason for hiding this comment

alice-i-cecile commented Oct 2, 2023

MrGVSV commented Oct 2, 2023

reflect: `TypePath` part 2 #8768

reflect: `TypePath` part 2 #8768

soqb commented Jun 6, 2023 •

edited

Loading

soqb commented Jun 23, 2023 •

edited

Loading

soqb commented Jun 26, 2023 •

edited

Loading