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

Implement Reflect for PhantomData #15313

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen commented Sep 19, 2024

Objective

Solution

  • impl_reflect! following the example

Testing

N/A
I don't see tests for the other core impl_reflects.

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Sep 19, 2024
@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 19, 2024
@BenjaminBrienen
Copy link
Contributor Author

#5145 (comment)

I'm curious why we want this now and not before.

@MrGVSV
Copy link
Member

MrGVSV commented Sep 19, 2024

#5145 (comment)

I'm curious why we want this now and not before.

It's still the same issues as before. I think the biggest problem is serialization. It really only adds noise to the serialized output. And if we make it Reflect, then it becomes easy for library authors to forget to ignore it themselves, which means downstream users will see it if they serialize it.

If do this, we may want to consider skipping it during serialization (and other operations such as reflect_partial_eq).

Copy link
Member

Choose a reason for hiding this comment

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

Also, can we add a serialization test for PhantomData in the serde module? Whether or not we're allowing this type to be serialized, we should ensure any type containing it can be deserialized and FromReflect-ed.

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Sep 19, 2024
@soqb
Copy link
Contributor

soqb commented Sep 20, 2024

do we want really need this? i feel like PhantomData is supposed to act as if its not there at all (its a 👻 after all). is there anything not accomplished by annotating #[reflect(ignore)] or is it simply an ergonomic improvement? i'd really rather not have all the "noise" that @MrGVSV mentioned (not just in serialization mind, also in runtime and binary size) just for the sake of a few less tokens to type out.

tldr; the implementation is correct but i'm not sure the feature is desirable.

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-Usability A simple quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PhantomData does not implement Reflect
4 participants