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

Nested untagged and externally tagged enums fail to deserialize #217

Closed
Tracked by #397
caelunshun opened this issue Apr 26, 2020 · 6 comments · Fixed by #476
Closed
Tracked by #397

Nested untagged and externally tagged enums fail to deserialize #217

caelunshun opened this issue Apr 26, 2020 · 6 comments · Fixed by #476
Labels

Comments

@caelunshun
Copy link
Contributor

The following example deserializes an untagged enum with two variants using ron, serde_json, and serde_yaml. The latter two libraries output the expected result, but RON fails with a "data did not match any variant of untagged enum Either" error.

use serde::Deserialize;

#[derive(Debug, Deserialize)]
struct Root {
    value: Either,
}

#[derive(Debug, Deserialize)]
#[serde(untagged)]
enum Either {
    One(One),
    Two(Two),
}

#[derive(Debug, Deserialize)]
enum One {
    OneA,
    OneB,
    OneC,
}

#[derive(Debug, Deserialize)]
enum Two {
    TwoA,
    TwoB,
    TwoC,
}

const RON_SRC: &str = "OneA";
const JSON_SRC: &str = "{ \"value\": \"OneA\" }";
const YAML_SRC: &str = "value: OneA";

fn main() {
    println!("{:?}", ron::de::from_str::<Either>(RON_SRC)); // Err(Message("data did not match any variant of untagged enum Either"))
    println!("{:?}", ron::de::from_str::<One>(RON_SRC)); // Ok(OneA)
    println!("{:?}", serde_json::from_str::<Root>(JSON_SRC)); // Ok(Root { value: One(OneA) })
    println!("{:?}", serde_yaml::from_str::<Root>(YAML_SRC)); // Ok(Root { value: One(OneA) })
}
@caelunshun
Copy link
Contributor Author

caelunshun commented Apr 27, 2020

I've tracked this down to relate to the implementation of handle_any_struct:

    /// Called from `deserialze_any` when a struct was detected. Decides if
    /// there is a unit, tuple or usual struct and deserializes it
    /// accordingly.
    ///
    /// This method assumes there is no identifier left.
    fn handle_any_struct<V>(&mut self, visitor: V) -> Result<V::Value>
    where
        V: Visitor<'de>,
    {
        // Create a working copy
        let mut bytes = self.bytes;

        match bytes.consume("(") {
            true => {
                bytes.skip_ws()?;

                match bytes.check_tuple_struct()? {
                    // first argument is technically incorrect, but ignored anyway
                    true => self.deserialize_tuple(0, visitor),
                    // first two arguments are technically incorrect, but ignored anyway
                    false => self.deserialize_struct("", &[], visitor),
                }
            }
            false => visitor.visit_unit(),
        }
    }

This function (through deserialize_any) is called by the generated Deserialize derive when the enum operates in untagged mode. It seems that on the input OneA, it visits a unit variant, which from what I understand is the empty tuple. However, this is actually a unit enum variant, so calling visit_unit is incorrect. Conversely, what serde_json does in this case is call visit_str, which I believe is the correct operation.

I'm having trouble correcting the implementation, however. I'm not able to determine whether the value is an enum variant or a unit struct without additional context, and my knowledge of serde is somewhat lacking in this regard. JSON handles this easily because enum variants are just strings encased in quotes, but for RON it's more difficult.

Edit: I believe it's not possible to correctly fix this bug with the current way serde works. Imagine the input OneA: should deserialize_any return a unit struct or an enum? There's no way to tell, since we don't have any other context. I hope someone else might have insights into a way to get around this, as it's a blocker for something we're doing.

@kvark
Copy link
Collaborator

kvark commented Apr 27, 2020

Thank you, this is great investigation! Please consider filing an issue on Serde for clarification or tracking of making this possible, if it's not filed yet.

@joonazan
Copy link

joonazan commented Jul 5, 2020

I found a possibly related weirdness:

The following code fails to deserialize the CountUnit case but if you replace Vec<UnitType> with Vec<u8> (and edit the string accordingly), it succeeds.

I was able to reproduce this on the newest master.

use serde::{Serialize, Deserialize};

#[derive(Debug, Serialize, Deserialize)]
pub enum UnitType {
    Explorer,
}

#[derive(Serialize, Deserialize, Debug)]
#[serde(untagged)]
enum CardTextNumberFlat {
    JustNum(u8),
    RangeW(Range_),
    CountUnitW(CountUnit_),
    PowerCardsPlayedW(PowerCardsPlayed),
}

#[derive(Serialize, Deserialize, Debug)]
struct Range_{ Range: u8 }
#[derive(Serialize, Deserialize, Debug)]
struct CountUnit_{ CountUnit: Vec<UnitType> }
#[derive(Serialize, Deserialize, Debug)]
struct PowerCardsPlayed;

fn main() {
    let x = ron::to_string(&CardTextNumberFlat::CountUnitW(CountUnit_{CountUnit: vec![UnitType::Explorer]}));
    let y = ron::to_string(&CardTextNumberFlat::RangeW(Range_{Range: 1}));
    println!("{} {}", x.unwrap(), y.unwrap());

    let x: CardTextNumberFlat = ron::from_str("Range(1)").unwrap();
    let y: CardTextNumberFlat = ron::from_str("CountUnit([Explorer])").unwrap();
    println!("{:?} {:?}", x, y);
}

@joonazan
Copy link

joonazan commented Jul 6, 2020

It seems that serde translates maps gotten from deserialize_any into almost anything... I was unable to find documentation on this.

The documentation on untagged enums says that serde tries each variant. That is not true. I went through it with gdb and found that it only deserializes once, which is why it needs to use deserialize_any.

I think that ron should return maps instead of structs, as there is no way to know if "A(B)" is a struct or an enum variant.

The below code shows how you can write maps instead of enums. {"CountUnit": ["Explorer"]} becomes Fancy(CountUnit([Explorer])). I don't think there is a way to reject these even though they are clearly not RON.

use serde::{Serialize, Deserialize};

#[derive(Debug, Serialize, Deserialize)]
pub enum UnitType {
    Explorer,
}
#[derive(Debug, Serialize, Deserialize)]
pub enum CardTextNumber {
    Constant(u8),
    Range(u8),
    CountUnit(Vec<UnitType>),
    PowerCardsPlayed,
}
use CardTextNumber::*;

#[derive(Serialize, Deserialize, Debug)]
#[serde(untagged)]
enum CardTextNumberFlat {
    JustNum(u8),
    Fancy(CardTextNumber),
}

fn main() {
    let x: CardTextNumberFlat = ron::from_str("{\"CountUnit\": [\"Explorer\"]}").unwrap();
    println!("{:?}", x);
}

@clouds56
Copy link
Contributor

Any update on this?

@juntyr
Copy link
Member

juntyr commented Aug 18, 2023

Thank you @caelunshun and @joonazan for providing very good test cases - I just stumbled across the issue and added a test and it seems like #451 fixed the issue.

It seems that serde translates maps gotten from deserialize_any into almost anything... I was unable to find documentation on this.

That is very true but, unfortunately, I don't think RON can do anything about this "serde-backdoor" for transforming a map into almost anything if it goes through deserialize_any. If you do find a way to do so, please feel free to submit a PR at any point :)

juntyr added a commit that referenced this issue Aug 18, 2023
Add a test to confirm that #217 is fixed
@juntyr juntyr removed the serde-bug label Aug 18, 2023
@NiklasEi NiklasEi mentioned this issue Sep 6, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants