isDouble() returns true for integer number.

[quezadaminter]

Describe the bug When integer numbers are used to instantiate a Json::Value object the query for isDouble() on that object returns true.

To Reproduce I modified a portion of the src/test_lib_json/main.cpp file to force the testing. But this can be easily reproduced with simple code such as:

int main(int argc, char *argv[]) { int v(1234); Json::Value jv(v); if(jv.isDouble() == true) { } }

JSONTEST_ASSERT_EQUAL(Json::Value(1234).isIntegral(), true); //<--- PASSES JSONTEST_ASSERT_EQUAL(Json::Value(1234).isDouble(), false); //<--- FAILS (320) JSONTEST_ASSERT_EQUAL(Json::Value(1234).isIntegral(), false); //<--- FAILS (321) JSONTEST_ASSERT_EQUAL(Json::Value(1234).isDouble(), true); //<--- PASSES

int v(1234); JSONTEST_ASSERT_EQUAL(Json::Value(v).isIntegral(), true); //<--- PASSES JSONTEST_ASSERT_EQUAL(Json::Value(v).isDouble(), false); //<--- FAILS (326) JSONTEST_ASSERT_EQUAL(Json::Value(v).isIntegral(), false); //<--- FAILS (327) JSONTEST_ASSERT_EQUAL(Json::Value(v).isDouble(), true); //<--- PASSES

std::cout << "With Double" << std::endl; JSONTEST_ASSERT_EQUAL(Json::Value(1234.56).isIntegral(), false);//<--- PASSES JSONTEST_ASSERT_EQUAL(Json::Value(1234.56).isDouble(), true); //<--- PASSES JSONTEST_ASSERT_EQUAL(Json::Value(1234.56).isIntegral(), true); //<--- FAILS (333) JSONTEST_ASSERT_EQUAL(Json::Value(1234.56).isDouble(), true); //<--- PASSES

Json::Value array1_; array1_.append(1234); JSONTEST_ASSERT_EQUAL(array1_[0].isIntegral(), true); //<--- PASSES JSONTEST_ASSERT_EQUAL(array1_[0].isDouble(), false); //<--- FAILS (337)

Results src/test_lib_json/main.cpp(320): Json::Value(1234).isDouble() == false Expected: true Actual : false src/test_lib_json/main.cpp(321): Json::Value(1234).isIntegral() == false Expected: true Actual : false src/test_lib_json/main.cpp(326): Json::Value(v).isDouble() == false Expected: true Actual : false src/test_lib_json/main.cpp(327): Json::Value(v).isIntegral() == false Expected: true Actual : false src/test_lib_json/main.cpp(333): Json::Value(1234.56).isIntegral() == true Expected: false Actual : true src/test_lib_json/main.cpp(337): array1_[0].isDouble() == false Expected: true Actual : false

Expected behavior If the number added is an integer then identifying it as a double should fail.

Desktop (please complete the following information): OS - Rocky Linux Version - 1.9.5 built with CMake.

Additional context I am using the jsoncpp library to encode and decode information. When the data is decoded it is imperative that we know the type since our use case has specific logic that runs for different data types. As such, our tests fail when an integer value is encoded with jsoncpp and returns true when queried with isDouble() on the receiving end.

[quezadaminter]

Here is some further testing:

for(Json::Value::const_iterator it = root.begin(); it != root.end(); ++it) { const Json::Value &obj(*it); Json::ValueType type(obj.type()); std::string key(it.key().asString()); }

(gdb) p key $12 = "int" (gdb) p type $13 = Json::intValue (gdb) p obj.type() $14 = Json::intValue (gdb) p obj.isDouble() $15 = true (gdb) p obj.isIntegral() $16 = true

[BillyDonahue]

When the data is decoded it is imperative that we know the type since our use case has specific logic that runs for different data types.

I believe this is working as intended.

The Json::Value type is not meant as a general purpose variant value. Its only purpose is to encapsulate a value that might appear in a JSON document. As your datum makes a round trip journey through the Json::Value type, the original type is lost. It's like making a round trip through a JSON document. JSON makes no distinction between integer or double. It only has a "number" type, so the Value type doesn't make that distinction either. The isDouble and isInteger are predicates to aid with semantic casting of JSON number values, and not variant type tag accessors.

[quezadaminter]

@BillyDonahue thank you for the reply and the explanation regarding the JSON format description. I am by no means an expert on it so I appreciate the information you shared.

As I understand it now, given that JSON is text based, it would not be incumbent on the JSON standard to identify value types since it is carrying strings. However, it seems to me that a library implementation, such as jsoncpp, with some work can identify data types and tag them accordingly when it holds them in memory. Obviously going from string to primitive and identifying a type can be tricky, but in going from primitive to JsonValue the work is handled by the language/compiler using the overloaded methods/constructors:

Value::Value(Int value) {
  initBasic(intValue);
  value_.int_ = value;
}

Value::Value(double value) {
  initBasic(realValue);
  value_.real_ = value;
}

This code can set two different values into the value_ union, but the type is correctly set for each and stored with the value. The problem is that later, calling isDouble() on both values would return true because the method tests both types:

bool Value::isDouble() const {
  return type() == intValue || type() == uintValue || type() == realValue;
}

Would you mind explaining why this method cannot be reworked to checking the expected type:

bool Value::isDouble() const {
  return type() == realValue;
}

Thank you again for your patience and time :).

[jmarrec]

I got tripped up by this today, but only because the first documentation page I found when googling was https://jsoncpp.sourceforge.net/class_json_1_1_value.html#ec4f74ef7b776b1d9c8a10fc3bb4add5 , and that's very outdated, it's from jsoncpp 0.5.0 if I can believe the sourceforge download package.

For reference, this commit from (cough ) 13 years ago (json 0.7.0) changed the behavior

https://github.com/open-source-parsers/jsoncpp/commit/1b138e8544d87f8bc4b35214be9f2bdcf78d1f36

[TTF2050]

I think I will chime in here and say this behavior is incorrect. If JSON only has a concept of "numeric", then that check is filled by the isNumeric() function. Every other function for specific numeric types should be deprecated.

OR

The isDouble() function should be considered nonstandard with respect to pure JSON, but behave as expected, returning true iff the internal value is real. The integer variants should only return true iff the internal data type is some kind of integer.

[quezadaminter]

I think that is sort of my point. If isDouble() is not covered by the standard it shouldn't matter as long as the standard is being met by isNumeric(). In this case isDouble() is a non standard extension available from this library. In this case it should provide the expected behavior given its name. If it can't or won't be made to provide the expected behavior then it should not be available, although in my opinion this would be an unnecessary loss of functionality.

[TTF2050]

Sounds like we are all saying the same thing. However, the code for isDouble() returns true for any numeric, and isNumeric() defers to isDouble().

[BillyDonahue]

isDouble is essentially saying that the Value can be a double, if that's what the caller's C++ code wants. It's a precursor to getting that double value out of it if true.

[quezadaminter]

That's an interesting way to think about it. Then should the semantics of the name be addressed to resolve the issue? I mean in this case it should be removed because that is the same behavior as isNumeric() but with ambiguity given its name.

[BillyDonahue]

Well maybe but we don't really want an API break over it. Better to just improve the docs if necessary.

[TTF2050]

Seems like there are a few things going on here. First, there are the types as defined in JSON: integer and number. Second, there are types as understood by computers: int, uint, float, double, etc. Having "is" functions for integer and number makes sense with respect to Json::Value objects. Having "as" functions to get the Json::Value as a specific C++ data type also makes sense. The complications arise when trying to test if a Json::Value "is" a C++ data type.

I sort of see the rational of taking the "isC++Type" pattern to mean "can it be a C++ type?", but it's not an obvious interpretation. Given that this is a C++ library with C++ types at its core, I think it makes more sense to let these functions reflect the implementation internals. Then I can make decisions about how to extract the information without loss of precision (with respect to whatever internal data type was used to store the original value) or make an informed decision that the extraction may incur loss of precision.

[quezadaminter]

I think that's a good point. At the core of the issue it seems to me the key is to determine if there will or will not be loss of precision when retrieving the JSON value into the C++ domain.

The use case that ran into and triggered this issue involved converting JSON data into binary with the goal of optimizing memory usage so being able to take an integer from JSON and then pack it in the smallest integer unit was essential for what we needed to achieve. Maintain precision would fit the bill here.

[baylesj]

Well maybe but we don't really want an API break over it. Better to just improve the docs if necessary.

This is my primary concern with merging the linked pull request.

[TTF2050]

Maybe save it for the next major release? I still stand by my assertion that the changes are overall beneficial and make the API more intuitive.

[quezadaminter]

Saving for major release makes sense if there is concern that the API change may break code 👍🏽.

[appgurueu]

Frankly I would have assumed that JSON numbers are always doubles because that's what being a subset of JS would imply but indeed the specifications do not require this, see https://blog.trl.sn/blog/what-is-a-json-number/.

I think isDouble should stay consistent with asDouble: isDouble should be safe to call if and only if calling asDouble is losslessy possible. This means that isDouble should return true for integers which can be exactly represented as doubles: Integers from $-2^{53}$ to $2^{53}$ (both inclusive) and powers of two.

Conversely, isInteger should check whether the value is exactly representable as an integer returned by asInteger, no matter how it is stored, because JSON has no concept of "storage types". It is perfectly fine for an application to format "the integer" 42 as 42.0 which would cause it to be stored internally as a double by jsoncpp. And the other way around, an application may choose to omit the trailing .0 for doubles. I may for example be producing JSON with a Lua script (which only knows doubles), only for jsoncpp to then tell me that I don't have doubles.

The current code does not quite do that: It will consider 64-bit integers which can't be exactly represented by doubles, doubles.

With the proposed PR, we would get:

• isDouble now checks whether something is internally stored as a double which you probably don't care about when you're reading doubles. This is already somewhat of a pitfall and would be quite an annoying breaking change.
• You could now use isNumeric, but isNumeric - asDouble reads somewhat weirdly, and for a reason: It has the same issue that it isn't checking whether the number is representable as a double.
• isConvertibleTo is definitely not what you want. It's even more permissive than isNumeric.

To wrap it up:

if (x.isDouble()) {
    double y = x.asDouble();
    ...
}

should be fine, otherwise the API would be unintuitive; I'm not fond of the linked PR. Currently it's merely somewhat questionable w.r.t 64-bit integers which could lose precision; a major release would be a good opportunity to address that.

I don't think the internal type tags are very useful for numbers since as I mentioned before, there is no real concept of a storage type in JSON, but if someone does want to query them, they can just use .type().