arcnmx / serde-ini

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

Numbers and &str deserialization fails #5

Open spease opened 6 years ago

spease commented 6 years ago

I was trying to deserialize a u64 and noticed it was failing, even though strings and other objects were working fine. There didn't seem to be any problem with the text being deserialized:

id=61

So I tried making a workaround to investigate:

fn u64_workaround<'de, D>(deserializer: D) -> std::result::Result<u64, D::Error>
where
    D: serde::de::Deserializer<'de>,
{
    use serde::Deserialize;
    <&str>::deserialize(deserializer)?.parse().map_err(serde::de::Error::custom)
}

However this failed, so I eventually tried using a full String

fn u64_workaround<'de, D>(deserializer: D) -> std::result::Result<u64, D::Error>
where
    D: serde::de::Deserializer<'de>,
{
    use serde::Deserialize;
    String::deserialize(deserializer)?.parse().map_err(serde::de::Error::custom)
}

This works, but obviously involves a heap allocation that would seem to be unnecessary.

Regardless, without this workaround, the typical behavior is that integers and real numbers of all types seem to fail.

arcnmx commented 6 years ago

Could you provide an isolated test case I could compile/run that fails? Note that deserializing to &str is expected to fail because the current implementation does not support zero-copy deserialization/borrows, but workarounds to deserialize into numeric types like that should not be necessary.

See also the smoke test for an example usage that as far as I know does deserialize and serialize a u32 properly, that might give us something to compare against.

spease commented 6 years ago

I don't have it easily accessible right now, but IIRC I was triyng to parse the output of wpa_supplicant's status command (sudo wpa_cli status on Linux if you're using wifi)

arcnmx commented 6 years ago

By "it failed" are you getting a particular error? Not sure what's going on, what you have there should be equivalent to what's done internally... I'm not able to run that command, could you attach some example output?

Larusso commented 5 years ago

I ran into the same issue and can provide a simple test setup.

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

arcnmx commented 5 years ago

Hm, that looks like an issue with the new flatten attribute because it's treated by serde as if the struct has a map subfield, and subsections aren't supported by the deserializer.

Given that flatten support in serde looks newer than this issue, it's maybe not the same issue? Or perhaps they were trying to use subsections somehow.

Larusso commented 5 years ago

Hmm maybe. I am still somewhat new to Rust and the cargo ecosystem and have no idea what feature is new and which crate is older etc :) Shall I open a new issue with some more version information etc?

arcnmx commented 5 years ago

Hm yeah, no need to add more information but you can just use the "Open New Issue" button next to your comment. There's an issue with how flatten works that doesn't allow the automatic conversions to work.