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

Add ability to deserialize enums from SeqAccessDeserializer #2445

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented May 7, 2023

Motivation of this PR is following: when I rebase #1922 I noticed, that test, introduced in the first commit of #2303 failed after the change. That failure appeared after changing

// seq: A where A: SeqAccess
let rest = SeqAccessDeserializer::new(seq);
let content = try!(Content::deserialize(rest));
let deserializer = ContentDeserializer::<A::Error>::new(content);
// use `deserializer`

to

// seq: A where A: SeqAccess
let deserializer = SeqAccessDeserializer::new(seq);
// use `deserializer`

i. e. after removing intermediate bufferisation step. Because code for the MapAccessDeserializer is the same and it does not suffer from such problem, I checked the difference and realized, that MapAccessDeserializer has a special handling fo the Deserializer::deserialize_enum hint:

serde/serde/src/de/value.rs

Lines 1474 to 1484 in 25381be

fn deserialize_enum<V>(
self,
_name: &str,
_variants: &'static [&'static str],
visitor: V,
) -> Result<V::Value, Self::Error>
where
V: de::Visitor<'de>,
{
visitor.visit_enum(self)
}

which is missing for the SeqAccessDeserializer.

Because SeqAccessDeserializer is used in such context when deserialize_enum could be requested from it, I think, it should provide the special implementation for that.

This implementation is following: enum is represented as:

  • two elements, where the first element is a tag and the second is a content
  • one element: a special case for unit variants, where the second element for content can be missed

Additionally, this PR:

  • extends testcases to cover all possible enum variants (Unit, Newtype, Tuple and Struct)
  • extends testcases to cover negative scenarios (wrong tag, empty map / seq, missing content for seq)
  • changes the error message for the empty map for MapAccessDeserializer from invalid type: map, expected enum to invalid length: 0, expected enum, because actually map is expected, but it should not be empty
  • document behavior of Deserializer::deserialize_enum for MapAccessDeserializer and SeqAccessDeserializer

@dtolnay dtolnay changed the title Add ability to deserialize enums from SeqAccessDeserialzier Add ability to deserialize enums from SeqAccessDeserializer Jul 26, 2023
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I am not convinced that this is the behavior that would be expected by users. As far as I can tell the motivation explained in the PR description is to make the following:

let deserializer = SeqAccessDeserializer::new(seq);

behave more like the following:

let rest = SeqAccessDeserializer::new(seq);
let content = try!(Content::deserialize(rest));
let deserializer = ContentDeserializer::<A::Error>::new(content);

But I tried that code, and ContentDeserializer also does not deserialize enums from sequences. The current SeqAccessDeserializer is already consistent with this.

use serde::de::value::SeqAccessDeserializer;
use serde::de::{Deserialize, Deserializer, SeqAccess, Visitor};
use serde_derive::Deserialize;
use std::fmt;

#[derive(Deserialize, Debug)]
enum Enum {
    Variant(usize),
}

struct MyVisitor;

impl<'de> Visitor<'de> for MyVisitor {
    type Value = Enum;
    fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
        formatter.write_str("repro")
    }
    fn visit_seq<A>(self, seq: A) -> Result<Self::Value, A::Error>
    where
        A: SeqAccess<'de>,
    {
        let rest = SeqAccessDeserializer::new(seq);
        let content = serde::__private::de::Content::deserialize(rest).unwrap();
        let deserializer = serde::__private::de::ContentDeserializer::<A::Error>::new(content);
        Enum::deserialize(deserializer)
    }
}

fn main() {
    let mut de = serde_json::Deserializer::from_str(r#" ["Variant", 1] "#);
    let e = de.deserialize_any(MyVisitor).unwrap();
    println!("{:?}", e);
}

I'm left unclear about what the motivation for this PR is.

(review this commit with "ignore whitespace changes" option on)
The new message is more logical, because actually map *is* expected,
but it should contain at least one entry
failures (6):
    access_to_enum::seq::empty_seq
    access_to_enum::seq::newtype
    access_to_enum::seq::struct_
    access_to_enum::seq::tuple
    access_to_enum::seq::unit
    access_to_enum::seq::wrong_tag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants