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 #5145

Closed
wants to merge 7 commits into from

Conversation

Gingeh
Copy link
Contributor

@Gingeh Gingeh commented Jun 30, 2022

Objective

Implement Reflect for PhantomData. Fixes #5144.

Solution

Use the impl_reflect_value and impl_from_reflect_value macros to derive the implementation.

I hope I did this correctly.

@MrGVSV MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Jun 30, 2022
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Nice and simple!

Although, I think we should include PhantomData in some of the tests (or make some new ones). Specifically, serializing/deserializing and using FromReflect for it. I just want to make sure that it behaves properly considering it's not really data.

@Gingeh Gingeh requested a review from MrGVSV June 30, 2022 07:04
@james7132
Copy link
Member

I'm not sure this is the best approach. ZSTs by definition hold zero state, it cannot have fields, and references can be made out of thin air, yet this allows codegen and even serialization of them, which is definitely not zero cost. Is there worth in reflecting/knowing that there's a piece of PhantomData available?

@MrGVSV
Copy link
Member

MrGVSV commented Jun 30, 2022

Is there worth in reflecting/knowing that there's a piece of PhantomData available?

I don't think we gain anything other than not having to manually ignore it. It shouldn't contain any type information we don't already have on the struct itself.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 30, 2022
@alice-i-cecile
Copy link
Member

Yeah; I could go either way on doing this versus explicitly telling people to ignore it. The changes themselves are correct.

crates/bevy_reflect/src/impls/std.rs Show resolved Hide resolved
Comment on lines 602 to 607
let reflect_serialize = type_registry
.get_type_data::<ReflectSerialize>(
std::any::TypeId::of::<std::marker::PhantomData<()>>(),
)
.unwrap();
let _serializable = reflect_serialize.get_serializable(&std::marker::PhantomData::<()>);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to serialize to a string and compare the output with an assert_eq!. That way, we can actually test that the serialized version looks like it should (I suspect this is one of the places where the ReflectSerializer might fail).

Also can we put the PhantomData in a struct? Since that's its most likely use case.

@Gingeh
Copy link
Contributor Author

Gingeh commented Jul 1, 2022

I'm struggling to write a test for this (it probably doesn't help that I have almost no idea what I'm doing).
This is what I have so far:

#[test]
fn can_serialize_phantomdata() {
    use super::*;
    use crate::serde::{ReflectDeserializer, ReflectSerializer};
    use crate::DynamicStruct;
    use ::serde::de::DeserializeSeed;
    use ron::{
        ser::{to_string_pretty, PrettyConfig},
        Deserializer,
    };

    #[derive(Reflect, PartialEq, Eq, Debug)]
    struct Foo {
        a: u32,
        _phantomdata: PhantomData<u8>,
    }

    let foo = Foo {
        a: 1,
        _phantomdata: PhantomData,
    };

    let mut registry = TypeRegistry::default();
    registry.register::<u32>();
    registry.register::<PhantomData<u8>>();

    let serializer = ReflectSerializer::new(&foo, &registry);
    let serialized = to_string_pretty(&serializer, PrettyConfig::default()).unwrap();

    let mut deserializer = Deserializer::from_str(&serialized).unwrap();
    let reflect_deserializer = ReflectDeserializer::new(&registry);
    let value = reflect_deserializer.deserialize(&mut deserializer).unwrap();
    let dynamic_struct = value.take::<DynamicStruct>().unwrap();

    assert_eq!(serialized, "{\n    \"type\": \"bevy_reflect::impls::std::tests::can_serialize_phantomdata::Foo\",\n    \"struct\": {\n        \"a\": {\n            \"type\": \"u32\",\n            \"value\": 1,\n        },\n        \"_phantomdata\": {\n            \"type\": \"core::marker::PhantomData<u8>\",\n            \"value\": (),\n        },\n    },\n}");
    assert!(foo.reflect_partial_eq(&dynamic_struct).unwrap());
}

The first assertion succeeds, but the second fails.

@MrGVSV
Copy link
Member

MrGVSV commented Jul 1, 2022

The first assertion succeeds, but the second fails.

Hm, when I run this code, it doesn't even get to the first assertion. I get:

called `Result::unwrap()` on an `Err` value: Error { code: Message("Type 'core::marker::PhantomData<u8>' did not register ReflectSerialize"), position: Position { line: 0, col: 0 } }

So you'd need to register those:

impl_reflect_value!(PhantomData<T: Reflect>(Serialize, Deserialize));

Actually it would probably be best to also register Debug, Hash, and PartialEq as well.

A few more suggestions:

  • Foo should be generic with the PhantomData taking on that generic (i.e. PhantomData<T> instead of PhantomData<u8>) since this is how it will probably be used out in the wild
  • I would avoid using anything from glam in your tests since that hides your test behind that feature
  • You may or may not need to register Foo as well
  • If you're checking the serialized string against a single line literal, I would use ron::to_string over ron::to_string_pretty. Otherwise, you should probably configure it to take PrettyConfig::default().new_line("\n"). This is because CI could fail since it also uses carriage returns ("\r").

@MrGVSV
Copy link
Member

MrGVSV commented Jul 1, 2022

I'm struggling to write a test for this (it probably doesn't help that I have almost no idea what I'm doing).

Lol no worries! This is just a part of learning. Besides, any contribution— big or small, noob or pro, accepted or not— is always appreciated!

@Gingeh
Copy link
Contributor Author

Gingeh commented Jul 1, 2022

Thanks for the quick reply!

Hm, when I run this code, it doesn't even get to the first assertion. I get:

called `Result::unwrap()` on an `Err` value: Error { code: Message("Type 'core::marker::PhantomData<u8>' did not register ReflectSerialize"), position: Position { line: 0, col: 0 } }

So you'd need to register those:

impl_reflect_value!(PhantomData<T: Reflect>(Serialize, Deserialize));

That's very weird, I definitely registered those in an earlier commit, not sure why that isn't working.

Actually it would probably be best to also register Debug, Hash, and PartialEq as well.

Ah thank you! The second assertion passes now!

* `Foo` should be generic with the `PhantomData` taking on that generic (i.e. `PhantomData<T>` instead of `PhantomData<u8>`) since this is how it will probably be used out in the wild

Yeah I totally forgot to do that, changed it now.

* I would avoid using anything from `glam` in your tests since that hides your test behind that feature

I don't see any glam usage in my test?

* You may or may not need to register `Foo` as well

The test works without it, and the examples in the documentation don't register derived structs either.

Updated test:

#[test]
fn can_serialize_phantomdata() {
    use super::*;
    use crate::serde::{ReflectDeserializer, ReflectSerializer};
    use crate::DynamicStruct;
    use ::serde::de::DeserializeSeed;
    use ron::{ser::to_string, Deserializer};

    #[derive(Reflect, PartialEq, Eq, Debug)]
    struct Foo<T: Reflect> {
        a: u32,
        _phantomdata: PhantomData<T>,
    }

    let foo: Foo<u8> = Foo {
        a: 1,
        _phantomdata: PhantomData,
    };

    let mut registry = TypeRegistry::default();
    registry.register::<u32>();
    registry.register::<PhantomData<u8>>();

    let serializer = ReflectSerializer::new(&foo, &registry);
    let serialized = to_string(&serializer).unwrap();

    let mut deserializer = Deserializer::from_str(&serialized).unwrap();
    let reflect_deserializer = ReflectDeserializer::new(&registry);
    let value = reflect_deserializer.deserialize(&mut deserializer).unwrap();
    let dynamic_struct = value.take::<DynamicStruct>().unwrap();

    assert_eq!(serialized, "{\"type\":\"bevy_reflect::impls::std::tests::can_serialize_phantomdata::Foo<u8>\",\"struct\":{\"a\":{\"type\":\"u32\",\"value\":1},\"_phantomdata\":{\"type\":\"core::marker::PhantomData<u8>\",\"value\":()}}}");
    assert!(foo.reflect_partial_eq(&dynamic_struct).unwrap());
}

@MrGVSV
Copy link
Member

MrGVSV commented Jul 1, 2022

That's very weird, I definitely registered those in an earlier commit, not sure why that isn't working.

My bad, I copied and pasted the code but missed the other half of that commit.

I don't see any glam usage in my test?

Also my bad 😅. This was a complaint from a different test (not sure why it poked its head just now, must've been my testing setup).


But test works now so good job!

crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
let value = reflect_deserializer.deserialize(&mut deserializer).unwrap();
let dynamic_struct = value.take::<DynamicStruct>().unwrap();

assert_eq!(serialized, "{\"type\":\"bevy_reflect::impls::std::tests::can_serialize_phantomdata::Foo<u8>\",\"struct\":{\"a\":{\"type\":\"u32\",\"value\":1},\"_phantomdata\":{\"type\":\"core::marker::PhantomData<u8>\",\"value\":()}}}");
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this is the not-so-great part about reflecting PhantomData:

{
  "type": "core::marker::PhantomData<u8>",
  "value": ()
}

Ideally, we wouldn't need to include this in our RON or scene files since it's essentially just metadata for the compiler. To me, this is the biggest reason why we might not want to reflect PhantomData.

Copy link
Contributor Author

@Gingeh Gingeh Jul 1, 2022

Choose a reason for hiding this comment

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

I absolutely agree. I feel that this overhead, even if it is small, is not worth it for the minuscule benefit.

Unless we can manually implement Reflect to avoid this, I don't think it is worth implementing.

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Jul 1, 2022
@alice-i-cecile
Copy link
Member

I agree about disliking the added serialization overhead. I'm going to block this on #4561, and see if the situation is any better after that.

@jakobhellermann
Copy link
Contributor

I agree about disliking the added serialization overhead. I'm going to block this on #4561, and see if the situation is any better after that.

In my opinion if you don't want the PhantomData in your serialized output you should #[reflect(ignore)], and the changes in #4561 are orthogonal to this change.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 4, 2022

Right. That's user-configurable, so it's not nearly as bad as I was worried about. This still makes things "just work" out of the box much more comfortably. Maybe we should have a way to always ignore certain types?

@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Jul 4, 2022
@MrGVSV
Copy link
Member

MrGVSV commented Jul 4, 2022

Maybe we should have a way to always ignore certain types?

What other types would we want to ignore? This might just be added complexity for a relatively small subsection of use-cases 😅

@alice-i-cecile
Copy link
Member

I was thinking:

  • unit structs
  • PhantomData
  • Instant
  • Entity
  • potentially smart pointers like Rc/Arc/Mutex, although perhaps that should be forced to be explicit

Those are some pretty common cases, and it would nice to make the "easy" thing to do automatically align with the "correct" thing to do.

@PROMETHIA-27
Copy link
Contributor

I think I would prefer to simply suggest ignoring those explicitly for now. That gives people a better understanding of what's actually happening, instead of running into a "bug" until they finally realize that some unit struct they expected to be reflected wasn't. I do agree they shouldn't be reflectable, at least by default, though. Particularly PhantomData which is always just noise, where some unit structs might convey meaning.

@makspll
Copy link
Contributor

makspll commented Jul 9, 2022

Out of curiosity,
What's the reason for possibly automatically not reflecting entities?
Is this due to the id's being different after save/load cycles?

@alice-i-cecile
Copy link
Member

The problem is serializing them :) They can't meaningfully be deserialized, as the end user cannot control ID allocation.

@makspll
Copy link
Contributor

makspll commented Jul 9, 2022

Ah i see,
I guess this links to #5250 in that instead of completely ignoring those they could be ignored only during serialization

@Gingeh
Copy link
Contributor Author

Gingeh commented Sep 20, 2022

I guess discussion for this can continue now that #5723 and #5250 have been merged

@MrGVSV
Copy link
Member

MrGVSV commented Oct 17, 2022

Another consideration here is that implicitly ignoring types like PhantomData may result in the pesky relfected-index error, where ignoring a field throws off the indices for subsequent fields.

@maxwellodri
Copy link
Contributor

Perhaps rather than implementing Reflect we could have a more meaningful compile error telling the user to explicitly ignore PhantomData

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 targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

PhantomData does not implement Reflect
8 participants