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.

Sign up for GitHub

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

Jump to bottom

Serialize and deserialize a tagged newtype variant over unit () as if it was a unit variant #2303

Merged
merged 3 commits into from Oct 21, 2022
Merged

Serialize and deserialize a tagged newtype variant over unit () as if it was a unit variant #2303

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90d28fc
Serialize and deserialize a tagged newtype variant over unit () as if…
tage64 Oct 20, 2022
a07d794
Update test_suite/tests/test_annotations.rs
tage64 Oct 21, 2022
e2ccfd9
Remove bad deserialization from sequence to internally tagged newtype…
tage64 Oct 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions serde/src/private/de.rs
View file
Open in desktop
Expand Up @@ -1262,6 +1262,17 @@ mod content {
{
match self.content {
Content::Unit => visitor.visit_unit(),

// As a special case, allow deserializing newtype variant containing unit. E.G:
// #[derive(Deserialize)]
// #[serde(tag = "result")]
// enum Response<T> {
// Success(T),
// }
//
// We want {"result": "Success"} to deserialize into `Response<T>`.
Content::Map(ref v) if v.is_empty() => visitor.visit_unit(),
Content::Seq(ref v) if v.is_empty() => visitor.visit_unit(),
Copy link
Member

Choose a reason for hiding this comment

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

I think I see the Content::Map case corresponds to the serialize_map that has been added to serde/src/private/ser.rs in this PR. But what is Content::Seq handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I first read your comment, I thought that The line with Content::Seq should not be there and I had made a mistake. I read the code for unit structs at the function below and somehow just thought that I should copy both lines, with Content::Map and Content::Seq. However, that would mean that my example enum above could be deserialized from ["success"], which may not be what we want. And I made a commit to fix this.

But after a bit more investigation, I see that the behaviour of unit structs seem to be just that. I.E. if we have a unit struct SomeStruct we could deserialize Response::Success(SomeStruct) from ["success"]. So for consistency this should perhaps also be allowed for units so that () and unit structs behave in the same way.

What do you think? Should we allow the sequence notation or not? If we decide to allow it, I will just revert my last commit.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should land it like this and find out whether anyone needs the Seq case in practice.

Thanks!

_ => Err(self.invalid_type(&visitor)),
}
}
Expand Down
6 changes: 3 additions & 3 deletions serde/src/private/ser.rs
View file
Open in desktop
Expand Up @@ -51,7 +51,6 @@ enum Unsupported {
String,
ByteArray,
Optional,
Unit,
#[cfg(any(feature = "std", feature = "alloc"))]
UnitStruct,
Sequence,
Expand All @@ -70,7 +69,6 @@ impl Display for Unsupported {
Unsupported::String => formatter.write_str("a string"),
Unsupported::ByteArray => formatter.write_str("a byte array"),
Unsupported::Optional => formatter.write_str("an optional"),
Unsupported::Unit => formatter.write_str("unit"),
#[cfg(any(feature = "std", feature = "alloc"))]
Unsupported::UnitStruct => formatter.write_str("unit struct"),
Unsupported::Sequence => formatter.write_str("a sequence"),
Expand Down Expand Up @@ -184,7 +182,9 @@ where
}

fn serialize_unit(self) -> Result<Self::Ok, Self::Error> {
Err(self.bad_type(Unsupported::Unit))
let mut map = try!(self.delegate.serialize_map(Some(1)));
try!(map.serialize_entry(self.tag, self.variant_name));
map.end()
}

fn serialize_unit_struct(self, _: &'static str) -> Result<Self::Ok, Self::Error> {
Expand Down
28 changes: 28 additions & 0 deletions test_suite/tests/test_annotations.rs
View file
Open in desktop
Expand Up @@ -2302,6 +2302,34 @@ fn test_internally_tagged_enum_containing_flatten() {
);
}

#[test]
fn test_internally_tagged_enum_new_type_with_unit() {
#[derive(Serialize, Deserialize, PartialEq, Debug)]
#[serde(tag = "t")]
enum Data {
A(()),
}

assert_tokens(
&Data::A(()),
&[
Token::Map { len: Some(1) },
Token::Str("t"),
Token::Str("A"),
Token::MapEnd,
],
);
assert_ser_tokens(
&Data::A(()),
&[
Token::Map { len: Some(1) },
Token::Str("t"),
Token::Str("A"),
Token::MapEnd,
],
);
tage64 marked this conversation as resolved.
Show resolved Hide resolved
}

#[test]
fn test_adjacently_tagged_enum_containing_flatten() {
#[derive(Serialize, Deserialize, PartialEq, Debug)]
Expand Down