Unexpected conversion from 'true'/'false' to some integer types

[jboelterintc]

Description

Converting from "value": true or "value": false behaves unexpectedly and differently for certain integer types.

I was expecting all conversions from a boolean true/false to integer value to fail, however it is stored as a 1 or 0.

Reproduction steps

Given:

template<typename T>
class Value {
public:
    T value;
private:
    NLOHMANN_DEFINE_TYPE_INTRUSIVE(Value<T>, value);
};

This will result in a 1 in value.

auto v = R"({
    "value" : true
})"_json.get<Value<int>>();

Using uint64_t will throw a type_error.

auto v = R"({
    "value" : true
})"_json.get<Value<uint64_t>>();

Expected vs. actual results

I expected all boolean to integer conversions to fail. Instead a true -> 1 and false -> 0 for certain integer values.

Various integer conversions - https://godbolt.org/z/7Wrh6EanW

The uint64_t case is taking the path through get_arithmetic_value

template<typename BasicJsonType>
inline void from_json(const BasicJsonType& j, typename BasicJsonType::number_unsigned_t& val)
{
    get_arithmetic_value(j, val);
}

The int case is taking the path through

struct from_json_fn
{
    template<typename BasicJsonType, typename T>
    auto operator()(const BasicJsonType& j, T&& val) const
    noexcept(noexcept(from_json(j, std::forward<T>(val))))
    -> decltype(from_json(j, std::forward<T>(val)))
    {
        return from_json(j, std::forward<T>(val));
    }
};

// which calls

template < typename BasicJsonType, typename ArithmeticType,
           enable_if_t <
               std::is_arithmetic<ArithmeticType>::value&&
               !std::is_same<ArithmeticType, typename BasicJsonType::number_unsigned_t>::value&&
               !std::is_same<ArithmeticType, typename BasicJsonType::number_integer_t>::value&&
               !std::is_same<ArithmeticType, typename BasicJsonType::number_float_t>::value&&
               !std::is_same<ArithmeticType, typename BasicJsonType::boolean_t>::value,
               int > = 0 >
inline void from_json(const BasicJsonType& j, ArithmeticType& val)
{
//...

        case value_t::boolean:
        {
            val = static_cast<ArithmeticType>(*j.template get_ptr<const typename BasicJsonType::boolean_t*>());
            break;
        }
//...
}

Minimal code example

https://godbolt.org/z/fs4frqz7G

#include <cassert>
#include <iostream>

#include <nlohmann/json.hpp>

template<typename T>
class Value {
public:
    T value;
private:
    NLOHMANN_DEFINE_TYPE_INTRUSIVE(Value<T>, value);
};

int main() {
    try {
        std::cout << "uint64_t" << std::endl;
        auto v = R"({
            "value" : true
        })"_json.get<Value<uint64_t>>();

        assert(false);
    } catch (const nlohmann::detail::type_error& e) {
        std::cout << e.what() << std::endl;
        std::cout << "ok - exception expected" << std::endl;
    }

    try {
        std::cout << "int" << std::endl;
        auto v = R"({
            "value" : true
        })"_json.get<Value<int>>();

        assert(false);
    } catch (const nlohmann::detail::type_error& e) {
        std::cout << e.what() << std::endl;
        std::cout << "ok - exception expected" << std::endl;
    }
}

Error messages

No response

Compiler and operating system

Latest MSVC & Clang

Library version

3.11.3

Validation

• The bug also occurs if the latest version from the develop branch is used.
• I can successfully compile and run the unit tests.

[jshastid]

I am having the same issue and apologies if I am not understanding this correctly as I am new to C++ and have only just started using this library, but it looks like the integer value is not being considered a basic_json and so is calling the explicit cast from_json. At least the comment above the function leads me to believe this.
// overload for arithmetic types, not chosen for basic_json template arguments
// (BooleanType, etc..); note: Is it really necessary to provide explicit
// overloads for boolean_t etc. in case of a custom BooleanType which is not
// an arithmetic type?

[thisisamardeep]

hi i am working on this.Will get something by this weekend.

[amirghaz]

Are you still working on this? I'm planning to look into it.

[amirghaz]

If I'm planning on contributing, is there any guidelines or styles that I need to follow while writing the code? If yes, where can I read about it? Thanks.

[nlohmann]

See https://github.com/nlohmann/json/blob/develop/.github/CONTRIBUTING.md

[nlohmann]

@amirghaz Do you need further assistance?

[amirghaz]

@nlohmann I'm a beginner with this library, so any insight that you have is highly appreciated.

[nlohmann]

The current proposal of #4523 is a breaking change. We need to decide how to guard this change to not affect client code. Yet another preprocessor macro? 🤔

[arifghaz]

I see, but I lack the experience dealing with preprocessor macro. @jboelterintc do you have any suggestion?

[arifghaz]

I managed to write a (partial) fix that passed all 98 unit tests without using preprocessor macro. However, it is only a partial fix because, although all other integer types return a type error, uint8_t still perform the conversion.
I first write a fix that force all integer types to throw a type error, but doing so will causes some unit tests (22, 23, 24) that involve uint8_t to bool conversion to fail. So, I end up implementing this partial fix.

[arifghaz]

It seems my newest fix still fail some checks. Before moving forward with another fix, maybe we should discuss if the partial fix I mentioned above is the way to go. Currently, only int64_t and uint64_t will return a type error, and based on the unit tests, it seems that conversion from uin8_t to boolean is required (except if there is a way to circumvent this that I am unaware of).

So, any fix potentially will still have some inconsistencies because of uint8_t. Is this really what we want? Any opinion @nlohmann @jboelterintc on this?