Skip to content
This repository has been archived by the owner on May 27, 2022. It is now read-only.

[Feature Request] Supporting #[serde(flatten)] #8

Closed
Ploppz opened this issue May 5, 2020 · 7 comments
Closed

[Feature Request] Supporting #[serde(flatten)] #8

Ploppz opened this issue May 5, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@Ploppz
Copy link
Contributor

Ploppz commented May 5, 2020

🚀 Feature Request

I would like to be able to generate a registry for these structs

#[derive(Deserialize)]
struct Foo {
    #[serde(flatten)]
    bar: Bar,
    x: i32
}

#[derive(Deserialize)]
struct Bar {
    ty: Choice,
    result: f32,
}

#[derive(Deserialize)]
enum Choice { A, B, C }

But I get the error NotSupported("deserialize_identifier").

In serde-reflection/src/de.rs I see

    // Not needed since we always deserialize structs as sequences.
    fn deserialize_identifier<V>(self, _visitor: V) -> Result<V::Value>
    where
        V: Visitor<'de>,
    {
        Err(Error::NotSupported("deserialize_identifier"))
    }

I can try to fix this and implement support for #[serde(flatten)], but I wonder:
Can someone explain the comment? It seems like this function should be supported after all. What was the previous assumption and why is it violated with #[serde(flatten)]?

@Ploppz Ploppz added the enhancement New feature or request label May 5, 2020
@Ploppz
Copy link
Contributor Author

Ploppz commented May 5, 2020

Just to try to proceed in understanding what is happening, I let deserialize_identifier call deserialize_str, and it seems like the next problem is that deserialize_any is called which is also not supported. So it seems that #[serde(flatten)] requires the format to be self-describing.
I have tried my best to understand the serde model and how it is used in serde_reflection but still have some holes in my understanding.
Do you think it's possible to support #[serde(flatten)] given that it requires deserialize_any and deserialize_identifier?

@ma2bd
Copy link
Contributor

ma2bd commented May 6, 2020

Thanks for the interesting feature request.

I looked into the Serde-generated code for serialization (which is always easier than deserialization to get started). So, if we start with this

#[derive(Serialize)]
struct Foo {
    a: u64,
    #[serde(flatten)]
    b: Bar,
}

then Serde generates roughly this: (I minimized the code by hand for readability)

impl Serialize for Foo {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        let mut serde_state = Serializer::serialize_map(serializer, None)?;
        SerializeMap::serialize_entry(&mut serde_state, "a", &self.a)?;
        Serialize::serialize(
            &&self.b,
            private::ser::FlatMapSerializer(&mut serde_state),
        )?;
        SerializeMap::end(serde_state)
    }
}

Unfortunately, it looks like using #[serde(flatten)] on any field turns the entire struct into a Map where the keys are strings and the values are typed dynamically.

This is a bit unfortunate in this simple use case where (if I understand the request correctly) we just wanted to inline the second struct like this:

struct Foo {
    a: u64,
    c: u32,
    d: u16,
}

assuming for instance:

#[derive(Serialize)]
struct Bar {
    c: u32,
    d: u16,
}

For binary formats like bincode, it seems to me that this behavior of #[serde(flatten)] is not quite what we want here, to begin with.

Until we have a more targeted macro, in this case, I would consider writing Serialize and Deserialize by hand. Something like this:

use serde::{de::Deserializer, ser::SerializeStruct, ser::Serializer, Deserialize, Serialize};

struct Foo {
    a: u64,
    b: Bar,
}

struct Bar {
    c: u8,
    d: u16,
}

impl Serialize for Foo {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
     	let mut state = serializer.serialize_struct("Foo", 3)?;
	state.serialize_field("a", &self.a)?;
	state.serialize_field("c", &self.b.c)?;
	state.serialize_field("d", &self.b.d)?;
	state.end()
    }
}

#[derive(Deserialize)]
#[serde(rename = "Foo")]
struct FlatFoo {
    a: u64,
    c: u8,
    d: u16,
}

impl<'de> Deserialize<'de> for Foo {
    fn deserialize<D>(deserializer: D) -> Result<Foo, D::Error>
    where
        D: Deserializer<'de>,
    {
     	let value = FlatFoo::deserialize(deserializer)?;
	Ok(Foo {
            a: value.a,
            b: Bar {
		c: value.c,
		d: value.d,
            },
	})
    }
}

@ma2bd
Copy link
Contributor

ma2bd commented May 6, 2020

p.s. This being said, the comment "// Not needed since ..." is at best confusing and needs to be fixed.

@ma2bd
Copy link
Contributor

ma2bd commented May 6, 2020

See also the discussion in bincode-org/bincode#245

@Ploppz
Copy link
Contributor Author

Ploppz commented May 6, 2020

Thanks for the reply and research!

So if I understand correctly, flatten is implemented that way in serde by necessity.

Until we have a more targeted macro, ...

Do you mean that this crate will eventually rely on a derive macro like many other introspection crates? Does that makes the Deserializer approach obsolete?

@Ploppz
Copy link
Contributor Author

Ploppz commented May 6, 2020

Just an idea: What if this crate defines a trait that is blanket-implemented on all T: Deserialize. The trait exposes a function that can provide the same information that Tracer and Registry does now (maybe it takes &mut Tracer).

I read in the link you provided:

The reason serde cannot provide size information to bincode when flattening a struct is related to the implementation of the macro system in Rust. There is no way for the derive(Serialize) on Foo to know anything about Bar. This means the knowledge of how many fields are on Bar, which is needed to determine the overall size of Foo, is not available to the derive macro.

But we have this information don't we? And if we use a trait to expose this information for all T: Deserialize, then we should be able to solve the flatten problem. Does it make sense?

Edit: Just realized something: our Foo's impl Deserialize would still be as problematic as now... My idea wrongly assumed that we could somehow get fields of Foo unflattened and call a trait function recursively on all of them.

@ma2bd
Copy link
Contributor

ma2bd commented May 6, 2020

Until we have a more targeted macro, ...

Do you mean that this crate will eventually rely on a derive macro like many other introspection crates? Does that makes the Deserializer approach obsolete?

I actually meant: until a macro (say #[serde(inline)]) is available to (only) address the particular use case of inlining structs, one has to implement Serialise/Deserialize by hand as above to make it work with serde-reflection.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants