stable reflect type name

[tguichaoua]

Objective

Drop the usage of std::any::type_name to introduce a stable type name for reflected types.

Based on https://github.com/bevyengine/bevy/issues/5745#issuecomment-1221389131

closes #3327

Solution

• Add a TypePath trait that can be derived with associated methods to get the type's path.
• Reflect::type_path (formely type_name) relied on TypePath::type_path

When TypePath is automatically derived, the base name is by default path::to::mod::MyType where path::to::mod is the result of module_path! and MyType the ident of the type.

If the type is generic then the base name is followed by the type name of its generic types. e.g. path::to::mod::MyType<'a, u32, glam::Vec3, 4>

mod a {
    mod b {
        mod c {
            #[derive(TypePath)]
            struct ABC;
        }

        #[derive(TypePath)]
        #[type_path(ident = "Foo")]
        struct AB<T>;
    }

    #[derive(TypePath)]
    struct A<T>;
}

Type	TypeName::name()
ABC	a::b::c::ABC
AB<ABC>	a::b::Foo<a::b::c::ABC>
A<ABC>	a::A<a::b::c::ABC>
A<AB<ABC>>	a::A<a::b::Foo<a::b::c::ABC>>

Open questions

• [x] How to handle the last std::any::type_names ? (see https://github.com/bevyengine/bevy/pull/5805#issuecomment-1229188350)
• [x] Whether or not keep the const before const generic parameters in type name ? e.g. MyType<42> vs MyType<const 42>. (see https://github.com/bevyengine/bevy/pull/5805#discussion_r956649449)
• [x] What convention for std types' type name ?
  • Only the ident (String)
  • Full module path (alloc::string::String)
  • Library name prefix (std::String)
• [ ] How to handle type path for tuples ? (see https://github.com/bevyengine/bevy/pull/5805#issuecomment-1237044093)
• [ ] Should custom type path and type name should be checked ? (see https://github.com/bevyengine/bevy/pull/5805#issuecomment-1237044093)

---

Changelog

• Added TypePath trait automatically implemented by #[derive(Reflect)]
• Added TypePath as super trait of Asset (because of Handle<T>)

Migration Guide

• Custom assets require the TypePath trait

- #[derive(TypeUuid)]
+ #[derive(TypeUuid, TypePath)]
#[uuid = "00000000-0000-0000-0000-000000000000"]
struct MyAsset {
    // ...
}

• Custom material require the TypePath trait

- #[derive(AsBindGroup, TypeUuid, Debug, Clone)]
+ #[derive(AsBindGroup, TypeUuid, TypePath, Debug, Clone)]
#[uuid = "00000000-0000-0000-0000-000000000000"]
struct CustomMaterial {
    // ...
}

impl Material for CustomMaterial {
    fn fragment_shader() -> ShaderRef {
        "shaders/custom_material.wgsl".into()
    }
}

• derive Reflect on generic type require the generic parameters to implement TypePath

struct MyType<T> {
    value: T
}

- impl<T: Reflect> Reflect for MyType<T> {
+ impl<T: Reflect + TypePath> Reflect for MyType<T> {
    // ...
}

• Input require the TypePath trait.

- #[derive(Copy, Clone, Eq, PartialEq, Hash)]
+ #[derive(Copy, Clone, Eq, PartialEq, Hash, TypePath)]
enum DummyInput {
    Input1,
    Input2,
}

fn system(input: Res<Input<DummyInput>>) {
    /* ... */
}

---

Additional context

Why not an associated const ?

It's not possible to concat string literal and const value in const context. There is the const_format crate but it's not working with generic (see limitations).

impl<T: TypeName> TypeName for Vec<T> {
    const NAME: &'static str = concatcp!("Vec", "<", T::NAME, ">"); // <-- won't compile
}

[tguichaoua]

Current state of replacing std::any::type_name

In the following cases std::any::type_name has not been replaced for different reason. Feedback on certain points will be appreciated ^^'

ValueInfo [SOLVED]

Cause

On this impl on &dyn Reflect there no access to TypeName because it's not object safe.

https://github.com/tguichaoua/bevy/blob/6fba583d809da89da21c0e2d2f4834a448dd8204/crates/bevy_reflect/src/reflect.rs#L207-L213

Solutions

• Keep std::any::type_name for this case : the type_name may differ from TypeName.
• Remove this impl, but I don't know what are the side effects.

TypeRegistry::register_type_data [SOLVED]

Cause

The type names are used only for a panic message. I don't think it worst it to add a TypeName restriction here.

https://github.com/tguichaoua/bevy/blob/6fba583d809da89da21c0e2d2f4834a448dd8204/crates/bevy_reflect/src/type_registry.rs#L123-L132

Solutions

• Leave as it.
• Add a TypeName restriction.

TypeUuidDynamic::type_name [SOLVED]

Cause

I don't kwow if TypeUuidDynamic::type_name must match TypeName.

Solutions

• Leave as it, if there is no link between TypeUuidDynamic::type_name and TypeName.
• Add TypeName restriction on type T.

[MrGVSV]

I think one solution for the issues you're coming across is to do what Typed and GetTypeRegistration do: create an implied bound.

For example, Typed is an implied requirement of Reflect due to Reflect::get_type_info. And it is added as a generic bound on places like TypeRegistration::of. This implied requirement is taken care of by the derive for Reflect so it works out totally fine.

Others might disagree, but I think we can do the same to TypeName. We can get rid of ReflectTypeName (we don't need object safety and removing it should reduce confusion). Instead, since we automatically derive it in #[derive(Reflect)], we can just use that impl in Reflect::type_name. And make a note in the documentation for manual implementors that this trait needs to be implemented as well.

[tguichaoua]

I added a "Open questions" section in the PR description for points that need feedback.

[MrGVSV]

How to handle the last std::any::type_names ? (see stable reflect type name #5805 (comment))

I think for TypeRegistry::register_type_data, it makes sense to just include it as a bound. If we can get #5772 to land, then this should all get cleaned up pretty easily. For TypeUuidDynamic::type_name, I would use TypeName. This might be fixed by simply adding it as a bound to T in the blanket impl.

Whether or not keep the const before const generic parameters in type name ? e.g. MyType<42> vs MyType<const 42>. (see stable reflect type name #5805 (comment))

I know in one of my comments I said we should keep it, but I changed my mind haha. I don't think it adds anything really. So I vote to exclude it.

What convention for std types' type name ?

• Only the ident (String)
• Full module path (alloc::string::String)
• Library name prefix (std::String)

I think it depends on the type. For things like String and Option, that are core to Rust and are unlikely to be user-generated, then we should go with the first option. However, it gets complicated for things like Duration which could be ambiguous with a user-defined type.

I'd say go with the first option for things that are part of the Rust prelude. Go with the third option for things that need to be imported.

[tguichaoua]

All done ! Thank you for all your help @MrGVSV.

[tguichaoua]

I was thinking about adding some shortcut when derive TypeName to use the lib name as prefix instead of the mod path or only the ident, see below. But to get the lib name we require a new dependency to const_str to compute the name at compile time.

const LIB_NAME: &'static str = const_str::split!(module_path!(), "::")[0];

Is that ok to add a new dependency for this ?

// my_lib::mod_a::mod_b::MyType
#[derive(TypeName)]
struct MyType;

// MyCustomName
#[derive(TypeName)]
#[type_name("MyCustomName")]
struct MyType;

// my_lib::MyType
#[derive(TypeName)]
#[type_name(lib)]
struct MyType;

// MyType
#[derive(TypeName)]
#[type_name(ident)]
struct MyType;

[tguichaoua]

Pending questions

How to implement TypePath for tuples ?

For type_path it's quite straight forward, but how to implement the other methods ? Currently implemented like this :

fn short_type_name_base() -> &'static str {
    Self::type_path()
}

fn short_type_name() -> &'static str {
    Self::type_path()
}

fn module_path() -> &'static str {
    ""
}

fn crate_name() -> &'static str {
    ""
}

Should custom path and ident be checked ?

If the user use custom path or ident with the derive macro, do we have to check those values are correct ?

#[derive(TypePath)]
#[type_path(path = "not well formated path *-*-*", ident = "Blah@ blah")
struct Foo;

[MrGVSV]

I tried to mark this PR as closing #3327, but it didn't seem to take. Could you added a Closes #3327 comment to the PR description?

[soqb]

we could allow associated constant type names with a fixed length array as storage which would panic on overflow instead of using const_format e.g.

trait TypeName {
    const NAME: &'static str;
}

impl TypeName for u8 {
    const NAME: &'static str = "u8";
}

impl<T: TypeName> TypeName for Vec<T> {
    const NAME: &'static str = ConstString::<1024>::from_str("Vec<")
        .with_str(T::NAME)
        .with_str(">")
        .as_str();
}

assert_eq!(Vec::<u8>::NAME, "Vec<u8>");

but const-panic isn't working very well.

are const typenames helpful at all? and would limiting their maximum length be particularly hindering?

[tguichaoua]

Since there is a lot of conflict with the main branch I think the best to do is not solve them, keep this PR open to solve the pending questions and then open a fresh PR to implement the feature.

[soqb]

to solve the tuple issue, what about seperate TypeName and TypePath: TypeName traits where tuples implement TypeName but not TypePath e.g.

impl<T: TypeName, U: TypeName> TypeName for (T, U) {
    fn type_name() -> &'static str {
        // e.g. "(my_crate::Foo, my_crate::Bar)"
        let name = format!("({}, {})", T::type_name(), U::type_name());
        Box::leak(Box::new(name)) // but better
    }
    
    fn short_type_name() -> &'static str {
        // e.g. "(Foo, Bar)"
        let name = format!("({}, {})", T::short_type_name(), U::short_type_name());
        Box::leak(Box::new(name))
    }
}

struct StableName<T, U> {
    a: T,
    b: (T, U),
}

// generated by derive macro
const PATH: &'static str = module_path!();
const IDENT: &'static str = "StableName";

impl<T: TypeName, U: TypeName> TypeName for StableName<T, U> {
    fn type_name() -> &'static str {
        <Self as TypePath>::type_path()
    }

    fn short_type_name() -> &'static str {
        <Self as TypePath>::short_type_path()
    }
}

// NB: not `T: TypePath`.
impl<T: TypeName, U: TypeName> TypePath for StableName<T, U> {
    fn type_path() -> &'static str {
        // e.g. "my_crate::StableName::<my_crate::Foo, my_crate::Bar>"
        let path = format!("{}::{}::<{}, {}>", PATH, IDENT, T::type_name(), U::type_name());
        Box::leak(Box::new(path))
    }
    
    fn short_type_path() -> &'static str {
        // e.g. "StableName::<Foo, Bar>"
        let path = format!("{}::<{}, {}>", IDENT, T::short_type_name(), U::short_type_name());
        Box::leak(Box::new(path))
    }
    
    // etc.
}

or something

[cart]

Sorry for taking so long to respond to this. I think this effort is important and worth driving forward.

How to implement TypePath for tuples ?

This will also be a problem for reflected array types, which also don't have a crate or module in their type name.

In addition to @soqb's idea to separate the traits (ill call that option 1) I think we have a few more options:

Option 2: Allow empty crates, but only for built in types like arrays and tuples. Specify this rule in the trait docs and enforce it in the derive macro (as any derived impl is not-built-in by definition).

Option 3: Arbitrarily define a crate for these types (ex: builtin::(usize, usize))

I personally like Option 2 the most. Keeps our traits simple, seems workable generally, and aligns pretty well with std type names. Slightly more complicated rules / constraints, but we do get to make the rules and I can't think of any negative implications.

Should custom path and ident be checked ?

I think yes. Better to ensure data integrity at the definition level.

[soqb]

should the methods on the TypePath (possibly TypeName) trait(s) take in &self so that the Dynamic* reflect types work as intended from a reflection standpoint, or is this out-of-scope for this pr/not desirable behavior?

[MrGVSV]

should the methods on the TypePath (possibly TypeName) trait(s) take in &self so that the Dynamic* reflect types work as intended from a reflection standpoint, or is this out-of-scope for this pr/not desirable behavior?

Imo probably not. We want to be able to access these methods without needing an actual instance of that field (i.e. when registering the type itself).

If we want access to this, we could probably do one of the following:

1. Add corresponding methods to the Reflect trait. Since this is theoretically a "core" reflection trait, we should be okay to add such methods. This would be similar to what we do for the Typed trait using Reflect::get_type_info.
2. Similar to 1, but with a custom struct. If we don't want to add a new method to Reflect for each one in TypePath, then we could combine everything into one method and have it return a struct with all the TypePath data (probably using Cow<'static, str> for the fields).
3. Lastly, we could just continue to use Reflect::type_name as we do now for the Dynamic types. It would just contain the full TypePath::type_path string value and nothing more (no short_type_name, module_path, etc.).

Any thoughts on these options? Maybe there are other solutions I'm missing here, but one of these would make sense to me.

[cart]

Imo probably not. We want to be able to access these methods without needing an actual instance of that field (i.e. when registering the type itself).

Agreed. I think option (1) is pretty good.

[soqb]

I personally like Option 2 the most.

i think there is a fourth option we haven't considered as much as we should.

moving the functionality into a TypePath struct (as @MrGVSV suggested for Reflect) and adding methods returning Options where applicable similar to TypeInfo.

returning Options is a nice compromise between Option 1 & 2/3 since it cuts down on traits but represents the optional state of the path segments clearly.

type A = T;
type B = (T, U);

impl TypePath {

    // A: "my_crate::foo::T"
    // B: "(my_crate::foo::T, my_crate::foo::bar::U)"
    fn type_path(&self) -> &str;
    
    // A: "T"
    // B: "(T, U)"
    fn short_type_path(&self) -> &str;
    
    // A: Some("T")
    // B: None
    fn type_name(&self) -> Option<&str>;
    
    // A: Some("my_crate")
    // B: None
    fn crate_name(&self) -> Option<&str>;
    
    // A: Some("my_crate::foo")
    // B: None
    fn module_path(&self) -> Option<&str>;
}

// i suppose `TypePath` would have a `Cow<'static, str>` (potentially `Option`) field for each method.

trait Typed {
    fn type_info() -> &'static TypeInfo;
    fn type_path() -> &'static TypePath;
}

trait Reflect {
    fn get_type_info(&self) -> &'static TypeInfo;
    fn get_type_path(&self) -> &'static TypePath;
    
    // ...
}

[soqb]

side note @tguichaoua: i would love to adopt/adapt this PR if you're not willing since its relevant to some of my reflection mad science experiments.

[tguichaoua]

side note @tguichaoua: i would love to adopt/adapt this PR if you're not willing since its relevant to some of my reflection mad science experiments.

@soqb np, feel free to adopt this PR 👍

[bravely-beep]

I noticed the current implementation can cause potential name conflicts:

use bevy::reflect::TypePath;

#[derive(TypePath)]
pub struct Foo;

pub fn bar() {
    #[derive(TypePath)]
    pub struct Foo;

    println!("{}", Foo::type_path());
}

fn main() {
    println!("{}", Foo::type_path());
    bar();
}

Not sure how to resolve this, module_path doesn't include functions in the path (even though std::any::type_name does 🤨).

[soqb]

i wonder if we can do something like this if it becomes an issue. it's very hacks and not guaranteed to work across compiler versions but it might be enough.

[bravely-beep]

Reading the first sentence of the top comment on this PR, I feel like that might be a non-starter?

Drop the usage of std::any::type_name

Making this stable across compiler versions is one of the core goals of this PR, right?

[soqb]

Reading the first sentence of the top comment on this PR, I feel like that might be a non-starter?

effectively, yes - this is the absolute last thing i'd want to do. but if and only if we really, really need this conflict-resolution, due to the crawling pace of the function_name!() and item_path!() macros put forward in a litany of different dead rust issues and prs, this would likely be our only option for some time to come.

[MrGVSV]

Closing this out as superseded by #7184