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.

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

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

Merged
merged 9 commits into from Aug 5, 2023

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented May 19, 2023

This PR fixes two problems:

  • The first one is that aliases seems to be sorted in the expecting message because their are collected from BTreeSet, but after adding a main name to the sorted list of aliases it is not sorted anymore
  • The second one is that #[serde(field_identifier)] and #[serde(variant_identifier)] does not include aliases in their default expecting message.

I've updated tests for identifiers and fix both problems. I put them into one PR because the first problem affects the newly added test for aliases for the second problem.

The last 4 commits are small refactoring that slightly reduces number of clones and optimizes generation.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I agree that the current order of names in the error message is probably the worst possible.

#[derive(serde::Deserialize, Debug)]
#[serde(deny_unknown_fields)]
pub struct Thing {
    #[serde(alias = "z", alias = "x")]
    pub y: u8,
}

fn main() {
    let j = r#" {"w":null} "#;
    println!("{}", serde_json::from_str::<Thing>(j).unwrap_err());
}
unknown field `w`, expected one of `x`, `z`, `y` at line 1 column 5

If I understand correctly, your PR changes that to "expected one of x, y, z".

I think my preference would be to respect the order given by the programmer, by putting first the name they chose to elevate uniquely as the real name of the field in Rust, and following it with the other names they wrote, in the order written. "expected one of y, z, x". What do you think?

If some other order would be especially desired in the context of a particular application, they can always include the real field name in the appropriate spot in the explicit list of aliases (I believe this is already allowed today). serde(alias = "z", alias = "x", alias = "y")

@Mingun
Copy link
Contributor Author

Mingun commented Aug 2, 2023

If I understand correctly, your PR changes that to "expected one of x, y, z".

Yes.

I think my preference would be to respect the order given by the programmer, by putting first the name they chose to elevate uniquely as the real name of the field in Rust, and following it with the other names they wrote, in the order written. "expected one of y, z, x". What do you think?

That easily could get out of control. What order we should expect in that case?

#[derive(serde::Deserialize, Debug)]
#[serde(deny_unknown_fields)]
pub struct Thing {
    pub w: u8,

    #[serde(alias = "z", alias = "x")]
    pub y: u8,

    #[serde(alias = "same", alias = "other", alias = "same")]
    pub same: u8,
}

Current output is

unknown field `j`, expected one of `w`, `x`, `z`, `y`, `other`, `same` at line 1 column 5

It seems that the sorted list of all fields is a better choice, IMO. While you can reorder fields in your struct to create a some sorted list, it

  • can break logical groups of fields in your struct
  • you cannot implement total sorting because you cannot insert one field between aliases of other field (field a with aliases a, c and field b with aliases b, d)

My implementation also does not keep totally sorted list, but that can be fixed

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense now — thanks!

@dtolnay dtolnay merged commit 431636a into serde-rs:master Aug 5, 2023
20 checks passed
@Mingun Mingun deleted the identifier branch August 6, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants