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

1b138e8

[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.