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

Remove questionable type conversions #839

Merged
merged 3 commits into from
Apr 5, 2017
Merged

Remove questionable type conversions #839

merged 3 commits into from
Apr 5, 2017

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Apr 5, 2017

Fixes #836.

Type No longer deserialized from
() empty seq
bool "true" and "false" strings
number FromStr
string unit
various sequence types unit
[T; 0] unit
various map types unit

These conversions are bad because they make untagged enums confusing.

#[macro_use]
extern crate serde_derive;

extern crate serde;
extern crate serde_json;

#[derive(Deserialize, Debug)]
#[serde(untagged)]
enum E {
    B(bool),
    S(String),
}

fn main() {
    // This should be E::S but is actually E::B because of implicit conversion.
    println!("{:?}", serde_json::from_str::<E>("\"false\"").unwrap());
}

It looks like bool from string and number from string were added only to support XML in #70. @RReverser can you make serde-xml-rs work without those conversions? Basically deserialize_bool and deserialize_u64 etc need to not forward to deserialize_string anymore but have their own logic for handling the appropriate type.

The rest were added before we had deserialize_with. I think deserialize_with changes the game and we no longer want these implicit conversions.

@oli-obk
Copy link
Member

oli-obk commented Apr 5, 2017

Yea, serde XML could have done without these hacks, but they were so common already, I didn't stop to think if it makes sense

@RReverser
Copy link

@RReverser can you make serde-xml-rs work without those conversions? Basically deserialize_bool and deserialize_u64 etc need to not forward to deserialize_string anymore but have their own logic for handling the appropriate type.

Sure. Are all deserialize_(integer) forwarded to deserialize_u64 and same for float?

@dtolnay
Copy link
Member Author

dtolnay commented Apr 5, 2017

Yes all the number types are still interconverted so you only need to write logic for u64, i64, f64.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants