Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SystemLabel proc-macro no longer allows for generic enums #5362

Closed
SarthakSingh31 opened this issue Jul 18, 2022 · 3 comments
Closed

SystemLabel proc-macro no longer allows for generic enums #5362

SarthakSingh31 opened this issue Jul 18, 2022 · 3 comments
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior
Milestone
Bevy 0.8

Comments

@SarthakSingh31
Copy link
Contributor

SarthakSingh31 commented Jul 18, 2022 •
edited
Loading

Bevy version

Since c43295a

The Problem

I was trying to update bevy_mod_raycast to bevy main but ran into a problem with this

https://github.com/bevyengine/bevy/pull/4957/files#diff-b131f1941aa2edc166d44e3411ad3e04c125bd541288357846e6c2e018b46b80R133-R142

change to the SystemLabel proc-macro.

https://github.com/aevyrie/bevy_mod_raycast/blob/0a5d1e3334285eaf85e97adc28b14cbb50ab1b48/src/lib.rs#L82-L89

needs a variant with a data field to store the PhantomData but this change makes that impossible.

Possible solutions

  1. Make the SystemLabel proc-macro detect if the variant is unreachable by insuring that it has a field with type ::std::convert::Infallible.
  2. Add a ignore attribute to the SystemLabel proc-macro.
@SarthakSingh31 SarthakSingh31 added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jul 18, 2022
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events P-Regression Functionality that used to work but no longer does. Add a test for this! and removed S-Needs-Triage This issue needs to be labelled labels Jul 18, 2022
@alice-i-cecile
Copy link
Member

Broken by #4957. @JoJoJet, any opinions?

@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone Jul 18, 2022
@alice-i-cecile alice-i-cecile removed the P-Regression Functionality that used to work but no longer does. Add a test for this! label Jul 18, 2022
@JoJoJet
Copy link
Member

JoJoJet commented Jul 18, 2022

@alice-i-cecile the attribute idea seems more promising. I'll claim this if that's ok.

@JoJoJet
Copy link
Member

JoJoJet commented Jul 18, 2022 •
edited
Loading

I don't want to magically change behavior based on type names since that can be potentially surprising. Instead I think the second suggestion or something similar to it would be best.

#[derive(Label)]
enum MyEnum {
    One,
    #[label(ignore_fields)]
    Two(/* Any data, potentially non-phantom */),
}

This would simply tell the derive macro to ignore the fields of that variant when comparing labels. Though I suspect this may have implications for equality which I'll look into more.

@JoJoJet JoJoJet mentioned this issue Jul 18, 2022
Closed
@bors bors bot closed this as completed in 44e9cd4 Jul 19, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this issue Aug 8, 2022
@JoJoJet @inodentry
Add attribute to ignore fields of derived labels (bevyengine#5366)
b0c0b41 
# 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.

<!--
## Migration Guide

> This section is optional. If there are no breaking changes, you can delete this section.

- If this PR is a breaking change (relative to the last release of Bevy), describe how a user might need to migrate their code to support these changes
- Simply adding new functionality is not a breaking change.
- Fixing behavior that was definitely a bug, rather than a questionable design choice is not a breaking change.
-->
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
@JoJoJet @james7132
Add attribute to ignore fields of derived labels (bevyengine#5366)
0cd3044 
# 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.

<!--
## Migration Guide

> This section is optional. If there are no breaking changes, you can delete this section.

- If this PR is a breaking change (relative to the last release of Bevy), describe how a user might need to migrate their code to support these changes
- Simply adding new functionality is not a breaking change.
- Fixing behavior that was definitely a bug, rather than a questionable design choice is not a breaking change.
-->
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@JoJoJet @ItsDoot
Add attribute to ignore fields of derived labels (bevyengine#5366)
a332207 
# 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.

<!--
## Migration Guide

> This section is optional. If there are no breaking changes, you can delete this section.

- If this PR is a breaking change (relative to the last release of Bevy), describe how a user might need to migrate their code to support these changes
- Simply adding new functionality is not a breaking change.
- Fixing behavior that was definitely a bug, rather than a questionable design choice is not a breaking change.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior
Projects
None yet
Milestone
Bevy 0.8
Development

Successfully merging a pull request may close this issue.

[Merged by Bors] - Add attribute to ignore fields of derived labels JoJoJet/bevy
3 participants
@alice-i-cecile @JoJoJet @SarthakSingh31