From c887a0b47267fb3e055f3d627a687ee1cbbbe9c1 Mon Sep 17 00:00:00 2001 From: Jeroen Bollen Date: Wed, 6 Dec 2017 21:07:16 +0100 Subject: [PATCH 1/2] Solved #1105. When a field should be skipped during deserialization, it will not use its own Default implementation when the container structure has `#[serde(default)]` set. --- serde_derive/src/de.rs | 23 +++++++++++ serde_derive_internals/src/ast.rs | 61 +++++++++++++++++++----------- serde_derive_internals/src/attr.rs | 15 ++++++-- test_suite/tests/test_de.rs | 20 ++++++++++ 4 files changed, 93 insertions(+), 26 deletions(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index bfa18b00..26afef55 100644 --- a/serde_derive/src/de.rs +++ b/serde_derive/src/de.rs @@ -447,7 +447,30 @@ fn deserialize_seq( }; } + let let_default = match *cattrs.default() { + attr::Default::Default => { + Some( + quote!( + let __default: Self::Value = _serde::export::Default::default(); + ), + ) + } + attr::Default::Path(ref path) => { + Some( + quote!( + let __default: Self::Value = #path(); + ), + ) + } + attr::Default::None => { + // We don't need the default value, to prevent an unused variable warning + // we'll leave the line empty. + None + } + }; + quote_block! { + #let_default #(#let_values)* _serde::export::Ok(#result) } diff --git a/serde_derive_internals/src/ast.rs b/serde_derive_internals/src/ast.rs index dce1acc1..5f8b8aae 100644 --- a/serde_derive_internals/src/ast.rs +++ b/serde_derive_internals/src/ast.rs @@ -49,9 +49,9 @@ impl<'a> Container<'a> { let attrs = attr::Container::from_ast(cx, item); let mut body = match item.body { - syn::Body::Enum(ref variants) => Body::Enum(enum_from_ast(cx, variants)), + syn::Body::Enum(ref variants) => Body::Enum(enum_from_ast(cx, variants, &attrs)), syn::Body::Struct(ref variant_data) => { - let (style, fields) = struct_from_ast(cx, variant_data, None); + let (style, fields) = struct_from_ast(cx, variant_data, None, &attrs); Body::Struct(style, fields) } }; @@ -98,36 +98,53 @@ impl<'a> Body<'a> { } } -fn enum_from_ast<'a>(cx: &Ctxt, variants: &'a [syn::Variant]) -> Vec> { +fn enum_from_ast<'a>( + cx: &Ctxt, + variants: &'a [syn::Variant], + container: &attr::Container, +) -> Vec> { variants .iter() - .map( - |variant| { - let attrs = attr::Variant::from_ast(cx, variant); - let (style, fields) = struct_from_ast(cx, &variant.data, Some(&attrs)); - Variant { - ident: variant.ident.clone(), - attrs: attrs, - style: style, - fields: fields, - } - }, - ) + .map(|variant| { + let attrs = attr::Variant::from_ast(cx, variant); + let (style, fields) = struct_from_ast(cx, &variant.data, Some(&attrs), container); + Variant { + ident: variant.ident.clone(), + attrs: attrs, + style: style, + fields: fields, + } + }) .collect() } -fn struct_from_ast<'a>(cx: &Ctxt, data: &'a syn::VariantData, attrs: Option<&attr::Variant>) -> (Style, Vec>) { +fn struct_from_ast<'a>( + cx: &Ctxt, + data: &'a syn::VariantData, + attrs: Option<&attr::Variant>, + container: &attr::Container, +) -> (Style, Vec>) { match *data { - syn::VariantData::Struct(ref fields) => (Style::Struct, fields_from_ast(cx, fields, attrs)), - syn::VariantData::Tuple(ref fields) if fields.len() == 1 => { - (Style::Newtype, fields_from_ast(cx, fields, attrs)) + syn::VariantData::Struct(ref fields) => { + (Style::Struct, fields_from_ast(cx, fields, attrs, container)) + } + syn::VariantData::Tuple(ref fields) if fields.len() == 1 => ( + Style::Newtype, + fields_from_ast(cx, fields, attrs, container), + ), + syn::VariantData::Tuple(ref fields) => { + (Style::Tuple, fields_from_ast(cx, fields, attrs, container)) } - syn::VariantData::Tuple(ref fields) => (Style::Tuple, fields_from_ast(cx, fields, attrs)), syn::VariantData::Unit => (Style::Unit, Vec::new()), } } -fn fields_from_ast<'a>(cx: &Ctxt, fields: &'a [syn::Field], attrs: Option<&attr::Variant>) -> Vec> { +fn fields_from_ast<'a>( + cx: &Ctxt, + fields: &'a [syn::Field], + attrs: Option<&attr::Variant>, + container: &attr::Container, +) -> Vec> { fields .iter() .enumerate() @@ -135,7 +152,7 @@ fn fields_from_ast<'a>(cx: &Ctxt, fields: &'a [syn::Field], attrs: Option<&attr: |(i, field)| { Field { ident: field.ident.clone(), - attrs: attr::Field::from_ast(cx, i, field, attrs), + attrs: attr::Field::from_ast(cx, i, field, attrs, container), ty: &field.ty, } }, diff --git a/serde_derive_internals/src/attr.rs b/serde_derive_internals/src/attr.rs index 7baeb59b..0cf43047 100644 --- a/serde_derive_internals/src/attr.rs +++ b/serde_derive_internals/src/attr.rs @@ -714,7 +714,13 @@ pub enum Default { impl Field { /// Extract out the `#[serde(...)]` attributes from a struct field. - pub fn from_ast(cx: &Ctxt, index: usize, field: &syn::Field, attrs: Option<&Variant>) -> Self { + pub fn from_ast( + cx: &Ctxt, + index: usize, + field: &syn::Field, + attrs: Option<&Variant>, + container: &Container, + ) -> Self { let mut ser_name = Attr::none(cx, "rename"); let mut de_name = Attr::none(cx, "rename"); let mut skip_serializing = BoolAttr::none(cx, "skip_serializing"); @@ -881,9 +887,10 @@ impl Field { } } - // Is skip_deserializing, initialize the field to Default::default() - // unless a different default is specified by `#[serde(default = "...")]` - if skip_deserializing.0.value.is_some() { + // Is skip_deserializing, initialize the field to Default::default() unless a different + // default is specified by `#[serde(default = "...")]` on ourselves or our container (e.g. + // the struct we are in). + if container.default == Default::None && skip_deserializing.0.value.is_some() { default.set_if_none(Default::Default); } diff --git a/test_suite/tests/test_de.rs b/test_suite/tests/test_de.rs index b95b9c49..4c801d62 100644 --- a/test_suite/tests/test_de.rs +++ b/test_suite/tests/test_de.rs @@ -80,6 +80,20 @@ struct StructSkipAll { #[serde(skip_deserializing)] a: i32, } +#[derive(PartialEq, Debug, Deserialize)] +#[serde(default)] +struct StructSkipDefault { + #[serde(skip_deserializing)] a: i32, +} + +impl Default for StructSkipDefault { + fn default() -> Self { + StructSkipDefault { + a: 16, + } + } +} + #[derive(PartialEq, Debug, Deserialize)] #[serde(deny_unknown_fields)] struct StructSkipAllDenyUnknown { @@ -600,6 +614,12 @@ declare_tests! { Token::StructEnd, ], } + test_struct_skip_default { + StructSkipDefault { a: 16 } => &[ + Token::Struct { name: "StructSkipDefault", len: 0 }, + Token::StructEnd, + ], + } test_struct_skip_all_deny_unknown { StructSkipAllDenyUnknown { a: 0 } => &[ Token::Struct { name: "StructSkipAllDenyUnknown", len: 0 }, From aa86b04714fd37044e46535387ffcd7974f6e625 Mon Sep 17 00:00:00 2001 From: Jeroen Bollen Date: Fri, 8 Dec 2017 15:13:05 +0100 Subject: [PATCH 2/2] Adressed concerns raised by @oli-obk. Specifically: - Change identation in `de.rs`. - Make `attr::Field` take a `attr::Default` as opposed to the entire parent `attr::Container`. --- serde_derive/src/de.rs | 16 ++++++++-------- serde_derive_internals/src/ast.rs | 23 +++++++++++++---------- serde_derive_internals/src/attr.rs | 4 ++-- 3 files changed, 23 insertions(+), 20 deletions(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index 26afef55..2012c10b 100644 --- a/serde_derive/src/de.rs +++ b/serde_derive/src/de.rs @@ -451,15 +451,15 @@ fn deserialize_seq( attr::Default::Default => { Some( quote!( - let __default: Self::Value = _serde::export::Default::default(); - ), + let __default: Self::Value = _serde::export::Default::default(); + ), ) } attr::Default::Path(ref path) => { Some( quote!( - let __default: Self::Value = #path(); - ), + let __default: Self::Value = #path(); + ), ) } attr::Default::None => { @@ -1724,15 +1724,15 @@ fn deserialize_map( attr::Default::Default => { Some( quote!( - let __default: Self::Value = _serde::export::Default::default(); - ), + let __default: Self::Value = _serde::export::Default::default(); + ), ) } attr::Default::Path(ref path) => { Some( quote!( - let __default: Self::Value = #path(); - ), + let __default: Self::Value = #path(); + ), ) } attr::Default::None => { diff --git a/serde_derive_internals/src/ast.rs b/serde_derive_internals/src/ast.rs index 5f8b8aae..83243bcb 100644 --- a/serde_derive_internals/src/ast.rs +++ b/serde_derive_internals/src/ast.rs @@ -49,9 +49,11 @@ impl<'a> Container<'a> { let attrs = attr::Container::from_ast(cx, item); let mut body = match item.body { - syn::Body::Enum(ref variants) => Body::Enum(enum_from_ast(cx, variants, &attrs)), + syn::Body::Enum(ref variants) => { + Body::Enum(enum_from_ast(cx, variants, &attrs.default())) + } syn::Body::Struct(ref variant_data) => { - let (style, fields) = struct_from_ast(cx, variant_data, None, &attrs); + let (style, fields) = struct_from_ast(cx, variant_data, None, &attrs.default()); Body::Struct(style, fields) } }; @@ -101,13 +103,14 @@ impl<'a> Body<'a> { fn enum_from_ast<'a>( cx: &Ctxt, variants: &'a [syn::Variant], - container: &attr::Container, + container_default: &attr::Default, ) -> Vec> { variants .iter() .map(|variant| { let attrs = attr::Variant::from_ast(cx, variant); - let (style, fields) = struct_from_ast(cx, &variant.data, Some(&attrs), container); + let (style, fields) = + struct_from_ast(cx, &variant.data, Some(&attrs), container_default); Variant { ident: variant.ident.clone(), attrs: attrs, @@ -122,18 +125,18 @@ fn struct_from_ast<'a>( cx: &Ctxt, data: &'a syn::VariantData, attrs: Option<&attr::Variant>, - container: &attr::Container, + container_default: &attr::Default, ) -> (Style, Vec>) { match *data { syn::VariantData::Struct(ref fields) => { - (Style::Struct, fields_from_ast(cx, fields, attrs, container)) + (Style::Struct, fields_from_ast(cx, fields, attrs, container_default)) } syn::VariantData::Tuple(ref fields) if fields.len() == 1 => ( Style::Newtype, - fields_from_ast(cx, fields, attrs, container), + fields_from_ast(cx, fields, attrs, container_default), ), syn::VariantData::Tuple(ref fields) => { - (Style::Tuple, fields_from_ast(cx, fields, attrs, container)) + (Style::Tuple, fields_from_ast(cx, fields, attrs, container_default)) } syn::VariantData::Unit => (Style::Unit, Vec::new()), } @@ -143,7 +146,7 @@ fn fields_from_ast<'a>( cx: &Ctxt, fields: &'a [syn::Field], attrs: Option<&attr::Variant>, - container: &attr::Container, + container_default: &attr::Default, ) -> Vec> { fields .iter() @@ -152,7 +155,7 @@ fn fields_from_ast<'a>( |(i, field)| { Field { ident: field.ident.clone(), - attrs: attr::Field::from_ast(cx, i, field, attrs, container), + attrs: attr::Field::from_ast(cx, i, field, attrs, container_default), ty: &field.ty, } }, diff --git a/serde_derive_internals/src/attr.rs b/serde_derive_internals/src/attr.rs index 0cf43047..d93257c6 100644 --- a/serde_derive_internals/src/attr.rs +++ b/serde_derive_internals/src/attr.rs @@ -719,7 +719,7 @@ impl Field { index: usize, field: &syn::Field, attrs: Option<&Variant>, - container: &Container, + container_default: &Default, ) -> Self { let mut ser_name = Attr::none(cx, "rename"); let mut de_name = Attr::none(cx, "rename"); @@ -890,7 +890,7 @@ impl Field { // Is skip_deserializing, initialize the field to Default::default() unless a different // default is specified by `#[serde(default = "...")]` on ourselves or our container (e.g. // the struct we are in). - if container.default == Default::None && skip_deserializing.0.value.is_some() { + if container_default == &Default::None && skip_deserializing.0.value.is_some() { default.set_if_none(Default::Default); }