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

Allowed Enum variants to be individually marked as untagged #2403

Merged
merged 8 commits into from Jun 8, 2023

Conversation

dewert99
Copy link
Contributor

This should hopefully resolve #1402.

This can be thought of as applying the following transformation

#[derive(Serialize, Deserialize)
enum Test {
  A(u32, u32),
  B(u32),
  #[serde(untagged)]
  C(u32),
  #[serde(untagged)]
  D(u32, u32),
}

to

#[derive(Serialize, Deserialize)
enum TestTagged {
  A(u32, u32),
  B(u32),
}

#[derive(Serialize, Deserialize)
#[serde(untagged)]
enum TestUntagged {
  Tagged(TestTagged),
  C(u32),
  D(u32, u32)
}

#[derive(Serialize, Deserialize)
#[serde(from = "TestUntagged")]
#[serde(into = "TestUntagged")]
enum Test {
  ...
}

The actual implementation generates the implementation directly allowing it to also work efficiently for recursive enums.

The current version includes a restriction that all variants marked as untagged appear at the end of the enum (after the tagged variants) in order to keep the implementation strategy consistent with an alternate strategy of attempting to deserialize each variant in order.

Deserialize could also be implemented to accept all of the variants with tags, only falling back to untagged if none of them match, I'm not sure if this would be preferred.

Copy link
Member

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Please also add some tests for non-recursive examples like in #1402 (comment) or generic examples like #1402 (comment)

serde_derive/src/de.rs Outdated Show resolved Hide resolved
serde_derive/src/de.rs Show resolved Hide resolved
serde_derive/src/de.rs Outdated Show resolved Hide resolved
test_suite/tests/test_annotations.rs Show resolved Hide resolved
@dewert99
Copy link
Contributor Author

Thanks for all the feedback. I've tried to implement the suggestions.

@dewert99 dewert99 requested a review from oli-obk March 21, 2023 17:56
Copy link
Member

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

one thing I just realized that we may want to reject is marking all enum variants as untagged, as then you could instead mark the entire enum as untagged.

lgtm otherwise. I'll ping a few folk from the issue so they can try out your PR and see if it works for their use cases.

serde_derive/src/de.rs Show resolved Hide resolved
@dewert99
Copy link
Contributor Author

I wasn't sure what the policy was on warning-like cases. Other cases to consider might be:

  • marking a variant as untagged when the enum itself is already untagged
  • marking a variant with rename/alias as well as untagged (I don't think this has a different effect than just using untagged)
  • marking a variant with rename/alias when the enum is untagged (This is currently allowed, so changing it would be a breaking change)

.collect()
}).inspect(|variant| {
if !variant.attrs.untagged() && seen_untagged {
cx.error_spanned_by(&variant.ident, "all variants with the #[serde(untagged)] attribute must be placed at the end of the enum")

Choose a reason for hiding this comment

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

Why only allowing #[serde(untagged)] to be at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This keeps the implementation more consistent with the deserialisation strategy of going though each variant in order and choosing the first variant that matches.

@gavrilikhin-d
Copy link

gavrilikhin-d commented May 20, 2023

@oli-obk Hi! This feature is really useful, but sadly there is no activity on PR more than a month. The PR itself looks quite finished except warnings. Can it be merged in this state?

@oli-obk
Copy link
Member

oli-obk commented May 22, 2023

Unfortunately no one on the issue has so far tested out this branch to see if it works for their use cases.

@gavrilikhin-d
Copy link

@oli-obk I tried to check it, but don't know how to replace serde dependency in serde_json

@oli-obk
Copy link
Member

oli-obk commented May 31, 2023

Someone was so kind and wrote instructions at #1402 (comment)

@gavrilikhin-d
Copy link

@oli-obk My case was covered too. #1402 (comment)

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!

@dtolnay dtolnay merged commit 48e5753 into serde-rs:master Jun 8, 2023
19 checks passed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 12, 2023
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.162` -> `1.0.164` |

---

### Release Notes

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

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

[Compare Source](serde-rs/serde@v1.0.163...v1.0.164)

-   Allowed enum variants to be individually marked as untagged ([#&#8203;2403](serde-rs/serde#2403), thanks [@&#8203;dewert99](https://github.com/dewert99))

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

[Compare Source](serde-rs/serde@v1.0.162...v1.0.163)

-   Eliminate build script from serde_derive crate to slightly reduce build time ([#&#8203;2442](serde-rs/serde#2442), thanks [@&#8203;taiki-e](https://github.com/taiki-e))

</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, check this box

---

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

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1896
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.

#[serde(flatten)] attribute support for enum variants
4 participants