encoding/json: allow e notation in int conversion

[Martchus]

According to RFC4627 the exponent notation is allowed for all numbers, so a conversion to int/uint for values like '1.234567e+06' should be possible.

In fact, the current behaviour leads to issues in software like Syncthing: syncthing/syncthing#4001

Proposed change: https://go-review.googlesource.com/#/c/37438

The change also extends to test cases, so you can easily reproduce the issue by just executing them without applying the fix itself.

Note that this change are the first lines of code I've ever written in Go. So it is the most simplest solution which comes with a disadvantage described in the commit message.

Alternative proposal by @martisch:

Without looking at the internals of ParseInt maybe we could parse a Uint as before and if that fails (or if we know a valid exponent part follows by looking ahead) we try ParseFloat afterwards.
Another option might be adding support for scientific notation into ParseInt.
If there are no tests for large int values we should add them to make sure the old precise conversion is not accidentally broken.

Sounds more complicated but also safer (regarding backward compatibility) than my approach. However, my knowledge of Go is very limited so I'm not sure whether I could implement it appropriately this way.

So I better leave the implementation to you if you don't accept my approach. I guess the extended test cases from my proposal can be taken in any case.

@martisch And yes, if you insist on retaining the old behaviour regarding precision for large ints you should add test cases for it. Currently this is not tested in decode_test.go, otherwise I would have broken that test with my implementation.

[ALTree]

Note that this was rejected in the past: #5562 (and #8460 and #6657).

[Martchus]

The last one is "FrozenDueToAge". Not sure why #5562 has been closed or what the final conclusion was. But the last two comments actually say, that this should be supported:

JSON numbers are floats. If the JSON is produced by some other language, it may very well encode larger integers in exponential notation in the expectation that they can be round-tripped back into an integer. Luckily the usual Javascript JSON stringify function does not produce e-notation for integers in the int32 range, otherwise this would be a much bigger issue than it is.

[ALTree]

Well on #6657 rsc said:

We decided against this a while back.

So I guess the final decision on #5562 was that this was working as intended.

I'm not saying it won't be reconsidered, but it's a fact that this issue is a duplicate of other 3 closed issues, and that in the past this was rejected.

cc @rsc

[Martchus]

By the way, this is about exchanging JSON between Qt and Syncthing/Go. If you decide to refuse to fix it, communication between Qt and Go apps via JSON will be impaired. Of course I could also try to include a workaround in Qt to support your parser, but it wouldn't be the right approach in my opinion.

[rsc]

If you decide to refuse to fix it, communication between Qt and Go apps via JSON will be impaired.

It sounds like the same could be said about Qt refusing to fix their encoder. ES6 never encodes a float64 that is an exact int64 using scientific notation, and Go just changed to do this too (see #14135). If Qt is encoding the int64 1000000 as "1e6", then I would argue Qt should be fixed, for the same reasons we fixed Go in #14135.

I'll leave this open for discussion for a bit, but I still agree with myself from #5562: "If you have a number that looks like 1.11e5 and you are trying to stuff it into an int64, you've probably screwed up somewhere, and it is reasonable for package json to tell you."

Changing strconv.ParseInt is certainly not on the table.

[Martchus]

It sounds like the same could be said about Qt refusing to fix their encoder.

No because their encoder encodes valid JSON.

ES6 never encodes a float64 that is an exact int64 using scientific notation

This might be correct, but as far as I'm concerned the module is a JSON enocder/decoder and JSON does not specify such restrictions.

I would argue Qt should be fixed

I already asked and in contrast to Go they do not refuse to include a patch for better compatibility with other implementations - although the fault isn't even on their side.

Changing strconv.ParseInt is certainly not on the table.

Then I close this because there is no point for further discussion.

Projects like Syncthing will have to state that their REST-API is actually not fully JSON compatible (only ES6) or find a workaround.

[rsc]

I would argue Qt should be fixed

I already asked and in contrast to Go they do not refuse to include a patch for better compatibility with other implementations - although the fault isn't even on their side.

Great, glad to hear it. I disagree with the claimed contrast, though: Go already did what you're asking Qt to do, in #14135.

[Martchus]

Just to clarify: The contrast refers to providing a fix for the current issue. On the Qt side it would be more a workaround than a fix, though.

But you convinced me a little bit :-) At least I have to admit that it can not harm to introduce that change in Qt whereas changing things in Go might break stuff.

It would be great if JSON would be more explicitly specified. Then it would be out of discussion what the correct behaviour would be.

[cespare]

Because JSON numbers are floating point, if you're unmarshaling into an integer type, it's up to Go (not the JSON spec) to say how the conversion is handled (or rejected). You are always free to unmarshal any JSON number into a float64 and then do the integer conversion of your choice. This will work even with numbers like 1e2.

[rsc]

Or use json.Number and try both. syncthing has plenty of options beyond "get a change into Go 1.9 and wait six months".

[Martchus]

syncthing has plenty of options

That is also my thought. But you have to tell that @AudriusButkevicius who closed the Syncthing issue because it is "out of their control".

[rsc]

How other projects handle bug reports is beyond my scope here. Closing the issue may be perfectly reasonable if Qt is at fault / not enough users hit this and there are workarounds / other reasons. My point was only that there are possible workarounds today; Go not changing this detail is not blocking users.