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

Implement Reflect for PhantomData #5145

Closed
wants to merge 7 commits into from
Closed

Implement Reflect for PhantomData #5145

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7110c67
Implement Reflect for PhantomData
Gingeh Jun 30, 2022
437479c
Reformat to appease the CI
Gingeh Jun 30, 2022
428c0a9
Add serializability test for PhantomData
Gingeh Jun 30, 2022
bf778ad
Fix formatting (again)
Gingeh Jun 30, 2022
6478671
Replace the PhantomData reflection test
Gingeh Jul 1, 2022
1b277aa
Update crates/bevy_reflect/src/impls/std.rs
Gingeh Jul 1, 2022
a21425a
Fix formatting
Gingeh Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions crates/bevy_reflect/src/impls/std.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use std::{
any::Any,
borrow::Cow,
hash::{Hash, Hasher},
marker::PhantomData,
ops::Range,
};

Expand All @@ -37,6 +38,7 @@ impl_reflect_value!(Option<T: Serialize + Clone + for<'de> Deserialize<'de> + Re
impl_reflect_value!(HashSet<T: Serialize + Hash + Eq + Clone + for<'de> Deserialize<'de> + Send + Sync + 'static>(Serialize, Deserialize));
impl_reflect_value!(Range<T: Serialize + Clone + for<'de> Deserialize<'de> + Send + Sync + 'static>(Serialize, Deserialize));
impl_reflect_value!(Duration(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(PhantomData<T: Reflect>(Debug, Hash, PartialEq, Serialize, Deserialize));

impl_from_reflect_value!(bool);
impl_from_reflect_value!(char);
Expand Down Expand Up @@ -65,6 +67,7 @@ impl_from_reflect_value!(
Range<T: Serialize + Clone + for<'de> Deserialize<'de> + Send + Sync + 'static>
);
impl_from_reflect_value!(Duration);
impl_from_reflect_value!(PhantomData<T: Reflect>);

impl<T: FromReflect> Array for Vec<T> {
#[inline]
Expand Down Expand Up @@ -576,8 +579,13 @@ impl FromReflect for Cow<'static, str> {

#[cfg(test)]
mod tests {
use super::*;
use crate::serde::{ReflectDeserializer, ReflectSerializer};
use crate::DynamicStruct;
use crate::{Reflect, ReflectSerialize, TypeRegistry};
use ::serde::de::DeserializeSeed;
use bevy_utils::HashMap;
use ron::{ser::to_string, Deserializer};
use std::f32::consts::{PI, TAU};

#[test]
Expand All @@ -591,6 +599,35 @@ mod tests {
let _serializable = reflect_serialize.get_serializable(&std::time::Duration::ZERO);
}

#[test]
fn can_serialize_phantomdata() {
Gingeh marked this conversation as resolved.
Show resolved Hide resolved
#[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\":()}}}");
Gingeh marked this conversation as resolved.
Show resolved Hide resolved
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 •
edited
Loading

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.

assert!(foo.reflect_partial_eq(&dynamic_struct).unwrap());
}

#[test]
fn should_partial_eq_char() {
let a: &dyn Reflect = &'x';
Expand Down