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 #5745 (comment)

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

• How to handle the last std::any::type_names ? (see stable reflect type name #5805 (comment))
• 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))
• 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 stable reflect type name #5805 (comment))
• Should custom type path and type name should be checked ? (see stable reflect type name #5805 (comment))

---

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
}

[MrGVSV]

Just some preliminary comments. I'll try to look at this more over the weekend 🙂

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

[MrGVSV]

Looking really good! I think this will be a good starting point for improving the type system even more :D

[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