From 0647a7c1feafebacea6de4cb69f0aee9b2ba2919 Mon Sep 17 00:00:00 2001 From: Mingun Date: Sun, 11 Aug 2024 18:16:19 +0500 Subject: [PATCH 1/5] Fix creating and filling a collections that was not read --- serde_derive/src/de.rs | 18 +++++++----------- test_suite/tests/test_gen.rs | 32 ++++++++++++++++++++++++-------- 2 files changed, 31 insertions(+), 19 deletions(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index b87ababf0..1ccecb26c 100644 --- a/serde_derive/src/de.rs +++ b/serde_derive/src/de.rs @@ -281,13 +281,9 @@ fn deserialize_body(cont: &Container, params: &Parameters) -> Fragment { } else if let attr::Identifier::No = cont.attrs.identifier() { match &cont.data { Data::Enum(variants) => deserialize_enum(params, variants, &cont.attrs), - Data::Struct(Style::Struct, fields) => deserialize_struct( - params, - fields, - &cont.attrs, - cont.attrs.has_flatten(), - StructForm::Struct, - ), + Data::Struct(Style::Struct, fields) => { + deserialize_struct(params, fields, &cont.attrs, StructForm::Struct) + } Data::Struct(Style::Tuple, fields) | Data::Struct(Style::Newtype, fields) => { deserialize_tuple( params, @@ -927,7 +923,6 @@ fn deserialize_struct( params: &Parameters, fields: &[Field], cattrs: &attr::Container, - has_flatten: bool, form: StructForm, ) -> Fragment { let this_type = ¶ms.this_type; @@ -976,6 +971,10 @@ fn deserialize_struct( ) }) .collect(); + + let has_flatten = fields + .iter() + .any(|field| field.attrs.flatten() && !field.attrs.skip_deserializing()); let field_visitor = deserialize_field_identifier(&field_names_idents, cattrs, has_flatten); // untagged struct variants do not get a visit_seq method. The same applies to @@ -1838,7 +1837,6 @@ fn deserialize_externally_tagged_variant( params, &variant.fields, cattrs, - variant.attrs.has_flatten(), StructForm::ExternallyTagged(variant_ident), ), } @@ -1882,7 +1880,6 @@ fn deserialize_internally_tagged_variant( params, &variant.fields, cattrs, - variant.attrs.has_flatten(), StructForm::InternallyTagged(variant_ident, deserializer), ), Style::Tuple => unreachable!("checked in serde_derive_internals"), @@ -1940,7 +1937,6 @@ fn deserialize_untagged_variant( params, &variant.fields, cattrs, - variant.attrs.has_flatten(), StructForm::Untagged(variant_ident, deserializer), ), } diff --git a/test_suite/tests/test_gen.rs b/test_suite/tests/test_gen.rs index 2f43b20ba..ed9dc725e 100644 --- a/test_suite/tests/test_gen.rs +++ b/test_suite/tests/test_gen.rs @@ -547,6 +547,12 @@ fn test_gen() { } assert::(); + #[derive(Serialize, Deserialize)] + pub struct Flatten { + #[serde(flatten)] + t: T, + } + #[derive(Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct FlattenDenyUnknown { @@ -554,6 +560,19 @@ fn test_gen() { t: T, } + #[derive(Serialize, Deserialize)] + pub struct SkipDeserializing { + #[serde(skip_deserializing)] + flat: T, + } + + #[derive(Serialize, Deserialize)] + #[serde(deny_unknown_fields)] + pub struct SkipDeserializingDenyUnknown { + #[serde(skip_deserializing)] + flat: T, + } + #[derive(Serialize, Deserialize)] pub struct StaticStrStruct<'a> { a: &'a str, @@ -720,14 +739,11 @@ fn test_gen() { flat: StdOption, } - #[allow(clippy::collection_is_never_read)] // FIXME - const _: () = { - #[derive(Serialize, Deserialize)] - pub struct FlattenSkipDeserializing { - #[serde(flatten, skip_deserializing)] - flat: T, - } - }; + #[derive(Serialize, Deserialize)] + pub struct FlattenSkipDeserializing { + #[serde(flatten, skip_deserializing)] + flat: T, + } #[derive(Serialize, Deserialize)] #[serde(untagged)] From fd5b5e9aa561e29d9312ecef2019ec670b4bc334 Mon Sep 17 00:00:00 2001 From: Mingun Date: Sun, 11 Aug 2024 18:46:31 +0500 Subject: [PATCH 2/5] Correctly calculate `has_flatten` attribute in all cases for deserialization Consequence: `FlattenSkipDeserializing[DenyUnknown]` - does not collect data in Field, because do not read them anyway - gets `deserialize_in_place` method - gets ability to deserialize from sequence (visit_seq method) - uses `deserialize_struct` instead of `deserialize_map` --- serde_derive/src/de.rs | 30 +++++++++++++----------------- serde_derive/src/internals/ast.rs | 1 - serde_derive/src/internals/attr.rs | 21 --------------------- 3 files changed, 13 insertions(+), 39 deletions(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index 1ccecb26c..be1666e7b 100644 --- a/serde_derive/src/de.rs +++ b/serde_derive/src/de.rs @@ -285,13 +285,7 @@ fn deserialize_body(cont: &Container, params: &Parameters) -> Fragment { deserialize_struct(params, fields, &cont.attrs, StructForm::Struct) } Data::Struct(Style::Tuple, fields) | Data::Struct(Style::Newtype, fields) => { - deserialize_tuple( - params, - fields, - &cont.attrs, - cont.attrs.has_flatten(), - TupleForm::Tuple, - ) + deserialize_tuple(params, fields, &cont.attrs, TupleForm::Tuple) } Data::Struct(Style::Unit, _) => deserialize_unit_struct(params, &cont.attrs), } @@ -465,11 +459,10 @@ fn deserialize_tuple( params: &Parameters, fields: &[Field], cattrs: &attr::Container, - has_flatten: bool, form: TupleForm, ) -> Fragment { assert!( - !has_flatten, + !has_flatten(fields), "tuples and tuple variants cannot have flatten fields" ); @@ -590,7 +583,7 @@ fn deserialize_tuple_in_place( cattrs: &attr::Container, ) -> Fragment { assert!( - !cattrs.has_flatten(), + !has_flatten(fields), "tuples and tuple variants cannot have flatten fields" ); @@ -972,9 +965,7 @@ fn deserialize_struct( }) .collect(); - let has_flatten = fields - .iter() - .any(|field| field.attrs.flatten() && !field.attrs.skip_deserializing()); + let has_flatten = has_flatten(fields); let field_visitor = deserialize_field_identifier(&field_names_idents, cattrs, has_flatten); // untagged struct variants do not get a visit_seq method. The same applies to @@ -1114,7 +1105,7 @@ fn deserialize_struct_in_place( ) -> Option { // for now we do not support in_place deserialization for structs that // are represented as map. - if cattrs.has_flatten() { + if has_flatten(fields) { return None; } @@ -1830,7 +1821,6 @@ fn deserialize_externally_tagged_variant( params, &variant.fields, cattrs, - variant.attrs.has_flatten(), TupleForm::ExternallyTagged(variant_ident), ), Style::Struct => deserialize_struct( @@ -1930,7 +1920,6 @@ fn deserialize_untagged_variant( params, &variant.fields, cattrs, - variant.attrs.has_flatten(), TupleForm::Untagged(variant_ident, deserializer), ), Style::Struct => deserialize_struct( @@ -2703,7 +2692,7 @@ fn deserialize_map_in_place( cattrs: &attr::Container, ) -> Fragment { assert!( - !cattrs.has_flatten(), + !has_flatten(fields), "inplace deserialization of maps does not support flatten fields" ); @@ -3038,6 +3027,13 @@ fn effective_style(variant: &Variant) -> Style { } } +/// True if there are fields that is not skipped and has a `#[serde(flatten)]` attribute. +fn has_flatten(fields: &[Field]) -> bool { + fields + .iter() + .any(|field| field.attrs.flatten() && !field.attrs.skip_deserializing()) +} + struct DeImplGenerics<'a>(&'a Parameters); #[cfg(feature = "deserialize_in_place")] struct InPlaceImplGenerics<'a>(&'a Parameters); diff --git a/serde_derive/src/internals/ast.rs b/serde_derive/src/internals/ast.rs index 4ec709952..a28d3ae7e 100644 --- a/serde_derive/src/internals/ast.rs +++ b/serde_derive/src/internals/ast.rs @@ -85,7 +85,6 @@ impl<'a> Container<'a> { for field in &mut variant.fields { if field.attrs.flatten() { has_flatten = true; - variant.attrs.mark_has_flatten(); } field.attrs.rename_by_rules( variant diff --git a/serde_derive/src/internals/attr.rs b/serde_derive/src/internals/attr.rs index ba3a2d8d8..28ed54267 100644 --- a/serde_derive/src/internals/attr.rs +++ b/serde_derive/src/internals/attr.rs @@ -810,18 +810,6 @@ pub struct Variant { rename_all_rules: RenameAllRules, ser_bound: Option>, de_bound: Option>, - /// True if variant is a struct variant which contains a field with - /// `#[serde(flatten)]`. - /// - /// ```ignore - /// enum Enum { - /// Variant { - /// #[serde(flatten)] - /// some_field: (), - /// }, - /// } - /// ``` - has_flatten: bool, skip_deserializing: bool, skip_serializing: bool, other: bool, @@ -991,7 +979,6 @@ impl Variant { }, ser_bound: ser_bound.get(), de_bound: de_bound.get(), - has_flatten: false, skip_deserializing: skip_deserializing.get(), skip_serializing: skip_serializing.get(), other: other.get(), @@ -1034,14 +1021,6 @@ impl Variant { self.de_bound.as_ref().map(|vec| &vec[..]) } - pub fn has_flatten(&self) -> bool { - self.has_flatten - } - - pub fn mark_has_flatten(&mut self) { - self.has_flatten = true; - } - pub fn skip_deserializing(&self) -> bool { self.skip_deserializing } From 005cb84593e73cb3ba263232cc5f09b77859ec5b Mon Sep 17 00:00:00 2001 From: Mingun Date: Sun, 11 Aug 2024 20:00:02 +0500 Subject: [PATCH 3/5] Fail with an understandable message is number of fields for serialization is too many --- serde_derive/src/ser.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/serde_derive/src/ser.rs b/serde_derive/src/ser.rs index 7d89d2212..a0291aff7 100644 --- a/serde_derive/src/ser.rs +++ b/serde_derive/src/ser.rs @@ -289,7 +289,13 @@ fn serialize_tuple_struct( } fn serialize_struct(params: &Parameters, fields: &[Field], cattrs: &attr::Container) -> Fragment { - assert!(fields.len() as u64 <= u64::from(u32::MAX)); + assert!( + fields.len() as u64 <= u64::from(u32::MAX), + "too many fields in {}: {}, maximum supported count is {}", + cattrs.name().serialize_name(), + fields.len(), + u32::MAX + ); if cattrs.has_flatten() { serialize_struct_as_map(params, fields, cattrs) From 547d843ccaa1349b074991f5ceeda722cf245824 Mon Sep 17 00:00:00 2001 From: Mingun Date: Sun, 11 Aug 2024 19:22:47 +0500 Subject: [PATCH 4/5] Remove dead code - serialize_struct_as_map always called when cattrs.has_flatten()==true --- serde_derive/src/ser.rs | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/serde_derive/src/ser.rs b/serde_derive/src/ser.rs index a0291aff7..f745bf675 100644 --- a/serde_derive/src/ser.rs +++ b/serde_derive/src/ser.rs @@ -376,26 +376,8 @@ fn serialize_struct_as_map( let let_mut = mut_if(serialized_fields.peek().is_some() || tag_field_exists); - let len = if cattrs.has_flatten() { - quote!(_serde::__private::None) - } else { - let len = serialized_fields - .map(|field| match field.attrs.skip_serializing_if() { - None => quote!(1), - Some(path) => { - let field_expr = get_member(params, field, &field.member); - quote!(if #path(#field_expr) { 0 } else { 1 }) - } - }) - .fold( - quote!(#tag_field_exists as usize), - |sum, expr| quote!(#sum + #expr), - ); - quote!(_serde::__private::Some(#len)) - }; - quote_block! { - let #let_mut __serde_state = _serde::Serializer::serialize_map(__serializer, #len)?; + let #let_mut __serde_state = _serde::Serializer::serialize_map(__serializer, _serde::__private::None)?; #tag_field #(#serialize_fields)* _serde::ser::SerializeMap::end(__serde_state) From 77a6a9d4e193c82439aa0c35dfdbbb34fafdf38e Mon Sep 17 00:00:00 2001 From: Mingun Date: Sun, 11 Aug 2024 19:56:27 +0500 Subject: [PATCH 5/5] Take into account only not skipped flatten fields when choose serialization form Consequence: `FlattenSkipSerializing` - uses `serialize_struct` instead of `serialize_map` --- serde_derive/src/internals/ast.rs | 13 +------------ serde_derive/src/internals/attr.rs | 26 -------------------------- serde_derive/src/ser.rs | 5 ++++- 3 files changed, 5 insertions(+), 39 deletions(-) diff --git a/serde_derive/src/internals/ast.rs b/serde_derive/src/internals/ast.rs index a28d3ae7e..3293823a7 100644 --- a/serde_derive/src/internals/ast.rs +++ b/serde_derive/src/internals/ast.rs @@ -63,7 +63,7 @@ impl<'a> Container<'a> { item: &'a syn::DeriveInput, derive: Derive, ) -> Option> { - let mut attrs = attr::Container::from_ast(cx, item); + let attrs = attr::Container::from_ast(cx, item); let mut data = match &item.data { syn::Data::Enum(data) => Data::Enum(enum_from_ast(cx, &data.variants, attrs.default())), @@ -77,15 +77,11 @@ impl<'a> Container<'a> { } }; - let mut has_flatten = false; match &mut data { Data::Enum(variants) => { for variant in variants { variant.attrs.rename_by_rules(attrs.rename_all_rules()); for field in &mut variant.fields { - if field.attrs.flatten() { - has_flatten = true; - } field.attrs.rename_by_rules( variant .attrs @@ -97,18 +93,11 @@ impl<'a> Container<'a> { } Data::Struct(_, fields) => { for field in fields { - if field.attrs.flatten() { - has_flatten = true; - } field.attrs.rename_by_rules(attrs.rename_all_rules()); } } } - if has_flatten { - attrs.mark_has_flatten(); - } - let mut item = Container { ident: item.ident.clone(), attrs, diff --git a/serde_derive/src/internals/attr.rs b/serde_derive/src/internals/attr.rs index 28ed54267..ac5f5d9a5 100644 --- a/serde_derive/src/internals/attr.rs +++ b/serde_derive/src/internals/attr.rs @@ -216,23 +216,6 @@ pub struct Container { type_into: Option, remote: Option, identifier: Identifier, - /// True if container is a struct and has a field with `#[serde(flatten)]`, - /// or is an enum with a struct variant which has a field with - /// `#[serde(flatten)]`. - /// - /// ```ignore - /// struct Container { - /// #[serde(flatten)] - /// some_field: (), - /// } - /// enum Container { - /// Variant { - /// #[serde(flatten)] - /// some_field: (), - /// }, - /// } - /// ``` - has_flatten: bool, serde_path: Option, is_packed: bool, /// Error message generated when type can't be deserialized @@ -603,7 +586,6 @@ impl Container { type_into: type_into.get(), remote: remote.get(), identifier: decide_identifier(cx, item, field_identifier, variant_identifier), - has_flatten: false, serde_path: serde_path.get(), is_packed, expecting: expecting.get(), @@ -671,14 +653,6 @@ impl Container { self.identifier } - pub fn has_flatten(&self) -> bool { - self.has_flatten - } - - pub fn mark_has_flatten(&mut self) { - self.has_flatten = true; - } - pub fn custom_serde_path(&self) -> Option<&syn::Path> { self.serde_path.as_ref() } diff --git a/serde_derive/src/ser.rs b/serde_derive/src/ser.rs index f745bf675..4f5e000d4 100644 --- a/serde_derive/src/ser.rs +++ b/serde_derive/src/ser.rs @@ -297,7 +297,10 @@ fn serialize_struct(params: &Parameters, fields: &[Field], cattrs: &attr::Contai u32::MAX ); - if cattrs.has_flatten() { + let has_non_skipped_flatten = fields + .iter() + .any(|field| field.attrs.flatten() && !field.attrs.skip_serializing()); + if has_non_skipped_flatten { serialize_struct_as_map(params, fields, cattrs) } else { serialize_struct_as_struct(params, fields, cattrs)