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

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

Conversation

tage64
Copy link
Contributor

@tage64 tage64 commented Oct 20, 2022

Let's say we have the following enum:

#[derive(Serialize, Deserialize, Debug)]
#[serde(tag = "result", rename_all = "snake_case")]
enum Response<T> {
    Success(T),
    Error {
        ...
    },
}

This PR makes it possible to serialize and deserialize Response::Success(()) into and from {"result": "success"}. Previously, this was not possible when the type parameter T was ().

Note that this is the same behaviour as if Response::Success would be a unit variant, but now it can be used in a more general context.

See This part of the docs for a background to internally tagged enums.

It's my first time contributing to Serde, so please appologise any mistakes regarding the contribution process.

References #2302

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!

test_suite/tests/test_annotations.rs Outdated Show resolved Hide resolved
Comment on lines 1274 to 1275
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!

@dtolnay dtolnay linked an issue Oct 20, 2022 that may be closed by this pull request
@dtolnay dtolnay merged commit 6aed101 into serde-rs:master Oct 21, 2022
crapStone added a commit to Calciumdibromid/CaBr2 that referenced this pull request Oct 23, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [serde](https://serde.rs) ([source](https://github.com/serde-rs/serde)) | dependencies | patch | `1.0.145` -> `1.0.147` |

---

### Release Notes

<details>
<summary>serde-rs/serde</summary>

### [`v1.0.147`](https://github.com/serde-rs/serde/releases/tag/v1.0.147)

[Compare Source](serde-rs/serde@v1.0.146...v1.0.147)

-   Add `serde::de::value::EnumAccessDeserializer` which transforms an `EnumAccess` into a `Deserializer` ([#&#8203;2305](serde-rs/serde#2305))

### [`v1.0.146`](https://github.com/serde-rs/serde/releases/tag/v1.0.146)

[Compare Source](serde-rs/serde@v1.0.145...v1.0.146)

-   Allow internally tagged newtype variant to contain unit ([#&#8203;2303](serde-rs/serde#2303), thanks [@&#8203;tage64](https://github.com/tage64))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yNDEuNSIsInVwZGF0ZWRJblZlciI6IjMyLjI0MS41In0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Co-authored-by: crapStone <crapstone01@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1597
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
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.

Serialize/deserialize tagged newtype variant containing unit
2 participants