From 227bea1d0b9454375e69e5a9fbdf46eeeeb2cfaa Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Thu, 12 Jan 2017 23:17:45 -0800 Subject: [PATCH 1/6] Treat skipped fields as unknown --- serde_codegen/src/de.rs | 73 +++++++++++++++++++--------------------- testing/tests/test_de.rs | 28 +++++++++++++++ 2 files changed, 62 insertions(+), 39 deletions(-) diff --git a/serde_codegen/src/de.rs b/serde_codegen/src/de.rs index 2b94cc2f..9a34b23b 100644 --- a/serde_codegen/src/de.rs +++ b/serde_codegen/src/de.rs @@ -480,11 +480,14 @@ fn deserialize_item_enum( let type_name = item_attrs.name().deserialize_name(); + let variant_names = variants.iter() + .enumerate() + .filter(|&(_, variant)| !variant.attrs.skip_deserializing()) + .map(|(i, variant)| (variant.attrs.name().deserialize_name(), field_i(i))) + .collect(); + let variant_visitor = deserialize_field_visitor( - variants.iter() - .filter(|variant| !variant.attrs.skip_deserializing()) - .map(|variant| variant.attrs.name().deserialize_name()) - .collect(), + variant_names, item_attrs, true, ); @@ -628,12 +631,12 @@ fn deserialize_newtype_variant( } fn deserialize_field_visitor( - field_names: Vec, + fields: Vec<(String, Ident)>, item_attrs: &attr::Item, is_variant: bool, ) -> Tokens { - // Create the field names for the fields. - let field_idents: &Vec<_> = &(0 .. field_names.len()).map(field_i).collect(); + let field_names = fields.iter().map(|&(ref name, _)| name); + let field_idents: &Vec<_> = &fields.iter().map(|&(_, ref ident)| ident).collect(); let ignore_variant = if is_variant || item_attrs.deny_unknown_fields() { None @@ -697,12 +700,14 @@ fn deserialize_struct_visitor( fields: &[Field], item_attrs: &attr::Item, ) -> (Tokens, Tokens, Tokens) { - let field_exprs = fields.iter() - .map(|field| field.attrs.name().deserialize_name()) + let field_names = fields.iter() + .enumerate() + .filter(|&(_, field)| !field.attrs.skip_deserializing()) + .map(|(i, field)| (field.attrs.name().deserialize_name(), field_i(i))) .collect(); let field_visitor = deserialize_field_visitor( - field_exprs, + field_names, item_attrs, false, ); @@ -733,16 +738,6 @@ fn deserialize_map( fields: &[Field], item_attrs: &attr::Item, ) -> Tokens { - if fields.is_empty() && item_attrs.deny_unknown_fields() { - return quote! { - // FIXME: Once we drop support for Rust 1.15: - // let None::<__Field> = try!(visitor.visit_key()); - try!(visitor.visit_key::<__Field>()).map(|impossible| match impossible {}); - try!(visitor.end()); - Ok(#struct_path {}) - }; - } - // Create the field names for the fields. let fields_names: Vec<_> = fields.iter() .enumerate() @@ -792,18 +787,6 @@ fn deserialize_map( } }); - // Match arms to ignore value for fields that have `skip_deserializing`. - // Ignored even if `deny_unknown_fields` is set. - let skipped_arms = fields_names.iter() - .filter(|&&(field, _)| field.attrs.skip_deserializing()) - .map(|&(_, ref name)| { - quote! { - __Field::#name => { - let _ = try!(visitor.visit_value::<_serde::de::impls::IgnoredAny>()); - } - } - }); - // Visit ignored values to consume them let ignored_arm = if item_attrs.deny_unknown_fields() { None @@ -813,6 +796,24 @@ fn deserialize_map( }) }; + let all_skipped = fields.iter().all(|field| field.attrs.skip_deserializing()); + let match_keys = if item_attrs.deny_unknown_fields() && all_skipped { + quote! { + // FIXME: Once we drop support for Rust 1.15: + // let None::<__Field> = try!(visitor.visit_key()); + try!(visitor.visit_key::<__Field>()).map(|impossible| match impossible {}); + } + } else { + quote! { + while let Some(key) = try!(visitor.visit_key::<__Field>()) { + match key { + #(#value_arms)* + #ignored_arm + } + } + } + }; + let extract_values = fields_names.iter() .filter(|&&(field, _)| !field.attrs.skip_deserializing()) .map(|&(field, ref name)| { @@ -840,13 +841,7 @@ fn deserialize_map( quote! { #(#let_values)* - while let Some(key) = try!(visitor.visit_key::<__Field>()) { - match key { - #(#value_arms)* - #(#skipped_arms)* - #ignored_arm - } - } + #match_keys try!(visitor.end()); diff --git a/testing/tests/test_de.rs b/testing/tests/test_de.rs index d1879fe8..7d9006d9 100644 --- a/testing/tests/test_de.rs +++ b/testing/tests/test_de.rs @@ -32,6 +32,14 @@ struct Struct { c: i32, } +#[derive(PartialEq, Debug, Deserialize)] +#[serde(deny_unknown_fields)] +struct StructDenyUnknown { + a: i32, + #[serde(skip_deserializing)] + b: i32, +} + #[derive(PartialEq, Debug, Deserialize)] enum Enum { Unit, @@ -788,6 +796,26 @@ fn test_net_ipaddr() { } declare_error_tests! { + test_unknown_field { + &[ + Token::StructStart("StructDenyUnknown", 2), + Token::StructSep, + Token::Str("a"), + Token::I32(0), + + Token::StructSep, + Token::Str("d"), + ], + Error::UnknownField("d".to_owned()), + } + test_skipped_field_is_unknown { + &[ + Token::StructStart("StructDenyUnknown", 2), + Token::StructSep, + Token::Str("b"), + ], + Error::UnknownField("b".to_owned()), + } test_unknown_variant { &[ Token::EnumUnit("Enum", "Foo"), From 4ef1128546ac00ae17cb03cd1e1949b60803018b Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Thu, 12 Jan 2017 23:30:57 -0800 Subject: [PATCH 2/6] More explicit about argument to deserialize_field_visitor --- serde_codegen/src/de.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/serde_codegen/src/de.rs b/serde_codegen/src/de.rs index 9a34b23b..09475ace 100644 --- a/serde_codegen/src/de.rs +++ b/serde_codegen/src/de.rs @@ -480,14 +480,14 @@ fn deserialize_item_enum( let type_name = item_attrs.name().deserialize_name(); - let variant_names = variants.iter() + let variant_names_idents = variants.iter() .enumerate() .filter(|&(_, variant)| !variant.attrs.skip_deserializing()) .map(|(i, variant)| (variant.attrs.name().deserialize_name(), field_i(i))) .collect(); let variant_visitor = deserialize_field_visitor( - variant_names, + variant_names_idents, item_attrs, true, ); @@ -700,14 +700,14 @@ fn deserialize_struct_visitor( fields: &[Field], item_attrs: &attr::Item, ) -> (Tokens, Tokens, Tokens) { - let field_names = fields.iter() + let field_names_idents = fields.iter() .enumerate() .filter(|&(_, field)| !field.attrs.skip_deserializing()) .map(|(i, field)| (field.attrs.name().deserialize_name(), field_i(i))) .collect(); let field_visitor = deserialize_field_visitor( - field_names, + field_names_idents, item_attrs, false, ); From cb5e7c6264e6eb91e162da55ab34ab686cb4056e Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Thu, 12 Jan 2017 23:35:39 -0800 Subject: [PATCH 3/6] Fix case of skipped variant followed by other variants --- serde_codegen/src/de.rs | 34 ++++++++++++++++++---------------- testing/tests/test_de.rs | 6 +++--- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/serde_codegen/src/de.rs b/serde_codegen/src/de.rs index 09475ace..0084271e 100644 --- a/serde_codegen/src/de.rs +++ b/serde_codegen/src/de.rs @@ -499,25 +499,27 @@ fn deserialize_item_enum( }; // Match arms to extract a variant from a string - let mut variant_arms = vec![]; - for (i, variant) in variants.iter().filter(|variant| !variant.attrs.skip_deserializing()).enumerate() { - let variant_name = field_i(i); + let variant_arms = variants.iter() + .enumerate() + .filter(|&(_, variant)| !variant.attrs.skip_deserializing()) + .map(|(i, variant)| { + let variant_name = field_i(i); - let block = deserialize_variant( - type_ident, - impl_generics, - ty.clone(), - variant, - item_attrs, - ); + let block = deserialize_variant( + type_ident, + impl_generics, + ty.clone(), + variant, + item_attrs, + ); - let arm = quote! { - __Field::#variant_name => #block - }; - variant_arms.push(arm); - } + quote! { + __Field::#variant_name => #block + } + }); - let match_variant = if variant_arms.is_empty() { + let all_skipped = variants.iter().all(|variant| variant.attrs.skip_deserializing()); + let match_variant = if all_skipped { // This is an empty enum like `enum Impossible {}` or an enum in which // all variants have `#[serde(skip_deserializing)]`. quote! { diff --git a/testing/tests/test_de.rs b/testing/tests/test_de.rs index 7d9006d9..4f455236 100644 --- a/testing/tests/test_de.rs +++ b/testing/tests/test_de.rs @@ -42,13 +42,13 @@ struct StructDenyUnknown { #[derive(PartialEq, Debug, Deserialize)] enum Enum { + #[allow(dead_code)] + #[serde(skip_deserializing)] + Skipped, Unit, Simple(i32), Seq(i32, i32, i32), Map { a: i32, b: i32, c: i32 }, - #[allow(dead_code)] - #[serde(skip_deserializing)] - Skipped, } ////////////////////////////////////////////////////////////////////////// From fff6c9cb662e30b4b496982b7fcf2295ca78980a Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 13 Jan 2017 01:12:31 -0800 Subject: [PATCH 4/6] Add tests for all fields skipped --- testing/tests/test_de.rs | 44 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/testing/tests/test_de.rs b/testing/tests/test_de.rs index 4f455236..6ed9cdb4 100644 --- a/testing/tests/test_de.rs +++ b/testing/tests/test_de.rs @@ -40,6 +40,19 @@ struct StructDenyUnknown { b: i32, } +#[derive(PartialEq, Debug, Deserialize)] +struct StructSkipAll { + #[serde(skip_deserializing)] + a: i32, +} + +#[derive(PartialEq, Debug, Deserialize)] +#[serde(deny_unknown_fields)] +struct StructSkipAllDenyUnknown { + #[serde(skip_deserializing)] + a: i32, +} + #[derive(PartialEq, Debug, Deserialize)] enum Enum { #[allow(dead_code)] @@ -689,6 +702,29 @@ declare_tests! { Token::StructEnd, ], } + test_struct_skip_all { + StructSkipAll { a: 0 } => &[ + Token::StructStart("StructSkipAll", 0), + Token::StructEnd, + ], + StructSkipAll { a: 0 } => &[ + Token::StructStart("StructSkipAll", 1), + Token::StructSep, + Token::Str("a"), + Token::I32(1), + + Token::StructSep, + Token::Str("b"), + Token::I32(2), + Token::StructEnd, + ], + } + test_struct_skip_all_deny_unknown { + StructSkipAllDenyUnknown { a: 0 } => &[ + Token::StructStart("StructSkipAllDenyUnknown", 0), + Token::StructEnd, + ], + } test_enum_unit { Enum::Unit => &[ Token::EnumUnit("Enum", "Unit"), @@ -816,6 +852,14 @@ declare_error_tests! { ], Error::UnknownField("b".to_owned()), } + test_skip_all_deny_unknown { + &[ + Token::StructStart("StructSkipAllDenyUnknown", 1), + Token::StructSep, + Token::Str("a"), + ], + Error::UnknownField("a".to_owned()), + } test_unknown_variant { &[ Token::EnumUnit("Enum", "Foo"), From c9f5d08ed1da2369a8ed40f25c096a565151d601 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 13 Jan 2017 01:14:03 -0800 Subject: [PATCH 5/6] Add test for all variants skipped --- testing/tests/test_de.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/testing/tests/test_de.rs b/testing/tests/test_de.rs index 6ed9cdb4..3359492e 100644 --- a/testing/tests/test_de.rs +++ b/testing/tests/test_de.rs @@ -64,6 +64,13 @@ enum Enum { Map { a: i32, b: i32, c: i32 }, } +#[derive(PartialEq, Debug, Deserialize)] +enum EnumSkipAll { + #[allow(dead_code)] + #[serde(skip_deserializing)] + Skipped, +} + ////////////////////////////////////////////////////////////////////////// macro_rules! declare_test { @@ -872,6 +879,12 @@ declare_error_tests! { ], Error::UnknownVariant("Skipped".to_owned()), } + test_enum_skip_all { + &[ + Token::EnumUnit("EnumSkipAll", "Skipped"), + ], + Error::UnknownVariant("Skipped".to_owned()), + } test_struct_seq_too_long { &[ Token::SeqStart(Some(4)), From 8c49e6d6a5d0086d1783eecedec8cdc3f8e643f6 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 13 Jan 2017 01:27:07 -0800 Subject: [PATCH 6/6] Resolve conflict between rename changes and skip changes --- serde_codegen/src/de.rs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/serde_codegen/src/de.rs b/serde_codegen/src/de.rs index 55607474..ed4f9e0a 100644 --- a/serde_codegen/src/de.rs +++ b/serde_codegen/src/de.rs @@ -702,20 +702,11 @@ fn deserialize_struct_visitor( fields: &[Field], item_attrs: &attr::Item, ) -> (Tokens, Tokens, Tokens) { -<<<<<<< HEAD let field_names_idents = fields.iter() .enumerate() .filter(|&(_, field)| !field.attrs.skip_deserializing()) .map(|(i, field)| (field.attrs.name().deserialize_name(), field_i(i))) -||||||| merged common ancestors - let field_exprs = fields.iter() - .map(|field| field.attrs.name().deserialize_name()) -======= - let field_exprs: Vec<_> = fields.iter() - .map(|field| field.attrs.name().deserialize_name()) ->>>>>>> origin/master .collect(); - let field_names = field_exprs.clone(); let field_visitor = deserialize_field_visitor( field_names_idents, @@ -731,6 +722,7 @@ fn deserialize_struct_visitor( item_attrs, ); + let field_names = fields.iter().map(|field| field.attrs.name().deserialize_name()); let fields_stmt = quote! { const FIELDS: &'static [&'static str] = &[ #(#field_names),* ]; };