Skip to content

Commit

Permalink
bevy_reflect: Fix binary deserialization not working for unit structs (…
Browse files Browse the repository at this point in the history 
…#6722)

# Objective 

Fixes #6713

Binary deserialization is failing for unit structs as well as structs with all ignored/skipped fields.

## Solution

Add a check for the number of possible fields in a struct before deserializing. If empty, don't attempt to deserialize any fields (as there will be none).

Note: ~~This does not apply to enums as they do not properly handle skipped fields (see #6721).~~ Enums still do not properly handle skipped fields, but I decided to include the logic for it anyways to account for `#[reflect(ignore)]`'d fields in the meantime.

---

## Changelog

- Fix bug where deserializing unit structs would fail for non-self-describing formats
  • Loading branch information
@MrGVSV @cart
MrGVSV authored and cart committed Nov 30, 2022
1 parent 0df3b19 commit 7c17f3f
Show file tree
Hide file tree
Showing 2 changed files with 193 additions and 18 deletions.
135 changes: 126 additions & 9 deletions crates/bevy_reflect/src/serde/de.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,7 @@ impl<'a, 'de> DeserializeSeed<'de> for TypedReflectDeserializer<'a> {
struct_info.field_names(),
StructVisitor {
struct_info,
registration: self.registration,
registry: self.registry,
},
)?;
Expand Down Expand Up @@ -411,6 +412,7 @@ impl<'a, 'de> DeserializeSeed<'de> for TypedReflectDeserializer<'a> {
enum_info.variant_names(),
EnumVisitor {
enum_info,
registration: self.registration,
registry: self.registry,
},
)?
Expand Down Expand Up @@ -439,6 +441,7 @@ impl<'a, 'de> DeserializeSeed<'de> for TypedReflectDeserializer<'a> {

struct StructVisitor<'a> {
struct_info: &'static StructInfo,
registration: &'a TypeRegistration,
registry: &'a TypeRegistry,
}

Expand All @@ -463,6 +466,18 @@ impl<'a, 'de> Visitor<'de> for StructVisitor<'a> {
let mut index = 0usize;
let mut output = DynamicStruct::default();

let ignored_len = self
.registration
.data::<SerializationData>()
.map(|data| data.len())
.unwrap_or(0);
let field_len = self.struct_info.field_len().saturating_sub(ignored_len);

if field_len == 0 {
// Handle unit structs and ignored fields
return Ok(output);
}

while let Some(value) = seq.next_element_seed(TypedReflectDeserializer {
registration: self
.struct_info
Expand Down Expand Up @@ -501,6 +516,21 @@ impl<'a, 'de> Visitor<'de> for TupleStructVisitor<'a> {
let mut index = 0usize;
let mut tuple_struct = DynamicTupleStruct::default();

let ignored_len = self
.registration
.data::<SerializationData>()
.map(|data| data.len())
.unwrap_or(0);
let field_len = self
.tuple_struct_info
.field_len()
.saturating_sub(ignored_len);

if field_len == 0 {
// Handle unit structs and ignored fields
return Ok(tuple_struct);
}

let get_field_registration = |index: usize| -> Result<&'a TypeRegistration, V::Error> {
let field = self.tuple_struct_info.field_at(index).ok_or_else(|| {
de::Error::custom(format_args!(
Expand Down Expand Up @@ -675,6 +705,7 @@ impl<'a, 'de> Visitor<'de> for MapVisitor<'a> {

struct EnumVisitor<'a> {
enum_info: &'static EnumInfo,
registration: &'a TypeRegistration,
registry: &'a TypeRegistry,
}

Expand All @@ -701,6 +732,7 @@ impl<'a, 'de> Visitor<'de> for EnumVisitor<'a> {
struct_info.field_names(),
StructVariantVisitor {
struct_info,
registration: self.registration,
registry: self.registry,
},
)?
Expand All @@ -722,6 +754,7 @@ impl<'a, 'de> Visitor<'de> for EnumVisitor<'a> {
tuple_info.field_len(),
TupleVariantVisitor {
tuple_info,
registration: self.registration,
registry: self.registry,
},
)?
Expand Down Expand Up @@ -787,6 +820,7 @@ impl<'de> DeserializeSeed<'de> for VariantDeserializer {

struct StructVariantVisitor<'a> {
struct_info: &'static StructVariantInfo,
registration: &'a TypeRegistration,
registry: &'a TypeRegistry,
}

Expand All @@ -811,6 +845,18 @@ impl<'a, 'de> Visitor<'de> for StructVariantVisitor<'a> {
let mut index = 0usize;
let mut output = DynamicStruct::default();

let ignored_len = self
.registration
.data::<SerializationData>()
.map(|data| data.len())
.unwrap_or(0);
let field_len = self.struct_info.field_len().saturating_sub(ignored_len);

if field_len == 0 {
// Handle all fields being ignored
return Ok(output);
}

while let Some(value) = seq.next_element_seed(TypedReflectDeserializer {
registration: self
.struct_info
Expand All @@ -831,6 +877,7 @@ impl<'a, 'de> Visitor<'de> for StructVariantVisitor<'a> {

struct TupleVariantVisitor<'a> {
tuple_info: &'static TupleVariantInfo,
registration: &'a TypeRegistration,
registry: &'a TypeRegistry,
}

Expand All @@ -845,6 +892,18 @@ impl<'a, 'de> Visitor<'de> for TupleVariantVisitor<'a> {
where
V: SeqAccess<'de>,
{
let ignored_len = self
.registration
.data::<SerializationData>()
.map(|data| data.len())
.unwrap_or(0);
let field_len = self.tuple_info.field_len().saturating_sub(ignored_len);

if field_len == 0 {
// Handle all fields being ignored
return Ok(DynamicTuple::default());
}

visit_tuple(&mut seq, self.tuple_info, self.registry)
}
}
Expand Down Expand Up @@ -1011,10 +1070,15 @@ mod tests {
map_value: HashMap<u8, usize>,
struct_value: SomeStruct,
tuple_struct_value: SomeTupleStruct,
unit_struct: SomeUnitStruct,
unit_enum: SomeEnum,
newtype_enum: SomeEnum,
tuple_enum: SomeEnum,
struct_enum: SomeEnum,
ignored_struct: SomeIgnoredStruct,
ignored_tuple_struct: SomeIgnoredTupleStruct,
ignored_struct_variant: SomeIgnoredEnum,
ignored_tuple_variant: SomeIgnoredEnum,
custom_deserialize: CustomDeserialize,
}

Expand All @@ -1026,6 +1090,18 @@ mod tests {
#[derive(Reflect, FromReflect, Debug, PartialEq)]
struct SomeTupleStruct(String);

#[derive(Reflect, FromReflect, Debug, PartialEq)]
struct SomeUnitStruct;

#[derive(Reflect, FromReflect, Debug, PartialEq)]
struct SomeIgnoredStruct {
#[reflect(ignore)]
ignored: i32,
}

#[derive(Reflect, FromReflect, Debug, PartialEq)]
struct SomeIgnoredTupleStruct(#[reflect(ignore)] i32);

#[derive(Reflect, FromReflect, Debug, PartialEq, Deserialize)]
struct SomeDeserializableStruct {
foo: i64,
Expand All @@ -1050,14 +1126,27 @@ mod tests {
Struct { foo: String },
}

#[derive(Reflect, FromReflect, Debug, PartialEq)]
enum SomeIgnoredEnum {
Tuple(#[reflect(ignore)] f32, #[reflect(ignore)] f32),
Struct {
#[reflect(ignore)]
foo: String,
},
}

fn get_registry() -> TypeRegistry {
let mut registry = TypeRegistry::default();
registry.register::<MyStruct>();
registry.register::<SomeStruct>();
registry.register::<SomeTupleStruct>();
registry.register::<SomeUnitStruct>();
registry.register::<SomeIgnoredStruct>();
registry.register::<SomeIgnoredTupleStruct>();
registry.register::<CustomDeserialize>();
registry.register::<SomeDeserializableStruct>();
registry.register::<SomeEnum>();
registry.register::<SomeIgnoredEnum>();
registry.register::<i8>();
registry.register::<String>();
registry.register::<i64>();
Expand Down Expand Up @@ -1090,12 +1179,19 @@ mod tests {
map_value: map,
struct_value: SomeStruct { foo: 999999999 },
tuple_struct_value: SomeTupleStruct(String::from("Tuple Struct")),
unit_struct: SomeUnitStruct,
unit_enum: SomeEnum::Unit,
newtype_enum: SomeEnum::NewType(123),
tuple_enum: SomeEnum::Tuple(1.23, 3.21),
struct_enum: SomeEnum::Struct {
foo: String::from("Struct variant value"),
},
ignored_struct: SomeIgnoredStruct { ignored: 0 },
ignored_tuple_struct: SomeIgnoredTupleStruct(0),
ignored_struct_variant: SomeIgnoredEnum::Struct {
foo: String::default(),
},
ignored_tuple_variant: SomeIgnoredEnum::Tuple(0.0, 0.0),
custom_deserialize: CustomDeserialize {
value: 100,
inner_struct: SomeDeserializableStruct { foo: 101 },
Expand Down Expand Up @@ -1125,12 +1221,17 @@ mod tests {
foo: 999999999,
),
tuple_struct_value: ("Tuple Struct"),
unit_struct: (),
unit_enum: Unit,
newtype_enum: NewType(123),
tuple_enum: Tuple(1.23, 3.21),
struct_enum: Struct(
foo: "Struct variant value",
),
ignored_struct: (),
ignored_tuple_struct: (),
ignored_struct_variant: Struct(),
ignored_tuple_variant: Tuple(),
custom_deserialize: (
value: 100,
renamed: (
Expand Down Expand Up @@ -1337,12 +1438,19 @@ mod tests {
map_value: map,
struct_value: SomeStruct { foo: 999999999 },
tuple_struct_value: SomeTupleStruct(String::from("Tuple Struct")),
unit_struct: SomeUnitStruct,
unit_enum: SomeEnum::Unit,
newtype_enum: SomeEnum::NewType(123),
tuple_enum: SomeEnum::Tuple(1.23, 3.21),
struct_enum: SomeEnum::Struct {
foo: String::from("Struct variant value"),
},
ignored_struct: SomeIgnoredStruct { ignored: 0 },
ignored_tuple_struct: SomeIgnoredTupleStruct(0),
ignored_struct_variant: SomeIgnoredEnum::Struct {
foo: String::default(),
},
ignored_tuple_variant: SomeIgnoredEnum::Tuple(0.0, 0.0),
custom_deserialize: CustomDeserialize {
value: 100,
inner_struct: SomeDeserializableStruct { foo: 101 },
Expand All @@ -1363,7 +1471,8 @@ mod tests {
108, 101, 32, 83, 116, 114, 117, 99, 116, 0, 0, 0, 0, 1, 0, 0, 0, 123, 0, 0, 0, 0, 0,
0, 0, 2, 0, 0, 0, 164, 112, 157, 63, 164, 112, 77, 64, 3, 0, 0, 0, 20, 0, 0, 0, 0, 0,
0, 0, 83, 116, 114, 117, 99, 116, 32, 118, 97, 114, 105, 97, 110, 116, 32, 118, 97,
108, 117, 101, 100, 0, 0, 0, 0, 0, 0, 0, 101, 0, 0, 0, 0, 0, 0, 0,
108, 117, 101, 1, 0, 0, 0, 0, 0, 0, 0, 100, 0, 0, 0, 0, 0, 0, 0, 101, 0, 0, 0, 0, 0, 0,
0,
];

let deserializer = UntypedReflectDeserializer::new(&registry);
Expand Down Expand Up @@ -1392,12 +1501,19 @@ mod tests {
map_value: map,
struct_value: SomeStruct { foo: 999999999 },
tuple_struct_value: SomeTupleStruct(String::from("Tuple Struct")),
unit_struct: SomeUnitStruct,
unit_enum: SomeEnum::Unit,
newtype_enum: SomeEnum::NewType(123),
tuple_enum: SomeEnum::Tuple(1.23, 3.21),
struct_enum: SomeEnum::Struct {
foo: String::from("Struct variant value"),
},
ignored_struct: SomeIgnoredStruct { ignored: 0 },
ignored_tuple_struct: SomeIgnoredTupleStruct(0),
ignored_struct_variant: SomeIgnoredEnum::Struct {
foo: String::default(),
},
ignored_tuple_variant: SomeIgnoredEnum::Tuple(0.0, 0.0),
custom_deserialize: CustomDeserialize {
value: 100,
inner_struct: SomeDeserializableStruct { foo: 101 },
Expand All @@ -1409,14 +1525,15 @@ mod tests {
let input = vec![
129, 217, 40, 98, 101, 118, 121, 95, 114, 101, 102, 108, 101, 99, 116, 58, 58, 115,
101, 114, 100, 101, 58, 58, 100, 101, 58, 58, 116, 101, 115, 116, 115, 58, 58, 77, 121,
83, 116, 114, 117, 99, 116, 158, 123, 172, 72, 101, 108, 108, 111, 32, 119, 111, 114,
108, 100, 33, 145, 123, 146, 202, 64, 73, 15, 219, 205, 5, 57, 149, 254, 255, 0, 1, 2,
149, 254, 255, 0, 1, 2, 129, 64, 32, 145, 206, 59, 154, 201, 255, 145, 172, 84, 117,
112, 108, 101, 32, 83, 116, 114, 117, 99, 116, 164, 85, 110, 105, 116, 129, 167, 78,
101, 119, 84, 121, 112, 101, 123, 129, 165, 84, 117, 112, 108, 101, 146, 202, 63, 157,
112, 164, 202, 64, 77, 112, 164, 129, 166, 83, 116, 114, 117, 99, 116, 145, 180, 83,
116, 114, 117, 99, 116, 32, 118, 97, 114, 105, 97, 110, 116, 32, 118, 97, 108, 117,
101, 146, 100, 145, 101,
83, 116, 114, 117, 99, 116, 220, 0, 19, 123, 172, 72, 101, 108, 108, 111, 32, 119, 111,
114, 108, 100, 33, 145, 123, 146, 202, 64, 73, 15, 219, 205, 5, 57, 149, 254, 255, 0,
1, 2, 149, 254, 255, 0, 1, 2, 129, 64, 32, 145, 206, 59, 154, 201, 255, 145, 172, 84,
117, 112, 108, 101, 32, 83, 116, 114, 117, 99, 116, 144, 164, 85, 110, 105, 116, 129,
167, 78, 101, 119, 84, 121, 112, 101, 123, 129, 165, 84, 117, 112, 108, 101, 146, 202,
63, 157, 112, 164, 202, 64, 77, 112, 164, 129, 166, 83, 116, 114, 117, 99, 116, 145,
180, 83, 116, 114, 117, 99, 116, 32, 118, 97, 114, 105, 97, 110, 116, 32, 118, 97, 108,
117, 101, 144, 144, 129, 166, 83, 116, 114, 117, 99, 116, 144, 129, 165, 84, 117, 112,
108, 101, 144, 146, 100, 145, 101,
];

let deserializer = UntypedReflectDeserializer::new(&registry);
Expand Down
Loading

0 comments on commit 7c17f3f

Please sign in to comment.