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

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

Open
nmsv opened this issue Sep 18, 2023 · 5 comments
Open

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

nmsv opened this issue Sep 18, 2023 · 5 comments

Comments

@nmsv
Copy link

nmsv commented Sep 18, 2023

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
Copy link
Member

juntyr commented Sep 19, 2023

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

juntyr added a commit to juntyr/ron that referenced this issue Sep 19, 2023
juntyr added a commit that referenced this issue Sep 19, 2023
* Add a test to track known bugs for #508

* Document ron::Value restriction
@nmsv
Copy link
Author

nmsv commented Sep 27, 2023

@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
Copy link
Member

juntyr commented Sep 27, 2023

@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
Copy link
Author

nmsv commented Sep 28, 2023

@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
Copy link
Member

juntyr commented Sep 28, 2023

@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

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

No branches or pull requests

2 participants