Fix incorrect deserialization of vectors of enums from sequences of tags #663
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
Complex types should pass the same deserializer that deserializes them instead of top-level deserializer. This commit is the first prepare step for that. It just manually expands the `forward!` macro. In the following commits, its usefulness will be reduced
Behavior does not changed, because all calls to `self` (which is MapValueDeserializer or SeqItemDeserializer) in the end still passed to Deserializer, as before. Now the only methods to fix are - deserialize_struct - deserialize_enum Both erroneously forward to the top-level deserializer which have it's own specific, that shouldn't be applied when we deserialize an XML fragment inside XML tree
This will be needed later when we split SeqItemDeserializer into two deserializers. Just copied implementation of crate::de::var::EnumAccess and slightly tune lifetimes
In this commit the implementation of SeqItemDeserializer just copied as TextDeserializer and SeqItemDeserializer renamed to ElementDeserializer.
… of events at creation time
…ize_tuple and deserialize_struct of the same deserializer
failures (2):
serde-de-seq (1):
seq::variable_name::fixed_size::list_of_enum
serde-issues (1):
issue567
…type variant The implementation of `VariantAccess::newtype_variant` should be the same as `Deserializer::visit_newtype_struct`
…e it by reference to Deserializer
Mingun
added
bug
serde
Mingun
commented
Oct 9, 2023
Comment on lines
+607
to
+609
| { | ||
| visitor.visit_enum(crate::de::var::EnumAccess::new(self.map.de)) | ||
| } |
There was a problem hiding this comment.
This call also incorrect, because it will use top-level deserializer for deserializing newtype variants, for example, but I leave fixing that for separate PR. We do not have tests for this situation yet, and I want to approve each change by tests that failed before and pass after the change.
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 fixes #567 which is an instance of generic problem, where top-level deserializer used to deserialize elements inside
<tags>. It changes things in that way that correct deserializer is used to deserialize types.The diagram below shows how each struct is related to each other in this PR (
SimpleTypeDeserializerhas more usages, not all of them shown in order to keep diagram small):flowchart TD Deserializer --maps and structs--> ElementMapAccess ElementMapAccess --for $text field--> SimpleTypeDeserialzier ElementMapAccess --for each field--> MapValueDeserializer MapValueDeserializer --for sequences--> MapValueSeqAccess MapValueSeqAccess --DeEvent::Start--> ElementDeserializer MapValueSeqAccess --DeEvent::Text--> TextDeserializer ElementDeserializer --maps and structs--> ElementMapAccess TextDeserializer --> SimpleTypeDeserialzierMost commits are prepare work, the actual fix is just one line change and yet another for consistence (and I believe also fixes some bug).
The last four commits just some cleanup work, but important because it adds documentation how deserializers work and what is their difference from each other.