Repository navigation
deriving(Encodable) does not use emit_enum_struct_variant or emit_tuple_struct #19756
Description
This issue is closely related to #17158, but affects the proposed RustcEncodable trait which will be stabilized as part of the compiler for Rust 1.0.
Currently, the code which derives Encodable has a few weird design decisions:
1. Rust always uses emit_enum_variant instead of emit_enum_struct_variant.
If we have a structure ExEnum as follows:
#[deriving(Encodable)]
enum ExEnum { Foo, Bar(f64), Baz{x: f64, y: f64} }…then both Bar and Baz will emit code to call emit_enum_variant. For example, JSON encoding will yield:
"Foo"
{"variant":"Bar","fields":[1]}
{"variant":"Baz","fields":[1,2]}
This would probably look a lot nicer with x: and y: in the Baz case.
2. Rust will always use emit_struct instead of emit_tuple_struct
#[deriving(Encodable)]
struct ExTupleStruct(f64);This will encode to JSON as:
{"_field0":1}
This is particularly unfortunate—the resulting data would look nicer if it went through emit_tuple_struct and used some sort of array-based encoding.
Proposed fixes
I can see two ways forward:
- Option 1: Officially drop
emit_enum_struct_variantandemit_tuple_structand always useemit_enum_variantandemit_struct. - Option 2: Fix the
Encoder(or the newRustcEncoder) to useemit_enum_struct_variantandemit_tuple_struct.
The current behavior feels like an oversight, and (2) would allow for finer-grained control over the serialization and nicer JSON. If people agree that this should happen before 1.0, and that (2) is the better design, I'd be interested in tackling this as my first rustc contribution.
Activity
emk commentedon Dec 11, 2014
See also #15659, which touches on the
Decodable/RustcDecodablehalf of this.steveklabnik commentedon Jan 27, 2015
Encode was moved out of tree, so if this is still an issue, please pursue it over at https://github.com/rust-lang/rustc-serialize
emk commentedon Jan 27, 2015
steveklabnik: This was a still a problem the last time I looked, and it is in the compiler tree. Specifically, the
#[deriving(Encode, Decode)]stuff doesn't use any of therustc-encodingAPIs for struct variants or tuple structs.There's a strong case to be made that the in-tree stuff is severely suboptimal, and I really wanted to propose a fix before the alpha. But I've been hit with one crisis or another all month. I'm happy to explain this in more detail, but I'm not sure if I can spearhead a fix before it's too late. :-/
steveklabnik commentedon Jan 27, 2015
The in-tree one is private to
rustc, so you don't need to stress out over it ❤️emk commentedon Jan 27, 2015
Are you sure? As far as I can tell, the in-tree
derive(RustcEncodable,RustcDecodeable)stuff is used directly by therustc-serializepackage, and will continue to be used at least until compiler plugins are stabilized. (Sorry to have gotten some of the names wrong in my previous message.)In other words, I think that the in-tree plugins are used directly by the out-of-tree code, and the in-tree plugins have some moderately serious issues. This can be worked around in
rustc-serialize, but only by discarding all special-case support for struct variants and tuple structs.I don't necessarily think that we can or should fix the in-tree stuff at this point. But I do think that the in-tree stuff could have used one more pass of API fixes, assuming it really is used by
rustc-serialize, which I understand to be the case. But I'm happy to be corrected if my information re: rustc-serialize and how it interfaces to the compiler is wrong or out of date.Sorry for the terse and possibly confused messages; I'm juggling way too much stuff this week.
steveklabnik commentedon Jan 27, 2015
@alexcrichton , am I in the right or in the wrong here?
alexcrichton commentedon Jan 27, 2015
Yeah
RustcEncodableandEncodableuse the same infrastructure so the compiler is technically still at fault here. This should be a fine backwards-compatible change, however!emk commentedon Jan 28, 2015
OK, here are the other related issues on the decode side:
Decodable/RustcDecodableto callread_enum_struct_variantandread_enum_struct_variant_field. This may involve a non-backwards-compatible change toread_enum, which currently offers the caller no way to determine whether to callread_enum_variantorread_enum_struct_variant.Decodable/RustcDecodableto callread_tuple_struct.I'm not sure whether this is 100% backwards-compatible. I'm not seeing an obvious solution. I might be able to find some time next week to work on this, if people think that's desirable and appropriate. If we can't fix this in the compiler, my tentative recommendation is to remove all the struct variant and tuple struct from
rustc-serialization. This is suboptimal, especially for tuple structs (which get bogus field names under the current system), but it might be the best we can do.Anyway, I'm very open to suggestions, and I might be able to find a day to work on this if that helps.
alexcrichton commentedon Jan 28, 2015
We're still in a period where it's still mostly ok to make breaking changes, so I think it's fine to make these changes (even the breaking ones) for now. The changes would likely only break decoders of which there are relatively few in the wild :)
emk commentedon Jan 28, 2015
OK, sounds good. I'll try to somehow find the time if that's the best way forward. The one piece I would need from you is feedback on the intended relationship between
read_enum,read_enum_variantandread_enum_struct_variant. Specifically, who decides whether to callread_enum_variantorread_enum_struct_variant. That's the part that I can't figure out from the existing code—everything else is pretty much straightforward.Thank you very much for your help and your advice! And my apologies for not getting to this sooner.
alexcrichton commentedon Jan 28, 2015
Hm, it looks like
read_enumdelegates to the yielded closure to callread_enum_variantorread_enum_struct_variantitself. It looks like you can giveread_enum_varianta list of names for possible enum variants that can be read, and the closure will respond with which enum variant should be decoded. It looks like there's no way to say "read a variant" and the decoder responds with "ok, here's a regular variant" or "ok, here's a struct variant". Changing that would likely change the inputnames: &[&str]toread_enum_variantto something likenames: &[RegularVariantOrStructVariantName](or something like that).Also no worries! There's certainly no rush here :)
emk commentedon Jan 29, 2015
OK. In that case, does it even make sense to have a separate
read_enum_struct_variantcall? Or shouldread_enum_varianthandle all kinds of variants, and then allow the callbackfto decide whether to callread_enum_variant_argandread_enum_struct_variant_fieldas appropriate? (And if we go with this design, shouldread_enumandread_enum_variantjust be collapsed into one function call?)I'm quite happy to proceed with whatever design makes sense, but I've only worked on one idiosyncratic encoder/decoder pair, so I'm especially eager for feedback on the design.
emk commentedon Jan 29, 2015
Also, should we write up an RFC or something like that, and solicit input from people using serialization? Thank you for helping guide me through this process!
alexcrichton commentedon Jan 29, 2015
This seems minor enough that it probably doesn't warrant an RFC, but your proposed solution seems like a good idea to me!
gyscos commentedon Jan 10, 2016
Any news on that? Or are we just waiting for serde?
I understand being backward compatible may be more of a priority now, which could impede this proposal...
dtolnay commentedon Dec 3, 2017
RustcEncodable is deprecated. Serde handles these cases in a more consistent way.