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

Implement serialization of tuple variants of flatten enums #2448

Merged
merged 10 commits into from Jul 30, 2023

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented May 9, 2023

Actually, I investigating a way of possible fixes of #1183, during which I try to narrow frontier of work as much as possible. I investigating an idea of using configurable buffer instead of generic Content. During that I found that generalizing of this type requires some knowledge of the Content internals -- VariantDeserializer::tuple_variant and VariantDeserializer::struct_variant does the matching over Contents content:

serde/serde/src/private/de.rs

Lines 1553 to 1557 in 25381be

match self.value {
Some(Content::Seq(v)) => {
de::Deserializer::deserialize_any(SeqDeserializer::new(v), visitor)
}
Some(other) => Err(de::Error::invalid_type(

serde/serde/src/private/de.rs

Lines 1576 to 1583 in 25381be

match self.value {
Some(Content::Map(v)) => {
de::Deserializer::deserialize_any(MapDeserializer::new(v), visitor)
}
Some(Content::Seq(v)) => {
de::Deserializer::deserialize_any(SeqDeserializer::new(v), visitor)
}
Some(other) => Err(de::Error::invalid_type(

Here I noticed, that MapDeserializer and SeqDeserializer are different structs from the common de::value module. At first glance the difference is cosmetic, so I tried to remove custom implementations and use counterparts from de::value. The last commit of this PR does that and actually that was the commit that I want to PR.

Because this change could have non-obvious consequences, I tried to find tests that reaches changed code paths. During that I found that for some of them there are no tests, so I write them.

However, in the process of writing tests, it turned out that deserialization of the enum tuple variant from sequence is supported, but such an enumeration cannot be serialized. So in the end I implemented that and close the gap.

When writing tests, I tried to organize them hierarchically, because in the current approach it is completely incomprehensible which parts are tested and which are not. Tests piled into a heap without any system. I found, that organizing tests using built-in capabilities of Rust -- modules -- greatly improves readability of tests and understanding of what is tested. Because tests in this PR not the main thing on which I focus, you can find that organizing of them are slightly incomplete. That is true. I plan to clean them up in the following PRs.

@Mingun Mingun force-pushed the ser-flatten-enums branch 2 times, most recently from 2fbce60 to 1986e92 Compare June 8, 2023 16:10
Mingun added 10 commits July 11, 2023 21:56
(review with "ignore whitespace" option on and editor that shows line moves,
for example, TortoiseGitMerge)
failures (1):
    flatten::enum_::externally_tagged::tuple
…ged enums

The Container struct

  struct Container {
    #[serde(flatten)]
    enum_field: Enum,
  }
  enum Enum {
    Tuple(u32, u32),
  }

now can be serialized to JSON as

  { "enum_field": [1, 2] }

Deserialization already works

Fixes (1):
    flatten::enum_::externally_tagged::tuple
…for enums

Those deserializers are used to deserialize tuple or struct variants from Content
which is used by internally tagged enums and by flatten

FlatMapDeserializer is reached in the following tests:
    flatten::enum_::externally_tagged::newtype
    flatten::enum_::externally_tagged::struct_from_map
    flatten::enum_::externally_tagged::struct_from_seq
    flatten::enum_::externally_tagged::tuple

ContentDeserializer is reached in the following tests:
    test_enum_in_internally_tagged_enum
    test_internally_tagged_struct_variant_containing_unit_variant
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!

I appreciate the detour into improving the tests.

@dtolnay dtolnay merged commit 427c839 into serde-rs:master Jul 30, 2023
20 checks passed
@Mingun Mingun deleted the ser-flatten-enums branch July 31, 2023 05:23
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