Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Keep aliases always sorted and include aliases in expecting message for field/variant_identifier #2458

Merged
merged 9 commits into from Aug 5, 2023
Merged

Keep aliases always sorted and include aliases in expecting message for field/variant_identifier #2458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bb51e68
Keep aliases sorted
Mingun May 19, 2023
967023b
Group field_identifier and variant_identifier tests in sub-modules
Mingun May 19, 2023
bc1960b
Add tests for unknown field / variant
Mingun May 19, 2023
1efb8b6
Add tests for aliases
Mingun May 19, 2023
78fea3a
Show possible aliases in the expected message
Mingun May 19, 2023
7ca7720
Slightly reduced number of allocations
Mingun May 19, 2023
855acaf
Eliminate additional allocations for flattening aliases
Mingun May 19, 2023
089aae1
Eliminate even more allocations
Mingun May 19, 2023
f709fc0
Do not run the code when results are not used
Mingun May 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
91 changes: 39 additions & 52 deletions serde_derive/src/de.rs
View file
Open in desktop
Expand Up @@ -2153,7 +2153,7 @@ fn deserialize_custom_identifier(
})
.collect();

let names = names_idents.iter().map(|(name, _, _)| name);
let names = names_idents.iter().flat_map(|(_, _, aliases)| aliases);

let names_const = if fallthrough.is_some() {
None
Expand Down Expand Up @@ -2216,34 +2216,24 @@ fn deserialize_identifier(
collect_other_fields: bool,
expecting: Option<&str>,
) -> Fragment {
let mut flat_fields = Vec::new();
for (_, ident, aliases) in fields {
flat_fields.extend(aliases.iter().map(|alias| (alias, ident)));
}

let field_strs: &Vec<_> = &flat_fields.iter().map(|(name, _)| name).collect();
let field_bytes: &Vec<_> = &flat_fields
.iter()
.map(|(name, _)| Literal::byte_string(name.as_bytes()))
.collect();

let constructors: &Vec<_> = &flat_fields
.iter()
.map(|(_, ident)| quote!(#this_value::#ident))
.collect();
let main_constructors: &Vec<_> = &fields
.iter()
.map(|(_, ident, _)| quote!(#this_value::#ident))
.collect();
let str_mapping = fields.iter().map(|(_, ident, aliases)| {
// `aliases` also contains a main name
quote!(#(#aliases)|* => _serde::__private::Ok(#this_value::#ident))
});
let bytes_mapping = fields.iter().map(|(_, ident, aliases)| {
// `aliases` also contains a main name
let aliases = aliases
.iter()
.map(|alias| Literal::byte_string(alias.as_bytes()));
quote!(#(#aliases)|* => _serde::__private::Ok(#this_value::#ident))
});

let expecting = expecting.unwrap_or(if is_variant {
"variant identifier"
} else {
"field identifier"
});

let index_expecting = if is_variant { "variant" } else { "field" };

let bytes_to_str = if fallthrough.is_some() || collect_other_fields {
None
} else {
Expand Down Expand Up @@ -2291,21 +2281,6 @@ fn deserialize_identifier(
&fallthrough_arm_tokens
};

let u64_fallthrough_arm_tokens;
let u64_fallthrough_arm = if let Some(fallthrough) = &fallthrough {
fallthrough
} else {
let fallthrough_msg = format!("{} index 0 <= i < {}", index_expecting, fields.len());
u64_fallthrough_arm_tokens = quote! {
_serde::__private::Err(_serde::de::Error::invalid_value(
_serde::de::Unexpected::Unsigned(__value),
&#fallthrough_msg,
))
};
&u64_fallthrough_arm_tokens
};

let variant_indices = 0_u64..;
let visit_other = if collect_other_fields {
quote! {
fn visit_bool<__E>(self, __value: bool) -> _serde::__private::Result<Self::Value, __E>
Expand Down Expand Up @@ -2400,32 +2375,50 @@ fn deserialize_identifier(
}
}
} else {
let u64_mapping = fields.iter().enumerate().map(|(i, (_, ident, _))| {
let i = i as u64;
quote!(#i => _serde::__private::Ok(#this_value::#ident))
});

let u64_fallthrough_arm_tokens;
let u64_fallthrough_arm = if let Some(fallthrough) = &fallthrough {
fallthrough
} else {
let index_expecting = if is_variant { "variant" } else { "field" };
let fallthrough_msg = format!("{} index 0 <= i < {}", index_expecting, fields.len());
u64_fallthrough_arm_tokens = quote! {
_serde::__private::Err(_serde::de::Error::invalid_value(
_serde::de::Unexpected::Unsigned(__value),
&#fallthrough_msg,
))
};
&u64_fallthrough_arm_tokens
};

quote! {
fn visit_u64<__E>(self, __value: u64) -> _serde::__private::Result<Self::Value, __E>
where
__E: _serde::de::Error,
{
match __value {
#(
#variant_indices => _serde::__private::Ok(#main_constructors),
)*
#(#u64_mapping,)*
_ => #u64_fallthrough_arm,
}
}
}
};

let visit_borrowed = if fallthrough_borrowed.is_some() || collect_other_fields {
let str_mapping = str_mapping.clone();
let bytes_mapping = bytes_mapping.clone();
let fallthrough_borrowed_arm = fallthrough_borrowed.as_ref().unwrap_or(fallthrough_arm);
Some(quote! {
fn visit_borrowed_str<__E>(self, __value: &'de str) -> _serde::__private::Result<Self::Value, __E>
where
__E: _serde::de::Error,
{
match __value {
#(
#field_strs => _serde::__private::Ok(#constructors),
)*
#(#str_mapping,)*
_ => {
#value_as_borrowed_str_content
#fallthrough_borrowed_arm
Expand All @@ -2438,9 +2431,7 @@ fn deserialize_identifier(
__E: _serde::de::Error,
{
match __value {
#(
#field_bytes => _serde::__private::Ok(#constructors),
)*
#(#bytes_mapping,)*
_ => {
#bytes_to_str
#value_as_borrowed_bytes_content
Expand All @@ -2465,9 +2456,7 @@ fn deserialize_identifier(
__E: _serde::de::Error,
{
match __value {
#(
#field_strs => _serde::__private::Ok(#constructors),
)*
#(#str_mapping,)*
_ => {
#value_as_str_content
#fallthrough_arm
Expand All @@ -2480,9 +2469,7 @@ fn deserialize_identifier(
__E: _serde::de::Error,
{
match __value {
#(
#field_bytes => _serde::__private::Ok(#constructors),
)*
#(#bytes_mapping,)*
_ => {
#bytes_to_str
#value_as_bytes_content
Expand Down
20 changes: 15 additions & 5 deletions serde_derive/src/internals/attr.rs
View file
Open in desktop
Expand Up @@ -184,12 +184,20 @@ impl Name {
}

fn deserialize_aliases(&self) -> Vec<String> {
let mut aliases = self.deserialize_aliases.clone();
let main_name = self.deserialize_name();
if !aliases.contains(&main_name) {
aliases.push(main_name);
self.deserialize_aliases.clone()
}

fn correct_aliases(&mut self) {
// `deserialize_aliases` got from a BTreeSet, so it sorted and does not
// contain duplicates.
// We cannot insert main name in `new` because rename_all rules not yet
// applied there.
match self.deserialize_aliases.binary_search(&self.deserialize) {
Ok(_) => {} // element already here
Err(pos) => self
.deserialize_aliases
.insert(pos, self.deserialize.clone()),
}
aliases
}
}

Expand Down Expand Up @@ -928,6 +936,7 @@ impl Variant {
if !self.name.deserialize_renamed {
self.name.deserialize = rules.deserialize.apply_to_variant(&self.name.deserialize);
}
self.name.correct_aliases();
}

pub fn rename_all_rules(&self) -> &RenameAllRules {
Expand Down Expand Up @@ -1267,6 +1276,7 @@ impl Field {
if !self.name.deserialize_renamed {
self.name.deserialize = rules.deserialize.apply_to_field(&self.name.deserialize);
}
self.name.correct_aliases();
}

pub fn skip_serializing(&self) -> bool {
Expand Down
4 changes: 2 additions & 2 deletions test_suite/tests/test_annotations.rs
View file
Open in desktop
Expand Up @@ -643,7 +643,7 @@ fn test_unknown_field_rename_struct() {
Token::Str("a4"),
Token::I32(3),
],
"unknown field `a4`, expected one of `a1`, `a3`, `a2`, `a5`, `a6`",
"unknown field `a4`, expected one of `a1`, `a2`, `a3`, `a5`, `a6`",
);
}

Expand Down Expand Up @@ -837,7 +837,7 @@ fn test_unknown_field_rename_enum() {
Token::Str("d"),
Token::I8(2),
],
"unknown field `d`, expected one of `a`, `c`, `b`, `e`, `f`",
"unknown field `d`, expected one of `a`, `b`, `c`, `e`, `f`",
);
}

Expand Down