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

has_flatten rework #2795

Merged
merged 5 commits into from
Aug 12, 2024
Merged

has_flatten rework #2795

merged 5 commits into from
Aug 12, 2024

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Aug 11, 2024

This PR removes workaround from #2794 in the first commit.

The 2nd commit unifies behaviour for all cases where flatten fields processed in the deserializer. Consequence of that commit: FlattenSkipDeserializing[DenyUnknown]

  • does not collect data in Field, because do not read them anyway
  • gets deserialize_in_place method
  • gets ability to deserialize from sequence (visit_seq method)
  • uses deserialize_struct instead of deserialize_map

The 3rd commit gives understandable message when derive failed due to assertion.

The 4th commit removed dead code that here since 2018.

The last commit does the unification of has_flatten behaviour for the serializer side. Consequence of that commit: FlattenSkipSerializing

  • uses serialize_struct instead of serialize_map

…ization

Consequence: `FlattenSkipDeserializing[DenyUnknown]`
- does not collect data in Field, because do not read them anyway
- gets `deserialize_in_place` method
- gets ability to deserialize from sequence (visit_seq method)
- uses `deserialize_struct` instead of `deserialize_map`
…zation form

Consequence: `FlattenSkipSerializing`
- uses `serialize_struct` instead of `serialize_map`
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This PR makes derive(Deserialize) ignore a flatten attribute on fields that have skip_deserializing, and derive(Serialize) ignore a flatten attribute on fields that have skip_serializing.

I would have to think whether that is the behavior that should be intended. It means that in the case of FlattenSkipSerializing and FlattenSkipDeserializing (using names from the test suite), we serialize using serialize_struct and deserialize using deserialize_map, or serialize using serialize_map and deserialize using deserialize_struct.

I think some users would expect that if a type is serialized using serialize_struct, it should be deserialized using deserialize_struct, and if a type is serialized using serialize_map, it should be deserialized using deserialize_map. I am not sure how sensitive various data formats are to this.

@Mingun
Copy link
Contributor Author

Mingun commented Aug 12, 2024

Boundary between (de)serialize_struct and (de)serialize_map is already blurred when you use #[serde(flatten)]. Actually, that is source of pain because (de)serialize_map looses information which is important for some formats (see for example #1529). There is no good solution of this problem in current approach which pretends that simply using special (De)serializer will be enough to support flattening. The alternative design is outlined in my comment here.

I think, if you explicitly skip only (de)serialization method you understand consequences. I would say that the base expectation that if field is invisible for (de)serialization it does not affects the format.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay — makes sense.

Thank you!

@dtolnay dtolnay merged commit f986609 into serde-rs:master Aug 12, 2024
15 checks passed
@Mingun Mingun deleted the has-flatten-rework branch August 12, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants