Exhaustive internally tagged tests + support of internally tagged enums in non self-describing formats #2569
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unit variant of externally tagged enum cannot be deserialized from the string
token by itself. It is ContentDeserializer + serde_test::Deserializer that makes
this possible, because serde_test::Deserializer produces Content::Str() from
Token::BorrowedStr() and ContentDeserializer produces unit variant from Content::Str().
The following tokens all produces Content::String(variant):
- Token::String(variant)
- Token::Str(variant)
- Token::UnitVariant { variant, .. }
Token::BorrowedStr(variant) produces Content::Str(variant) that was the real purpose to
use it in test in serde-rs#933. This actually makes this test testing `Content` rather than type itself.
Correct way to represent enum one of:
- [xxxVariant { .. }]
- [Enum { .. }, xxxVariant { variant, .. }]
- [Enum { .. }, String(variant), <variant content>]
- [Enum { .. }, Str(variant), <variant content>]
- [Enum { .. }, BorrowedStr(variant), <variant content>]
(review this commit with "ignore whitespace changes" option on)
Moved: - test_internally_tagged_bytes => string_and_bytes mod - test_internally_tagged_struct_variant_containing_unit_variant => struct_variant_containing_unit_variant - test_internally_tagged_borrow => borrow - test_enum_in_internally_tagged_enum => newtype_variant_containing_externally_tagged_enum - test_internally_tagged_newtype_variant_containing_unit_struct => newtype_variant_containing_unit_struct (review this commit with "ignore whitespace changes" option on)
We already have two tests where inner types serialized as a Struct container and as a Map container. No need yet another test for a Map container
Ctrl+X, Ctrl+V
Moved all except flattened tests: - test_internally_tagged_enum_with_skipped_conflict => with_skipped_conflict - test_internally_tagged_enum_containing_flatten => containing_flatten - test_internally_tagged_enum_new_type_with_unit => newtype_variant_containing_unit - test_internally_tagged_unit_enum_with_unknown_fields => unit_variant_with_unknown_fields - test_expecting_message_internally_tagged_enum => expecting_message
…ct_variant_containing_unit_variant
- newtype_variant_containing_unit -> newtype_unit - newtype_variant_containing_unit_struct -> newtype_unit_struct - newtype_variant_containing_externally_tagged_enum -> newtype_enum - struct_variant_containing_unit_variant -> struct_enum
(review this commit with "ignore whitespace changes" option on)
…internally tagged enum failures (3) - until serde-rs/test#31 is merged: struct_enum::newtype struct_enum::struct_ struct_enum::tuple
failures(1):
newtype_unit
In theory, ContentRefDeserializer should have matching implementation, but handling empty maps and sequences as units breaks some cases with untagged enums: - test_untagged_newtype_variant_containing_unit_struct_not_map
…sibility of the Visitor
Mingun
changed the title
Deserializer methods are only hints which deserializer is not obliged to follow. Both TaggedContentVisitor and InternallyTaggedUnitVisitor accepts only visit_map and visit_seq and that is what derived implementation of Deserialize does for structs. Therefore it is fine to call deserialize_map here, as that already did in derived deserialize implementation
Mingun
force-pushed
the
internally-tagged-tests
branch
from
3ea3cc4
to
dafede9
Compare
…ms in non self-describing formats Visitor that passed th the deserialize_any supports only visit_map method, so we can always requests deserialize_map
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR starts as a refactor of internally tagged enums tests in preparation to updating #1922, but in the process of development, I found some interesting consequences.
The first is that
ContentDeserializerandContentRefDeserializerdo what they should not do, namely, determine the validity of certain data when calling deserialization methods. AllDeserializermethods are just a hints what data is expected from the deserializer, and the deserializer is not obliged to follow them. It is free to call anyVisitormethod it sees fit. It isVisitorresponsibility to detect if calledvisit_*method appropriate or not. So I removed all decisions fromContentDeserializerandContentRefDeserializerabout validity of content in certain methods.This change will allow to make the next step and eliminate all calls to
deserialize_anyfrom internally tagged enums. Because this method does not called anymore, this would allow to support internally tagged enums in non-self-describing formats. I've replaceddeserialize_anywithdeserialize_mapbecause:serialize_map(for newtype variants) /serialize_struct(for unit and struct variants), that means that deserialization also should expect mapDeserializer::deserialize_maporDeserializer::deserialize_structand provide a visitor withvisit_mapandvisit_seq. This is what bothTaggedContentVisitorandInternallyTaggedUnitVisitorprovidesThe same considerations are used when replacing
deserialize_anywithdeserialize_mapfor untagged variant; that code path is also called for adjacently tagged enums. Passed visitor supports onlyvisit_mapmethod.This PR depends on the serde-rs/test#31 and I temporary add a
[patch]section to use patched version to make tests passable. That is why this PR in a draft stage.Closes #2187
Replaces #2557