fix: serde::Visitors should have visit_map rather that visit_seq called when deserializing structs
#2905
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@jeswr is attempting to deploy a commit to the RISC Zero Team on Vercel. A member of the Team first needs to authorize it. |
| visitor.visit_map(MapAccess { | ||
| deserializer: self, | ||
| len: fields.len(), | ||
| }) |
There was a problem hiding this comment.
This is a slight hack; to implement this properly MapAccess should be checking that only the fields defined in fields are being passed to the map visitor.
There was a problem hiding this comment.
@flaub Before I proceed to:
- add this check to
MapAccess, and - add a test cases where there are entries in the struct
Could I please ask you to review this draft PR - to confirm:
- The previous use of
deserialize_seqhere was a bug rather than the intended behavior - That you are comfortable with moving forward by adding an
Optionalfieldsproperty toMapAccess; which would add validation behavior to the keys iterated through byMapAccess.
There was a problem hiding this comment.
Note that the change introduces the following test failures:
failures:
---- serde::deserializer::tests::test_struct stdout ----
thread 'serde::deserializer::tests::test_struct' panicked at risc0/zkvm/src/serde/deserializer.rs:576:49:
called `Result::unwrap()` on an `Err` value: NotSupported
---- serde::deserializer::tests::test_str stdout ----
thread 'serde::deserializer::tests::test_str' panicked at risc0/zkvm/src/serde/deserializer.rs:594:49:
called `Result::unwrap()` on an `Err` value: NotSupported
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
serde::deserializer::tests::test_str
serde::deserializer::tests::test_struct
It is unclear to me whether this is a "bug in the tests"; in that I am unsure if they are testing for behviors which conflict with serde behavior.
The error here appears to be because deserialize_identifer is not implemented - which I am unsure how to implement.
There was a problem hiding this comment.
Created #2906 to try and answer this question, would be good if CI can be run on that PR.
jeswr
changed the title
serde::Visitors should have visit_map rather that visit_seq called when deserializing structs
Working towards a fix of #2871. The underlying issue of which is that in
serde::Visitorsshould havevisit_maprather thatvisit_seqcalled when deserializing structs.Marking as ready for review as feedback is needed on this comment.
Note, the implementation of
WordReadfor&stris co-pilot generated and requires checking before merging.