Merge pull request #682 from serde-rs/unknown

Treat skipped fields as unknown

serde_codegen/src/de.rs

@@ -480,11 +480,14 @@ fn deserialize_item_enum(
 
     let type_name = item_attrs.name().deserialize_name();
 
+    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(
-        variants.iter()
+        variant_names_idents,
-            .filter(|variant| !variant.attrs.skip_deserializing())
-            .map(|variant| variant.attrs.name().deserialize_name())
-            .collect(),
         item_attrs,
         true,
     );
@@ -496,25 +499,27 @@ fn deserialize_item_enum(
     };
 
     // Match arms to extract a variant from a string
-    let mut variant_arms = vec![];
+    let variant_arms = variants.iter()
-    for (i, variant) in variants.iter().filter(|variant| !variant.attrs.skip_deserializing()).enumerate() {
+        .enumerate()
-        let variant_name = field_i(i);
+        .filter(|&(_, variant)| !variant.attrs.skip_deserializing())
+        .map(|(i, variant)| {
+            let variant_name = field_i(i);
 
-        let block = deserialize_variant(
+            let block = deserialize_variant(
-            type_ident,
+                type_ident,
-            impl_generics,
+                impl_generics,
-            ty.clone(),
+                ty.clone(),
-            variant,
+                variant,
-            item_attrs,
+                item_attrs,
-        );
+            );
 
-        let arm = quote! {
+            quote! {
-            __Field::#variant_name => #block
+                __Field::#variant_name => #block
-        };
+            }
-        variant_arms.push(arm);
+        });
-    }
 
-    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! {
@@ -628,12 +633,12 @@ fn deserialize_newtype_variant(
 }
 
 fn deserialize_field_visitor(
-    field_names: Vec<String>,
+    fields: Vec<(String, Ident)>,
     item_attrs: &attr::Item,
     is_variant: bool,
 ) -> Tokens {
-    // Create the field names for the fields.
+    let field_names = fields.iter().map(|&(ref name, _)| name);
-    let field_idents: &Vec<_> = &(0 .. field_names.len()).map(field_i).collect();
+    let field_idents: &Vec<_> = &fields.iter().map(|&(_, ref ident)| ident).collect();
 
     let ignore_variant = if is_variant || item_attrs.deny_unknown_fields() {
         None
@@ -697,13 +702,14 @@ fn deserialize_struct_visitor(
     fields: &[Field],
     item_attrs: &attr::Item,
 ) -> (Tokens, Tokens, Tokens) {
-    let field_exprs: Vec<_> = fields.iter()
+    let field_names_idents = fields.iter()
-        .map(|field| field.attrs.name().deserialize_name())
+        .enumerate()
+        .filter(|&(_, field)| !field.attrs.skip_deserializing())
+        .map(|(i, field)| (field.attrs.name().deserialize_name(), field_i(i)))
         .collect();
-    let field_names = field_exprs.clone();
 
     let field_visitor = deserialize_field_visitor(
-        field_exprs,
+        field_names_idents,
         item_attrs,
         false,
     );
@@ -716,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),* ];
     };
@@ -730,16 +737,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()
@@ -789,18 +786,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
@@ -810,6 +795,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)| {
@@ -837,13 +840,7 @@ fn deserialize_map(
     quote! {
         #(#let_values)*
 
-        while let Some(key) = try!(visitor.visit_key::<__Field>()) {
+        #match_keys
-            match key {
-                #(#value_arms)*
-                #(#skipped_arms)*
-                #ignored_arm
-            }
-        }
 
         try!(visitor.end());
 

testing/tests/test_de.rs

@@ -32,12 +32,40 @@ 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)]
+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)]
+    #[serde(skip_deserializing)]
+    Skipped,
     Unit,
     Simple(i32),
     Seq(i32, i32, i32),
     Map { a: i32, b: i32, c: i32 },
+}
+
+#[derive(PartialEq, Debug, Deserialize)]
+enum EnumSkipAll {
     #[allow(dead_code)]
     #[serde(skip_deserializing)]
     Skipped,
@@ -681,6 +709,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"),
@@ -788,6 +839,34 @@ fn test_net_ipaddr() {
 }
 
 declare_error_tests! {
+    test_unknown_field<StructDenyUnknown> {
+        &[
+            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<StructDenyUnknown> {
+        &[
+            Token::StructStart("StructDenyUnknown", 2),
+                Token::StructSep,
+                Token::Str("b"),
+        ],
+        Error::UnknownField("b".to_owned()),
+    }
+    test_skip_all_deny_unknown<StructSkipAllDenyUnknown> {
+        &[
+            Token::StructStart("StructSkipAllDenyUnknown", 1),
+                Token::StructSep,
+                Token::Str("a"),
+        ],
+        Error::UnknownField("a".to_owned()),
+    }
     test_unknown_variant<Enum> {
         &[
             Token::EnumUnit("Enum", "Foo"),
@@ -800,6 +879,12 @@ declare_error_tests! {
         ],
         Error::UnknownVariant("Skipped".to_owned()),
     }
+    test_enum_skip_all<EnumSkipAll> {
+        &[
+            Token::EnumUnit("EnumSkipAll", "Skipped"),
+        ],
+        Error::UnknownVariant("Skipped".to_owned()),
+    }
     test_struct_seq_too_long<Struct> {
         &[
             Token::SeqStart(Some(4)),