bevy_reflect: put serialize into external ReflectSerialize type (#4782)

jakobhellermann · jakobhellermann · commit 218b0fd3b6ed · 2022-06-20T17:18:58.000Z
builds on top of #4780 # Objective `Reflect` and `Serialize` are currently very tied together because `Reflect` has a `fn serialize(&self) -> Option<Serializable<'_>>` method. Because of that, we can either implement `Reflect` for types like `Option<T>` with `T: Serialize` and have `fn serialize` be implemented, or without the bound but having `fn serialize` return `None`. By separating `ReflectSerialize` into a separate type (like how it already is for `ReflectDeserialize`, `ReflectDefault`), we could separately `.register::<Option<T>>()` and `.register_data::<Option<T>, ReflectSerialize>()` only if the type `T: Serialize`. This PR does not change the registration but allows it to be changed in a future PR. ## Solution - add the type ```rust struct ReflectSerialize { .. } impl<T: Reflect + Serialize> FromType<T> for ReflectSerialize { .. } ``` - remove `#[reflect(Serialize)]` special casing. - when serializing reflect value types, look for `ReflectSerialize` in the `TypeRegistry` instead of calling `value.serialize()`
diff --git a/crates/bevy_asset/src/handle.rs b/crates/bevy_asset/src/handle.rs
@@ -10,7 +10,7 @@ use crate::{
     Asset, Assets,
 };
 use bevy_ecs::{component::Component, reflect::ReflectComponent};
-use bevy_reflect::{FromReflect, Reflect, ReflectDeserialize};
+use bevy_reflect::{FromReflect, Reflect, ReflectDeserialize, ReflectSerialize};
 use bevy_utils::Uuid;
 use crossbeam_channel::{Receiver, Sender};
 use serde::{Deserialize, Serialize};
diff --git a/crates/bevy_asset/src/path.rs b/crates/bevy_asset/src/path.rs
@@ -1,4 +1,4 @@
-use bevy_reflect::{Reflect, ReflectDeserialize};
+use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize};
 use bevy_utils::AHasher;
 use serde::{Deserialize, Serialize};
 use std::{
diff --git a/crates/bevy_core_pipeline/src/clear_color.rs b/crates/bevy_core_pipeline/src/clear_color.rs
@@ -1,6 +1,6 @@
 use bevy_derive::{Deref, DerefMut};
 use bevy_ecs::prelude::*;
-use bevy_reflect::{Reflect, ReflectDeserialize};
+use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize};
 use bevy_render::{color::Color, extract_resource::ExtractResource};
 use serde::{Deserialize, Serialize};
 
diff --git a/crates/bevy_core_pipeline/src/core_3d/camera_3d.rs b/crates/bevy_core_pipeline/src/core_3d/camera_3d.rs
@@ -1,6 +1,6 @@
 use crate::clear_color::ClearColorConfig;
 use bevy_ecs::{prelude::*, query::QueryItem};
-use bevy_reflect::{Reflect, ReflectDeserialize};
+use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize};
 use bevy_render::{
     camera::{Camera, CameraRenderGraph, Projection},
     extract_component::ExtractComponent,
diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs
@@ -8,6 +8,7 @@ use crate::{
 };
 use bevy_reflect::{
     impl_from_reflect_value, impl_reflect_value, FromType, Reflect, ReflectDeserialize,
+    ReflectSerialize,
 };
 
 #[derive(Clone)]
diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs b/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs
@@ -18,7 +18,6 @@ use syn::{Meta, NestedMeta, Path};
 const DEBUG_ATTR: &str = "Debug";
 const PARTIAL_EQ_ATTR: &str = "PartialEq";
 const HASH_ATTR: &str = "Hash";
-const SERIALIZE_ATTR: &str = "Serialize";
 
 // The traits listed below are not considered "special" (i.e. they use the `ReflectMyTrait` syntax)
 // but useful to know exist nonetheless
@@ -54,7 +53,6 @@ impl Default for TraitImpl {
 /// * `Debug`
 /// * `Hash`
 /// * `PartialEq`
-/// * `Serialize`
 ///
 /// When registering a trait, there are a few things to keep in mind:
 /// * Traits must have a valid `Reflect{}` struct in scope. For example, `Default`
@@ -110,7 +108,6 @@ pub(crate) struct ReflectTraits {
     debug: TraitImpl,
     hash: TraitImpl,
     partial_eq: TraitImpl,
-    serialize: TraitImpl,
     idents: Vec<Ident>,
 }
 
@@ -133,7 +130,6 @@ impl ReflectTraits {
                         DEBUG_ATTR => traits.debug = TraitImpl::Implemented,
                         PARTIAL_EQ_ATTR => traits.partial_eq = TraitImpl::Implemented,
                         HASH_ATTR => traits.hash = TraitImpl::Implemented,
-                        SERIALIZE_ATTR => traits.serialize = TraitImpl::Implemented,
                         // We only track reflected idents for traits not considered special
                         _ => traits.idents.push(utility::get_reflect_ident(&ident)),
                     }
@@ -156,7 +152,6 @@ impl ReflectTraits {
                                 DEBUG_ATTR => traits.debug = trait_func_ident,
                                 PARTIAL_EQ_ATTR => traits.partial_eq = trait_func_ident,
                                 HASH_ATTR => traits.hash = trait_func_ident,
-                                SERIALIZE_ATTR => traits.serialize = trait_func_ident,
                                 _ => {}
                             }
                         }
@@ -230,25 +225,6 @@ impl ReflectTraits {
         }
     }
 
-    /// Returns the implementation of `Reflect::serializable` as a `TokenStream`.
-    ///
-    /// If `Serialize` was not registered, returns `None`.
-    pub fn get_serialize_impl(&self, bevy_reflect_path: &Path) -> Option<proc_macro2::TokenStream> {
-        match &self.serialize {
-            TraitImpl::Implemented => Some(quote! {
-                fn serializable(&self) -> Option<#bevy_reflect_path::serde::Serializable> {
-                    Some(#bevy_reflect_path::serde::Serializable::Borrowed(self))
-                }
-            }),
-            TraitImpl::Custom(impl_fn) => Some(quote! {
-                fn serializable(&self) -> Option<#bevy_reflect_path::serde::Serializable> {
-                    Some(#impl_fn(self))
-                }
-            }),
-            TraitImpl::NotImplemented => None,
-        }
-    }
-
     /// Returns the implementation of `Reflect::debug` as a `TokenStream`.
     ///
     /// If `Debug` was not registered, returns `None`.
diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs b/crates/bevy_reflect/bevy_reflect_derive/src/impls.rs
@@ -40,7 +40,6 @@ pub(crate) fn impl_struct(derive_data: &ReflectDeriveData) -> TokenStream {
     let field_indices = (0..field_count).collect::<Vec<usize>>();
 
     let hash_fn = derive_data.traits().get_hash_impl(bevy_reflect_path);
-    let serialize_fn = derive_data.traits().get_serialize_impl(bevy_reflect_path);
     let partial_eq_fn = derive_data
         .traits()
         .get_partial_eq_impl(bevy_reflect_path)
@@ -192,8 +191,6 @@ pub(crate) fn impl_struct(derive_data: &ReflectDeriveData) -> TokenStream {
             #partial_eq_fn
 
             #debug_fn
-
-            #serialize_fn
         }
     })
 }
@@ -216,7 +213,6 @@ pub(crate) fn impl_tuple_struct(derive_data: &ReflectDeriveData) -> TokenStream
     let field_indices = (0..field_count).collect::<Vec<usize>>();
 
     let hash_fn = derive_data.traits().get_hash_impl(bevy_reflect_path);
-    let serialize_fn = derive_data.traits().get_serialize_impl(bevy_reflect_path);
     let partial_eq_fn = derive_data
         .traits()
         .get_partial_eq_impl(bevy_reflect_path)
@@ -344,8 +340,6 @@ pub(crate) fn impl_tuple_struct(derive_data: &ReflectDeriveData) -> TokenStream
             #partial_eq_fn
 
             #debug_fn
-
-            #serialize_fn
         }
     })
 }
@@ -359,7 +353,6 @@ pub(crate) fn impl_value(
     reflect_traits: &ReflectTraits,
 ) -> TokenStream {
     let hash_fn = reflect_traits.get_hash_impl(bevy_reflect_path);
-    let serialize_fn = reflect_traits.get_serialize_impl(bevy_reflect_path);
     let partial_eq_fn = reflect_traits.get_partial_eq_impl(bevy_reflect_path);
     let debug_fn = reflect_traits.get_debug_impl();
 
@@ -445,8 +438,6 @@ pub(crate) fn impl_value(
             #partial_eq_fn
 
             #debug_fn
-
-            #serialize_fn
         }
     })
 }
diff --git a/crates/bevy_reflect/src/array.rs b/crates/bevy_reflect/src/array.rs
@@ -1,6 +1,5 @@
 use crate::{
-    serde::Serializable, utility::NonGenericTypeInfoCell, DynamicInfo, Reflect, ReflectMut,
-    ReflectRef, TypeInfo, Typed,
+    utility::NonGenericTypeInfoCell, DynamicInfo, Reflect, ReflectMut, ReflectRef, TypeInfo, Typed,
 };
 use std::{
     any::{Any, TypeId},
@@ -217,10 +216,6 @@ unsafe impl Reflect for DynamicArray {
     fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option<bool> {
         array_partial_eq(self, value)
     }
-
-    fn serializable(&self) -> Option<Serializable> {
-        None
-    }
 }
 
 impl Array for DynamicArray {
diff --git a/crates/bevy_reflect/src/impls/glam.rs b/crates/bevy_reflect/src/impls/glam.rs
@@ -1,7 +1,7 @@
 use crate as bevy_reflect;
 use crate::prelude::ReflectDefault;
 use crate::reflect::Reflect;
-use crate::ReflectDeserialize;
+use crate::{ReflectDeserialize, ReflectSerialize};
 use bevy_reflect_derive::{impl_from_reflect_value, impl_reflect_struct, impl_reflect_value};
 use glam::*;
 
diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs
@@ -1,8 +1,8 @@
 use crate as bevy_reflect;
 use crate::{
-    map_partial_eq, serde::Serializable, Array, ArrayInfo, ArrayIter, DynamicMap, FromReflect,
-    FromType, GetTypeRegistration, List, ListInfo, Map, MapInfo, MapIter, Reflect,
-    ReflectDeserialize, ReflectMut, ReflectRef, TypeInfo, TypeRegistration, Typed, ValueInfo,
+    map_partial_eq, Array, ArrayInfo, ArrayIter, DynamicMap, FromReflect, FromType,
+    GetTypeRegistration, List, ListInfo, Map, MapInfo, MapIter, Reflect, ReflectDeserialize,
+    ReflectMut, ReflectRef, ReflectSerialize, TypeInfo, TypeRegistration, Typed, ValueInfo,
 };
 
 use crate::utility::{GenericTypeInfoCell, NonGenericTypeInfoCell};
@@ -159,10 +159,6 @@ unsafe impl<T: FromReflect> Reflect for Vec<T> {
     fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option<bool> {
         crate::list_partial_eq(self, value)
     }
-
-    fn serializable(&self) -> Option<Serializable> {
-        None
-    }
 }
 
 impl<T: FromReflect> Typed for Vec<T> {
@@ -420,11 +416,6 @@ unsafe impl<T: Reflect, const N: usize> Reflect for [T; N] {
     fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option<bool> {
         crate::array_partial_eq(self, value)
     }
-
-    #[inline]
-    fn serializable(&self) -> Option<Serializable> {
-        None
-    }
 }
 
 impl<T: FromReflect, const N: usize> FromReflect for [T; N] {
@@ -541,10 +532,6 @@ unsafe impl Reflect for Cow<'static, str> {
             Some(false)
         }
     }
-
-    fn serializable(&self) -> Option<Serializable> {
-        Some(Serializable::Borrowed(self))
-    }
 }
 
 impl Typed for Cow<'static, str> {
@@ -558,6 +545,7 @@ impl GetTypeRegistration for Cow<'static, str> {
     fn get_type_registration() -> TypeRegistration {
         let mut registration = TypeRegistration::of::<Cow<'static, str>>();
         registration.insert::<ReflectDeserialize>(FromType::<Cow<'static, str>>::from_type());
+        registration.insert::<ReflectSerialize>(FromType::<Cow<'static, str>>::from_type());
         registration
     }
 }
@@ -570,13 +558,19 @@ impl FromReflect for Cow<'static, str> {
 
 #[cfg(test)]
 mod tests {
-    use crate::Reflect;
+    use crate::{Reflect, ReflectSerialize, TypeRegistry};
     use bevy_utils::HashMap;
     use std::f32::consts::{PI, TAU};
 
     #[test]
     fn can_serialize_duration() {
-        assert!(std::time::Duration::ZERO.serializable().is_some());
+        let mut type_registry = TypeRegistry::default();
+        type_registry.register::<std::time::Duration>();
+
+        let reflect_serialize = type_registry
+            .get_type_data::<ReflectSerialize>(std::any::TypeId::of::<std::time::Duration>())
+            .unwrap();
+        let _serializable = reflect_serialize.get_serializable(&std::time::Duration::ZERO);
     }
 
     #[test]
diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs
@@ -34,8 +34,8 @@ pub mod prelude {
     pub use crate::std_traits::*;
     #[doc(hidden)]
     pub use crate::{
-        reflect_trait, GetField, GetTupleStructField, Reflect, ReflectDeserialize, Struct,
-        TupleStruct,
+        reflect_trait, GetField, GetTupleStructField, Reflect, ReflectDeserialize,
+        ReflectSerialize, Struct, TupleStruct,
     };
 }
 
@@ -877,7 +877,8 @@ bevy_reflect::tests::should_reflect_debug::Test {
             let v = vec3(12.0, 3.0, -6.9);
 
             let mut registry = TypeRegistry::default();
-            registry.add_registration(Vec3::get_type_registration());
+            registry.register::<f32>();
+            registry.register::<Vec3>();
 
             let ser = ReflectSerializer::new(&v, &registry);
 
diff --git a/crates/bevy_reflect/src/list.rs b/crates/bevy_reflect/src/list.rs
@@ -3,8 +3,8 @@ use std::fmt::{Debug, Formatter};
 
 use crate::utility::NonGenericTypeInfoCell;
 use crate::{
-    serde::Serializable, Array, ArrayIter, DynamicArray, DynamicInfo, FromReflect, Reflect,
-    ReflectMut, ReflectRef, TypeInfo, Typed,
+    Array, ArrayIter, DynamicArray, DynamicInfo, FromReflect, Reflect, ReflectMut, ReflectRef,
+    TypeInfo, Typed,
 };
 
 /// An ordered, mutable list of [Reflect] items. This corresponds to types like [`std::vec::Vec`].
@@ -229,10 +229,6 @@ unsafe impl Reflect for DynamicList {
         list_partial_eq(self, value)
     }
 
-    fn serializable(&self) -> Option<Serializable> {
-        None
-    }
-
     fn debug(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         write!(f, "DynamicList(")?;
         list_debug(self, f)?;
diff --git a/crates/bevy_reflect/src/serde/ser.rs b/crates/bevy_reflect/src/serde/ser.rs
@@ -1,6 +1,6 @@
 use crate::{
-    serde::type_fields, Array, List, Map, Reflect, ReflectRef, Struct, Tuple, TupleStruct,
-    TypeRegistry,
+    serde::type_fields, Array, List, Map, Reflect, ReflectRef, ReflectSerialize, Struct, Tuple,
+    TupleStruct, TypeRegistry,
 };
 use serde::{
     ser::{SerializeMap, SerializeSeq},
@@ -22,13 +22,19 @@ impl<'a> Serializable<'a> {
     }
 }
 
-fn get_serializable<E: serde::ser::Error>(reflect_value: &dyn Reflect) -> Result<Serializable, E> {
-    reflect_value.serializable().ok_or_else(|| {
-        serde::ser::Error::custom(format_args!(
-            "Type '{}' does not support ReflectValue serialization",
-            reflect_value.type_name()
-        ))
-    })
+fn get_serializable<'a, E: serde::ser::Error>(
+    reflect_value: &'a dyn Reflect,
+    type_registry: &TypeRegistry,
+) -> Result<Serializable<'a>, E> {
+    let reflect_serialize = type_registry
+        .get_type_data::<ReflectSerialize>(reflect_value.type_id())
+        .ok_or_else(|| {
+            serde::ser::Error::custom(format_args!(
+                "Type '{}' did not register ReflectSerialize",
+                reflect_value.type_name()
+            ))
+        })?;
+    Ok(reflect_serialize.get_serializable(reflect_value))
 }
 
 pub struct ReflectSerializer<'a> {
@@ -101,7 +107,7 @@ impl<'a> Serialize for ReflectValueSerializer<'a> {
         state.serialize_entry(type_fields::TYPE, self.value.type_name())?;
         state.serialize_entry(
             type_fields::VALUE,
-            get_serializable::<S::Error>(self.value)?.borrow(),
+            get_serializable::<S::Error>(self.value, self.registry)?.borrow(),
         )?;
         state.end()
     }
diff --git a/crates/bevy_reflect/src/type_registry.rs b/crates/bevy_reflect/src/type_registry.rs
@@ -1,3 +1,4 @@
+use crate::serde::Serializable;
 use crate::{Reflect, TypeInfo, Typed};
 use bevy_utils::{HashMap, HashSet};
 use downcast_rs::{impl_downcast, Downcast};
@@ -389,6 +390,35 @@ pub trait FromType<T> {
     fn from_type() -> Self;
 }
 
+/// A struct used to serialize reflected instances of a type.
+///
+/// A `ReflectSerialize` for type `T` can be obtained via
+/// [`FromType::from_type`].
+#[derive(Clone)]
+pub struct ReflectSerialize {
+    get_serializable: for<'a> fn(value: &'a dyn Reflect) -> Serializable,
+}
+
+impl<T: Reflect + erased_serde::Serialize> FromType<T> for ReflectSerialize {
+    fn from_type() -> Self {
+        ReflectSerialize {
+            get_serializable: |value| {
+                let value = value.downcast_ref::<T>().unwrap_or_else(|| {
+                    panic!("ReflectSerialize::get_serialize called with type `{}`, even though it was created for `{}`", value.type_name(), std::any::type_name::<T>())
+                });
+                Serializable::Borrowed(value)
+            },
+        }
+    }
+}
+
+impl ReflectSerialize {
+    /// Turn the value into a serializable representation
+    pub fn get_serializable<'a>(&self, value: &'a dyn Reflect) -> Serializable<'a> {
+        (self.get_serializable)(value)
+    }
+}
+
 /// A struct used to deserialize reflected instances of a type.
 ///
 /// A `ReflectDeserialize` for type `T` can be obtained via
diff --git a/crates/bevy_render/src/camera/projection.rs b/crates/bevy_render/src/camera/projection.rs
@@ -4,7 +4,9 @@ use super::DepthCalculation;
 use bevy_app::{App, CoreStage, Plugin, StartupStage};
 use bevy_ecs::{prelude::*, reflect::ReflectComponent};
 use bevy_math::Mat4;
-use bevy_reflect::{std_traits::ReflectDefault, GetTypeRegistration, Reflect, ReflectDeserialize};
+use bevy_reflect::{
+    std_traits::ReflectDefault, GetTypeRegistration, Reflect, ReflectDeserialize, ReflectSerialize,
+};
 use bevy_window::ModifiesWindows;
 use serde::{Deserialize, Serialize};
 
diff --git a/crates/bevy_render/src/color/mod.rs b/crates/bevy_render/src/color/mod.rs
diff --git a/crates/bevy_ui/src/focus.rs b/crates/bevy_ui/src/focus.rs
diff --git a/crates/bevy_ui/src/widget/image.rs b/crates/bevy_ui/src/widget/image.rs

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-use bevy_reflect::{Reflect, ReflectDeserialize};`
	`1`	`+use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize};`
`2`	`2`	`use bevy_utils::AHasher;`
`3`	`3`	`use serde::{Deserialize, Serialize};`
`4`	`4`	`use std::{`