Structs with flattened fields are serialized and deserialized as maps

[torkleyy]

This causes problems for formats that use different syntax for structs and maps. Is there some way to fix it?

[dtolnay]

Rust structs with flattened fields are not "structs" by Serde's definition of "struct". A struct in Serde means that the fields are known at compile time. This is enforced for example by Deserializer::deserialize_struct which requires a &'static [&'static str] of the field names. But in a Rust struct with flattened fields, the Serialize impl of the flattened type can potentially insert any fields it wants at runtime.

RON could work around this by allowing deserializing Serde maps from RON structs, or I would be willing to consider a PR to support a compile-time declared set of what fields the flattened type is allowed to insert:

#[derive(Deserialize, Serialize)]
struct Main {
    #[serde(flatten(first, second))]
    required: Required,
    #[serde(flatten(third))]
    optional: Optional,
    some_other_field: u32,
}

[torkleyy]

Thanks for your response! The thing is, RON just ignores the fields list, so theoretically it could work without it. Also, I wonder, the workaround you're describing wouldn't work for the "everything else" flattening where you catch all the other fields, right? Or is there something else I could do?

…

On Sun, Aug 5, 2018, 18:37 David Tolnay ***@***.***> wrote: Rust structs with flattened fields are not "structs" by Serde's definition of "struct". A struct in Serde means that the fields are known at compile time. This is enforced for example by Deserializer::deserialize_struct <https://docs.serde.rs/serde/de/trait.Deserializer.html#tymethod.deserialize_struct> which requires a &'static [&'static str] of the field names. But in a Rust struct with flattened fields, the Serialize impl of the flattened type can potentially insert any fields it wants at runtime. RON could work around this by allowing deserializing Serde maps from RON structs, or I would be willing to consider a PR to support a compile-time declared set of what fields the flattened type is allowed to insert: #[derive(Deserialize, Serialize)]struct Main { #[serde(flatten(first, second))] required: Required, #[serde(flatten(third))] optional: Optional, some_other_field: u32, } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1346 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWFFuXOF24clSbEYXoMNqSEM9i-BJCahks5uNx81gaJpZM4VvV-D> .

[dtolnay]

Right, this would be for flattening types that have a compile-time knowable set of fields.

[RReverser]

Just ran into the same issue with serdebug.

@dtolnay I see there is already deserialize_identifier. Could we just add a counterpart for it in Serializer and for flattened structs continue to use serialize_map but use that new serialize_identifier for keys? I imagine that would be easier to use than enumerating flattened fields by hand.

[RReverser]

Ideally, I would also add Token::Ident to distinguish these from regular strings, but that would be a breaking change, so I guess not an option.

[Mingun]

Deserializing of structures as maps, gives one more inconvenience. For example, I have the self-described format (GFF) which does not support the bool type as data type. However in the implementation I allow to deserialize bool fields from int data type. At the same time format has several kinds of int data types with different sizes and in the existing files bool fields backended as numbers with different sizes. When using structure like

#[derive(Deserialize)]
struct Outer {
  b: bool,
  //...other fields
}

everything work, as expected. However if make minor change in structure:

#[derive(Deserialize)]
struct Outer {
  #[serde(flatten)]
  inner: Inner,
  //...other fields
}
#[derive(Deserialize)]
struct Inner {
  b: bool,
  //...other fields
}

suddenly would seem everything breaks. So occurs because the deserializer sees that in a data stream the int data type is located and will deserialize it as int field. And then, in attempt to write it in the field of the bool type there is an error.

Of course, such situation can be easily resolved: it is enough to deserialize not directly to final structure, but to special internal intermediate structure which exactly match format and then transform it to final structure. However it means to write boilerplate code, and all these serde attributes exist to avoid writing it.

[dtolnay]

Considering this issue again, I would prefer to simply leave this as a known issue affecting a subset of data formats. There are many different choices for how a flatten attribute could be implemented, the most powerful of which might be requiring some flatten-specific trait to be implemented for any type which you want to be able to flatten, but I believe the current implementation built around just the Serialize and Deserialize traits is making the correct tradeoffs.

[Mingun]

Whether it is possible to call hinted deserialising methods if serde(flatten) is applied only to structures (but not to HashMap, for example, when the count of fields can be unknown that how I understand, is the main motivation when deserialising such structures as map)? It can be implemented also as in this case we will make manually -- to implement deserialising for the generated intermediate structure

#[derive(Deserialize)]
struct SomeGeneratedStructureName {
  /// This field does not even require renaming in style
  /// ```
  /// #[serde(rename = "b")]
  /// some_generated_name: bool
  /// ```
  /// as it is already unique, otherwise deserialising would not work absolutely.
  /// And I think that `serde(flatten)` already prohibits identical names of fields
  b: bool,
  //...other fields from Outer and Inner
}

and its conversion to the Outer

[RReverser]

@dtolnay Sorry, I appreciate the desire to triage and close as many long-standing issues as possible, but I don't believe this one was addressed as some of the suggestions above, that would help implementers, remained unresponded, and there are still ways to fix this issue fully that weren't evaluated. This means someone inevitably will open similar issue again and it's usually better to have all the conversation in one place, where someone can find an existing issue and join discussion, rather than spread it to more separate repetitive discussions with likely same arguments.

[RReverser]

For now, it would be nice to have some answers to explicit proposals written down above rather than just close an issue without explanations.

[dtolnay]

@RReverser -- I acknowledge that suggestions have been made, but I would still make the choice of leaving this as a known issue rather than pursuing any of them. It is necessary to be strategic about which issues to address. The state of the world is that there are maybe 100 known ways that Serde could be extended to cater to small groups of users; each one involving minor API additions here or there. If we simply work through and do all 100 new features, the entire thing becomes a disaster suitable for nobody. As such, it is inevitable that suggestions which may seem reasonable and well justified in isolation are going to get dropped, and I don't consider an obligation to explain individually why we can't pursue each one.

[Mingun]

@dtolnay, but in any case if suddenly there is a PR implementing the changes offered by me, it will be considered or not?

[dtolnay]

None of the current suggestions would be accepted as a PR.

[Mingun]

But why? My suggestion has some problems? In my opinion not. Closing of an issue without explanations in my opinion nonconstructive approach -- to these you only push away potential collaborators. For me, for example, desire to help the project which so fastidiously refuses the help of those who are interested in it absolutely vanishes

[dtolnay]

@Mingun -- You are welcome to contribute on any of the remaining open issues! It is unfortunately not realistic to permit every contributor to add what they want into a mature project; I hope you understand.

• The method you described in Structs with flattened fields are serialized and deserialized as maps #1346 (comment) is not possible to implement under the current model for procedural macros in Rust, which are restricted to operating on individual types in isolation without knowing what fields exist on any other types until runtime.

• The behavior around bool that you described in Structs with flattened fields are serialized and deserialized as maps #1346 (comment) is not related to this issue but to Internal buffering disrupts format-specific deserialization features #1183, which remains open.

[Mingun]

@dtolnay, I thank for explanations -- now clearly that the problem depends on other problem in rust. If issue in the rust repository for that not yet exists, then it should be open that instead of close this issue -- because from closing of an issue the problem of developers did not go out! Besides, it turns out that another serde bug is related with it: the serde(flatten) attribute does not check that fields in structures will be unique and allows to generate incorrect result: playground. However, serde(rename) also suffers from the same problem: playground

[RReverser]

Ugh, today ran into this again with yet another target that has different representations for structs and maps, resulting in incompatibilities :(

@dtolnay If you feel strongly about not fixing this issue, could we maybe soft-deprecate serde(flatten), and move current implementation to a separate proc-macro crate as you suggested for few other issues?

That would hopefully bring more attention to differences for users, and would make this less of a footgun for Serializer/Deserializer authors like me.

Currently we can't neither provide correct implementation (because we don't know that user tries to (de)serialize a typed struct and not a map), nor throw an error (because this flatten interaction looks like map with no additional hints to Serializer).

[RReverser]

@dtolnay Also you said:

I would still make the choice of leaving this as a known issue

but this issue is not "known" as in "mentioned in the docs for serde(flatten)", making it really hard to debug if you don't already know what you're looking for...

[clouds56]

I think a important thing here is that we lost struct name when using serialize_map instead of serialize_struct. is it possible to add a struct-like map to serializer

trait Serializer {
  // ...
  fn serialize_struct_map(
    self,
    name: &'static str,
    len: usize
  ) -> Result<Self::SerializeMap, Self::Error> {
    self.serialize_map(Some(len))
  }
}
trait Deserializer {
  // ...
  fn deserialize_struct_map<V>(
    self,
    name: &'static str,
    fields: &'static [&'static str],
    visitor: V
  ) -> Result<V::Value, Self::Error> {
    self.deserialize_map()
  }
}

which takes parameters like a struct, while returns a value like map.
Ideally the serialize_struct_map should return Self::SerializeStructMap, but for backward compatibility, it just returns SerializeMap.

Now ron-rs/ron#115 is blocking on this. With this information, they could tell the different between "map because of flatten" and "map because of container", and land fix to handle with flatten