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

Derive permits internally tagged enum with tuple variants, errors at runtime #1307

Open
abonander opened this issue Jun 6, 2018 · 8 comments
Labels

Comments

@abonander
Copy link

abonander commented Jun 6, 2018

The Enum Representations page specifies the following:

Using a #[serde(tag = "...")] attribute on an enum containing a tuple variant is an error at compile time.

I take that to mean that the following will produce a compile-time error, because there's not really a good way to format internally tagged enums with tuple variants:

#[derive(Serialize, Deserialize)]
#[serde(tag = "type")]
pub enum Foo {
    Bar(String),
    Baz(String),
}

However, it turns out that this only becomes an error at runtime, and only when serializing the specific variant: http://play.rust-lang.org/?gist=9f5656641805cd794d79ef715bcf850e&version=stable&mode=debug

Deserialization, on the other hand, just produces a hugely unhelpful error if you try something like { "type": "Foo", "__field0": "some string" }:

invalid type: map, expected a string

@abonander abonander changed the title Specify format of internally tagged enum with tuple variants Serde_derive permits internally tagged enum with tuple variants, errors at runtime Jun 6, 2018
@abonander abonander changed the title Serde_derive permits internally tagged enum with tuple variants, errors at runtime Derive permits internally tagged enum with tuple variants, errors at runtime Jun 6, 2018
@dtolnay
Copy link
Member

dtolnay commented Jun 6, 2018

In Serde we categorize Bar(String) as a newtype variant rather than a tuple variant. https://serde.rs/data-model.html touches on this. Internally tagged tuple variants indeed error at compile time, while internally tagged newtype variants are fine in general, as in:

#[derive(Serialize, Deserialize)]
#[serde(tag = "type")]
enum Foo {
    Bar(Inner),
}

#[derive(Serialize, Deserialize)]
struct Inner {
    q: String,
}

The names of local variables in serde_derive like __field0 have no bearing on serialized representation.

@dtolnay
Copy link
Member

dtolnay commented Jun 6, 2018

For a representation like { "type": "Bar", "__field0": "some string" } you could try an adjacently tagged enum:

#[derive(Serialize, Deserialize)]
#[serde(tag = "type", content = "__field0")]
enum Foo {
    Bar(String),
    Baz(String),
}

@abonander
Copy link
Author

abonander commented Jun 6, 2018

@dtolnay

while internally tagged newtype variants are fine in general, as in

It's still an error at runtime to try and serialize an internally tagged newtype variant that wraps a primitive datatype; shouldn't that be a compile-time error emitted by the derive instead? It would have saved me a lot of time and confusion.

@dtolnay
Copy link
Member

dtolnay commented Jun 6, 2018

I don't know of a way to make that a compile time error. Macro expansion of enum Foo occurs before type information is computed so a macro cannot distinguish Inner referring to struct Inner { q: String } (which is fine) vs referring to type Inner = String (which is not fine). Also there is no way for it to be a typecheck error because the Serde data model representation of Inner is only determined by executing its Serialize impl.

@abonander
Copy link
Author

abonander commented Jun 6, 2018

I guess that is a problem, though you could do it with custom OIBITs if those ever become stable:

pub trait InternallyTaggable {}
impl InternallyTaggable for ... {}

impl !InternallyTaggable for String {} // etc.

Do what you want with this issue then, but I would advise maybe duplicating the definitions of the different kinds of variants to the Enum Representations page and maybe make it clear that Serde-primitive datatypes can't be used with internally tagged newtype variants. The error when attempting to deserialize is also quite unhelpful but I'm not sure what can be done about that either.

@dtolnay
Copy link
Member

dtolnay commented Jun 7, 2018

Neat idea! Thanks. I filed #1308 to follow up on the custom OIBIT and we can track the documentation aspect in this issue.

@abonander
Copy link
Author

@dtolnay for the common case where the primitive datatype isn't aliased, can we have it error then? There could be false-positives if the user shadows the type name (type String = Foo;) but that would be rather uncommon I'd think; I'd trade a few rare false positives for good footgun mitigation.

@Pzixel
Copy link

Pzixel commented Mar 19, 2021

So what is the answer now? For example consider following enum

#[derive(Serialize)]
#[serde(tag = "mytag")]
enum MyEnum {
    NoMeaningfulNameForValue(i32),
    Some { other: Struct }
}

and i'd like it to be able to serialize into both{"mytag" : {"Some": {"other" : {...}}}} and {"mytag" : {"NoMeaningfulNameForValue": {"__field0" : 20}}}

Artificial adjacently tagged enum won't help because there are variants that should be placed in the root object.

Or at least make it compile time error

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

No branches or pull requests

3 participants