Skip to content

Commit

Permalink
de: Make flattened structs and aliases interact correctly.
Browse files Browse the repository at this point in the history 
The issue is that FlatStructAccess has no access to the aliases of the struct
it's deserializing.

Ideally we'd try to serialize the key rather than checking whether we're going
to use it before-hand, then actually take the value out, but that happens to be
tricky with the current seed API.

So we need to somehow get the aliased names back to FlatStructAccess. Introduce
a serialize_struct-like API that takes them in a backwards-compatible way. For
parallelism, and since we also support aliases on enum variants, also extend the
struct_variant API in a similar way. I'm open to better ways to fix it, but I
can't think of any other that isn't a breaking change...

Fixes serde-rs#1504.
  • Loading branch information
@emilio
emilio committed May 4, 2019
1 parent 3f6548b commit 0fc8cd8
Show file tree
Hide file tree
Showing 4 changed files with 131 additions and 36 deletions.
46 changes: 46 additions & 0 deletions serde/src/de/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1112,6 +1112,28 @@ pub trait Deserializer<'de>: Sized {
where
V: Visitor<'de>;

/// Hint that the `Deserialize` type is expecting a struct with a particular
/// name and fields.
///
/// `_fields_with_aliases` includes all valid field names, including
/// aliases. `fields` only includes the canonical struct fields.
///
/// Use this if you care about aliased fields. For backwards compatibility,
/// by default this calls `deserialize_struct`. If you implement this, you
/// probably want `deserialize_struct` to call this instead.
fn deserialize_struct_with_aliases<V>(
self,
name: &'static str,
fields: &'static [&'static str],
_fields_with_aliases: &'static [&'static str],
visitor: V,
) -> Result<V::Value, Self::Error>
where
V: Visitor<'de>
{
self.deserialize_struct(name, fields, visitor)
}

/// Hint that the `Deserialize` type is expecting an enum value with a
/// particular name and possible variants.
fn deserialize_enum<V>(
Expand Down Expand Up @@ -2191,6 +2213,30 @@ pub trait VariantAccess<'de>: Sized {
) -> Result<V::Value, Self::Error>
where
V: Visitor<'de>;

/// Called when deserializing a struct-like variant.
///
/// The `fields` are the names of the fields of the struct variant.
///
/// `_fields_with_aliases` includes all valid field names, including
/// aliases. `fields` only includes the canonical struct fields.
///
/// Use this if you care about aliased fields. For backwards compatibility,
/// by default this calls `struct_variant`. If you implement this, you
/// probably want `struct_variant` to call this instead.
///
/// Same constraints as `struct_vriant` applies.
fn struct_variant_with_aliases<V>(
self,
fields: &'static [&'static str],
_fields_with_aliases: &'static [&'static str],
visitor: V,
) -> Result<V::Value, Self::Error>
where
V: Visitor<'de>
{
self.struct_variant(fields, visitor)
}
}

////////////////////////////////////////////////////////////////////////////////
Expand Down
17 changes: 15 additions & 2 deletions serde/src/private/de.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2723,14 +2723,27 @@ where

fn deserialize_struct<V>(
self,
_: &'static str,
name: &'static str,
fields: &'static [&'static str],
visitor: V,
) -> Result<V::Value, Self::Error>
where
V: Visitor<'de>,
{
visitor.visit_map(FlatStructAccess::new(self.0.iter_mut(), fields))
self.deserialize_struct_with_aliases(name, fields, fields, visitor)
}

fn deserialize_struct_with_aliases<V>(
self,
_: &'static str,
_: &'static [&'static str],
fields_with_aliases: &'static [&'static str],
visitor: V,
) -> Result<V::Value, Self::Error>
where
V: Visitor<'de>,
{
visitor.visit_map(FlatStructAccess::new(self.0.iter_mut(), fields_with_aliases))
}

fn deserialize_newtype_struct<V>(self, _name: &str, visitor: V) -> Result<V::Value, Self::Error>
Expand Down
103 changes: 70 additions & 33 deletions serde_derive/src/de.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -933,7 +933,12 @@ fn deserialize_struct(
}
} else if is_enum {
quote! {
_serde::de::VariantAccess::struct_variant(__variant, FIELDS, #visitor_expr)
_serde::de::VariantAccess::struct_variant_with_aliases(
__variant,
FIELDS,
FIELDS_WITH_ALIASES,
#visitor_expr,
)
}
} else if cattrs.has_flatten() {
quote! {
Expand All @@ -942,7 +947,13 @@ fn deserialize_struct(
} else {
let type_name = cattrs.name().deserialize_name();
quote! {
_serde::Deserializer::deserialize_struct(__deserializer, #type_name, FIELDS, #visitor_expr)
_serde::Deserializer::deserialize_struct_with_aliases(
__deserializer,
#type_name,
FIELDS,
FIELDS_WITH_ALIASES,
#visitor_expr,
)
}
};

Expand Down Expand Up @@ -1066,12 +1077,23 @@ fn deserialize_struct_in_place(
}
} else if is_enum {
quote! {
_serde::de::VariantAccess::struct_variant(__variant, FIELDS, #visitor_expr)
_serde::de::VariantAccess::struct_variant_with_aliases(
__variant,
FIELDS,
FIELDS_WITH_ALIASES,
#visitor_expr,
)
}
} else {
let type_name = cattrs.name().deserialize_name();
quote! {
_serde::Deserializer::deserialize_struct(__deserializer, #type_name, FIELDS, #visitor_expr)
_serde::Deserializer::deserialize_struct_with_aliases(
__deserializer,
#type_name,
FIELDS,
FIELDS_WITH_ALIASES,
#visitor_expr,
)
}
};

Expand Down Expand Up @@ -1162,6 +1184,7 @@ fn prepare_enum_variant_enum(
)
})
.collect();
let flat_fields = flatten_fields(&variant_names_idents);

let other_idx = variants
.iter()
Expand All @@ -1176,6 +1199,7 @@ fn prepare_enum_variant_enum(

let variant_visitor = Stmts(deserialize_generated_identifier(
&variant_names_idents,
&flat_fields,
cattrs,
true,
other_idx,
Expand Down Expand Up @@ -1600,10 +1624,11 @@ fn deserialize_adjacently_tagged_enum(
}

const FIELDS: &'static [&'static str] = &[#tag, #content];
_serde::Deserializer::deserialize_struct(
_serde::Deserializer::deserialize_struct_with_aliases(
__deserializer,
#type_name,
FIELDS,
FIELDS,
__Visitor {
marker: _serde::export::PhantomData::<#this #ty_generics>,
lifetime: _serde::export::PhantomData,
Expand Down Expand Up @@ -1845,6 +1870,7 @@ fn deserialize_untagged_newtype_variant(

fn deserialize_generated_identifier(
fields: &[(String, Ident, Vec<String>)],
flat_fields: &[(&String, &Ident)],
cattrs: &attr::Container,
is_variant: bool,
other_idx: Option<usize>,
Expand All @@ -1871,6 +1897,7 @@ fn deserialize_generated_identifier(
let visitor_impl = Stmts(deserialize_identifier(
&this,
fields,
flat_fields,
is_variant,
fallthrough,
!is_variant && cattrs.has_flatten(),
Expand Down Expand Up @@ -1955,21 +1982,19 @@ fn deserialize_custom_identifier(
)
})
.collect();
let flat_fields = flatten_fields(&names_idents);

let names = names_idents.iter().map(|&(ref name, _, _)| name);

let names_const = if fallthrough.is_some() {
None
} else if is_variant {
let names = names_idents.iter().map(|&(ref name, _, _)| name);
let variants = quote! {
const VARIANTS: &'static [&'static str] = &[ #(#names),* ];
};
Some(variants)
} else {
let fields = quote! {
const FIELDS: &'static [&'static str] = &[ #(#names),* ];
};
Some(fields)
Some(decl_fields_consts(&names_idents, &flat_fields))
};

let (de_impl_generics, de_ty_generics, ty_generics, where_clause) =
Expand All @@ -1978,6 +2003,7 @@ fn deserialize_custom_identifier(
let visitor_impl = Stmts(deserialize_identifier(
&this,
&names_idents,
&flat_fields,
is_variant,
fallthrough,
false,
Expand Down Expand Up @@ -2008,15 +2034,11 @@ fn deserialize_custom_identifier(
fn deserialize_identifier(
this: &TokenStream,
fields: &[(String, Ident, Vec<String>)],
flat_fields: &[(&String, &Ident)],
is_variant: bool,
fallthrough: Option<TokenStream>,
collect_other_fields: bool,
) -> Fragment {
let mut flat_fields = Vec::new();
for &(_, ref ident, ref aliases) in fields {
flat_fields.extend(aliases.iter().map(|alias| (alias, ident)))
}

let field_strs = flat_fields.iter().map(|&(ref name, _)| name);
let field_borrowed_strs = flat_fields.iter().map(|&(ref name, _)| name);
let field_bytes = flat_fields
Expand Down Expand Up @@ -2272,6 +2294,29 @@ fn deserialize_identifier(
}
}

fn flatten_fields(fields: &[(String, Ident, Vec<String>)]) -> Vec<(&String, &Ident)> {
let mut flat = vec![];
for (_, ident, ref aliases) in fields {
flat.extend(aliases.iter().map(|alias| (alias, ident)))
}
flat
}

fn decl_fields_consts(
fields: &[(String, Ident, Vec<String>)],
flat_fields: &[(&String, &Ident)],
) -> TokenStream {
let block = {
let field_names = fields.iter().map(|&(ref name, _, _)| name);
let aliases = flat_fields.iter().map(|&(ref name, _)| name);
quote! {
const FIELDS: &'static [&'static str] = &[ #(#field_names),* ];
const FIELDS_WITH_ALIASES: &'static [&'static str] = &[ #(#aliases),* ];
}
};
block
}

fn deserialize_struct_as_struct_visitor(
struct_path: &TokenStream,
params: &Parameters,
Expand All @@ -2293,18 +2338,14 @@ fn deserialize_struct_as_struct_visitor(
})
.collect();

let fields_stmt = {
let field_names = field_names_idents.iter().map(|&(ref name, _, _)| name);
quote_block! {
const FIELDS: &'static [&'static str] = &[ #(#field_names),* ];
}
};
let flat_fields = flatten_fields(&field_names_idents);
let fields_stmt = decl_fields_consts(&field_names_idents, &flat_fields);

let field_visitor = deserialize_generated_identifier(&field_names_idents, cattrs, false, None);
let field_visitor = deserialize_generated_identifier(&field_names_idents, &flat_fields, cattrs, false, None);

let visit_map = deserialize_map(struct_path, params, fields, cattrs);

(field_visitor, Some(fields_stmt), visit_map)
(field_visitor, Some(quote_block! { #fields_stmt }), visit_map)
}

fn deserialize_struct_as_map_visitor(
Expand All @@ -2326,7 +2367,8 @@ fn deserialize_struct_as_map_visitor(
})
.collect();

let field_visitor = deserialize_generated_identifier(&field_names_idents, cattrs, false, None);
let flat_fields = flatten_fields(&field_names_idents);
let field_visitor = deserialize_generated_identifier(&field_names_idents, &flat_fields, cattrs, false, None);

let visit_map = deserialize_map(struct_path, params, fields, cattrs);

Expand Down Expand Up @@ -2570,18 +2612,13 @@ fn deserialize_struct_as_struct_in_place_visitor(
})
.collect();

let fields_stmt = {
let field_names = field_names_idents.iter().map(|&(ref name, _, _)| name);
quote_block! {
const FIELDS: &'static [&'static str] = &[ #(#field_names),* ];
}
};

let field_visitor = deserialize_generated_identifier(&field_names_idents, cattrs, false, None);
let flat_fields = flatten_fields(&field_names_idents);
let fields_stmt = decl_fields_consts(&field_names_idents, &flat_fields);
let field_visitor = deserialize_generated_identifier(&field_names_idents, &flat_fields, cattrs, false, None);

let visit_map = deserialize_map_in_place(params, fields, cattrs);

(field_visitor, fields_stmt, visit_map)
(field_visitor, quote_block! { #fields_stmt }, visit_map)
}

#[cfg(feature = "deserialize_in_place")]
Expand Down
1 change: 0 additions & 1 deletion test_suite/tests/test_annotations.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,6 @@ fn test_rename_struct() {
}

#[test]
#[should_panic] // FIXME(emilio): It shouldn't!
fn test_alias_flattened() {
#[derive(Debug, PartialEq, Deserialize)]
struct Flattened {
Expand Down

0 comments on commit 0fc8cd8

Please sign in to comment.