-
-
Notifications
You must be signed in to change notification settings - Fork 873
Exhaustive internally tagged tests + support of internally tagged enums in non self-describing formats #2569
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
base: master
Are you sure you want to change the base?
Conversation
3ea3cc4 to
dafede9
Compare
a08c600 to
a20d574
Compare
8fe93e5 to
9702ce1
Compare
9702ce1 to
8222225
Compare
8222225 to
02835cd
Compare
02835cd to
86108c7
Compare
As mentioned in jamesmunns/postcard#125 (comment), I don't think this is compatible with non-self-describing formats--at least postcard--because maps and structs are serialized differently and cannot be distinguished during deserialization:
For instance, if variant |
86108c7 to
4800f46
Compare
…dd comments about coverage
failures(1):
newtype_unit
…lize enums Helper methods visit_content_[seq|map] does the same as [Seq|Map]Deserializer::deserialize_any and used everywhere except here. Reuse them for consistency
…sibility of the Visitor
Examples of errors produced during deserialization of internally tagged enums in tests
if instead of a Seq/Map a Str("unexpected string") will be provided:
In tests/test_annotations.rs
flatten::enum_::internally_tagged::tuple:
before: `invalid type: string "unexpected string", expected tuple variant`
after : `invalid type: string "unexpected string", expected tuple variant Enum::Tuple`
flatten::enum_::internally_tagged::struct_from_map:
before: `invalid type: string "unexpected string", expected struct variant`
after : `invalid type: string "unexpected string", expected struct variant Enum::Struct`
…zer directly That is cheap, creating a Content[Ref]Deserializer is a no-op
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
…ms in non self-describing formats Visitor passed to the deserialize_any supports only visit_map method, so we can always request deserialize_map
…untagged and adjacently tagged enums
4800f46 to
0293c0d
Compare
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-deprecated/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