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

Untagged and internally tagged enums #739

Merged
merged 4 commits into from
Feb 3, 2017
Merged

Untagged and internally tagged enums #739

merged 4 commits into from
Feb 3, 2017

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Feb 3, 2017

Fixes #415 and fixes serde-rs/json#67.

The attribute for untagged enums is:

#[derive(Serialize, Deserialize)]
#[serde(untagged)]
enum E {
    /* variants */
}

During serialization, the variant is serialized as though it were not part of an enum. Struct variants are serialized as structs, tuple variants are serialized as tuples, unit variants are serialized as unit, and newtype variants are serialized as the value.

During deserialization, we buffer the content of the Deserializer and try it against each variant in order, returning the first one that deserializes successfully from that content.

The attribute for internally tagged enums is:

#[derive(Serialize, Deserialize)]
#[serde(tag = "type")]
enum E {
    /* variants */
}

The enum must contain only unit, newtype and struct variants. Tuple variants are not supported. During serialization unit variants are serialized as structs with a single field: {"type": "T"}, struct variants are serialized with "type": "T" inserted as the first field, and newtype variants must contain either a struct or a map and are serialized with "type": "T" inserted as the first field.

During deserialization, we buffer the content of the Deserializer, pull out the tag, and deserialize the rest of the content against the correct variant.

The use case from #415 looks like:

#[derive(Serialize, Deserialize, Debug)]
#[serde(tag = "type")]
enum Motion {
    Column { content: i32 },
    Line { content: i32 },
}

#[derive(Serialize, Deserialize, Debug)]
#[serde(tag = "type")]
enum Command {
    Move { content: Motion },
    Delete { content: Motion },
    Input { content: String },
}

And the use case from serde-rs/json#67 looks like:

#[derive(Serialize, Deserialize, Debug)]
#[serde(tag = "type")]
enum Message {
    #[serde(rename = "ok")]
    Ok { response: Response },
    #[serde(rename = "error")]
    Error { message: String },
}

cc @sfackler who asked about this in IRC and @alexcrichton who is removing this functionality from toml.

@dtolnay dtolnay requested a review from oli-obk February 3, 2017 03:01
@alexcrichton
Copy link
Contributor

Awesome, thanks @dtolnay!

@sfackler
Copy link
Contributor

sfackler commented Feb 3, 2017

Yay!

@sfackler
Copy link
Contributor

sfackler commented Feb 3, 2017

Out of curiosity, why is Content not public API?

@dtolnay
Copy link
Member Author

dtolnay commented Feb 3, 2017

I would prefer to be able to iterate on the implementation for a few releases and only have to think about backward compatibility for the attributes rather than the entire mechanism. This is quite a large PR and I think we can reach consensus on the attributes and the semantics significantly sooner than we could reach consensus on the entire Content API. The focus of this PR should be whether these two attributes solve the right problem in the right way, and not on designing a large API that we feel comfortable supporting for the rest of the year.

Once this merges I will file a ticket to follow up on polishing and exposing some of the pieces.

@sfackler
Copy link
Contributor

sfackler commented Feb 3, 2017

Cool, sounds good.

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.

cool! yes let's delay iterating on the exact structures later.

Implementation looks good to me.

@@ -0,0 +1,732 @@
use core::fmt;
Copy link
Member

Choose a reason for hiding this comment

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

hide the entire module with ![doc(hidden)] or at least write a doc comment stating that nothing in this module should be used outside of generated code. The PR comments contains all of that already, maybe just place them here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment in 066c9a1 and filed #741 to follow up on making it public.

impl<E> Deserialize for Content<E> {
fn deserialize<D: Deserializer>(deserializer: D) -> Result<Self, D::Error> {
// Untagged and internally tagged enums are only supported in
// self-describing formats.
Copy link
Member

Choose a reason for hiding this comment

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

We could find some way to support other formats through some rewinding API. This would also have uses for streaming.

Copy link
Member Author

Choose a reason for hiding this comment

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

I filed #742 to follow up on this.

@dtolnay dtolnay merged commit ce230ad into master Feb 3, 2017
@dtolnay dtolnay deleted the tag branch February 3, 2017 15:50
@dtolnay dtolnay mentioned this pull request Feb 9, 2017
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.

Is it possible to handle tagged enums? How to handle a tagged union?
4 participants