Merge pull request #1382 from roblabla/serde-other

Implement #[serde(other)] on enum variant

serde_derive/src/de.rs

@@ -1157,6 +1157,10 @@ fn deserialize_externally_tagged_enum(
         .map(|(i, variant)| (variant.attrs.name().deserialize_name(), field_i(i)))
         .collect();
 
+    let other_idx = variants
+        .iter()
+        .position(|ref variant| variant.attrs.other());
+
     let variants_stmt = {
         let variant_names = variant_names_idents.iter().map(|&(ref name, _)| name);
         quote! {
@@ -1168,6 +1172,7 @@ fn deserialize_externally_tagged_enum(
         &variant_names_idents,
         cattrs,
         true,
+        other_idx
     ));
 
     // Match arms to extract a variant from a string
@@ -1255,6 +1260,10 @@ fn deserialize_internally_tagged_enum(
         .map(|(i, variant)| (variant.attrs.name().deserialize_name(), field_i(i)))
         .collect();
 
+    let other_idx = variants
+        .iter()
+        .position(|ref variant| variant.attrs.other());
+
     let variants_stmt = {
         let variant_names = variant_names_idents.iter().map(|&(ref name, _)| name);
         quote! {
@@ -1266,6 +1275,7 @@ fn deserialize_internally_tagged_enum(
         &variant_names_idents,
         cattrs,
         true,
+        other_idx
     ));
 
     // Match arms to extract a variant from a string
@@ -1324,6 +1334,10 @@ fn deserialize_adjacently_tagged_enum(
         .map(|(i, variant)| (variant.attrs.name().deserialize_name(), field_i(i)))
         .collect();
 
+    let other_idx = variants
+        .iter()
+        .position(|ref variant| variant.attrs.other());
+
     let variants_stmt = {
         let variant_names = variant_names_idents.iter().map(|&(ref name, _)| name);
         quote! {
@@ -1335,6 +1349,7 @@ fn deserialize_adjacently_tagged_enum(
         &variant_names_idents,
         cattrs,
         true,
+        other_idx
     ));
 
     let variant_arms: &Vec<_> = &variants
@@ -1842,6 +1857,7 @@ fn deserialize_generated_identifier(
     fields: &[(String, Ident)],
     cattrs: &attr::Container,
     is_variant: bool,
+    other_idx: Option<usize>
 ) -> Fragment {
     let this = quote!(__Field);
     let field_idents: &Vec<_> = &fields.iter().map(|&(_, ref ident)| ident).collect();
@@ -1850,6 +1866,10 @@ fn deserialize_generated_identifier(
         let ignore_variant = quote!(__other(_serde::private::de::Content<'de>),);
         let fallthrough = quote!(_serde::export::Ok(__Field::__other(__value)));
         (Some(ignore_variant), Some(fallthrough))
+    } else if let Some(other_idx) = other_idx {
+        let ignore_variant = fields[other_idx].1.clone();
+        let fallthrough = quote!(_serde::export::Ok(__Field::#ignore_variant));
+        (None, Some(fallthrough))
     } else if is_variant || cattrs.deny_unknown_fields() {
         (None, None)
     } else {
@@ -2272,7 +2292,7 @@ fn deserialize_struct_as_struct_visitor(
         }
     };
 
-    let field_visitor = deserialize_generated_identifier(&field_names_idents, cattrs, false);
+    let field_visitor = deserialize_generated_identifier(&field_names_idents, cattrs, false, None);
 
     let visit_map = deserialize_map(struct_path, params, fields, cattrs);
 
@@ -2292,7 +2312,7 @@ fn deserialize_struct_as_map_visitor(
         .map(|(i, field)| (field.attrs.name().deserialize_name(), field_i(i)))
         .collect();
 
-    let field_visitor = deserialize_generated_identifier(&field_names_idents, cattrs, false);
+    let field_visitor = deserialize_generated_identifier(&field_names_idents, cattrs, false, None);
 
     let visit_map = deserialize_map(struct_path, params, fields, cattrs);
 
@@ -2527,7 +2547,7 @@ fn deserialize_struct_as_struct_in_place_visitor(
         }
     };
 
-    let field_visitor = deserialize_generated_identifier(&field_names_idents, cattrs, false);
+    let field_visitor = deserialize_generated_identifier(&field_names_idents, cattrs, false, None);
 
     let visit_map = deserialize_map_in_place(params, fields, cattrs);
 

serde_derive/src/internals/check.rs

@@ -93,7 +93,7 @@ fn check_flatten_field(cx: &Ctxt, style: Style, field: &Field) {
 }
 
 /// The `other` attribute must be used at most once and it must be the last
-/// variant of an enum that has the `field_identifier` attribute.
+/// variant of an enum.
 ///
 /// Inside a `variant_identifier` all variants must be unit variants. Inside a
 /// `field_identifier` all but possibly one variant must be unit variants. The
@@ -111,42 +111,48 @@ fn check_identifier(cx: &Ctxt, cont: &Container) {
             variant.style,
             cont.attrs.identifier(),
             variant.attrs.other(),
+            cont.attrs.tag()
         ) {
-            // The `other` attribute may only be used in a field_identifier.
+            // The `other` attribute may not be used in a variant_identifier.
-            (_, Identifier::Variant, true) | (_, Identifier::No, true) => {
+            (_, Identifier::Variant, true, _) => {
-                cx.error("#[serde(other)] may only be used inside a field_identifier");
+                cx.error("#[serde(other)] may not be used on a variant_identifier");
+            },
+
+            // Variant with `other` attribute cannot appear in untagged enum
+            (_, Identifier::No, true, &EnumTag::None) => {
+                cx.error("#[serde(other)] cannot appear on untagged enum");
             }
 
             // Variant with `other` attribute must be the last one.
-            (Style::Unit, Identifier::Field, true) => {
+            (Style::Unit, Identifier::Field, true, _) | (Style::Unit, Identifier::No, true, _) => {
                 if i < variants.len() - 1 {
                     cx.error("#[serde(other)] must be the last variant");
                 }
             }
 
             // Variant with `other` attribute must be a unit variant.
-            (_, Identifier::Field, true) => {
+            (_, Identifier::Field, true, _) | (_, Identifier::No, true, _) => {
                 cx.error("#[serde(other)] must be on a unit variant");
-            }
+            },
 
             // Any sort of variant is allowed if this is not an identifier.
-            (_, Identifier::No, false) => {}
+            (_, Identifier::No, false, _) => {}
 
             // Unit variant without `other` attribute is always fine.
-            (Style::Unit, _, false) => {}
+            (Style::Unit, _, false, _) => {}
 
             // The last field is allowed to be a newtype catch-all.
-            (Style::Newtype, Identifier::Field, false) => {
+            (Style::Newtype, Identifier::Field, false, _) => {
                 if i < variants.len() - 1 {
                     cx.error(format!("`{}` must be the last variant", variant.ident));
                 }
             }
 
-            (_, Identifier::Field, false) => {
+            (_, Identifier::Field, false, _) => {
                 cx.error("field_identifier may only contain unit variants");
             }
 
-            (_, Identifier::Variant, false) => {
+            (_, Identifier::Variant, false, _) => {
                 cx.error("variant_identifier may only contain unit variants");
             }
         }

test_suite/tests/compile-fail/identifier/not_identifier.rs

@@ -1,20 +0,0 @@
-// Copyright 2017 Serde Developers
-//
-// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
-// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
-// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
-// option. This file may not be copied, modified, or distributed
-// except according to those terms.
-
-#[macro_use]
-extern crate serde_derive;
-
-#[derive(Deserialize)]
-enum F {
-    A,
-    #[serde(other)]
-    //~^^^^ ERROR: #[serde(other)] may only be used inside a field_identifier
-    B,
-}
-
-fn main() {}

test_suite/tests/test_de.rs

@@ -135,6 +135,13 @@ enum EnumSkipAll {
     Skipped,
 }
 
+#[derive(PartialEq, Debug, Deserialize)]
+enum EnumOther {
+    Unit,
+    #[serde(other)]
+    Other
+}
+
 //////////////////////////////////////////////////////////////////////////
 
 macro_rules! declare_tests {
@@ -753,6 +760,20 @@ declare_tests! {
             Token::Unit,
         ],
     }
+    test_enum_other_unit {
+        EnumOther::Unit => &[
+            Token::Enum { name: "EnumOther" },
+            Token::Str("Unit"),
+            Token::Unit,
+        ],
+    }
+    test_enum_other {
+        EnumOther::Other => &[
+            Token::Enum { name: "EnumOther" },
+            Token::Str("Foo"),
+            Token::Unit,
+        ],
+    }
     test_box {
         Box::new(0i32) => &[Token::I32(0)],
     }