Skip to content

Commit

Permalink
remove Serialize impl for dyn Array and friends (#4780)
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
jakobhellermann committed May 30, 2022
1 parent 1e8ca45 commit 4b7f904
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 63 deletions.
40 changes: 1 addition & 39 deletions crates/bevy_reflect/src/array.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use crate::{serde::Serializable, Reflect, ReflectMut, ReflectRef};
use serde::ser::SerializeSeq;
use std::fmt::Debug;
use std::{
any::Any,
Expand Down Expand Up @@ -145,7 +144,7 @@ unsafe impl Reflect for DynamicArray {
}

fn serializable(&self) -> Option<Serializable> {
Some(Serializable::Borrowed(self))
None
}
}

Expand Down Expand Up @@ -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> {
Expand Down
16 changes: 2 additions & 14 deletions crates/bevy_reflect/src/impls/std.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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
}
}

Expand All @@ -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.
Expand Down
11 changes: 1 addition & 10 deletions crates/bevy_reflect/src/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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>;
Expand Down

0 comments on commit 4b7f904

Please sign in to comment.