From 7c17f3f0a081410bca47d8bc965111161beea5c6 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Wed, 23 Nov 2022 00:01:36 +0000 Subject: [PATCH] bevy_reflect: Fix binary deserialization not working for unit structs (#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 --- crates/bevy_reflect/src/serde/de.rs | 135 +++++++++++++++++++++++++-- crates/bevy_reflect/src/serde/ser.rs | 76 +++++++++++++-- 2 files changed, 193 insertions(+), 18 deletions(-) diff --git a/crates/bevy_reflect/src/serde/de.rs b/crates/bevy_reflect/src/serde/de.rs index 4f6c0f31f7517..656d4b3f55ba8 100644 --- a/crates/bevy_reflect/src/serde/de.rs +++ b/crates/bevy_reflect/src/serde/de.rs @@ -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, }, )?; @@ -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, }, )? @@ -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, } @@ -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::() + .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 @@ -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::() + .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!( @@ -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, } @@ -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, }, )? @@ -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, }, )? @@ -787,6 +820,7 @@ impl<'de> DeserializeSeed<'de> for VariantDeserializer { struct StructVariantVisitor<'a> { struct_info: &'static StructVariantInfo, + registration: &'a TypeRegistration, registry: &'a TypeRegistry, } @@ -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::() + .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 @@ -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, } @@ -845,6 +892,18 @@ impl<'a, 'de> Visitor<'de> for TupleVariantVisitor<'a> { where V: SeqAccess<'de>, { + let ignored_len = self + .registration + .data::() + .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) } } @@ -1011,10 +1070,15 @@ mod tests { map_value: HashMap, 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, } @@ -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, @@ -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::(); registry.register::(); registry.register::(); + registry.register::(); + registry.register::(); + registry.register::(); registry.register::(); registry.register::(); registry.register::(); + registry.register::(); registry.register::(); registry.register::(); registry.register::(); @@ -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 }, @@ -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: ( @@ -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 }, @@ -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(®istry); @@ -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 }, @@ -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(®istry); diff --git a/crates/bevy_reflect/src/serde/ser.rs b/crates/bevy_reflect/src/serde/ser.rs index 1dee7dda46bf5..6403b5e99933f 100644 --- a/crates/bevy_reflect/src/serde/ser.rs +++ b/crates/bevy_reflect/src/serde/ser.rs @@ -488,10 +488,15 @@ mod tests { map_value: HashMap, 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_serialize: CustomSerialize, } @@ -503,6 +508,18 @@ mod tests { #[derive(Reflect, 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, Debug, PartialEq)] enum SomeEnum { Unit, @@ -511,6 +528,15 @@ mod tests { Struct { foo: String }, } + #[derive(Reflect, FromReflect, Debug, PartialEq)] + enum SomeIgnoredEnum { + Tuple(#[reflect(ignore)] f32, #[reflect(ignore)] f32), + Struct { + #[reflect(ignore)] + foo: String, + }, + } + #[derive(Reflect, Debug, PartialEq, Serialize)] struct SomeSerializableStruct { foo: i64, @@ -532,6 +558,10 @@ mod tests { registry.register::(); registry.register::(); registry.register::(); + registry.register::(); + registry.register::(); + registry.register::(); + registry.register::(); registry.register::(); registry.register::(); registry.register::(); @@ -557,12 +587,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: 123 }, + ignored_tuple_struct: SomeIgnoredTupleStruct(123), + ignored_struct_variant: SomeIgnoredEnum::Struct { + foo: String::from("Struct Variant"), + }, + ignored_tuple_variant: SomeIgnoredEnum::Tuple(1.23, 3.45), custom_serialize: CustomSerialize { value: 100, inner_struct: SomeSerializableStruct { foo: 101 }, @@ -600,12 +637,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_serialize: ( value: 100, renamed: ( @@ -745,12 +787,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: 123 }, + ignored_tuple_struct: SomeIgnoredTupleStruct(123), + ignored_struct_variant: SomeIgnoredEnum::Struct { + foo: String::from("Struct Variant"), + }, + ignored_tuple_variant: SomeIgnoredEnum::Tuple(1.23, 3.45), custom_serialize: CustomSerialize { value: 100, inner_struct: SomeSerializableStruct { foo: 101 }, @@ -774,7 +823,8 @@ mod tests { 112, 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, ]; assert_eq!(expected, bytes); @@ -795,12 +845,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: 123 }, + ignored_tuple_struct: SomeIgnoredTupleStruct(123), + ignored_struct_variant: SomeIgnoredEnum::Struct { + foo: String::from("Struct Variant"), + }, + ignored_tuple_variant: SomeIgnoredEnum::Tuple(1.23, 3.45), custom_serialize: CustomSerialize { value: 100, inner_struct: SomeSerializableStruct { foo: 101 }, @@ -815,14 +872,15 @@ mod tests { let expected: Vec = vec![ 129, 217, 41, 98, 101, 118, 121, 95, 114, 101, 102, 108, 101, 99, 116, 58, 58, 115, 101, 114, 100, 101, 58, 58, 115, 101, 114, 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, + 121, 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, ]; assert_eq!(expected, bytes);