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

UPDATED: Integer/boolean tags for internally/adjacently tagged enums #2525

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

Astavie
Copy link

@Astavie Astavie commented Jul 23, 2023

this is basically just #2056 but synced with the latest serde master
closes #745

for implementation details see #2056

Example

#[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,
}

@7sDream
Copy link

7sDream commented Jul 26, 2023

@dtolnay

Is there still any blocking point for this PR after update and pass CI?
Using integers as tags is the feature I am most looking forward to and find useful.

@Astavie
Copy link
Author

Astavie commented Aug 2, 2023

The new adjacently tagged enum code really put a wrench into this. It effectively now serializes tags into a unit enum like:

enum EnumTag {
    A,
    B,
    // ...
}

This way some (de)serializers can use the variant index instead of the name for smaller sizes. However there is no (de)serialize method that takes a variant index and a generic data type. The closest is serialize_newtype_variant but that wraps the newtype.

A new (de)serialize method could be created for this, something like serialize_unit_newtype_variant. However that would be a breaking change (unless given a default impl I suppose).

A default impl would simply redirect to the newtype's serialize impl I think.

@Astavie
Copy link
Author

Astavie commented Aug 2, 2023

@dtolnay is making a new base serialization method a good solution here? It could even expand enum tags to more than booleans and integers later on and add support for non-string tags for unit enums.

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.

I would not accept a new Serializer method.

I have not gotten a chance to look over the code in the PR, but in response to the last 2 comments: I don't immediately see why recent adjacently tagged enum changes (referring to #2505/#2496) would be an obstacle. Enums that use all integer tags need to serialize the tag using the appropriately sized integer type's Serialize impl, and deserialize using that same integer type. Similarly enums that use all boolean tags need to serialize using serialize_bool and deserialize using deserialize_bool. Enums that use a mix of integer and boolean and string tags, if this is supported, need to deserialize using deserialize_any. It's not clear to me that the solution would have been any different prior to #2505.

@Astavie
Copy link
Author

Astavie commented Aug 4, 2023

It not necessarily an obstacle, but it does mean I need to do a slight rework of the (de)serialization of tags. Your implementation suggestion actually makes a lot of sense, I'll work on that one.
Also yes, a mix is currently supported.

@Astavie
Copy link
Author

Astavie commented Aug 4, 2023

Alright, that should be up-to-date again now

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.

Thank you for getting this moving.

I have not looked at any deserialization code yet, but a few comments on the rest.

serde/src/private/de.rs Outdated Show resolved Hide resolved
serde_derive/src/internals/ast.rs Outdated Show resolved Hide resolved
serde_derive/src/internals/ast.rs Outdated Show resolved Hide resolved
serde_derive/src/internals/ast.rs Outdated Show resolved Hide resolved
serde/src/private/ser.rs Outdated Show resolved Hide resolved
serde_derive/src/internals/check.rs Outdated Show resolved Hide resolved
serde_derive/src/internals/check.rs Outdated Show resolved Hide resolved
serde_derive/src/ser.rs Show resolved Hide resolved
Comment on lines +4 to +6
5 | / #[serde(rename = 1)]
6 | | A,
| |_____^
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to highlight the rename = 1 attribute for this error.

          #[serde(rename = 1)]
                  ^^^^^^^^^^

Comment on lines +4 to +6
6 | / #[serde(rename = 1)]
7 | | A,
| |_____^
Copy link
Member

Choose a reason for hiding this comment

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

Same here, please put the span on rename = 1.

@Astavie
Copy link
Author

Astavie commented Sep 22, 2023

I've now done most of the simple changes. The rest of the requested changes will be completed on a later date

@xamgore
Copy link

xamgore commented Dec 25, 2023

@Astavie, that's an amazing piece of work, well done! May we help you somehow?

@Astavie
Copy link
Author

Astavie commented Dec 27, 2023

@xamgore
There still needs to be support for different type of integer tags (see #2525 (comment)). This requires a bit of a refactor on top of the refactor i've already done. If you're able to help with that it's much appreciated as I've had less time to devote to this.
For the rest, tidying up some error messages as discussed before.

@Astavie
Copy link
Author

Astavie commented Feb 8, 2024

Support for marking specific integer types has been added. Specifically: i8, i16, i32, i64, u8, u16, u32, u64

Rules for unmarked integers:

  • If all variants are unmarked integers, the smallest fitting integer type is chosen
  • If there exist variants with one integer type and the rest unmarked, that type will be chosen
  • If there is a mix of integer types or there are string variants or boolean variants, it will default to i64

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
5 participants