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

Add minimal support for internally tagged and untagged enums #451

Merged
merged 4 commits into from
Jul 17, 2023

Conversation

juntyr
Copy link
Member

@juntyr juntyr commented Apr 19, 2023

Quite hacky but minimally invasive implementation to support internally tagged and untagged enums.

This PR switches the self-describing enum deserialisation behaviour to match serde's internal JSONy Content type format if we think we are deserialising into serde's internal type. Specifically, it uses std::any::type_name to detect when serde::__private::de::content::Content is deserialised to. This is relying on undocumented behaviour in all the horrible ways ... but that's what we need anyways need to do if we want to support internally tagged and untagged enums in serde. In some distant future, serde-rs/serde#2420 might make this less horrible.

Unlike #409, this PR is very targeted as it does not change the deserialisation behaviour for any other types. This allows us to stick with the existing self-describing enum behaviour and gives us space to evolve ron::Value separately.

Closes #449

  • I've included my change in CHANGELOG.md

@juntyr juntyr self-assigned this Apr 19, 2023
@juntyr
Copy link
Member Author

juntyr commented Apr 19, 2023

@torkleyy I really don't like that serde forces us to do this but the issue comes up again and again and a slightly hacky solution that relies on observable serde behaviour that has not changed since 1.0.0 (https://github.com/serde-rs/serde/blame/0c6a2bbf794abe966a4763f5b7ff23acb535eb7f/serde/src/private/de.rs#L220) and does not change RON's behaviour in any other way might be the best we can get for the next few years. I'd thus suggest merging this (with a heavy heart).

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2023

Codecov Report

Patch coverage: 97.89% and project coverage change: -0.53 ⚠️

Comparison is base (94a5ce9) 85.24% compared to head (9f5f3b4) 84.72%.

❗ Current head 9f5f3b4 differs from pull request most recent head 479d672. Consider uploading reports for the commit 479d672 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
- Coverage   85.24%   84.72%   -0.53%     
==========================================
  Files          62       63       +1     
  Lines        7830     8227     +397     
==========================================
+ Hits         6675     6970     +295     
- Misses       1155     1257     +102     
Impacted Files Coverage Δ
tests/307_stack_overflow.rs 97.91% <ø> (ø)
tests/value.rs 95.30% <91.66%> (-3.96%) ⬇️
src/de/mod.rs 75.72% <92.30%> (+1.35%) ⬆️
src/parse.rs 82.09% <98.27%> (-11.25%) ⬇️
tests/117_untagged_tuple_variant.rs 97.36% <100.00%> (ø)
tests/449_tagged_enum.rs 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@juntyr
Copy link
Member Author

juntyr commented Apr 19, 2023

Dang, now I remember why #409 got so large ... Unfortunately supporting untagged and internally tagged enums also means needing to support self-describing newtype variants for these cases ... I also cleaned up our tuple struct detection along the way but it still becomes a bit like spaghetti code since something newtype-looking cannot just always be unwrapped since our ron::Value still depends on these structs to become singleton tuples ... the code may become slightly cleaner if I also introduce better support for Value so that it just supports newtypes itself, but for now it's the best I can do

Copy link
Contributor

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

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

This seems a like a decent approach given our options 👍
I think we'll need some documentation. Unfortunate that these hacks are needed...

tests/117_untagged_tuple_variant.rs Show resolved Hide resolved
@juntyr
Copy link
Member Author

juntyr commented Apr 23, 2023

TODO @juntyr: check if #357 would be solved by this

@juntyr juntyr mentioned this pull request May 1, 2023
1 task
@juntyr juntyr force-pushed the minimal-internally-un-tagged-enums branch from c9ca82a to 9f5f3b4 Compare July 17, 2023 12:15
@juntyr juntyr merged commit b3128dd into ron-rs:master Jul 17, 2023
7 checks passed
@juntyr juntyr deleted the minimal-internally-un-tagged-enums branch July 17, 2023 12:23
juntyr added a commit to juntyr/ron that referenced this pull request Aug 16, 2023
)

* Add minimal support for internally tagged and untagged enums

* Edge cases over edge cases

* Add CHANGELOG entry and update README

* Fix the PR references in the README
@juntyr
Copy link
Member Author

juntyr commented Aug 18, 2023

TODO: check if #357 would be solved by this

#477 shows that #357 was indeed fixed by this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C-like enum nested inside of an internally tagged enum fails to deserialize
3 participants