Skip to content

Commit

Permalink
Merge pull request #2458 from Mingun/identifier
Browse files Browse the repository at this point in the history 
Keep aliases always sorted and include aliases in expecting message for field/variant_identifier
  • Loading branch information
@dtolnay
dtolnay committed Aug 5, 2023
2 parents 891ced5 + f709fc0 commit 431636a
Show file tree
Hide file tree
Showing 4 changed files with 212 additions and 115 deletions.
91 changes: 39 additions & 52 deletions serde_derive/src/de.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2131,7 +2131,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 @@ -2194,34 +2194,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 @@ -2269,21 +2259,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 @@ -2378,32 +2353,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 @@ -2416,9 +2409,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 @@ -2443,9 +2434,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 @@ -2458,9 +2447,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
Original file line number Diff line number Diff line change
Expand Up @@ -183,12 +183,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 @@ -988,6 +996,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 @@ -1327,6 +1336,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
Original file line number Diff line number Diff line change
Expand Up @@ -605,7 +605,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 @@ -799,7 +799,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

0 comments on commit 431636a

Please sign in to comment.