-
Notifications
You must be signed in to change notification settings - Fork 13.3k
auto: Add json enum encoding #4840
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
Conversation
Sure, it should emit, e.g., ["None"] for None. I can add a test case for this, if you like. |
Right, but I mean, the code I left the comment on looks like it'll emit |
Ah! you left the comment on a line that does indeed contain a bug, but a later commit in this pull request (978d33b3decf0620b50) fixes that bug. Perhaps in the future I should flatten my commits before putting them into a pull request? |
I would be cautious landing this. I serialized the Option type like this on purpose so that we can use the serialization tools with APIs that take optional values. Unfortunately I couldn't come up with a clever way to support nullable values and enums at the same time. It may be possible to do something special for just Option types, and fall through to this string based techniques for other types, but I wasn't really happy with that as it could lead to unexpected behavior. @nikomatsakis, any chance you've given any thought to this problem? |
@erickt, I think I see two different use cases for JSON serialization--and I can see how they'd lead to different solutions. Use Case 1: Twitter API. In a situation like this, the goal is to make it easy for a programmer to define a set of rust values that auto-encode into a given JSON format, where "null" has an existing and externally-defined meaning. Use Case 2: Rust object serialization. In a situation like this, the goal is to serialize Rust objects in an information-preserving way, say for instance to allow the comparison of ASTs while developing a regression test suite for parsing tools (just for instance :) .) In this use case, the goal isn't to match an existing external API, but instead to preserve the information necessary to compare Rust values. I see the existing implementation (only Option enums are supported) as supporting use case 1. I see my proposed implementation as supporting use case 2. Finally, I should add that neither decision is entirely incompatible with the other; it's possible to avoid using #[auto_encode], and instead define your own implementation that does whatever you want on the particular type that you have in mind... at the potential cost of quite a bit of code. For instance, serializing the enums defined in ast.rs without the help of #[auto_encode] would be pretty dreadful. How does this all sound to you? |
There are several heavier-engineering ways to handle this but ... thinking about it, I think we always drive serialization and de-serialization from rust types, so ... tentatively, I don't think it's problematic to handle Keep in mind, there's are going to be quite a few mismatches between rust types and the values representable in various serialization schemes; most won't differentiate (say) (It won't, for example, understand rust scope rules or versioning either, so matching string-names of variants against "variants a given type-directed deserializer is looking for" is the best we can ever manage; it won't precisely identify a type) |
@graydon, @erickt: Okey dokey, I think I can make everyone happy here. I've restored the special-case on Option, though I had to change the formatting of the other enums slightly because the lack of information transfer between writing the option header and writing the option values made the existing proposal incompatible with the Option special-case. There are test cases for Some, None, and other enums. |
@bors: retry |
…orphism r? I added code to the JSON encoder to support the serialization of enums. Before this, the JSON serializer only handled Option, and encoded None as 'null'. Following this change, all enums are encoded as arrays containing the enum name followed by the encoded fields. This appears consistent with the unstated invariant that the resulting output can be mapped back to the input *if* there's a decoder around that knows the types that were in existence when the serialization occurred. Also, added test cases.
r?
I added code to the JSON encoder to support the serialization of enums. Before this, the JSON serializer only handled Option, and encoded None as 'null'. Following this change, all enums are encoded as arrays containing the enum name followed by the encoded fields. This appears consistent with the unstated invariant that the resulting output can be mapped back to the input if there's a decoder around that knows the types that were in existence when the serialization occurred.
Also, added test cases.