libserialize: encoding non string key map to json silently produces invalid json instead of failing

[oli-obk]

importing object type string key maps is still supported
writing them should be explicit, and can be done as follows

let some_tree_map : TreeMap<String, Json> = ...;
Json::Object(some_tree_map).to_writer(&mut writer);

related to #8335, #9028, #9142

[lifthrasiir]

This seems too "ugly" for normal uses. Can we do better? For example, we may optionally implement (or insert something like if TypeId::of::<K>() == TypeId::of::<String>(), which rustc will optimize out) an object version of maps, then fall back to this code.

[oli-obk]

i'd prefer keeping it explicit.
if you want to manually create json structures, do it with the Json objects.
if you want serialization, have as little surprises as possible.
if you want deserialization, allow reading everything that is legal and matches what you expect to read

[lifthrasiir]

The autoserialization is commonly used for types that are not aware of serialization (but could have been made aware of it). Adding Json::Object to such types would be annoying.

(I have made it clear that this specific issue is subjective, as "ugly" is quoted, but I don't think that making the output format overly unconventional is good in general. If that were a concern we should have used some other format specific to us.)

[alexcrichton]

I agree with @lifthrasiir that this seems like the wrong default. This means that any users encoding a HashMap are encoding into an array of arrays, right?

[oli-obk]

yes, which is the only sensible way for non-string (or not trivially convertable to string) keys.
another way would be to serialize the key to json, escape it, and use that as the key.
a third way would be to differentiate between string/numeric keys and all other keys. which would probably cause a lot of confusion (wrap a string in a newtype and you get a different serialization)

[alexcrichton]

I think that not serializing maps as maps is such a gotcha that serialize::json would not be very useful in the world at large. I personally think it's OK for serialization to just fail for non-string keys as that's simply just a limitation of the json format. All serializers shouldn't necessarily strive to be able to encode all Rust types in existence I believe.

[sfackler]

I agree with @alexcrichton.

I've felt a bit weird with the JSON module's use of Serializable and Deserializable for this reason. It seems more "correct" for those traits to only be used with things that can truly serialize and deserialize anything with an implementation, and we'd have separate traits for JSON. That might end up being way to inconvenient, but it's a bit of a bummer that you don't find out until runtime that some type can't be JSON serialized.

[sfackler]

We also serialize enums with fields in a way that feels a bit shoehorned in.

[oli-obk]

another alternative would be to output invalid json (map key is a json value, not a string), and to require the json serializer to be created with a flag explicitly allowing that.

[oli-obk]

go-lang: http://golang.org/pkg/encoding/json/#Marshal (has flag to convert key to json and escape as string)

python: https://docs.python.org/2/library/json.html (convert to string in python-specific format)

Keys in key/value pairs of JSON are always of the type str. When a dictionary is converted into JSON, all the keys of the dictionary are coerced to strings. As a result of this, if a dictionary is converted into JSON and then back into a dictionary, the dictionary may not equal the original one.

.net: http://james.newtonking.com/json/help/index.html?topic=html/SerializationGuide.htm
ToString or custom TypeConverter for converting to string

other languages can't get their head around one or another way either. I agree now with @alexcrichton, an error in serialization is the right way to go. i'll update this PR.

[oli-obk]

I have tried several schemes:

1. check if the format is string (starts and ends with a quote) or if it is integer (no quotes, but can be converted to integer), else panic. This is rather hacky and requires unsafe and transmute (as it currently already does)
2. make emit_map_elt_key generic over a type parameter that requires map keys to implement a trait EncodableAsMapKey<S : Encoder<E>, E> and Encodable. This way there's no function parameter, and Encoders can specify which Types can be map keys by implementing that trait for them. Does not help about the unsafe and transmute, as quotes still need to be added sometimes. Checks at compile-time.
3. check type through TypeId (+ pass type instead of function to emit_map_elt_key). Forces a limited set of hardcoded types allowed as keys (types, not traits, implementing your own string type won't work).

A few more possible ways to do these checks

1. create emit_map_elt_key_* for all the types that exist as emit_*
2. wait for generic specialization and overwrite the Encodable impls for collections to check stuff for json encoder
3. create emit_map_elt_key_num, emit_map_elt_key_string and emit_map_elt_key_object
4. add trait parameters to Encoder for choosing a trait that all map keys must implement (checks at compile time)
5. add trait parameters to Encoder for choosing a trait that all Encodables must implement. (checks at runtime)

[alexcrichton]

One possible solution would be to set a flag when emitting a string and resetting it in all other emissions, and then after you emit a key if the flag is set you know you emitted a string and otherwise you know that it wasn't a key.

The other direction is also fine where you set a flag when emitting a key and then when you emit a type elsewhere it'll check the flag to make sure it's valid

[oli-obk]

That sounds reasonable.
I actually implemented both. when emitting a map key the flag is set, when a string, char or number is written, the flag is checked and number formatting is adjusted and the flag is changed. then the mapkey-function checks the flag again and either panics or clears the flag.

panicking -> error reporting will be done in another PR (after Encoder+PrettyEncoder merge)

[alexcrichton]

Awesome, looks great! Could you actually go a head an add a variant to the error enum returned to reflect this new condition? (as opposed to waiting to a further PR)

[oli-obk]

r? @alexcrichton
not the prettiest way to do this, but converting errors only works nicely through the try! macro

[oli-obk]

rebased and fixed

[oli-obk]

fixed unit test, rebased and fixed rebase fallout

[oli-obk]

phew, good thing it took me so long to get this ready... it doesn't work!
if a single sub-element of the map key is a valid map key, the flag is set and the map key is accepted.
what i could do is blacklist all invalid elements, but any changes in the future to the Encoder or Decoder trait might mess that up again.

[oli-obk]

@alexcrichton it is fixed and tested now, i actually messed up the unit test before, that's why the mistake wasn't caught by it.
make check-stage1 ran through, make check is running right now
edit: make check finished

[oli-obk]

should i squash the fix?

[alexcrichton]

Sure!

[oli-obk]

done

[oli-obk]

@alexcrichton those failures don't seem to be mine, should i rebase or sth?

[oli-obk]

rebased

[oli-obk]

the only fix required was changing a 0..n to a ..n

[alexcrichton]

@bors: r+ a320149

[bors]

⌛ Testing commit a320149 with merge 7f8c687...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt