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

Integer/boolean tags for internally/adjacently tagged enums #2056

Conversation

AmaranthineCodices
Copy link

@AmaranthineCodices AmaranthineCodices commented Jul 11, 2021

Closes #745.

Implements integer/boolean renames / aliases for internally and adjacently tagged enum variants:

#[derive(Serialize, Deserialize, Debug)]
#[serde(tag = "op")]
enum Something {
    #[serde(rename = 1)]
    A { a: i32 },
    #[serde(rename = 2)]
    B(u64),
    #[serde(rename = true)]
    C,
}

#[derive(Serialize, Deserialize, Debug)]
#[serde(tag = "op", content = "d")]
enum Adjacent {
    #[serde(rename = 1)]
    A { a: i32 },
    #[serde(rename = 2)]
    B(u64),
    #[serde(rename = true)]
    C,
}

UI

In attr.rs, I introduced a new Name variant, VariantName. To cut down on code duplication, I made Name generic over its internal name representation; Name is now Names<String>. I added a step to check.rs to ensure that non-string renames are only used on internally/adjacently tagged enums. It's not necessary to check this for structs or the container name; the types of those names are still String.

Deserialization

Deserialization was relatively straightforward to implement: rather than unconditionally generating constructors/fields for strings/bytes, we only do this if there are string-named variants. We do the same for ints/bools. One concern is that VARIANTS is still &'static [&'static str], but I think this is pretty low-impact, as it's used only for stringifying error messages. Changing it would be a breaking change as it would be visible in the signatures of Error::unknown_variant and Error::unknown_field.

Serialization

For serialization, I had to add a VariantName enum to serde itself, to be used in places where an integer/boolean can appear where previously only a String existed. We generate code in serde_derive for this enum wherever we invoke these methods.

The one part of this change that I'm not happy about is here:
https://github.com/AmaranthineCodices/serde/blob/37b4c68c0a9aa7aba8c6ee148c9c5e64c6f3f28c/serde_derive/src/ser.rs#L700
I am not fond of this to_string call, but getting rid of it would be a breaking change as far as I can tell - the change would be visible in the public-facing signatures of Serializer::serialize_struct and Serializer::serialize_struct_variant. This would be a good change to include in a new major serde version, though.

TODO

  • Make sure we handle collect_other_fields in deserialization
  • Revisit deserialize_generated_identifier and see if I can remove the code duplication between it and deserialize_identifier
  • Add unit test for integer/boolean aliases
  • Revisit main_constructors in deserialize_identifier

serde/src/private/ser.rs Outdated Show resolved Hide resolved
@AmaranthineCodices AmaranthineCodices marked this pull request as ready for review July 15, 2021 01:14
@AmaranthineCodices
Copy link
Author

@dtolnay I think this is ready for review now.

@s1341
Copy link

s1341 commented Aug 26, 2021

Is there an example of a workaround until this lands?

I need to deserialize something like:

enum Field {
    Null,
    Integer(u64),
    Float(f64),
    String((u32, u32)),
    Blob((u32, u32)),
    Object(MyObject),
}

Where the discriminators are 0, 1, 2, 5, 7, and 12, respectively. I cannot change the data format.

I believe that I need to use a custom Visitor, but cannot find a concise example of how to do so for this kind of enum. Can you point me in the right direction please?

@ByteAlex
Copy link

ByteAlex commented Sep 5, 2021

Hey!

Any news on this?
Can one somehow hack this serde repo in as dependency?
When I tried to use it from cargo via "git = ..., branch = ...", the Deserializers of the standard serde were not recognized.

I'd really need that for a project I'm working on.

@itohatweb

This comment was marked as spam.

@ByteAlex
Copy link

I think @AmaranthineCodices needs to fix the CI / Test suite before @dtolnay is going to look at this.

@MForster
Copy link

MForster commented Feb 1, 2022

I have rebased this PR to HEAD. With the exception of a few minor clippy issues it passes the CI, see https://github.com/MForster/serde/pull/1.

@dtolnay, can you take look at this PR? If it is basically good to go and just needs a few minor fixes, I can take care of that.

@Tamschi
Copy link

Tamschi commented Feb 26, 2022

This being integrated would be helpful for me too; It's needed to describe the scripting commands in RPG Maker MV projects, which are saved in JSON with the following shape:

{
    "code": <integer>, // tag
    "indent": <integer>, // shared field, in an outer struct that the enum is flattened into
    "parameters": [<values>] // types vary by variant
}

Currently, the most effective way to proceed there for me is to deserialise the partial object of non-shared fields into a dynamically typed Value provided by serde_json and then unpack it further from there, matching on the "code" more or less manually, but that's far from ideal.

This particular enum likely has upwards of 100 variants, so being able to generate an efficient implementation there would be very welcome. For now I'll see how well I can bodge it with an unhygienic utility macro.


Update: I moved to a lazy typing system that doesn't use Serde (weird use case), so I currently don't need this anymore.

@raiguard

This comment was marked as off-topic.

@raiguard

This comment was marked as off-topic.

@Milo123459

This comment was marked as spam.

@GnomedDev
Copy link

This would be amazing for Discord's GatewayEvent enum, specifically replacing the custom deserialization of https://github.com/serenity-rs/serenity/blob/f97322475eadf2dc78ea3e6d31b55811b3eb50ba/src/model/event.rs#L624-L711

@ByteAlex

This comment was marked as spam.

@7sDream
Copy link

7sDream commented Jun 12, 2022

This is an awesome feature that will be helpful to many of my projects.
I'm really looking forward to it being merged one day.

@zitsen

This comment was marked as spam.

@ImUrX
Copy link

ImUrX commented Oct 22, 2022

whats missing for this to merge? 1 year 😭

@ByteAlex
Copy link

@dtolnay Can we please get an information what is with this PR? There's lots of people waiting for this particular feature and the workaround to compile AmranthineCodices branch manually is getting really annoying, because we have to compile everything which uses serde with our custom branch as dependency.

LucasOe added a commit to LucasOe/tts-script-tool that referenced this pull request Jan 7, 2023
This doesn't work because this PR isn't merged: serde-rs/serde#2056
LucasOe added a commit to LucasOe/tts-script-tool that referenced this pull request Jan 8, 2023
This doesn't work because this PR isn't merged: serde-rs/serde#2056
@bobozaur
Copy link

Any updates regarding this? This seems to be the last piece preventing some of the private constructs of serde to be made public.

Really looking forward to reusing some of the components for custom deserializations.

@QuantumExplorer

This comment was marked as spam.

@ysndr
Copy link

ysndr commented Apr 4, 2023

In the meantime I found #745 (comment) to be a reasonable workaround

@ByteAlex
Copy link

@ maintainers
Could someone please react in some way to this PR?
I've been using a completely messed up serde setup, which compiles from this branch for almost 2 years now.

It'd be great to get this into a release...

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.

Most of the comments in #2525 (review) apply to this PR.

@AmaranthineCodices
Copy link
Author

I am going to close this, as I don't have the personal bandwidth to push this through. Sorry for the delay, but I've lost all context for this and I can't drive it right now.

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.

Allow integer tags for internally tagged enums