From 47fd94f9dfc02df244cf1d8c6dcb280df34d77ba Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Wed, 12 Sep 2018 22:42:53 +0200 Subject: [PATCH 01/12] Add new struct for enum variants values This will enable enum variants to support in addition the type int and bool --- serde_derive/src/internals/attr.rs | 140 ++++++++++++++++++++++++++--- serde_derive_internals/Cargo.toml | 1 + serde_derive_internals/lib.rs | 1 + 3 files changed, 128 insertions(+), 14 deletions(-) diff --git a/serde_derive/src/internals/attr.rs b/serde_derive/src/internals/attr.rs index 3f399ded9..70cf60d3b 100644 --- a/serde_derive/src/internals/attr.rs +++ b/serde_derive/src/internals/attr.rs @@ -16,6 +16,7 @@ use syn::synom::{ParseError, Synom}; use syn::Ident; use syn::Meta::{List, NameValue, Word}; use syn::NestedMeta::{Literal, Meta}; +use quote::ToTokens; // This module handles parsing of `#[serde(...)]` attributes. The entrypoints // are `attr::Container::from_ast`, `attr::Variant::from_ast`, and @@ -85,6 +86,84 @@ impl<'c> BoolAttr<'c> { } } + +#[derive(Debug, Clone, Eq, PartialEq)] +pub enum VariantNameType { + Str(String), + Bool(bool), + Int(u64), +} + +impl VariantNameType { + pub fn stringify(&self) -> String { + match *self { + VariantNameType::Str(ref s) => s.clone(), + VariantNameType::Bool(true) => "true".to_owned(), + VariantNameType::Bool(false) => "false".to_owned(), + VariantNameType::Int(i) => format!("{}", i), + } + } + + pub fn as_str(&self) -> Option<&str> { + match *self { + VariantNameType::Str(ref s) => Some(&s), + _ => None, + } + } + + pub fn as_int(&self) -> Option { + match *self { + VariantNameType::Int(i) => Some(i), + _ => None, + } + } + + pub fn as_bool(&self) -> Option { + match *self { + VariantNameType::Bool(b) => Some(b), + _ => None, + } + } +} + +impl From for VariantNameType { + fn from(src: String) -> VariantNameType { + VariantNameType::Str(src) + } +} + +#[derive(Debug)] +pub struct VariantName { + serialize: VariantNameType, + deserialize: VariantNameType, +} + +impl VariantName { + /// Return the container name for the container when serializing. + pub fn serialize_name(&self) -> VariantNameType { + self.serialize.clone() + } + + /// Return the container name for the container when deserializing. + pub fn deserialize_name(&self) -> VariantNameType { + self.deserialize.clone() + } +} + +impl ToTokens for VariantNameType { + fn to_tokens(&self, out: &mut TokenStream) { + match *self { + VariantNameType::Str(ref s) => s.to_tokens(out), + VariantNameType::Bool(b) => { + b.to_tokens(out); + }, + VariantNameType::Int(i) => { + i.to_tokens(out); + } + } + } +} + pub struct Name { serialize: String, deserialize: String, @@ -539,7 +618,7 @@ fn decide_identifier( /// Represents variant attribute information pub struct Variant { - name: Name, + name: VariantName, ser_renamed: bool, de_renamed: bool, rename_all: RenameRule, @@ -572,17 +651,17 @@ impl Variant { match meta_item { // Parse `#[serde(rename = "foo")]` Meta(NameValue(ref m)) if m.ident == "rename" => { - if let Ok(s) = get_lit_str(cx, &m.ident, &m.ident, &m.lit) { - ser_name.set(s.value()); - de_name.set(s.value()); + if let Ok(value) = get_lit_variant_name_type(cx, &m.ident, &m.ident, &m.lit) { + ser_name.set(value.clone()); + de_name.set(value); } } // Parse `#[serde(rename(serialize = "foo", deserialize = "bar"))]` Meta(List(ref m)) if m.ident == "rename" => { - if let Ok((ser, de)) = get_renames(cx, &m.nested) { - ser_name.set_opt(ser.map(syn::LitStr::value)); - de_name.set_opt(de.map(syn::LitStr::value)); + if let Ok((ser, de)) = get_ser_and_de(cx, "rename", &m.nested, get_lit_variant_name_type) { + ser_name.set_opt(ser); + de_name.set_opt(de); } } @@ -700,9 +779,9 @@ impl Variant { let de_name = de_name.get(); let de_renamed = de_name.is_some(); Variant { - name: Name { - serialize: ser_name.unwrap_or_else(|| unraw(&variant.ident)), - deserialize: de_name.unwrap_or_else(|| unraw(&variant.ident)), + name: VariantName { + serialize: ser_name.unwrap_or_else(|| VariantNameType::Str(unraw(&variant.ident))), + deserialize: de_name.unwrap_or_else(|| VariantNameType::Str(unraw(&variant.ident))), }, ser_renamed: ser_renamed, de_renamed: de_renamed, @@ -718,17 +797,29 @@ impl Variant { } } - pub fn name(&self) -> &Name { + pub fn name(&self) -> &VariantName { &self.name } pub fn rename_by_rule(&mut self, rule: &RenameRule) { if !self.ser_renamed { - self.name.serialize = rule.apply_to_variant(&self.name.serialize); + let res = match self.name.serialize { + VariantNameType::Str(ref s) => Some(VariantNameType::Str(rule.apply_to_variant(s))), + _ => None, + }; + if let Some(r) = res { + self.name.serialize = r; + } } if !self.de_renamed { - self.name.deserialize = rule.apply_to_variant(&self.name.deserialize); - } + let res = match self.name.deserialize { + VariantNameType::Str(ref s) => Some(VariantNameType::Str(rule.apply_to_variant(s))), + _ => None, + }; + if let Some(r) = res { + self.name.deserialize = r; + } + } } pub fn rename_all(&self) -> &RenameRule { @@ -1239,6 +1330,27 @@ fn get_lit_str<'a>( } } +fn get_lit_variant_name_type<'a>( + cx: &Ctxt, + attr_name: &Ident, + meta_item_name: &Ident, + lit: &'a syn::Lit, +) -> Result { + match *lit { + syn::Lit::Str(ref s) => Ok(VariantNameType::Str(s.value())), + syn::Lit::Int(ref i) => Ok(VariantNameType::Int(i.value())), + syn::Lit::Bool(ref b) => Ok(VariantNameType::Bool(b.value)), + _ => { + cx.error(format!( + "expected serde {} attribute to be a string|int|bool: `{} = \"...\"`", + attr_name, meta_item_name + )); + Err(()) + } + } +} + + fn parse_lit_into_path(cx: &Ctxt, attr_name: &Ident, lit: &syn::Lit) -> Result { let string = try!(get_lit_str(cx, attr_name, attr_name, lit)); parse_lit_str(string) diff --git a/serde_derive_internals/Cargo.toml b/serde_derive_internals/Cargo.toml index 12cc7c8d4..6e309f38f 100644 --- a/serde_derive_internals/Cargo.toml +++ b/serde_derive_internals/Cargo.toml @@ -17,6 +17,7 @@ path = "lib.rs" [dependencies] proc-macro2 = "0.4" syn = { version = "0.14", default-features = false, features = ["derive", "parsing", "clone-impls"] } +quote = "0.6.3" [badges] travis-ci = { repository = "serde-rs/serde" } diff --git a/serde_derive_internals/lib.rs b/serde_derive_internals/lib.rs index 56004e787..616865823 100644 --- a/serde_derive_internals/lib.rs +++ b/serde_derive_internals/lib.rs @@ -21,6 +21,7 @@ extern crate syn; extern crate proc_macro2; +extern crate quote; #[path = "src/mod.rs"] mod internals; From bc1a59b14820da6d0c43fd74aab063f4f234b00f Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Sat, 15 Sep 2018 15:32:23 +0200 Subject: [PATCH 02/12] Add a check to prevent the use of int|bool in variant name for external and untagged enum --- serde_derive/src/internals/check.rs | 33 ++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/serde_derive/src/internals/check.rs b/serde_derive/src/internals/check.rs index b42a30a35..8c977c967 100644 --- a/serde_derive/src/internals/check.rs +++ b/serde_derive/src/internals/check.rs @@ -7,7 +7,7 @@ // except according to those terms. use internals::ast::{Container, Data, Field, Style}; -use internals::attr::{EnumTag, Identifier}; +use internals::attr::{EnumTag, Identifier, VariantNameType}; use internals::{Ctxt, Derive}; use syn::{Member, Type}; @@ -18,6 +18,7 @@ pub fn check(cx: &Ctxt, cont: &mut Container, derive: Derive) { check_flatten(cx, cont); check_identifier(cx, cont); check_variant_skip_attrs(cx, cont); + check_variant_name(cx, cont); check_internal_tag_field_name_conflict(cx, cont); check_adjacent_tag_conflict(cx, cont); check_transparent(cx, cont, derive); @@ -332,6 +333,36 @@ fn check_transparent(cx: &Ctxt, cont: &mut Container, derive: Derive) { } } +/// Renaming variants is only allowed for internal and adjacent tagging. +fn check_variant_name(cx: &Ctxt, cont: &Container) { + let tag_type = cont.attrs.tag(); + + match cont.data { + Data::Struct(_, _) => {} + Data::Enum(ref variants) => { + for name in variants.iter().map(|v| v.attrs.name()) { + match (name.serialize_name(), name.deserialize_name()) { + (VariantNameType::Bool(_), _) | + (_, VariantNameType::Bool(_)) | + (_, VariantNameType::Int(_)) | + (VariantNameType::Int(_), _) => { + match *tag_type { + EnumTag::External => { + cx.error("#[serde(rename = int|bool)] cannot be used with external tagging"); + }, + EnumTag::None => { + cx.error("#[serde(rename = int|bool)] cannot be used with #[serde(untagged)]"); + }, + _ => {} + } + }, + _ => {} + } + } + } + } +} + fn member_message(member: &Member) -> String { match *member { Member::Named(ref ident) => format!("`{}`", ident), From c9f6caebf8e8b4339f2d7e460f3311c67b927f76 Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Sun, 16 Sep 2018 23:11:31 +0200 Subject: [PATCH 03/12] Add serde export method to support convertion from int|bool for internally tagged enums variants --- serde/src/export.rs | 16 +++++++++++ serde/src/private/ser.rs | 60 ++++++++++++++++++++++++++++++++-------- 2 files changed, 65 insertions(+), 11 deletions(-) diff --git a/serde/src/export.rs b/serde/src/export.rs index 0f7cd1a21..eef53d6be 100644 --- a/serde/src/export.rs +++ b/serde/src/export.rs @@ -21,6 +21,7 @@ pub use lib::Vec; mod string { use lib::*; + use lib::fmt::Write; #[cfg(any(feature = "std", feature = "alloc"))] pub fn from_utf8_lossy(bytes: &[u8]) -> Cow { @@ -41,4 +42,19 @@ mod string { // UTF-8. str::from_utf8(bytes).unwrap_or("\u{fffd}\u{fffd}\u{fffd}") } + + pub fn from_bool(b : bool) -> &'static str { + if b { + "true" + } else { + "false" + } + } + + pub fn from_int(i: u64) -> Vec { + let mut buf = String::with_capacity(20); + + write!(&mut buf, "{}", i).ok(); + buf.into_bytes() + } } diff --git a/serde/src/private/ser.rs b/serde/src/private/ser.rs index d58c5409a..57e87befa 100644 --- a/serde/src/private/ser.rs +++ b/serde/src/private/ser.rs @@ -21,24 +21,62 @@ pub fn constrain(t: &T) -> &T { t } +pub enum VariantNameType { + Str(&'static str), + Bool(bool), + Int(u64), +} + +impl From<&'static str> for VariantNameType { + fn from(src: &'static str) -> Self { + VariantNameType::Str(src) + } +} + +impl<'a> From<&'a bool> for VariantNameType { + fn from(src: &bool) -> Self { + VariantNameType::Bool(*src) + } +} + +impl<'a> From<&'a u64> for VariantNameType { + fn from(src: &u64) -> Self { + VariantNameType::Int(*src) + } +} + +impl Serialize for VariantNameType { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer + { + match *self { + VariantNameType::Str(s) => serializer.serialize_str(s), + VariantNameType::Bool(b) => serializer.serialize_bool(b), + VariantNameType::Int(i) => serializer.serialize_u64(i), + } + } +} + /// Not public API. -pub fn serialize_tagged_newtype( +pub fn serialize_tagged_newtype( serializer: S, type_ident: &'static str, variant_ident: &'static str, tag: &'static str, - variant_name: &'static str, + variant_name: U, value: &T, ) -> Result where S: Serializer, T: Serialize, + U: Into, { value.serialize(TaggedSerializer { type_ident: type_ident, variant_ident: variant_ident, tag: tag, - variant_name: variant_name, + variant_name: variant_name.into(), delegate: serializer, }) } @@ -47,7 +85,7 @@ struct TaggedSerializer { type_ident: &'static str, variant_ident: &'static str, tag: &'static str, - variant_name: &'static str, + variant_name: VariantNameType, delegate: S, } @@ -197,7 +235,7 @@ where fn serialize_unit_struct(self, _: &'static str) -> Result { let mut map = try!(self.delegate.serialize_map(Some(1))); - try!(map.serialize_entry(self.tag, self.variant_name)); + try!(map.serialize_entry(self.tag, &self.variant_name)); map.end() } @@ -208,7 +246,7 @@ where inner_variant: &'static str, ) -> Result { let mut map = try!(self.delegate.serialize_map(Some(2))); - try!(map.serialize_entry(self.tag, self.variant_name)); + try!(map.serialize_entry(self.tag, &self.variant_name)); try!(map.serialize_entry(inner_variant, &())); map.end() } @@ -235,7 +273,7 @@ where T: Serialize, { let mut map = try!(self.delegate.serialize_map(Some(2))); - try!(map.serialize_entry(self.tag, self.variant_name)); + try!(map.serialize_entry(self.tag, &self.variant_name)); try!(map.serialize_entry(inner_variant, inner_value)); map.end() } @@ -278,7 +316,7 @@ where len: usize, ) -> Result { let mut map = try!(self.delegate.serialize_map(Some(2))); - try!(map.serialize_entry(self.tag, self.variant_name)); + try!(map.serialize_entry(self.tag, &self.variant_name)); try!(map.serialize_key(inner_variant)); Ok(SerializeTupleVariantAsMapValue::new( map, @@ -289,7 +327,7 @@ where fn serialize_map(self, len: Option) -> Result { let mut map = try!(self.delegate.serialize_map(len.map(|len| len + 1))); - try!(map.serialize_entry(self.tag, self.variant_name)); + try!(map.serialize_entry(self.tag, &self.variant_name)); Ok(map) } @@ -299,7 +337,7 @@ where len: usize, ) -> Result { let mut state = try!(self.delegate.serialize_struct(name, len + 1)); - try!(state.serialize_field(self.tag, self.variant_name)); + try!(state.serialize_field(self.tag, &self.variant_name)); Ok(state) } @@ -325,7 +363,7 @@ where len: usize, ) -> Result { let mut map = try!(self.delegate.serialize_map(Some(2))); - try!(map.serialize_entry(self.tag, self.variant_name)); + try!(map.serialize_entry(self.tag, &self.variant_name)); try!(map.serialize_key(inner_variant)); Ok(SerializeStructVariantAsMapValue::new( map, From 8c067d8f65be5292c289f64fde38440f10b070ac Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Mon, 17 Sep 2018 21:31:23 +0200 Subject: [PATCH 04/12] Add a new unit test for internally tagged enum renamed with an int|bool --- test_suite/tests/test_macros.rs | 151 ++++++++++++++++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/test_suite/tests/test_macros.rs b/test_suite/tests/test_macros.rs index 9def00ae1..c00041bdc 100644 --- a/test_suite/tests/test_macros.rs +++ b/test_suite/tests/test_macros.rs @@ -1035,6 +1035,157 @@ fn test_internally_tagged_borrow() { ); } +#[test] +fn test_internally_tagged_enum_renamed() { + #[derive(Debug, PartialEq, Serialize, Deserialize)] + struct Newtype(BTreeMap); + + #[derive(Debug, PartialEq, Serialize, Deserialize)] + struct Struct { + f: u8, + } + + #[derive(Debug, PartialEq, Serialize, Deserialize)] + #[serde(tag = "type")] + enum InternallyTagged { + #[serde(rename=1)] + A, + #[serde(rename=2)] + B, + #[serde(rename=true)] + C, + #[serde(rename=false)] + D, + #[serde(rename="abc")] + E, + } + + assert_tokens( + &InternallyTagged::A, + &[ + Token::Struct { + name: "InternallyTagged", + len: 2, + }, + Token::Str("type"), + Token::U64(1), + Token::StructEnd, + ], + ); + + assert_de_tokens( + &InternallyTagged::A, + &[ + Token::Seq { len: Some(1) }, + Token::Str("type"), + Token::U64(1), + Token::SeqEnd + ], + ); + + + assert_tokens( + &InternallyTagged::B, + &[ + Token::Struct { + name: "InternallyTagged", + len: 1, + }, + Token::Str("type"), + Token::U64(2), + Token::StructEnd, + ], + ); + + assert_de_tokens( + &InternallyTagged::B, + &[ + Token::Seq { len: Some(1) }, + Token::Str("type"), + Token::U64(2), + Token::SeqEnd + ], + ); + + assert_tokens( + &InternallyTagged::C, + &[ + Token::Struct { + name: "InternallyTagged", + len: 1, + }, + Token::Str("type"), + Token::Bool(true), + Token::StructEnd, + ], + ); + + assert_tokens( + &InternallyTagged::C, + &[ + Token::Struct { + name: "InternallyTagged", + len: 1, + }, + Token::Str("type"), + Token::Bool(true), + Token::StructEnd, + ], + ); + + assert_tokens( + &InternallyTagged::D, + &[ + Token::Struct { + name: "InternallyTagged", + len: 1, + }, + Token::Str("type"), + Token::Bool(false), + Token::StructEnd, + ], + ); + + assert_tokens( + &InternallyTagged::C, + &[ + Token::Struct { + name: "InternallyTagged", + len: 1, + }, + Token::Str("type"), + Token::Bool(false), + Token::StructEnd, + ], + ); + + assert_tokens( + &InternallyTagged::E, + &[ + Token::Struct { + name: "InternallyTagged", + len: 1, + }, + Token::Str("type"), + Token::Str("abc"), + Token::StructEnd, + ], + ); + + assert_tokens( + &InternallyTagged::E, + &[ + Token::Struct { + name: "InternallyTagged", + len: 1, + }, + Token::Str("type"), + Token::Str("abc"), + Token::StructEnd, + ], + ); +} + #[test] fn test_adjacently_tagged_enum() { #[derive(Debug, PartialEq, Serialize, Deserialize)] From 268b8a05d110f4988cbfd8fa8211201865949c0b Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Mon, 17 Sep 2018 21:34:13 +0200 Subject: [PATCH 05/12] Modify the variant type name to accept int|bool|str in internally tagged enum --- serde_derive/src/de.rs | 20 ++++++++++---------- serde_derive/src/ser.rs | 11 +++++++---- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index af5d5b0ca..65e00a091 100644 --- a/serde_derive/src/de.rs +++ b/serde_derive/src/de.rs @@ -1252,11 +1252,11 @@ fn deserialize_internally_tagged_enum( .iter() .enumerate() .filter(|&(_, variant)| !variant.attrs.skip_deserializing()) - .map(|(i, variant)| (variant.attrs.name().deserialize_name(), field_i(i))) + .map(|(i, variant)| (attr::VariantNameType::from(variant.attrs.name().deserialize_name()), field_i(i))) .collect(); let variants_stmt = { - let variant_names = variant_names_idents.iter().map(|&(ref name, _)| name); + let variant_names = variant_names_idents.iter().map(|&(ref name, _)| name.stringify()); quote! { const VARIANTS: &'static [&'static str] = &[ #(#variant_names),* ]; } @@ -1839,7 +1839,7 @@ fn deserialize_untagged_newtype_variant( } fn deserialize_generated_identifier( - fields: &[(String, Ident)], + fields: &[(attr::VariantNameType, Ident)], cattrs: &attr::Container, is_variant: bool, ) -> Fragment { @@ -1995,7 +1995,7 @@ fn deserialize_custom_identifier( fn deserialize_identifier( this: &TokenStream, - fields: &[(String, Ident)], + fields: &[(attr::VariantNameType, Ident)], is_variant: bool, fallthrough: Option, collect_other_fields: bool, @@ -2004,10 +2004,10 @@ fn deserialize_identifier( let field_borrowed_strs = fields.iter().map(|&(ref name, _)| name); let field_bytes = fields .iter() - .map(|&(ref name, _)| Literal::byte_string(name.as_bytes())); + .map(|&(ref name, _)| Literal::byte_string(name.stringify().as_bytes())); let field_borrowed_bytes = fields .iter() - .map(|&(ref name, _)| Literal::byte_string(name.as_bytes())); + .map(|&(ref name, _)| Literal::byte_string(name.stringify().as_bytes())); let constructors: &Vec<_> = &fields .iter() @@ -2262,7 +2262,7 @@ fn deserialize_struct_as_struct_visitor( .iter() .enumerate() .filter(|&(_, field)| !field.attrs.skip_deserializing()) - .map(|(i, field)| (field.attrs.name().deserialize_name(), field_i(i))) + .map(|(i, field)| (attr::VariantNameType::from(field.attrs.name().deserialize_name()), field_i(i))) .collect(); let fields_stmt = { @@ -2289,7 +2289,7 @@ fn deserialize_struct_as_map_visitor( .iter() .enumerate() .filter(|&(_, field)| !field.attrs.skip_deserializing() && !field.attrs.flatten()) - .map(|(i, field)| (field.attrs.name().deserialize_name(), field_i(i))) + .map(|(i, field)| (attr::VariantNameType::from(field.attrs.name().deserialize_name()), field_i(i))) .collect(); let field_visitor = deserialize_generated_identifier(&field_names_idents, cattrs, false); @@ -2517,11 +2517,11 @@ fn deserialize_struct_as_struct_in_place_visitor( .iter() .enumerate() .filter(|&(_, field)| !field.attrs.skip_deserializing()) - .map(|(i, field)| (field.attrs.name().deserialize_name(), field_i(i))) + .map(|(i, field)| (attr::VariantNameType::from(field.attrs.name().deserialize_name()), field_i(i))) .collect(); let fields_stmt = { - let field_names = field_names_idents.iter().map(|&(ref name, _)| name); + let field_names = field_names_idents.iter().map(|&(ref name, _)| name.stringify()); quote_block! { const FIELDS: &'static [&'static str] = &[ #(#field_names),* ]; } diff --git a/serde_derive/src/ser.rs b/serde_derive/src/ser.rs index 0c4c26d4a..fdab2b970 100644 --- a/serde_derive/src/ser.rs +++ b/serde_derive/src/ser.rs @@ -476,7 +476,10 @@ fn serialize_externally_tagged_variant( cattrs: &attr::Container, ) -> Fragment { let type_name = cattrs.name().serialize_name(); - let variant_name = variant.attrs.name().serialize_name(); + let variant_name = match variant.attrs.name().serialize_name() { + attr::VariantNameType::Str(s) => s, + _ => unreachable!(), // check_variant_name prevents this case + }; if let Some(path) = variant.attrs.serialize_with() { let ser = wrap_serialize_variant_with(params, path, variant); @@ -547,7 +550,7 @@ fn serialize_internally_tagged_variant( tag: &str, ) -> Fragment { let type_name = cattrs.name().serialize_name(); - let variant_name = variant.attrs.name().serialize_name(); + let variant_name = attr::VariantNameType::from(variant.attrs.name().serialize_name()); let enum_ident_str = params.type_name(); let variant_ident_str = variant.ident.to_string(); @@ -658,7 +661,7 @@ fn serialize_adjacently_tagged_variant( StructVariant::Untagged, params, &variant.fields, - &variant_name, + &variant_name.stringify(), ), } }); @@ -828,7 +831,7 @@ enum StructVariant<'a> { }, InternallyTagged { tag: &'a str, - variant_name: String, + variant_name: attr::VariantNameType, }, Untagged, } From 791822b8b0788786416e0b2a4915a22c2076f24c Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Tue, 2 Oct 2018 22:48:51 +0200 Subject: [PATCH 06/12] Modify deserialize_identifier to work with VariantNameType --- serde_derive/src/de.rs | 74 +++++++++++++++++++++++++++++++++++------- 1 file changed, 63 insertions(+), 11 deletions(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index 65e00a091..35f23d21d 100644 --- a/serde_derive/src/de.rs +++ b/serde_derive/src/de.rs @@ -2000,8 +2000,10 @@ fn deserialize_identifier( fallthrough: Option, collect_other_fields: bool, ) -> Fragment { - let field_strs = fields.iter().map(|&(ref name, _)| name); - let field_borrowed_strs = fields.iter().map(|&(ref name, _)| name); + let field_strs = fields.iter().filter_map(|&(ref name, _)| name.as_str()); + let field_borrowed_strs = fields.iter().filter_map(|&(ref name, _)| name.as_str()); + let field_ints = fields.iter().filter_map(|&(ref name, _)| name.as_int()); + let field_bools = fields.iter().filter_map(|&(ref name, _)| name.as_bool()); let field_bytes = fields .iter() .map(|&(ref name, _)| Literal::byte_string(name.stringify().as_bytes())); @@ -2009,7 +2011,25 @@ fn deserialize_identifier( .iter() .map(|&(ref name, _)| Literal::byte_string(name.stringify().as_bytes())); - let constructors: &Vec<_> = &fields + let constructors_strs: &Vec<_> = &fields + .iter() + .filter_map(|&(ref name, ref ident)| name.as_str().map(|_| ident)) + .map(|ident| quote!(#this::#ident)) + .collect(); + + let constructors_bools: &Vec<_> = &fields + .iter() + .filter_map(|&(ref name, ref ident)| name.as_bool().map(|_| ident)) + .map(|ident| quote!(#this::#ident)) + .collect(); + + let constructors_ints: &Vec<_> = &fields + .iter() + .filter_map(|&(ref name, ref ident)| name.as_int().map(|_| ident)) + .map(|ident| quote!(#this::#ident)) + .collect(); + + let constructors: &Vec<_> = &fields .iter() .map(|&(_, ref ident)| quote!(#this::#ident)) .collect(); @@ -2022,12 +2042,21 @@ fn deserialize_identifier( let index_expecting = if is_variant { "variant" } else { "field" }; - let bytes_to_str = if fallthrough.is_some() || collect_other_fields { - None + let (bytes_to_str, int_to_str, bool_to_str) = if fallthrough.is_some() || collect_other_fields { + (None, None, None) } else { - Some(quote! { - let __value = &_serde::export::from_utf8_lossy(__value); - }) + ( + Some(quote! { + let __value = &_serde::export::from_utf8_lossy(__value); + }), + Some(quote! { + let __tmp_value = &_serde::export::from_int(__value); + let __value = &_serde::export::from_utf8_lossy(__tmp_value); + }), + Some(quote! { + let __value = &_serde::export::from_bool(__value); + }), + ) }; let ( @@ -2200,7 +2229,7 @@ fn deserialize_identifier( { match __value { #( - #variant_indices => _serde::export::Ok(#constructors), + #field_ints => _serde::export::Ok(#constructors_ints), )* _ => _serde::export::Err(_serde::de::Error::invalid_value( _serde::de::Unexpected::Unsigned(__value), @@ -2210,6 +2239,27 @@ fn deserialize_identifier( } }; + let visit_bool = if constructors_bools.is_empty() { + None + } else { + let visit_bool = quote! { + fn visit_bool<__E>(self, __value: bool) -> _serde::export::Result + where __E: _serde::de::Error + { + match __value { + #( + #field_bools => _serde::export::Ok(#constructors_bools), + )* + _ => { + #bool_to_str + #fallthrough_arm + } + } + } + }; + Some(visit_bool) + }; + quote_block! { fn expecting(&self, __formatter: &mut _serde::export::Formatter) -> _serde::export::fmt::Result { _serde::export::Formatter::write_str(__formatter, #expecting) @@ -2217,13 +2267,15 @@ fn deserialize_identifier( #visit_other + #visit_bool + fn visit_str<__E>(self, __value: &str) -> _serde::export::Result where __E: _serde::de::Error, { match __value { #( - #field_strs => _serde::export::Ok(#constructors), + #field_strs => _serde::export::Ok(#constructors_strs), )* _ => { #value_as_str_content @@ -2238,7 +2290,7 @@ fn deserialize_identifier( { match __value { #( - #field_bytes => _serde::export::Ok(#constructors), + #field_bytes => _serde::export::Ok(#constructors_strs), )* _ => { #bytes_to_str From c08234bbde137a1e6d9b8ec7dd35bdb8615d2876 Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Wed, 3 Oct 2018 23:01:13 +0200 Subject: [PATCH 07/12] Add new type and struct cases in internally tagged enum renamed unit test --- test_suite/tests/test_macros.rs | 186 ++++++++++++++++++++++++-------- 1 file changed, 141 insertions(+), 45 deletions(-) diff --git a/test_suite/tests/test_macros.rs b/test_suite/tests/test_macros.rs index c00041bdc..e24279e32 100644 --- a/test_suite/tests/test_macros.rs +++ b/test_suite/tests/test_macros.rs @@ -1058,6 +1058,30 @@ fn test_internally_tagged_enum_renamed() { D, #[serde(rename="abc")] E, + #[serde(rename=1)] + F(BTreeMap), + #[serde(rename=1)] + G(Newtype), + #[serde(rename=1)] + H(Struct), + #[serde(rename=true)] + I(BTreeMap), + #[serde(rename=true)] + J(Newtype), + #[serde(rename=true)] + K(Struct), + #[serde(rename="abc")] + L(BTreeMap), + #[serde(rename="abc")] + M(Newtype), + #[serde(rename="abc")] + N(Struct), + #[serde(rename=1)] + O { a: u8 }, + #[serde(rename=true)] + P { a: u8 }, + #[serde(rename="abc")] + Q { a: u8 }, } assert_tokens( @@ -1065,7 +1089,7 @@ fn test_internally_tagged_enum_renamed() { &[ Token::Struct { name: "InternallyTagged", - len: 2, + len: 1, }, Token::Str("type"), Token::U64(1), @@ -1073,23 +1097,12 @@ fn test_internally_tagged_enum_renamed() { ], ); - assert_de_tokens( - &InternallyTagged::A, - &[ - Token::Seq { len: Some(1) }, - Token::Str("type"), - Token::U64(1), - Token::SeqEnd - ], - ); - - assert_tokens( &InternallyTagged::B, &[ Token::Struct { name: "InternallyTagged", - len: 1, + len: 1 }, Token::Str("type"), Token::U64(2), @@ -1097,16 +1110,6 @@ fn test_internally_tagged_enum_renamed() { ], ); - assert_de_tokens( - &InternallyTagged::B, - &[ - Token::Seq { len: Some(1) }, - Token::Str("type"), - Token::U64(2), - Token::SeqEnd - ], - ); - assert_tokens( &InternallyTagged::C, &[ @@ -1121,66 +1124,159 @@ fn test_internally_tagged_enum_renamed() { ); assert_tokens( - &InternallyTagged::C, + &InternallyTagged::D, &[ Token::Struct { name: "InternallyTagged", - len: 1, + len: 1 }, Token::Str("type"), - Token::Bool(true), + Token::Bool(false), Token::StructEnd, ], ); assert_tokens( - &InternallyTagged::D, + &InternallyTagged::E, &[ Token::Struct { name: "InternallyTagged", - len: 1, + len: 1 }, Token::Str("type"), - Token::Bool(false), + Token::Str("abc"), Token::StructEnd, ], ); assert_tokens( - &InternallyTagged::C, + &InternallyTagged::F(BTreeMap::new()), &[ - Token::Struct { - name: "InternallyTagged", - len: 1, - }, + Token::Map { len: Some(1) }, Token::Str("type"), - Token::Bool(false), + Token::U64(1), + Token::MapEnd, + ], + ); + + assert_tokens( + &InternallyTagged::G(Newtype(BTreeMap::new())), + &[ + Token::Map { len: Some(1) }, + Token::Str("type"), + Token::U64(1), + Token::MapEnd, + ], + ); + + assert_tokens( + &InternallyTagged::H(Struct { f: 6 }), + &[ + Token::Struct { name: "Struct", len: 2 }, + Token::Str("type"), + Token::U64(1), + Token::Str("f"), + Token::U8(6), Token::StructEnd, ], ); assert_tokens( - &InternallyTagged::E, + &InternallyTagged::I(BTreeMap::new()), &[ - Token::Struct { - name: "InternallyTagged", - len: 1, - }, + Token::Map { len: Some(1) }, + Token::Str("type"), + Token::Bool(true), + Token::MapEnd, + ], + ); + + assert_tokens( + &InternallyTagged::J(Newtype(BTreeMap::new())), + &[ + Token::Map { len: Some(1) }, + Token::Str("type"), + Token::Bool(true), + Token::MapEnd, + ], + ); + + assert_tokens( + &InternallyTagged::K(Struct { f: 6 }), + &[ + Token::Struct { name: "Struct", len: 2 }, + Token::Str("type"), + Token::Bool(true), + Token::Str("f"), + Token::U8(6), + Token::StructEnd, + ], + ); + + assert_tokens( + &InternallyTagged::L(BTreeMap::new()), + &[ + Token::Map { len: Some(1) }, + Token::Str("type"), + Token::Str("abc"), + Token::MapEnd, + ], + ); + + assert_tokens( + &InternallyTagged::M(Newtype(BTreeMap::new())), + &[ + Token::Map { len: Some(1) }, Token::Str("type"), Token::Str("abc"), + Token::MapEnd, + ], + ); + + assert_tokens( + &InternallyTagged::N(Struct { f: 6 }), + &[ + Token::Struct { name: "Struct", len: 2 }, + Token::Str("type"), + Token::Str("abc"), + Token::Str("f"), + Token::U8(6), Token::StructEnd, ], ); assert_tokens( - &InternallyTagged::E, + &InternallyTagged::O { a: 1 }, &[ - Token::Struct { - name: "InternallyTagged", - len: 1, - }, + Token::Struct { name: "InternallyTagged", len: 2 }, + Token::Str("type"), + Token::U64(1), + Token::Str("a"), + Token::U8(1), + Token::StructEnd, + ], + ); + + assert_tokens( + &InternallyTagged::P { a: 1 }, + &[ + Token::Struct { name: "InternallyTagged", len: 2 }, + Token::Str("type"), + Token::Bool(true), + Token::Str("a"), + Token::U8(1), + Token::StructEnd, + ], + ); + + assert_tokens( + &InternallyTagged::Q { a: 1 }, + &[ + Token::Struct { name: "InternallyTagged", len: 2 }, Token::Str("type"), Token::Str("abc"), + Token::Str("a"), + Token::U8(1), Token::StructEnd, ], ); From b127a1703875c85462f072340692ffa022c611eb Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Wed, 3 Oct 2018 23:19:10 +0200 Subject: [PATCH 08/12] Modify the serialization of variant new type and struct in internally tagged enum --- serde/src/export.rs | 2 +- serde/src/private/ser.rs | 12 ++++++++++++ serde_derive/src/ser.rs | 4 ++-- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/serde/src/export.rs b/serde/src/export.rs index eef53d6be..565e88c2a 100644 --- a/serde/src/export.rs +++ b/serde/src/export.rs @@ -14,7 +14,7 @@ pub use lib::marker::PhantomData; pub use lib::option::Option::{self, None, Some}; pub use lib::result::Result::{self, Err, Ok}; -pub use self::string::from_utf8_lossy; +pub use self::string::{from_utf8_lossy, from_int, from_bool}; #[cfg(any(feature = "alloc", feature = "std"))] pub use lib::Vec; diff --git a/serde/src/private/ser.rs b/serde/src/private/ser.rs index 57e87befa..eef2a40b7 100644 --- a/serde/src/private/ser.rs +++ b/serde/src/private/ser.rs @@ -45,6 +45,18 @@ impl<'a> From<&'a u64> for VariantNameType { } } +impl From for VariantNameType { + fn from(src: bool) -> Self { + VariantNameType::Bool(src) + } +} + +impl From for VariantNameType { + fn from(src: u64) -> Self { + VariantNameType::Int(src) + } +} + impl Serialize for VariantNameType { fn serialize(&self, serializer: S) -> Result where diff --git a/serde_derive/src/ser.rs b/serde_derive/src/ser.rs index fdab2b970..ef7825383 100644 --- a/serde_derive/src/ser.rs +++ b/serde_derive/src/ser.rs @@ -575,7 +575,7 @@ fn serialize_internally_tagged_variant( let mut __struct = try!(_serde::Serializer::serialize_struct( __serializer, #type_name, 1)); try!(_serde::ser::SerializeStruct::serialize_field( - &mut __struct, #tag, #variant_name)); + &mut __struct, #tag, &#variant_name)); _serde::ser::SerializeStruct::end(__struct) } } @@ -899,7 +899,7 @@ fn serialize_struct_variant<'a>( try!(_serde::ser::SerializeStruct::serialize_field( &mut __serde_state, #tag, - #variant_name, + &#variant_name, )); #(#serialize_fields)* _serde::ser::SerializeStruct::end(__serde_state) From b495c5bbef7373900093837b4036c621f39a5663 Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Wed, 28 Nov 2018 16:44:31 +0100 Subject: [PATCH 09/12] deserialize identifiers with the correct deserializer instead of using deserialize_identifier --- serde_derive/src/de.rs | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index 35f23d21d..67e92b170 100644 --- a/serde_derive/src/de.rs +++ b/serde_derive/src/de.rs @@ -1872,6 +1872,27 @@ fn deserialize_generated_identifier( None }; + let first_field_ident = &fields[0].0; + let same_variant_type = fields.iter().all(|ref f| { + match (&f.0, first_field_ident) { + (attr::VariantNameType::Bool(_), attr::VariantNameType::Bool(_)) => true, + (attr::VariantNameType::Int(_), attr::VariantNameType::Int(_)) => true, + (attr::VariantNameType::Str(_), attr::VariantNameType::Str(_)) => true, + _ => false + } + }); + + let serde_deserializer = if same_variant_type { + match first_field_ident { + attr::VariantNameType::Bool(_) => quote!(_serde::Deserializer::deserialize_bool(__deserializer, __FieldVisitor)), + attr::VariantNameType::Int(_) => quote!(_serde::Deserializer::deserialize_u64(__deserializer, __FieldVisitor)), + attr::VariantNameType::Str(_) => quote!(_serde::Deserializer::deserialize_str(__deserializer, __FieldVisitor)), + } + } + else { + quote!(_serde::Deserializer::deserialize_any(__deserializer, __FieldVisitor)) + }; + quote_block! { #[allow(non_camel_case_types)] enum __Field #lifetime { @@ -1893,7 +1914,7 @@ fn deserialize_generated_identifier( where __D: _serde::Deserializer<'de>, { - _serde::Deserializer::deserialize_identifier(__deserializer, __FieldVisitor) + #serde_deserializer } } } From c754e6b7336a1aa3de46885ee06c85ccaca7c7c7 Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Wed, 28 Nov 2018 18:35:47 +0100 Subject: [PATCH 10/12] Remove unreachable pattern in deserialize::visit_bool Generate the true|false arms to avoid pattern true|false not covered error when the enum has only true or false arms --- serde_derive/src/de.rs | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index 67e92b170..77fee9327 100644 --- a/serde_derive/src/de.rs +++ b/serde_derive/src/de.rs @@ -2263,6 +2263,22 @@ fn deserialize_identifier( let visit_bool = if constructors_bools.is_empty() { None } else { + // generate all arms to have an exhaustive patterns for bool + let missing_true_arm = !field_bools.clone().any(|b| b == true); + let missing_false_arm = !field_bools.clone().any(|b| b == false); + + let fallthrough_true_arm = if missing_true_arm { + quote!(true => _serde::export::Err(_serde::de::Error::unknown_variant("true", VARIANTS)),) + } else { + quote!() + }; + + let fallthrough_false_arm = if missing_false_arm { + quote!(false => _serde::export::Err(_serde::de::Error::unknown_variant("false", VARIANTS)),) + } else { + quote!() + }; + let visit_bool = quote! { fn visit_bool<__E>(self, __value: bool) -> _serde::export::Result where __E: _serde::de::Error @@ -2271,10 +2287,8 @@ fn deserialize_identifier( #( #field_bools => _serde::export::Ok(#constructors_bools), )* - _ => { - #bool_to_str - #fallthrough_arm - } + #fallthrough_true_arm + #fallthrough_false_arm } } }; From 95344ca83ed284215060becf3d7460fe41141c3e Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Wed, 28 Nov 2018 19:35:24 +0100 Subject: [PATCH 11/12] Omit visitors when we don't need them --- serde_derive/src/de.rs | 113 ++++++++++++++++++++++------------------- 1 file changed, 62 insertions(+), 51 deletions(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index 77fee9327..6c38d39a9 100644 --- a/serde_derive/src/de.rs +++ b/serde_derive/src/de.rs @@ -2119,7 +2119,7 @@ fn deserialize_identifier( let variant_indices = 0_u64..; let fallthrough_msg = format!("{} index 0 <= i < {}", index_expecting, fields.len()); let visit_other = if collect_other_fields { - quote! { + Some(quote! { fn visit_bool<__E>(self, __value: bool) -> _serde::export::Result where __E: _serde::de::Error, @@ -2241,22 +2241,26 @@ fn deserialize_identifier( } } } - } + }) } else { - quote! { - fn visit_u64<__E>(self, __value: u64) -> _serde::export::Result - where - __E: _serde::de::Error, - { - match __value { - #( - #field_ints => _serde::export::Ok(#constructors_ints), - )* - _ => _serde::export::Err(_serde::de::Error::invalid_value( - _serde::de::Unexpected::Unsigned(__value), - &#fallthrough_msg)) + if constructors_ints.is_empty() { + None + } else { + Some(quote! { + fn visit_u64<__E>(self, __value: u64) -> _serde::export::Result + where + __E: _serde::de::Error, + { + match __value { + #( + #field_ints => _serde::export::Ok(#constructors_ints), + )* + _ => _serde::export::Err(_serde::de::Error::invalid_value( + _serde::de::Unexpected::Unsigned(__value), + &#fallthrough_msg)) + } } - } + }) } }; @@ -2279,11 +2283,11 @@ fn deserialize_identifier( quote!() }; - let visit_bool = quote! { + Some(quote! { fn visit_bool<__E>(self, __value: bool) -> _serde::export::Result - where __E: _serde::de::Error - { - match __value { + where __E: _serde::de::Error + { + match __value { #( #field_bools => _serde::export::Ok(#constructors_bools), )* @@ -2291,8 +2295,44 @@ fn deserialize_identifier( #fallthrough_false_arm } } - }; - Some(visit_bool) + }) + }; + + let visit_str_and_bytes = if constructors_strs.is_empty() { + None + } else { + Some(quote! { + fn visit_str<__E>(self, __value: &str) -> _serde::export::Result + where + __E: _serde::de::Error, + { + match __value { + #( + #field_strs => _serde::export::Ok(#constructors_strs), + )* + _ => { + #value_as_str_content + #fallthrough_arm + } + } + } + + fn visit_bytes<__E>(self, __value: &[u8]) -> _serde::export::Result + where + __E: _serde::de::Error, + { + match __value { + #( + #field_bytes => _serde::export::Ok(#constructors_strs), + )* + _ => { + #bytes_to_str + #value_as_bytes_content + #fallthrough_arm + } + } + } + }) }; quote_block! { @@ -2304,36 +2344,7 @@ fn deserialize_identifier( #visit_bool - fn visit_str<__E>(self, __value: &str) -> _serde::export::Result - where - __E: _serde::de::Error, - { - match __value { - #( - #field_strs => _serde::export::Ok(#constructors_strs), - )* - _ => { - #value_as_str_content - #fallthrough_arm - } - } - } - - fn visit_bytes<__E>(self, __value: &[u8]) -> _serde::export::Result - where - __E: _serde::de::Error, - { - match __value { - #( - #field_bytes => _serde::export::Ok(#constructors_strs), - )* - _ => { - #bytes_to_str - #value_as_bytes_content - #fallthrough_arm - } - } - } + #visit_str_and_bytes } } From 1d0146bc698a693ed9d91cb886a5a803aa8bf9e9 Mon Sep 17 00:00:00 2001 From: Alessio Coltellacci Date: Thu, 6 Dec 2018 11:21:52 +0100 Subject: [PATCH 12/12] Add guard on fields when we generate the serde_deserializer We verify if the len > 0 to avoid a runtime failure --- serde_derive/src/de.rs | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/serde_derive/src/de.rs b/serde_derive/src/de.rs index 6c38d39a9..3cf6da27d 100644 --- a/serde_derive/src/de.rs +++ b/serde_derive/src/de.rs @@ -1872,25 +1872,29 @@ fn deserialize_generated_identifier( None }; - let first_field_ident = &fields[0].0; - let same_variant_type = fields.iter().all(|ref f| { - match (&f.0, first_field_ident) { - (attr::VariantNameType::Bool(_), attr::VariantNameType::Bool(_)) => true, - (attr::VariantNameType::Int(_), attr::VariantNameType::Int(_)) => true, - (attr::VariantNameType::Str(_), attr::VariantNameType::Str(_)) => true, - _ => false - } - }); + let serde_deserializer = if !fields.is_empty() { + let first_field_ident = &fields[0].0; + let same_variant_type = fields.iter().all(|ref f| { + match (&f.0, first_field_ident) { + (attr::VariantNameType::Bool(_), attr::VariantNameType::Bool(_)) => true, + (attr::VariantNameType::Int(_), attr::VariantNameType::Int(_)) => true, + (attr::VariantNameType::Str(_), attr::VariantNameType::Str(_)) => true, + _ => false + } + }); - let serde_deserializer = if same_variant_type { - match first_field_ident { - attr::VariantNameType::Bool(_) => quote!(_serde::Deserializer::deserialize_bool(__deserializer, __FieldVisitor)), - attr::VariantNameType::Int(_) => quote!(_serde::Deserializer::deserialize_u64(__deserializer, __FieldVisitor)), - attr::VariantNameType::Str(_) => quote!(_serde::Deserializer::deserialize_str(__deserializer, __FieldVisitor)), + if same_variant_type { + match first_field_ident { + attr::VariantNameType::Bool(_) => quote!(_serde::Deserializer::deserialize_bool(__deserializer, __FieldVisitor)), + attr::VariantNameType::Int(_) => quote!(_serde::Deserializer::deserialize_u64(__deserializer, __FieldVisitor)), + attr::VariantNameType::Str(_) => quote!(_serde::Deserializer::deserialize_str(__deserializer, __FieldVisitor)), + } } - } - else { - quote!(_serde::Deserializer::deserialize_any(__deserializer, __FieldVisitor)) + else { + quote!(_serde::Deserializer::deserialize_any(__deserializer, __FieldVisitor)) + } + } else { + quote!(_serde::Deserializer::deserialize_identifier(__deserializer, __FieldVisitor)) }; quote_block! {