Floating point rounding

[quicknir]

I'll preface this by saying I know there's been some discussion about this already, and by saying that my version is not the latest, so this may already be fixed. Right now, you can do:

auto j = json::parse(R"({"x":1.5})");
int x = j.at("x");

With no complaint. Is this intentional? I thought that json checks to see if precision is lost before converting to an integer.

[nlohmann]

The library behaves just like C++, meaning it interprets your code just like

auto x = 1.5;
int y = x;

We do store 1.5 as double internally, so we try to keep its precision. But if you decide to store it as an int, there is nothing JSON or the library will do about it.

[quicknir]

Hmm, if I compile with -Wfloat-conversion will a warning be raised? If you're busy I'll test this myself and get back to you; right now I'm using your library through our build system and it's a bit convoluted for me to see what the situation is with that flag.

[quicknir]

So I checked, and compiling with -Wfloat-conversion does not cause any warning, at least when json is included via -isystem, possibly it would be different with -I but I'm not sure. I don't really understand the rationale for this; these types of narrowing conversions happening quietly is rather widely regarded as a bad thing, that's why there's warnings and even why newer forms of initialization often disallow them. Obviously it's a breaking change but I think that version 3 should see this changed.

[nlohmann]

You can check for the type with is_number_float() and avoid conversions to integers. But whether the library should drop a warning would be something I like to discuss.

[nlohmann]

FYI: This is the currently used conversion code: https://github.com/nlohmann/json/blob/develop/src/json.hpp#L1084

[quicknir]

Another way to approach this problem, which you may be philosophically against, is to allow more arguments/control to the parsing of the json in the first place. Whether or not to throw when you try to extract negative to unsigned, or floating point to integer, could be controlled via optional arguments to parse/serialize functions, as well as other things like Nan/Infty parsing.

I think that with sane defaults this will not affect the usability of the library (I wouldn't really care whether implicit conversion or throwing is the default, Nan probably shouldn't be), but it will make it more easily usable for more people, and the C++ approach is generally to allow as much customization as possible.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[quicknir]

Hi, I'm disappointed that this has been closed without further discussion. I don't think that mimicking C++ behavior that itself is a more-or-less-widely-acknowledged Bad Thing (tm) that was inherited from C, is a good basis for a design decision. Silently throwing away user information is something that no good C++ library should do; it violates fail fast and makes debugging harder. There's two consistent positions here, IMHO:

1. Not allow conversions directly to integer at all. That means a statement like int x = j.at("x"); either doesn't compile, or (would depend on the C++ implementation details) would extract a double from the json that would then in C++ (as opposed to in library) implicitly convert to int. This way at least narrowing conversions would catch it. This is the most consistent in some sense with the spirit of json itself; json doesn't really do integers at all, it's number type is floating point, plain and simple. And you offload the problem into C++/user space, minimizing the number of choices the json library has to make.
2. Allow extraction as an integer, but throw if rounding is occurring. Note also that negative numbers have the same issue. Extracting a value of -1 into unsigned just silently wraps around. This is relatively easy to handle in the sense that the library is already making some intelligent decisions about how to store the parsed json value (since it will sometimes use an integral type, and sometimes a floating point type).

Either of these approaches I think is fine. Both of them at least are no less type safe than C++. Currently, this library is more type unsafe than C++ with typical warnings on. Those warnings were added as people realized all the issues with C playing fast and loose with typing. We can't change this for backwards compatibility reasons but new libraries should strive to be more type safe than C++, not less.

[nlohmann]

I reopened the issue, hoping that there are more people to discuss it.

[t-b]

I agree here that the current behaviour wrt. to integer/float precision is not nice.

But the proposed solutions of @quicknir have the following flaws:
1.) According to http://json.org the numbers in a JSON document have an unspecified precision. So it is not possible to treat all numbers as a double as this breaks large 64bit integer values. And we don't have long double on all platforms.
2.) For that to work you would have to pass in the target type as at does not know that.

The unwanted rounding also happens already at parsing the JSON document, see #647.

[quicknir]

1. Yes, precisely, it is unspecified, so a conforming implementation can use whatever precision it wants. Since javascript does not even have integers, people should definitely not be storing enormous integers in json and depending on their implementation to be able to handle it. double lets us store, IIRC, a 53 bit signed integer or something like that, with no loss of precision. After that, it will fail gracefully (i.e. lose significance but be approximately correct). I don't see anything unreasonable about this state of affairs.

2. You don't need to pass in the target type. If you can assign the result of at (which I assume is an instance of json) to an integer, that means that the result of at simply implements conversion operators to integer, float, etc. In the conversion to integer operator of whatever the result of at is, you can check the stored value and throw if necessary.

[t-b]

After that, it will fail gracefully (i.e. lose significance but be approximately correct). I don't see anything unreasonable about this state of affairs.

And that is something I would like to avoid. IMHO keeping only 53bits of an large integer is equally bad than rounding 1.5 to 1.

2. According to 1 we land at get_arithmetic_value later on. And the static cast in that function silences the compiler warning. So you are correct, sorry for the confusion.

[quicknir]

And that is something I would like to avoid. IMHO keeping only 53bits of an large integer is equally bad than rounding 1.5 to 1.

I don't think this statement is true because in real programs getting a rounding error from a float to an int is so much more likely than deciding to store a bunch of data that happens to be between 53 and 64 bits. There are use cases though, e.g. nanoseconds past epoch for representing time points.

At any rate I do prefer solution 2, and it seems like your concern regarding it has been assuaged, so maybe we can try to build some consensus in that direction?

[nlohmann]

What's wrong with checking for the type of the stored number with j.is_number_float()? Then you can be sure that the number is stored as a double, and double x = j; will involve no rounding. The same with j.is_number_integer() and int64_t and j.is_number_unsigned() and uint64_t.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[janvdijk]

I have been using the code for only a week before finding out about this behaviour the hard way. In my view there is nothing wrong, but a few lines of documentation in README.md would definitely have helped. May I kindly suggest something like the attached patch?
777.txt