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

Allow to deserialize struct variants of untagged and adjacently tagged enums from seq #2465

Closed
wants to merge 2 commits into from

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented May 28, 2023

The artificial limitation on use of visit_seq method was introduced in #1058 without any rationale. I suggest to remove it. The consequences are:

  • Struct variants of adjacently tagged enums now can be deserialized using visit_seq method
  • Struct variants of untagged enums now can be deserialized using visit_seq method
    • Because each variant in untagged enum are tried in the declaration order that means that tuple variants always should be defined before struct variants in enum declaration. We can check that during compilation time and issue a warning about that (we can also issue some other warnings in that stage (Warn when defining untagged enum with multiple unit variants #1740 related), for example, structs with similar fields that cannot be distinguished or can be incorrectly distinguished, that is why I didn't include that code in that PR).

This change has a sense, because you already can do the same by wrapping an ordinary struct into a Newtype variant, but I think, that change from

enum Enum {
  Struct {
    // fields
  },
}

to

struct Struct {
  // fields
}
enum Enum {
  Struct(Struct),
}

should not change the behavior because this situation can arise during refactoring and changing behavior can be surprising.

I didn't include tests in that PR, because correct test suite should cover all possible situations, and many of them have certain problems. I plan to include a full testsuite in my upcoming PR for fixing #2105.

Right now you can see what changed in the generated code by running the following commands:

  • place skip.txt into test_suite/tests folder and change its extension to .rs
  • run the following command in master and this PR and compare the generated output:
    ...\serde\test_suite> cargo expand --all-features --test skip

Closes #2473, closes serde-rs/json#1049

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.

"that means that tuple variants always should be defined before struct variants in enum declaration" — I think this kind of consideration is part of why this was implemented the current way. When writing Rust data structures to deserialize a particular pattern of input data into, usually it's relatively harmless if serde contains various implicit conversions which don't apply to the data that you expect to receive. Untagged enums are one of the main exceptions to this: at some point each additional implicit behavior makes it less likely that a data structure has the behavior that the programmer was after, not more likely.

When combined with the other consideration of increasing the compile time bloat of untagged enums further, and the rarity of compact binary formats that try to support untagged enums in the first place, and the existence of a reasonable workaround for those few formats to apply (#2473 (comment)), my sense is this ends up being a net negative change. I can see the rationale in favor of it but I'd like to keep the original behavior in this case.

Thanks anyway for the PR!

@dtolnay dtolnay closed this Jul 31, 2023
Mingun added a commit to Mingun/serde that referenced this pull request Jul 31, 2023
…djacently tagged and untagged enums

serde-rs#2465 maked such deserialization possible
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants