From 4b7f904cfc97ca901d649abe801e5a1e681cb040 Mon Sep 17 00:00:00 2001
From: Jakob Hellermann <jakob.hellermann@protonmail.com>
Date: Mon, 30 May 2022 20:22:57 +0000
Subject: [PATCH] remove `Serialize` impl for dyn Array and friends (#4780)

# Objective

`bevy_reflect` as different kinds of reflected types (each with their own trait), `trait Struct: Reflect`, `trait List: Reflect`, `trait Map: Reflect`, ...
Types that don't fit either of those are called reflect value types, they are opaque and can't be deconstructed further.

`bevy_reflect` can serialize `dyn Reflect` values. Any container types (struct, list, map) get deconstructed and their elements serialized separately, which can all happen without serde being involved ever (happens [here](https://github.com/bevyengine/bevy/blob/main/crates/bevy_reflect/src/serde/ser.rs#L50-L85=)).
 The only point at which we require types to be serde-serializable is for *value types* (happens [here](https://github.com/bevyengine/bevy/blob/main/crates/bevy_reflect/src/serde/ser.rs#L104=)).

So reflect array serializing is solved, since arrays are container types which don't require serde.

#1213 also introduced added the `serialize` method and `Serialize` impls for `dyn Array` and `DynamicArray` which use their element's `Reflect::serializable` function. This is 1. unnecessary, because it is not used for array serialization, and 2. annoying for removing the `Serialize` bound on container types, because these impls don't have access to the `TypeRegistry`, so we can't move the serialization code there.

# Solution

Remove these impls and `fn serialize`. It's not used and annoying for other changes.
---
 crates/bevy_reflect/src/array.rs     | 40 +---------------------------
 crates/bevy_reflect/src/impls/std.rs | 16 ++---------
 crates/bevy_reflect/src/list.rs      | 11 +-------
 3 files changed, 4 insertions(+), 63 deletions(-)

diff --git a/crates/bevy_reflect/src/array.rs b/crates/bevy_reflect/src/array.rs
index 74e0da561d353..93fc0450369e4 100644
--- a/crates/bevy_reflect/src/array.rs
+++ b/crates/bevy_reflect/src/array.rs
@@ -1,5 +1,4 @@
 use crate::{serde::Serializable, Reflect, ReflectMut, ReflectRef};
-use serde::ser::SerializeSeq;
 use std::fmt::Debug;
 use std::{
     any::Any,
@@ -145,7 +144,7 @@ unsafe impl Reflect for DynamicArray {
     }
 
     fn serializable(&self) -> Option<Serializable> {
-        Some(Serializable::Borrowed(self))
+        None
     }
 }
 
@@ -211,43 +210,6 @@ impl<'a> Iterator for ArrayIter<'a> {
 
 impl<'a> ExactSizeIterator for ArrayIter<'a> {}
 
-impl serde::Serialize for dyn Array {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: serde::Serializer,
-    {
-        array_serialize(self, serializer)
-    }
-}
-
-impl serde::Serialize for DynamicArray {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: serde::Serializer,
-    {
-        array_serialize(self, serializer)
-    }
-}
-
-/// Serializes the given [array](Array).
-#[inline]
-pub fn array_serialize<A: Array + ?Sized, S>(array: &A, serializer: S) -> Result<S::Ok, S::Error>
-where
-    S: serde::Serializer,
-{
-    let mut seq = serializer.serialize_seq(Some(array.len()))?;
-    for element in array.iter() {
-        let serializable = element.serializable().ok_or_else(|| {
-            serde::ser::Error::custom(format!(
-                "Type '{}' does not support `Reflect` serialization",
-                element.type_name()
-            ))
-        })?;
-        seq.serialize_element(serializable.borrow())?;
-    }
-    seq.end()
-}
-
 /// Returns the `u64` hash of the given [array](Array).
 #[inline]
 pub fn array_hash<A: Array>(array: &A) -> Option<u64> {
diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs
index f8614cdb0078c..a0d167966019b 100644
--- a/crates/bevy_reflect/src/impls/std.rs
+++ b/crates/bevy_reflect/src/impls/std.rs
@@ -156,7 +156,7 @@ unsafe impl<T: FromReflect> Reflect for Vec<T> {
     }
 
     fn serializable(&self) -> Option<Serializable> {
-        Some(Serializable::Owned(Box::new(SerializeArrayLike(self))))
+        None
     }
 }
 
@@ -396,7 +396,7 @@ unsafe impl<T: Reflect, const N: usize> Reflect for [T; N] {
 
     #[inline]
     fn serializable(&self) -> Option<Serializable> {
-        Some(Serializable::Owned(Box::new(SerializeArrayLike(self))))
+        None
     }
 }
 
@@ -414,18 +414,6 @@ impl<T: FromReflect, const N: usize> FromReflect for [T; N] {
     }
 }
 
-// Supports dynamic serialization for types that implement `Array`.
-struct SerializeArrayLike<'a>(&'a dyn Array);
-
-impl<'a> serde::Serialize for SerializeArrayLike<'a> {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: serde::Serializer,
-    {
-        crate::array_serialize(self.0, serializer)
-    }
-}
-
 // TODO:
 // `FromType::from_type` requires `Deserialize<'de>` to be implemented for `T`.
 // Currently serde only supports `Deserialize<'de>` for arrays up to size 32.
diff --git a/crates/bevy_reflect/src/list.rs b/crates/bevy_reflect/src/list.rs
index a3fabcc5edf30..3ea8d6050f666 100644
--- a/crates/bevy_reflect/src/list.rs
+++ b/crates/bevy_reflect/src/list.rs
@@ -166,7 +166,7 @@ unsafe impl Reflect for DynamicList {
     }
 
     fn serializable(&self) -> Option<Serializable> {
-        Some(Serializable::Borrowed(self))
+        None
     }
 
     fn debug(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
@@ -182,15 +182,6 @@ impl Debug for DynamicList {
     }
 }
 
-impl serde::Serialize for DynamicList {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: serde::Serializer,
-    {
-        crate::array_serialize(self, serializer)
-    }
-}
-
 impl IntoIterator for DynamicList {
     type Item = Box<dyn Reflect>;
     type IntoIter = std::vec::IntoIter<Self::Item>;