Update json::Encoder to properly encode maps.

[brandonson]

According to extra::serialize, maps where the key and value types
are encodable should also be encodable, but extra::json only handled
maps with string keys. This updates extra::json to allow for
non-string keys.

Closes #8883

May also have some effect on #9028

[brandonson]

Essentially, instead of using the key stored in the map as the json key, "key" and "val" are now used as the keys when emitting maps.

In general the only problem I've found is that the json output is less readable.

[erickt]

@singingboyo: Hey there! Thanks for this, but I'm leaning towards r- on this. Landing this will break our ability to use rust maps to interact with JSON apis.

[brandonson]

@erickt If I'm understanding you correctly, the concern you have is addressed by using emit_object(v, e) rather than v.encode(e) for Objects in the implementation of Encodable for Json. Because of that usage it's still possible to interact with json via rust maps, as long as the map is wrapped in Object before encoding. This requires calling someMap.to_json().encode(e) rather than someMap.encode(e), but beyond that I don't think there's any breaking changes here.

If the Encodable implementation for Json is changed/hidden as suggested in #9028 then that would become someMap.to_json().to_writer(wr), but again this seems rather small, especially given the fact that we're currently producing invalid json...

[thestinger]

I don't think it makes sense to do this. You can never make a reversible encoding of non-string keys with JSON so it should be prevented.

[brandonson]

@thestinger How so? The test case encodes/decodes a map with non-string keys without any issues, so I'm not sure what the concern is here. The json keys themselves can't be non-string, but this outputs {"key0":<json-version-of-key>, "val0":<json-version-of-val>, ...} to work around that.

[erickt]

@singingboyo: the problem is that it's really easy to break round-tripping the encoding with:

let map1: TreeMap<~str,~str> = TreeMap::new();
...
let encoded = io::with_str_writer |wr| {
    let mut encoder = json::Encoder(wr);
    map1.encode(&mut encoder);
};
let mut decoder = Decoder::decode(json::from_str(encoded).unwrap());
let map2: TreeMap<~str, ~str> = Decodable::decode(&mut decoder); 

Yes you could use map1.to_json().encode(&mut encoder) but that feels pretty unintuitive. Right now I'm leaning towards putting fixing this off until #9145.

[brandonson]

@erickt: I think there's still some misunderstanding on what this PR allows/prevents. The short version of this mini-essay is that round-tripping works intuitively as long as you don't mix and match encode/decode mechanisms.

As long as values encoded using Encoder/PrettyEncoder are decoded using Decoder, and values encoded via the Json enum (so in most cases by using to_json()) are decoded using json::from_str (without Decoder), you'll be able to encode/decode just fine.

To steal your example:

let mut map1: TreeMap<~str,~str> = TreeMap::new();
map1.insert(~"k", ~"v");
map1.insert(~"foo", ~"bar");
let encoded = do io::with_str_writer |wr| {
    let mut encoder = json::Encoder(wr);
    map1.encode(&mut encoder);
};
let mut decoder = json::Decoder(json::from_str(encoded).unwrap());
let map2: TreeMap<~str, ~str> = Decodable::decode(&mut decoder);

This works and results in map1 == map2. (It should also probably be a test...) I will agree that the json it outputs ({"key0":"foo","val0":"bar","key1":"k","val1":"v"}) is somewhat ugly. However, if you need the output that is produced before this change ({"foo":"bar", "k":"v"}) then you can just use the map1.to_json().encode(...). Using to_json() would require slightly more work when decoding to convert from json::Object to a map (as Decoder would not work for the json produced), though it might be possible to add helper methods for that.

Regarding things being unintuitive: Mostly I'd be worried about someone encoding a map using Encoder and expecting {"foo":"bar"...} but getting {"key0":"foo"...}. However, as long as values encoded using Encoder are decoded using Decoder, and values encoded using Json.to_str() (and similar) are decoded using json::from_str (without Decoder), the code will work, though the json may be overly complex.

The encoding/decoding implementation is fairly complex and is still possible to break (encode with Encoder, decode with just json::from_str, get a json::Object you probably don't expect). Waiting for #9145 may indeed be warranted, but as I said I think there was (is?) misunderstanding on what exactly this allows/prevents.

TL;DR: Round-tripping works intuitively as long as you don't mix and match encode/decode mechanisms.

[emberian]

This needs a rebase.

[alexcrichton]

Closing due to inactivity, but please reopen if you make progress on this!

[brandonson]

Whoops, didn't intend to leave this sitting unattended for so long.

In any case, I'm not sure I want to re-open. I feel like there's some doubt as to the efficacy of/need for this change. As I mentioned in my last reply here, it does work, and round-tripping is in a functional state. (Though, of course, it needs updating after several changes in other parts of the library.) However, looking at it again, I feel like the need to not have data flow from ToJson to Decoder or from Encoder to FromJson could potentially be a sticking point.

If there's input saying that this change is wanted, then I'll happily jump back in and update it to match up with the latest rust, but without that input I feel I could better spend my time doing things like doc-fixing. (Which I haven't done enough of lately!)