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

Return a static string for Enum::variant_name #12504

Closed

Conversation

cBournhonesque
Copy link
Contributor

@cBournhonesque cBournhonesque commented Mar 16, 2024

Objective

The reflect::Enum::variant_name() should return a static string; the name returned shouldn't depend on the lifetime of the enum itself. I'm using reflection with Enums in a library and I was expecting the variant_name function to give me a string that is indenpendent to my object's lifetime

Caveats:

  • are there some cases where we wouldn't want to return a &'static str?
  • we should probably have the same thing for reflect::Enum::name_at() but I wasn't able to get it working because of DynamicEnum

Migration Guide

  • reflect::DynamicEnum now requires a &'static str for the variant_name instead of a String

@cBournhonesque cBournhonesque added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Mar 16, 2024
@james7132 james7132 added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 16, 2024
@@ -135,13 +135,13 @@ impl DynamicEnum {
}

/// Set the current enum variant represented by this struct.
pub fn set_variant<I: Into<String>, V: Into<DynamicVariant>>(&mut self, name: I, variant: V) {
pub fn set_variant<I: Into<&'static str>, V: Into<DynamicVariant>>(&mut self, name: I, variant: V) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern is: how would a user generate these dynamically?

For example, they might have their own custom deserialization logic which returns a String. Or maybe they're parsing a script that contains non-Rust enum types.

They could intern their strings, but I'm not sure we want to require that until we have a better idea of how we might want to handle these dynamic cases. And I have a feeling we won't be able to do that until we see Unique Reflect in action.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One approach would be to leak the memory of the string? Set_variant shouldn't be called very often.

I guess it's problematic to return &'static str for variant_names in the DynamicEnum case..

@alice-i-cecile
Copy link
Member

Yeah, I'm similarly worried about how this interacts with dynamic enums. Unless we can prove that this is fine, I don't think we should merge this.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Mar 17, 2024
@cBournhonesque
Copy link
Contributor Author

Will close for now since there's issues related to DynamicEnum

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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants