bevy_reflect: Opt-out attribute for TypePath

[MrGVSV]

Objective

Fixes #9094

Solution

Takes a bit from this comment as well as a comment from @soqb.

This allows users to opt-out of the TypePath implementation that is automatically generated by the Reflect derive macro, allowing custom TypePath implementations.

#[derive(Reflect)]
#[reflect(type_path = false)]
struct Foo<T> {
    #[reflect(ignore)]
    _marker: PhantomData<T>,
}

struct NotTypePath;

impl<T: 'static> TypePath for Foo<T> {
    fn type_path() -> &'static str {
        std::any::type_name::<Self>()
    }

    fn short_type_path() -> &'static str {
        static CELL: GenericTypePathCell = GenericTypePathCell::new();
        CELL.get_or_insert::<Self, _>(|| {
            bevy_utils::get_short_name(std::any::type_name::<Self>())
        })
    }

    fn crate_name() -> Option<&'static str> {
        Some("my_crate")
    }

    fn module_path() -> Option<&'static str> {
        Some("my_crate::foo")
    }

    fn type_ident() -> Option<&'static str> {
        Some("Foo")
    }
}

// Can use `TypePath`
let _ = <Foo<NotTypePath> as TypePath>::type_path();

// Can register the type
let mut registry = TypeRegistry::default();
registry.register::<Foo<NotTypePath>>();

Type Path Stability

The stability of type paths mainly come into play during serialization. If a type is moved between builds, an unstable type path may become invalid.

Users that opt-out of TypePath and rely on something like std::any::type_name as in the example above, should be aware that this solution removes the stability guarantees. Deserialization thus expects that type to never move. If it does, then the serialized type paths will need to be updated accordingly.

If a user depends on stability, they will need to implement that stability logic manually (probably by looking at the expanded output of a typical Reflect/TypePath derive). This could be difficult for type parameters that don't/can't implement TypePath, and will need to make heavy use of string parsing and manipulation to achieve the same effect (alternatively, they can choose to simply exclude any type parameter that doesn't implement TypePath).

---

Changelog

• Added the #[reflect(type_path = false)] attribute to opt out of the TypePath impl when deriving Reflect

[paul-hansen]

I like this solution a lot! Fixes the problem, doesn't require wrappers, still just as easy to use in normal use cases 👍

I made a PR to leafwing input testing this out here: Leafwing-Studios/leafwing-input-manager#366 Didn't have any issues using it.

I couldn't find anything in the code to complain about, all looks great!

[Bluefinger]

I gave this a spin, and I guess #[reflect_value] also works with #[reflect_value(type_path = false)], since I was able to get my tests passing with bevy_rand. Works nicely for my use case! So if this merges for 11.1, then this will unblock bevy_rand's upgrade path for sure.

[Bluefinger]

I tested this with #[reflect_value(type_path = false)] and that worked as well. Maybe could add a comment to say this works with reflect_value too, not just #[reflect]?

[Bluefinger]

Maybe add a test for reflect_value since this is affected by the changes too.

[soqb]

overall this LGTM, this works to resolve one issue but if this is merged with type path part 2 as it is it will be a stability hole (using std::any::type_name).

i wonder if (soon in the future) we could add an associated constant like the following:

trait TypePath {
    const TYPE_PATH_IS_STABLE: bool = false;
}

which would let us to panic upon (de)serialization.

[MrGVSV]

overall this LGTM, this works to resolve one issue but if this is merged with type path part 2 as it is it will be a stability hole (using std::any::type_name).

Yeah. Maybe if we want to be super clear, I could add a warning in the docs to let users know that this is generally recommended against.

But yeah, for any type that can't effectively be (de)serialized, I don't think type name stability will be a major issue.

[Bluefinger]

overall this LGTM, this works to resolve one issue but if this is merged with type path part 2 as it is it will be a stability hole (using std::any::type_name).

i wonder if (soon in the future) we could add an associated constant like the following:

trait TypePath {
    const TYPE_PATH_IS_STABLE: bool = false;
}

which would let us to panic upon (de)serialization.

The issue with that is a library like bevy_rand becomes unusable in the future in its current state, because rand types don't implement TypePath and newtyping will basically be forced onto users. I would like to avoid that if possible, else if not possible, I'd have to resort to macros to remove some of that boilerplate. Not ideal in my eyes for something that should be plug and play.

[soqb]

i agree that this fix is absolutely necessary for bevy_rand, however that implies that the types cannot ever have a stable type path.

i feel very strongly that we should not allow serialisation on types that do not have stable type path, so we cannot allow serialisation on the type parameter with the offending error in bevy_rand. is that a dealbreaker?

technically, in the specific case of the non-stable type path'd type appearing as a field of a type with a stable type path, it would actually be serialisable!

[Bluefinger]

i feel very strongly that we should not allow serialisation on types that do not have stable type path, so we cannot allow serialisation on the type parameter with the offending error in bevy_rand. is that a dealbreaker?

Yes, because being able to serialise/deserialise the PRNG is a huge part of being able to store/send/reload a game's state (including its PRNG states) for determinism. Implementing a replay feature that is able to replay a game's 'random' parts, like crits, rolls, etc, requires that you are able to serialise/deserialise the PRNG, and bevy_rand component/resource types rely on the type parameter to determine which kind of PRNG is being used. For something like ChaCha8Rng, it's not just the seed you need to serialise, but also the stream and word position values. Different PRNGs have different states (WyRand is pretty much just a u64 value, ChaCha is a 256-bit state, etc), so to be able to ensure when we have multiple clients synced to the same exact PRNG state, or loading a saved game, that we are initialising the game state (PRNG included) to be exactly what was saved. Not including the type means we are having a lossy conversion, or are straight-up unable to save/restore PRNGs.

So in summary, for bevy_rand, we can't avoid serialising/deserialising the part that isn't TypePath stable, otherwise it pretty much kills the point of the crate. There would be not nice generic way for users to plug in rand PRNGs, because they'd have to newtype it into a concrete type to implement a stable TypePath. At that point, they are doing more boilerplate than my crate solves, by which they might as well implement the functionality themselves entirely.

[Bluefinger]

Ultimately though, if it must be no other way, then bevy_rand will have to be a very different sort of crate. It won't be a nice generic shim to plug any PRNG in, I'd have to provide concrete types, but that does mean bevy_rand won't be plug and play with the rest of the rand ecosystem, it would only support the PRNGs that I implement types for. But this also makes it not so nice for anything that tries to bring in external types from other rust crates into bevy.

[paul-hansen]

It should be noted that the example in the original post can be achieved in Bevy 0.11.0 already, you just have to manually implement Reflect instead of deriving it. This PR just allows you to disable the automatic implementation of TypePath when you derive Reflect. It doesn't introduce any stability issues that aren't already there.
Here's the same example working with Bevy 0.11.0 by implementing Reflect manually: https://gist.github.com/paul-hansen/c47f6af05732c0c03230fad5027f55e4

I think this PR could also be helpful for cases where there might be another crate that implements a stable type path and we want to implement TypePath using the methods that crate provides. So I don't really see this PR as solely helping negative use cases.

That said, I'm thinking I'll probably not use this for LWIM after all as serializing input maps is a core feature so it would benefit from the stability that TypePath brings for the same reasons that Bevy does and I can't think of a case where a user wouldn't own the type we are asking them to provide. (I'm open to discussion on this though. Feel free to use the LWIM PR)

[cart]

I think enabling opting out here is reasonable, but I also think it seems weird to not capitalize on our ability to make the top level type's path "as stable as possible". I think ideally in these cases the developer only needs to fill in the type path for the generic type. (which could be done by passing in a function to the derive):

#[derive(Reflect)]
#[type_path(T = get_t_type_path())]
struct MyType<T> {}

I also suspect that 99% of these cases will want to fall back to type_name. Feels like we could make this easier than "implement TypePath manually".

#[derive(Reflect)]
#[unstable_type_path(T)]
struct MyType<T> {}

But this seems ok for 0.11.1