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

Deserializers should attempt a final next_element or next_key, even when there's a fixed set of elements. #2198

Closed
Lucretiel opened this issue Apr 7, 2022 · 2 comments

Comments

@Lucretiel
Copy link

Lucretiel commented Apr 7, 2022

Sometimes, Deserializers don't check to ensure that a SeqAccess or MapAccess was fully consumed during visitation (for instance, 3Hren/msgpack-rust#287). While those Deserializers should implement the proper checks (because there's no way to guarantee that the sequence was consumed), it would be nice if serde could help them out by having fixed-size collections call next_element anyway, to ensure that the collection is fully drained.

For example, the tuple implementation could be improved as follows:

                        fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
                        where
                            A: SeqAccess<'de>,
                        {
                            $(
                                let $name = match try!(seq.next_element()) {
                                    Some(value) => value,
                                    None => return Err(Error::invalid_length($n, &self)),
                                };
                            )+

                            let mut excess: usize = 0;
                            while let Some(de::IgnoredAny) = try!(seq.next_element()) {
                                excess += 1;
                            }

                            // Ensure there aren't too *many* elements in the sequence
                            match excess {
                                0 => Ok(($($name,)+)),
                                n => Err(Error::invalid_length($len+n, &self)),
                            }
                        }

This proposal applies both to derived Deserializers (eg, for tuple structs) and to built-in standard library implementations (eg, for [T; N]).

Related to #1557

@Lucretiel
Copy link
Author

Lucretiel commented Apr 7, 2022

Upon reflection, it's possible that this might be too damaging to non-self-describing formats. My expectation would be that such formats make use of the static parameters provided by the relevant deserialize methods, though (eg, use len in deserialize_tuple).

@Lucretiel
Copy link
Author

It actually occurred to me today that this behavior might be outright undesireable, as it would preclude certain useful compositions (for instance, flattening sequences into tuple structs, in addition to flattening maps into regular structs). Between that idea and the self-describing issue I'm going to retract this proposal.

See also 3Hren/msgpack-rust#296

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant