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.

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

Remove ability to #[reflect(ignore)] enum variants #5983

Closed
MrGVSV opened this issue Sep 14, 2022 · 3 comments
Closed

Remove ability to #[reflect(ignore)] enum variants #5983

MrGVSV opened this issue Sep 14, 2022 · 3 comments
Labels
A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior
Milestone

Comments

@MrGVSV
Copy link
Member

MrGVSV commented Sep 14, 2022

What problem does this solve or what need does it fill?

#4761 added the ability to reflect enums. With this, it also allowed for entire variants to be excluded from reflection using the #[reflect(ignore)] attribute. As mentioned in this comment, this can cause some problems.

Essentially, the code will panic if we try to call certain methods on an ignored variant.

#[derive(Reflect)]
enum Foo {
  #[reflect(ignore)]
  Bar
}

let value = Foo::Bar;

let name = Enum::variant_name(&value); // <-- PANIC!

What solution would you like?

Remove the ability to ignore entire variants (along with the relevant tests).

What alternative(s) have you considered?

Other solutions could be to:

  1. Allow ignored variants to be used by certain methods
  2. Return default-able values (Option<T> rather than T)
  3. Ignore only the fields within the variant, not the variant itself

Option 1 is probably the best, but now we break the reflection promise that the variant is ignored since we have to use and return its data.

Option 2 negatively affects ergonomics.

Option 3 is okay, but it's not obvious that we are ignoring the fields and not the variant itself.

@MrGVSV MrGVSV added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types labels Sep 14, 2022
@MrGVSV MrGVSV added this to the Bevy 0.9 milestone Sep 14, 2022
@tguichaoua
Copy link
Contributor

tguichaoua commented Sep 14, 2022

I don't get where it's useful to ignore a variant of an enum.
If we consider enums like a safe tagged union, ignoring a variant is like rejecting a specific value for a field. For example, reject Vec2 when x is 0.
I'm in favor of the third option. We can use ignore_fields instead of ignore to make clear this is not the variant that is ignored but its fields.

@MrGVSV
Copy link
Member Author

MrGVSV commented Sep 14, 2022

I don't get where it's useful to ignore a variant of an enum.
If we consider enums like a safe tagged union, ignoring a variant is like rejecting a specific value for a field. For example, reject Vec2 when x is 0.

Yeah, I added it with the thought that maybe it could be used to separate "public" runtime variants from "private" serialized variants. But I didn't really go beyond that and realize how broken it could be if used incorrectly.

I'm in favor of the third option. We can use ignore_fields instead of ignore to make clear this is not the variant that is ignored but its fields.

Yeah this could work. However, I don't think it's a priority. So for sure remove the ability to ignore variants, but maybe add the ability to ignore all fields if there's a real need for it.

@MrGVSV
Copy link
Member Author

MrGVSV commented Oct 19, 2022

I'm going to close this issue as I think this behavior was actually removed by #5250.

@MrGVSV MrGVSV closed this as completed Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior
Projects
Status: Done
Development

No branches or pull requests

2 participants