ron-rs / ron

Rusty Object Notation
https://docs.rs/ron
Apache License 2.0
3.36k stars 123 forks source link

Updating serde breaks Value::into_rust for adjacently tagged enums #508

Open nmsv opened 1 year ago

nmsv commented 1 year ago

The conversion of Value into a type fails in the following code snippet:

/*
[dependencies]
serde = { version = "=1.0.181", features = ["derive"] }
ron = "*"
*/

use serde::{Deserialize, Serialize};

#[derive(Debug, Serialize, Deserialize)]
#[serde(tag = "type", content = "value")]
enum TheEnum {
    Variant([f32; 3]),
}

fn main() {
    let ron_value: ron::Value = ron::from_str("(type: \"Variant\", value: (0.1, 0.1, 0.1),)").unwrap();
    println!("--- deserialized as ron::Value ---\n{ron_value:#?}\n");
    ron_value.into_rust::<TheEnum>().unwrap();
}

The code panics on any serde version higher than 1.0.180, while it's totally fine on 1.0.180 or lower versions:

 InvalidValueForType { expected: "variant of enum TheEnum", found: "the string \"Variant\"" }

Edit: I also checked serde_json with the same enum and it works with any serde version.

juntyr commented 1 year ago

Thank you for the report @nmsv! serde unfortunately changed how adjacently tagged enums work in a non-breaking version update, which ron can do very little about. Whereas before the correct ron would have been: "(type: \"Variant\", value: (0.1, 0.1, 0.1),)", now "(type: Variant, value: (0.1, 0.1, 0.1),)" roundtrips through typed deserialising into the enum (i.e. when you use ron::from_str::<TheEnum>(...)).

Going through ron::Value is a whole other story, unfortunately. ron::Value still does not support roundtrips of arbitrary ron values. The way serde worked before, adjacently tagged enums just happened to work with ron::Value as well, but it was more of a lucky accident. With serde's breaking change, this is no longer the case.

This bug will most likely fall under the v0.10 revamp of how ron::Value works, which would then also start making guarantees about what can roundtrip through it. As I am still finishing up v0.9, this might unfortunately take a while. For now, I will add your test case, so we remember that this is broken and know when it is fixed.

Funnily enough, "(\"Variant\",(0.1,0.1,0.1))" still works to go through ron::Value into TheEnum :)

I hope this helps a bit, I'm sorry that is not a bug that can be fixed immediately

nmsv commented 1 year ago

@juntyr Thanks for the answer and awesome job on ron. Keeping serde at the lower version is currently a totally viable solution for me, so no worries.

Regarding the Value issues: reliable partial deserialization is of my interest here. Would it be simpler for ron to implement something like serde_json::value::RawValue for that instead of the whole Value rework? I'm not sure that would fix other issues with the current Value implementation though.

juntyr commented 1 year ago

@nmsv On the main branch, and in the soon-coming v0.9 version, there is a ron::value::RawValue type, which captures some valid ron literally. Does this fit your needs? If not, what functionality would you need?

nmsv commented 1 year ago

@juntyr Capturing a fragment of ron for later deserialization is exactly what I need, thank you. Look forward to v0.9 release.

Should we close this or leave it until Value rework for anyone encountering the same problem after updating serde dependency?

juntyr commented 1 year ago

@nmsv I’m happy to hear that this is what you needed! We should probably leave this issue open for now to track its fix in some distant future