[Merged by Bors] - Add attribute to ignore fields of derived labels by joseph-gio · Pull Request #5366 · bevyengine/bevy

joseph-gio · 2022-07-18T11:18:38Z

Objective

Solution

Add the attribute #[label(ignore_fields)] for *Label types.

#[derive(SystemLabel)]
pub enum MyLabel {
    One,

    // Previously this was not allowed since labels cannot contain data.
    #[system_label(ignore_fields)]
    Two(PhantomData<usize>),
}

Notes

This label makes it possible for equality to behave differently depending on whether or not you are treating the type as a label. For example:

#[derive(SystemLabel, PartialEq, Eq)]
#[system_label(ignore_fields)]
pub struct Foo(usize);

If you compare it as a label, it will ignore the wrapped fields as the user requested. But if you compare it as a Foo, the derive will incorrectly compare the inner fields. I see a few solutions

Do nothing. This is technically intended behavior, but I think we should do our best to prevent footguns.
Generate impls of PartialEq and Eq along with the #[derive(Label)] macros. This is a breaking change as it requires all users to remove these derives from their types.
Only allow PhantomData to be used with ignore_fields -- seems needlessly prescriptive.

Changelog

Added the ignore_fields attribute to the derive macros for *Label types.
Added an example showing off different forms of the derive macro.

alice-i-cecile · 2022-07-18T12:47:19Z

crates/bevy_ecs/examples/derive_label.rs

+#[derive(SystemLabel)]
+pub enum GenericLabel<T> {
+    One,
+    #[system_label(ignore_fields)]


Suggested change

#[system_label(ignore_fields)]

#[system_label(ignore)]

Bikeshedding: I think this is marginally clearer, less verbose and more idiomatic.

To me, that implies that we're ignoring the entire variant, but we're only ignoring the fields.

Oh I see 🤔 Yeah I think you're right, but we should make sure to make this clear in the docs.

Weird to think that you could set your enum label to the phantomdata variant...

crates/bevy_ecs/examples/derive_label.rs

crates/bevy_macro_utils/src/lib.rs

alice-i-cecile

Some nits, but I'm on board with this solution and the underlying code implementation looks correct.

alice-i-cecile · 2022-07-18T12:52:33Z

Adding to the 0.8 milestone; I think it's important that we have a migration path here.

alice-i-cecile · 2022-07-18T12:53:12Z

@IceSentry this plus #4957 will have to be added to the migration guide.

alice-i-cecile · 2022-07-18T12:54:19Z

crates/bevy_ecs/examples/derive_label.rs

+
+fn main() {
+    // Unit labels are always equal.
+    assert_eq!(UnitLabel.as_label(), UnitLabel.as_label());


These tests are critical, but won't be run in CI. I actually think that these should be moved to doc tests on SystemLabel.

I agree, but we can't actually add doctests to label types until #5367

crates/bevy_ecs/examples/derive_label.rs

Co-authored-by: François <mockersf@gmail.com>

crates/bevy_macro_utils/src/lib.rs

cart

All of this additional complexity around ensuring labels don't have values makes me want to revisit #4957. All of this weirdness could be avoided if we just let labels have normal values, be normal structs, and use the derived Hash/PartialEq impls. I think that would be easier to document / understand for users.

Likewise, the benchmarks used to justify #4957 don't account for enums / longer names in the comparison, which I suspect might paint that impl in a worse light. Given the runtime cost scales with the struct name length, we should be benching real world names / enum lengths like VisibilitySystems::UpdateOrthographicFrusta, not the unit struct DummyLabel. And ideally we benchmark more complicated schedules with multiple labels. I'm a bit wary of things being optimized out when code only contains instance of a thing (ex: hashes, comparisons, collections, etc).

Too late to rethink for 0.8 (and i think we should merge this to make migrations smoother), but I think the @bevyengine/ecs-team should give this one last pass for 0.9 to do a final weighing of the pros and cons.

I'm not fully convinced one way or the other (and i do really like the elimination of boxing + removing the need for some trait impls in the api), but this doesn't feel clear cut to me yet.

cart · 2022-07-19T05:21:02Z

bors r+

# Objective Fixes #5362 ## Solution Add the attribute `#[label(ignore_fields)]` for `*Label` types. ```rust #[derive(SystemLabel)] pub enum MyLabel { One, // Previously this was not allowed since labels cannot contain data. #[system_label(ignore_fields)] Two(PhantomData<usize>), } ``` ## Notes This label makes it possible for equality to behave differently depending on whether or not you are treating the type as a label. For example: ```rust #[derive(SystemLabel, PartialEq, Eq)] #[system_label(ignore_fields)] pub struct Foo(usize); ``` If you compare it as a label, it will ignore the wrapped fields as the user requested. But if you compare it as a `Foo`, the derive will incorrectly compare the inner fields. I see a few solutions 1. Do nothing. This is technically intended behavior, but I think we should do our best to prevent footguns. 2. Generate impls of `PartialEq` and `Eq` along with the `#[derive(Label)]` macros. This is a breaking change as it requires all users to remove these derives from their types. 3. Only allow `PhantomData` to be used with `ignore_fields` -- seems needlessly prescriptive. --- ## Changelog * Added the `ignore_fields` attribute to the derive macros for `*Label` types. * Added an example showing off different forms of the derive macro.

bors · 2022-07-19T05:41:10Z

Pull request successfully merged into main.

Build succeeded:

joseph-gio · 2022-07-19T05:56:03Z

Likewise, the benchmarks used to justify #4957 don't account for enums / longer names in the comparison, which I suspect might paint that impl in a worse light. Given the runtime cost scales with the struct name length, we should be benching real world names / enum lengths like VisibilitySystems::UpdateOrthographicFrusta, not the unit struct DummyLabel. And ideally we benchmark more complicated schedules with multiple labels. I'm a bit wary of things being optimized out when code only contains instance of a thing (ex: hashes, comparisons, collections, etc).

Note that on this branch I've optimized comparisons by storing a "key" (u64) along with the TypeId -- this means comparing labels is effectively the same as comparing u128s. This is blocked by #4341, though.

cart · 2022-07-19T06:02:43Z

Ooh that would definitely close the gap / increase my comfort. I think thats worth pursuing.

alice-i-cecile · 2022-07-19T13:19:32Z

I agree; I like the new PR quite a bit and it alleviates some of my worries. And, obviously, I think that #4341 is a good idea (especially with the autolabelling).

# Objective I noticed while working on #5366 that the documentation for label types wasn't working correctly. Having experimented with this for a few weeks, I believe that generating docs in macros is more effort than it's worth. ## Solution Add more boilerplate, copy-paste and edit the docs across types. This also lets us add custom doctests for specific types. Also, we don't need `concat_idents` as a dependency anymore.

# Objective Fixes bevyengine#5362 ## Solution Add the attribute `#[label(ignore_fields)]` for `*Label` types. ```rust #[derive(SystemLabel)] pub enum MyLabel { One, // Previously this was not allowed since labels cannot contain data. #[system_label(ignore_fields)] Two(PhantomData<usize>), } ``` ## Notes This label makes it possible for equality to behave differently depending on whether or not you are treating the type as a label. For example: ```rust #[derive(SystemLabel, PartialEq, Eq)] #[system_label(ignore_fields)] pub struct Foo(usize); ``` If you compare it as a label, it will ignore the wrapped fields as the user requested. But if you compare it as a `Foo`, the derive will incorrectly compare the inner fields. I see a few solutions 1. Do nothing. This is technically intended behavior, but I think we should do our best to prevent footguns. 2. Generate impls of `PartialEq` and `Eq` along with the `#[derive(Label)]` macros. This is a breaking change as it requires all users to remove these derives from their types. 3. Only allow `PhantomData` to be used with `ignore_fields` -- seems needlessly prescriptive. --- ## Changelog * Added the `ignore_fields` attribute to the derive macros for `*Label` types. * Added an example showing off different forms of the derive macro.

# Objective I noticed while working on bevyengine#5366 that the documentation for label types wasn't working correctly. Having experimented with this for a few weeks, I believe that generating docs in macros is more effort than it's worth. ## Solution Add more boilerplate, copy-paste and edit the docs across types. This also lets us add custom doctests for specific types. Also, we don't need `concat_idents` as a dependency anymore.

# Objective Fixes bevyengine#5362 ## Solution Add the attribute `#[label(ignore_fields)]` for `*Label` types. ```rust #[derive(SystemLabel)] pub enum MyLabel { One, // Previously this was not allowed since labels cannot contain data. #[system_label(ignore_fields)] Two(PhantomData<usize>), } ``` ## Notes This label makes it possible for equality to behave differently depending on whether or not you are treating the type as a label. For example: ```rust #[derive(SystemLabel, PartialEq, Eq)] #[system_label(ignore_fields)] pub struct Foo(usize); ``` If you compare it as a label, it will ignore the wrapped fields as the user requested. But if you compare it as a `Foo`, the derive will incorrectly compare the inner fields. I see a few solutions 1. Do nothing. This is technically intended behavior, but I think we should do our best to prevent footguns. 2. Generate impls of `PartialEq` and `Eq` along with the `#[derive(Label)]` macros. This is a breaking change as it requires all users to remove these derives from their types. 3. Only allow `PhantomData` to be used with `ignore_fields` -- seems needlessly prescriptive. --- ## Changelog * Added the `ignore_fields` attribute to the derive macros for `*Label` types. * Added an example showing off different forms of the derive macro.

# Objective I noticed while working on bevyengine#5366 that the documentation for label types wasn't working correctly. Having experimented with this for a few weeks, I believe that generating docs in macros is more effort than it's worth. ## Solution Add more boilerplate, copy-paste and edit the docs across types. This also lets us add custom doctests for specific types. Also, we don't need `concat_idents` as a dependency anymore.

# Objective Fixes bevyengine#5362 ## Solution Add the attribute `#[label(ignore_fields)]` for `*Label` types. ```rust #[derive(SystemLabel)] pub enum MyLabel { One, // Previously this was not allowed since labels cannot contain data. #[system_label(ignore_fields)] Two(PhantomData<usize>), } ``` ## Notes This label makes it possible for equality to behave differently depending on whether or not you are treating the type as a label. For example: ```rust #[derive(SystemLabel, PartialEq, Eq)] #[system_label(ignore_fields)] pub struct Foo(usize); ``` If you compare it as a label, it will ignore the wrapped fields as the user requested. But if you compare it as a `Foo`, the derive will incorrectly compare the inner fields. I see a few solutions 1. Do nothing. This is technically intended behavior, but I think we should do our best to prevent footguns. 2. Generate impls of `PartialEq` and `Eq` along with the `#[derive(Label)]` macros. This is a breaking change as it requires all users to remove these derives from their types. 3. Only allow `PhantomData` to be used with `ignore_fields` -- seems needlessly prescriptive. --- ## Changelog * Added the `ignore_fields` attribute to the derive macros for `*Label` types. * Added an example showing off different forms of the derive macro.

# Objective I noticed while working on bevyengine#5366 that the documentation for label types wasn't working correctly. Having experimented with this for a few weeks, I believe that generating docs in macros is more effort than it's worth. ## Solution Add more boilerplate, copy-paste and edit the docs across types. This also lets us add custom doctests for specific types. Also, we don't need `concat_idents` as a dependency anymore.

joseph-gio marked this pull request as ready for review July 18, 2022 12:13

alice-i-cecile reviewed Jul 18, 2022

View reviewed changes

crates/bevy_ecs/examples/derive_label.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jul 18, 2022

View reviewed changes

crates/bevy_macro_utils/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jul 18, 2022

View reviewed changes

crates/bevy_macro_utils/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes Jul 18, 2022

View reviewed changes

alice-i-cecile added A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior labels Jul 18, 2022

alice-i-cecile added this to the Bevy 0.8 milestone Jul 18, 2022

alice-i-cecile reviewed Jul 18, 2022

View reviewed changes

joseph-gio mentioned this pull request Jul 18, 2022

[Merged by Bors] - improve documentation for macro-generated label types #5367

Closed

alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Jul 18, 2022

mockersf reviewed Jul 18, 2022

View reviewed changes

crates/bevy_ecs/examples/derive_label.rs Outdated Show resolved Hide resolved

mockersf reviewed Jul 18, 2022

View reviewed changes

crates/bevy_ecs/examples/derive_label.rs Outdated Show resolved Hide resolved

joseph-gio and others added 5 commits July 18, 2022 15:13

add *label::ignore_fields attribute

6262c42

add an example for label derives

2e12912

improve diagnostics for Label derive

cc895ee

use spanned errors instead of panics

ae0769d

Apply suggestions from code review

dc56e1d

Co-authored-by: François <mockersf@gmail.com>

joseph-gio force-pushed the generic-labels branch from 779b608 to dc56e1d Compare July 18, 2022 19:15

add docs to the derive macros

66d687a

joseph-gio changed the title ~~Optionally ignore label fields~~ Add attribute to ignore fields of derived labels Jul 19, 2022

cart reviewed Jul 19, 2022

View reviewed changes

crates/bevy_macro_utils/src/lib.rs Outdated Show resolved Hide resolved

remove dependency convert_case

a633baf

cart approved these changes Jul 19, 2022

View reviewed changes

bors bot changed the title ~~Add attribute to ignore fields of derived labels~~ [Merged by Bors] - Add attribute to ignore fields of derived labels Jul 19, 2022

bors bot closed this Jul 19, 2022

joseph-gio deleted the generic-labels branch July 19, 2022 06:55

joseph-gio mentioned this pull request Jul 19, 2022

Optimize label comparisons and remove restrictions on types that can be labels #5377

Closed

Uh oh!

Conversation

joseph-gio commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Notes

Changelog

Uh oh!

alice-i-cecile Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

joseph-gio Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Jul 18, 2022

Uh oh!

alice-i-cecile commented Jul 18, 2022

Uh oh!

alice-i-cecile Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

joseph-gio Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cart left a comment

Choose a reason for hiding this comment

Uh oh!

cart commented Jul 19, 2022

Uh oh!

bors bot commented Jul 19, 2022

Uh oh!

joseph-gio commented Jul 19, 2022

Uh oh!

cart commented Jul 19, 2022

Uh oh!

alice-i-cecile commented Jul 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joseph-gio commented Jul 18, 2022 •

edited

Loading

alice-i-cecile Jul 18, 2022 •

edited

Loading