Typed SystemLabels regress generic labeling

[aevyrie]

Bevy version

bevy\main

Operating system & version

Win10

What you did

Updating to main branch with #1473 disallows creating labels that are generic over a type.

What you expected to happen

When stringly typed, I could have labels that were unique per concrete type over a generic. I haven't found a workaround to do the same with typed labels in their current form. I'd like to have generic labels so that users of my plugin can have multiple concrete types of my generic system active at the same time.

What actually happened

Attempting to derive SystemLabel for an enum with generic type annotations results in an error, probably because the derive macro doesn't support generics in the signature. Using an enum without generics results in a runtime error due to a non-unique SystemLabel.

[Ratysz]

For what it's worth, you can derive the label traits on structs or continue to use &'static str and Cow<'static, str> as labels. Lack of generics support is indeed an oversight, however.

[aevyrie]

For what it's worth, you can derive the label traits on structs or continue to use &'static str and Cow<'static, str> as labels. Lack of generics support is indeed an oversight, however.

I tried to do so here: #1498 (comment)
However deriving the traits on non-trivial components I want to be generic over is not simple.

[alice-i-cecile]

@cart I'd like to see if we can get a fix for this in for 0.5 to avoid a regression, especially for plugin authors. Would you be okay with adding the 0.5 milestone to this?

[cart]

This doesn't sound like a regression, given that SystemLabel is implemented for static strings and Cow strings (as @Ratysz mentioned). @aevyrie it looks like you tried creating a custom generic type and deriving SystemLabel for it (which you eventually got working). If you use strings (like you previously did in bevy_mod_picking) does it still work?

Supporting generics is still a good idea and seems worth adding to the milestone, but I wouldn't call this a regression. Its a "missing feature" that we've never had before.

[aevyrie]

Supporting generics is still a good idea and seems worth adding to the milestone, but I wouldn't call this a regression. Its a "missing feature" that we've never had before.

Sorry if I'm being obtuse here. By "regression" I mean that I was previously able to make labels generic over components (just like systems), but I am now unable to do so. I'm using this in a bounding volume plugin so my systems can be generic over different types of bounding volume, which requires generic labels to solve system order ambiguity.

• When system labels were added, my string labels took the typename of the component the systems were generic over to prevent label collisions. e.g. my_system<T> has the label format!("my_system_{}", any::typename<T>)
• With this update to SystemLabels, I am no longer able to make static strings that reference my type. This could simply be a gap in my rust knowledge.
• I then tried to create a label struct/enum that used this type in a PhantomData<T> field and deriving SystemLabel, but the trait bounds on SystemLabel were impossible for me to resolve. It only "worked" insofar as the derive bug was fixed, it didn't solve my useability issue.

Does anyone have an example of a generic system with generic labels? That's all I really need, and I can't seem to find anyone else doing this (or the source doing this) for some reason.

[cart]

First: sorry for saying bevy_mod_picking instead of bevy_mod_bounding 😄

Your old approach should still work like this:

.label(Cow::Owned(format!("update_boundvols_{}", std::any::type_name::<T>()))),

The Cow::Owned is necessary because you weren't actually using static strings in your old impl. format!() returns a dynamically allocated String. It worked because String implements Into<Cow<'static, str>>, which is what the method used to take. Now it takes impl SystemLabel, so the conversion is no longer implicit.

[aevyrie]

Haha, no worries. 😁

Your old approach should still work like this: [...]

Ah, fantastic, well now I feel silly. This was a gap in my understanding of rust.

This is definitely something worth documenting alongside the generic systems pattern. This generic label + generic systems is a really useful abstraction when you have many components that implement the same trait. I wonder if it's worth making a generic_label!() macro that hides some of the Cow weirdness.

I'll attempt the suggested fix, document, and close if it works.

Thanks!

[aevyrie]

The suggested fix worked exactly as suggested. Closing.
https://github.com/aevyrie/bevy_mod_bounding/blob/1b68ac1e041add7ea8d9b180c0a776c71aa8316c/src/lib.rs#L33-L42

[Ratysz]

For future reference, here's the correct solution: [...]

Ah, beat me by a couple of seconds. Although I would argue that putting bounds on T rather than Self is more idiomatic.

EDIT: Actually, no, your snippet does not compile.

[aevyrie]

Although I would argue that putting bounds on T rather than Self is more idiomatic.

The main thing that prevented me from moving away from stringly typing this was that this required all sorts of trait impls on my generic type. This is painful because my generics are components that often have fields which do not implement Hash or Eq.

[TheRawMeatball]

Hmm, this is a serious usability issue IMO. With the last change in #1498, the theoretical correct solution would be just

#[derive(Debug, Hash, Eq, PartialEq, Copy, Clone, SystemLabel)]
enum Systems<T> {
    Sys1,
    Sys2,
    _Marker(std::marker::PhantomData<T>),
}

However, this does not work in practice because the built-in derives are too restrictive, and place excessive bounds on T, requiring it to implement all derived traits. (see rust/#26925).

One possible solution would be to have bevy-specific macros that generate more precise impls at the cost of leaking the internals.

[aevyrie]

I know this is unsatisfying, but to address usability, what do you think of making a generic_label!(system_fn, T) macro that outputs a Cow string which appends the type_name of the function and the type_name of the generic? If we find a way to get ZSTs working, the macro could be updated internally or completely removed.

[Ratysz]

This is something best left to user code.

[TheRawMeatball]

One possible solution would be to have bevy-specific macros that generate more precise impls at the cost of leaking the internals.

Can I get some more opinions on this? In such a world, we'd have derive macros like LeakyClone that derive the std traits, but place more precise impls at the cost of leaking the internal fields of the struct/enum. In the end, this is how it would look:

#[derive(LeakyDebug, LeakyHash, LeakyEq, LeakyPartialEq, LeakyCopy, LeakyClone, SystemLabel)]
enum Systems<T> {
    Sys1,
    Sys2,
    _Marker(PhantomData<T>),
}

// This is the the type of code that gets generated:
impl Debug for Systems<T> where PhantomData<T>: Debug {}
impl Hash for Systems<T> where PhantomData<T>: Hash {}
// and so on

[Ratysz]

Can I get some more opinions on this?

I'll give you mine again anyway 😄

This feels like a lot of complexity on Bevy side to fix something that isn't Bevy's fault in a way that isn't Bevy-specific.

[aevyrie]

This feels like a lot of complexity on Bevy side to fix something that isn't Bevy's fault in a way that isn't Bevy-specific.

I agree. I will again reiterate that strings solve the problem of generic labels in bevy, until upstream Rust limitations are solved. I think a macro to help prevent global collisions would be sufficient.

This is something best left to user code.

Assuming this was in response to my suggestion of a generic_label!() macro, I disagree. The new typed labels prevent collisions, especially important with how Bevy encourages users to make and share plugins. Relying on stringly typed labels has the danger of plugins colliding as the Bevy ecosystem grows. I'd prefer some Bevy-idomatic way of generating these lables for generic systems.

In fact, I think I'd prefer that stringly-typed labels be wrapped in a Bevy type, so we can enforce some rules about how label strings are constructed.

[TheRawMeatball]

Hmm, alright. Since there doesn't seem to be a crate with these macros, I guess I'll make mine and then link it in this issue. For now I'm closing this issue

[cart]

I have a feeling that most people using generics in their labels just want a type to uniquely identify their system. A workaround we could consider is to provide a built in SystemLabel<T>(PhantomData<T>) type (non-conflicting name TBD) that we implement SystemLabel for (with the manual impls for PartialEq / Debug / etc). Then users could just throw any type into that. Ex: Label<Foo>, Label<Bar>, etc.

#[derive(SystemLabel)]
pub struct Label<T>(PhantomData<T>);

impl<T> Default for Label<T> {
    fn default() -> Self {
        Self(PhantomData)
    }
}

impl<T> Clone for Label<T> {
    fn clone(&self) -> Self {
        Self(PhantomData)
    }
}

impl<T> PartialEq for Label<T> {
    fn eq(&self, _other: &Self) -> bool {
        true
    }
}

impl<T> fmt::Debug for Label<T> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "Label<{}>", std::any::type_name::<T>())
    }
}

impl<T> std::hash::Hash for Label<T> {
    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
        self.0.hash(state)
    }
}

impl<T> Eq for Label<T> {}

[alice-i-cecile]

@aevyrie will many-to-many labels (#1576) improve your use case? It feels like in many cases you'd often be able to just share one label for each copy of a generic system :)

[aevyrie]

I have a feeling that most people using generics in their labels just want a type to uniquely identify their system. A workaround we could consider is to provide a built in SystemLabel<T>(PhantomData<T>) type (non-conflicting name TBD) that we implement SystemLabel for (with the manual impls for PartialEq / Debug / etc). Then users could just throw any type into that. Ex: Label<Foo>, Label<Bar>, etc. [...]

With generic systems, often you will have a bunch of systems that share the same generic type. With this proposal, their labels would be identical, which is not desired because you want to specify order between system of the same type. However, I think the many-to-many labels @alice-i-cecile mentioned would solve that, in which case the generic is completely ignored and a system that is generic has the same label for all concrete types it is generic over.

That's not to say a Label<T> isn't useful! It would be an awesome way to use existing types instead of creating yet another type just for labeling.

[Ratysz]

The crux of the issue is that derives on types containing PhantomData<T> plumb the bounds onto T, which, as demonstrated here, is not always desirable. I think the proper solution is a drop-in replacement for PhantomData that already implements all the "underivable" traits needed for labels - which is something that can be provided by an external crate.

That would allow labels like this to Just Work:

#[derive(Debug, Hash, Eq, PartialEq, Copy, Clone, SystemLabel)]
enum Systems<T> {
    Sys1,
    Sys2,
    _Marker(BetterPhantomData<T>),
}

Implementation-wise, BetterPhantomData<T> would most likely have to be equvalent to TypeId::of::<T>().

[aevyrie]

Yeah, that's right. The derives were what I struggled with when trying to use generic labels with a PhantomData<T> recently.