From 8cc7e6aa90523fc2192eeeeea54a4d06ce2688a7 Mon Sep 17 00:00:00 2001 From: roblabla Date: Mon, 10 Sep 2018 15:12:15 +0000 Subject: [PATCH 1/5] Implement #serde(other) on enum variant --- serde_derive/src/de.rs | 26 +++++++++++++++++++++--- serde_derive/src/internals/check.rs | 31 ++++++++++++++++++----------- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index af5d5b0c..7cfb959b 100644 --- a/serde_derive/src/de.rs +++ b/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 ) -> 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 other_idx.is_some() { + let ignore_variant = fields[other_idx.unwrap()].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); diff --git a/serde_derive/src/internals/check.rs b/serde_derive/src/internals/check.rs index b42a30a3..3168edbb 100644 --- a/serde_derive/src/internals/check.rs +++ b/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,49 @@ 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. - (_, Identifier::Variant, true) | (_, Identifier::No, true) => { - cx.error("#[serde(other)] may only be used inside a field_identifier"); + // The `other` attribute may not be used in a variant_identifier. + (_, Identifier::Variant, true, _) => { + 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, _) | (_, 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) => { + // TODO: Only in field_identifier ? + (_, Identifier::Field, 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"); } } From 7870b5835682f494df2a73e10dcb23e102ecba3a Mon Sep 17 00:00:00 2001 From: roblabla Date: Mon, 10 Sep 2018 16:25:02 +0000 Subject: [PATCH 2/5] Add tests for serde(other) in enum --- test_suite/tests/test_de.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test_suite/tests/test_de.rs b/test_suite/tests/test_de.rs index 4897abd0..ef5ca3df 100644 --- a/test_suite/tests/test_de.rs +++ b/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)], } From 61bf901048ec8660d85a75e8b6ecfd1657a20c7b Mon Sep 17 00:00:00 2001 From: roblabla Date: Mon, 10 Sep 2018 17:12:33 +0000 Subject: [PATCH 3/5] Fix for rust 1.15 --- serde_derive/src/internals/check.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/serde_derive/src/internals/check.rs b/serde_derive/src/internals/check.rs index 3168edbb..516afeab 100644 --- a/serde_derive/src/internals/check.rs +++ b/serde_derive/src/internals/check.rs @@ -119,7 +119,7 @@ fn check_identifier(cx: &Ctxt, cont: &Container) { }, // Variant with `other` attribute cannot appear in untagged enum - (_, Identifier::No, true, EnumTag::None) => { + (_, Identifier::No, true, &EnumTag::None) => { cx.error("#[serde(other)] cannot appear on untagged enum"); } From 0156f1355a84c1802f594b02f58b39044692f48f Mon Sep 17 00:00:00 2001 From: roblabla Date: Mon, 10 Sep 2018 17:15:22 +0000 Subject: [PATCH 4/5] Remove obsolete compile-fail test --- .../compile-fail/identifier/not_identifier.rs | 20 ------------------- 1 file changed, 20 deletions(-) delete mode 100644 test_suite/tests/compile-fail/identifier/not_identifier.rs diff --git a/test_suite/tests/compile-fail/identifier/not_identifier.rs b/test_suite/tests/compile-fail/identifier/not_identifier.rs deleted file mode 100644 index 7a240d53..00000000 --- a/test_suite/tests/compile-fail/identifier/not_identifier.rs +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright 2017 Serde Developers -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , 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() {} From dcd2232f6984c03dc79ded2080ae9bcd125eb2ab Mon Sep 17 00:00:00 2001 From: roblabla Date: Tue, 11 Sep 2018 17:12:37 +0000 Subject: [PATCH 5/5] Enforce unit struct for #[serde(other)] --- serde_derive/src/de.rs | 4 ++-- serde_derive/src/internals/check.rs | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index 7cfb959b..69def6a6 100644 --- a/serde_derive/src/de.rs +++ b/serde_derive/src/de.rs @@ -1866,8 +1866,8 @@ 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 other_idx.is_some() { - let ignore_variant = fields[other_idx.unwrap()].1.clone(); + } 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() { diff --git a/serde_derive/src/internals/check.rs b/serde_derive/src/internals/check.rs index 516afeab..54738f4e 100644 --- a/serde_derive/src/internals/check.rs +++ b/serde_derive/src/internals/check.rs @@ -124,15 +124,14 @@ fn check_identifier(cx: &Ctxt, cont: &Container) { } // Variant with `other` attribute must be the last one. - (Style::Unit, Identifier::Field, true, _) | (_, Identifier::No, 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. - // TODO: Only in field_identifier ? - (_, Identifier::Field, true, _) => { + (_, Identifier::Field, true, _) | (_, Identifier::No, true, _) => { cx.error("#[serde(other)] must be on a unit variant"); },