arcnmx / serde-ini

Windows INI file {de,}serialization
MIT License
19 stars 17 forks source link

auto convert problem with serde `flatten` #6

Open Larusso opened 5 years ago

Larusso commented 5 years ago

https://github.com/Larusso/serde-ini-fail-example/blob/master/src/lib.rs

This example is derived from some experimentation I do at the moment. It creates three different struct layouts and tries to deserialize the same test data into it.

If you run the tests you will see that the test data_3 (which deserilized into Data3 struct) fails because of the number deserialization issue.

For a better visibility here are the three structs:

#[derive(Deserialize, Debug)]
#[serde(rename_all = "PascalCase")]
pub struct Data {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    section_1: Option<Box<FieldData>>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    section_2: Option<Box<FieldData>>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    section_3: Option<Box<FieldData>>,
}

#[derive(Deserialize, Debug)]
#[serde(transparent)]
pub struct Data2(HashMap<Keys,FieldData>);

#[derive(Deserialize, Debug)]
#[serde(rename_all = "PascalCase")]
pub struct Data3 {
    section_1: FieldData,

    #[serde(flatten)]
    components: HashMap<Keys, FieldData>,
}

All three tests in this sample setup use the same struct: FieldData for the fields and the deserializer has no issues with it in the data_1 and data_2 tests.

I hope this helps you

Originally posted by @Larusso in https://github.com/arcnmx/serde-ini/issues/5#issuecomment-436680993

arcnmx commented 5 years ago

Okay so, the issue here is that the ini data is untyped, all values are strings. This deserializer tries to use FromStr when needed as a convenience, so that deserialize_with isn't necessary to use for anything that's not a string type. However, serde's flatten collects all extra fields until the end of deserializing (as typed strings), then uses its own Deserializer to convert that to the HashMap<Keys, FieldData> which does not use the same conversion magic - this results in the inconsistency.

Perhaps @dtolnay could chime in on how best to approach this. Removing the conversions and requiring deserialize_with="from_str" on everything would be more consistent and explicit. This seems like an annoying and unnecessary burden for the majority of use cases, but perhaps the conversions are too magic to be something a deserializer should be doing?

dtolnay commented 5 years ago

This is the same underlying limitation as https://github.com/serde-rs/serde/issues/1183 and https://github.com/serde-rs/json/issues/497. We will be able to handle this better in Serde once associated type defaults are available (https://github.com/rust-lang/rust/issues/29661).

For now I would recommend keeping the type conversions in serde_ini but using a #[serde(with = "...")] attribute on non-string fields that get flattened, in this case size. The implementation in serde_with::rust::display_fromstr should do the trick.