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

Cannot deserialize with Struct without fields #461

Open
zhongzc opened this issue Sep 27, 2023 · 8 comments
Open

Cannot deserialize with Struct without fields #461

zhongzc opened this issue Sep 27, 2023 · 8 comments

Comments

@zhongzc
Copy link

zhongzc commented Sep 27, 2023

#[derive(Debug, Serialize, Deserialize)]
struct A {
    b: Option<B>,
}
#[derive(Debug, Serialize, Deserialize)]
struct B {}

let a = A { b: Some(B {}) };
let de: A = Config::try_from(&a).unwrap().try_deserialize().unwrap();
assert!(de.b.is_some()); // Failed
@matthiasbeyer
Copy link
Member

How would you specify that in a configuration file?

@zhongzc
Copy link
Author

zhongzc commented Sep 27, 2023

How would you specify that in a configuration file?

#[derive(Debug, Serialize, Deserialize, PartialEq)]
struct A {
    b: Option<B>,
}
#[derive(Debug, Serialize, Deserialize, PartialEq)]
struct B {}

impl Default for A {
    fn default() -> Self {
        Self { b: Some(B {}) }
    }
}

let a = A::default();
let toml_str = toml::to_string(&a).unwrap();
println!("toml str: {}", toml_str);

// let de_from_toml = toml::from_str::<A>(&toml_str).unwrap();
// assert_eq!(a, de_from_toml); // Passed

let de_from_toml: A = Config::builder()
    .add_source(File::from_str(&toml_str, FileFormat::Toml))
    .build()
    .unwrap()
    .try_deserialize()
    .unwrap();
assert_eq!(a, de_from_toml); // Passed

let de_from_default_object: A = Config::builder()
    .add_source(Config::try_from(&a).unwrap())
    .build()
    .unwrap()
    .try_deserialize()
    .unwrap();
assert_eq!(a, de_from_default_object); // Failed

Output:

toml str: [b]

assertion failed: `(left == right)`
  left: `A { b: Some(B) }`,
 right: `A { b: None }`

@matthiasbeyer
Copy link
Member

And again I ask: How would you specify a instance of A { b: Some(B) } in a configuration file?

@zhongzc
Copy link
Author

zhongzc commented Sep 28, 2023

Just like the previous example, for toml file, I specify it like this:

[b]

@matthiasbeyer
Copy link
Member

Ah, I missed that bit. Hnm, indeed that's an issue. Thanks for reporting!

matthiasbeyer added a commit to matthiasbeyer/config-rs that referenced this issue Sep 28, 2023
Adds a test for an empty inner object as reported in rust-cli#461.

Reported-by: Zhenchi <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]>
@matthiasbeyer
Copy link
Member

I opened #463 with a testcase for this issue. If you have an idea solving that problem, you're of course very much welcome!

@zhongzc
Copy link
Author

zhongzc commented Sep 28, 2023

The scenarios that cause B to be lost are related to iterators. An example that better reflects the situation I found:

#[derive(Debug, Serialize, Deserialize, PartialEq)]
struct A {
    b_option: Option<B>,
    b_vec: Vec<B>,
    b_set: HashSet<B>,
    b_map: HashMap<String, B>,
}
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Hash)]
struct B {}

let a = A {
    b_option: Some(B {}),
    b_vec: vec![B {}],
    b_set: HashSet::from([B {}]),
    b_map: HashMap::from([("".to_owned(), B {})]),
};

// Serialize that `a` with `ConfigSerializer` will produce an empty config
let config = Config::try_from(&a).unwrap();

@polarathene
Copy link
Collaborator

polarathene commented Oct 20, 2023

TL;DR: Looks like this can be resolved easily enough with the below bolded fixes.

NOTE: Depending on when this is acted on, the referenced snippets below may change due to #465 (suggested fixes remain compatible, other referenced snippets may be refactored)


Note that there is two ways to define a unit-like struct (technically more?):

// Officially documented way:
// https://doc.rust-lang.org/book/ch05-01-defining-structs.html#unit-like-structs-without-any-fields
// https://learning-rust.github.io/docs/structs/
struct B;

// RFC:
// https://rust-lang.github.io/rfcs/0218-empty-struct-with-braces.html
// Previously not supported: https://github.com/rust-lang/rfcs/pull/147#issuecomment-47589168
// Accepted 2015: https://github.com/rust-lang/rfcs/pull/218#issuecomment-91558974
// Stabilized 2016: https://github.com/rust-lang/rust/issues/29720#issuecomment-189523437
struct B {}

These are roughly the same but have some differences.

If you omit the Option<>, and always have the unit struct:

  • struct B {} will fail with try_deserialize() due to error: value: missing field 'b'. Does not happen for struct B; (successful with struct).
  • toml::to_string(a) will fail for struct B; with: Error { inner: UnsupportedType(Some("B")) }. This also happens with the same error when wrapped in an Option<>.

struct B; would hit this route:

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/ser.rs#L165-L182

Even though it does reach serialize_some() you can see that it takes the same path as serialize_none(), as value.serialize(self) will resolve to the serialize_unit_struct() method, which like serialize_none() redirects to serialize_unit() which normalizes to the Nil type, aka None.

Fix: Instead of self.serialize_unit(), a unit struct is more likely to map to a Table/Map like other structs. So an empty struct seems more appropriate?:

// Call this instead:
self.serialize_primitive(Value::from(crate::value::Table::new()))

EDIT: This fails to deserialize the unit struct properly with error:

It requires adding an additional deserialize method for Value in de.rs instead of relying on unit_struct in serde::forward_to_deserialize_any!:

#[inline]
fn deserialize_unit_struct<V: de::Visitor<'de>>(self, _name: &'static str, visitor: V) -> Result<V::Value> {
    visitor.visit_unit()
}

struct B {} instead hits the same serialize_some() route to start, but value.serialize(self) recognizes it as a struct thus calls serialize_struct():

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/ser.rs#L243-L249

  • The _name is the struct B, and len is 0.
  • The returned result for both may look like different types but they are type aliases to Self?:

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/ser.rs#L85-L94

This is also None presumably because there is no logic involved that serializes to the config-rs internal Value type (which serialize_unit() did for struct B; earlier by calling serialize_primitive()):

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/ser.rs#L15-L22

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/ser.rs#L31-L32

Fix: A similar fix here is to also create a table, presumably only when len is 0. This should not replace the existing call, due to conflicting return type. Modify serialize_struct():

// Struct is empty (unit struct with empty braces):
if len == 0 {
    self.serialize_primitive(Value::from(crate::value::Table::new()))?;
}
// Keep the original call:
self.serialize_map(Some(len))

In both cases, I assume the expected serialization is what I've observed the deserialized format to Value is, an empty variant of Table (Map<String, Value>).

Applying either fix for that as suggested above now serializes the unit struct type option successfully, which will properly deserialize the option (since ValueKind::Nil would resolve to None but any other ValueKind variant like Table resolves to Some):

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/de.rs#L132-L142


Deserialization isn't an issue without the Option wrapping type, since Nil seems to resolve implicitly via the unit_struct -> unit derived methods generated here (deserialize_unit_struct() that calls self.deserialize_unit(visitor) that calls visitor.visit_unit()):

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/de.rs#L167-L171

The value prior to calling visitor.visit_unit() is Value of ValueKind::Nil:

Value {
    origin: None,
    kind: Nil,
}

I assume directly after this call it's converted to the unit struct, but I'm not sure how that works 😅

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

No branches or pull requests

3 participants