Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message for const fields #2818

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

AlexanderLanin
Copy link

The goal here is to propose an idea for a better error message, which says what is wrong instead of pointing to some internal implementation detail that does not compile.

As I'm a very novice user of the library and even less familiar with the code itself, I'll just create this draft for someone else to finish / abandon the idea. I'm not familiar enough with it to create a proper pull request. Sorry!

It should even be possible to test the static_assert, but that is another week of fiddling.
e.g. https://www.youtube.com/watch?v=zxDzMjfsgjg

The result is certainly not perfect, but it's significantly improved in both tested compilers.

Note: using enable_if or require doesn't really improve the situation.
It just says error: no matching member function for call to 'get_to'
And like 20 lines later ../single_include/nlohmann/json.hpp:20258:16: note: because 'const int' does not satisfy 'Writeable'

Scenario:

    struct person_with_const_age
    {
        std::string name{};
        const int age = 0;

        NLOHMANN_DEFINE_TYPE_INTRUSIVE(person_with_const_age, name, age)
    };

Output with g++ 9.3.0 without the patch:

/usr/bin/c++  -DDOCTEST_CONFIG_SUPER_FAST_ASSERTS -DJSON_DIAGNOSTICS=0 -DJSON_USE_IMPLICIT_CONVERSIONS=1 -Iinclude -I../test/thirdparty/doctest -I../test/thirdparty/fifo_map -I../single_include -Wno-deprecated -Wno-float-equal -Wno-deprecated-declarations -MD -MT test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o -MF test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o.d -o test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o -c ../test/src/unit-udt_macro.cpp
In file included from ../test/src/unit-udt_macro.cpp:34:
../single_include/nlohmann/json.hpp: In instantiation of ‘void nlohmann::detail::from_json(const BasicJsonType&, ArithmeticType&) [with BasicJsonType = nlohmann::basic_json<>; ArithmeticType = const int; typename std::enable_if<((((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>::type <anonymous> = 0]’:
../single_include/nlohmann/json.hpp:4255:25:   required from ‘decltype (nlohmann::detail::from_json(j, forward<T>(val))) nlohmann::detail::from_json_fn::operator()(const BasicJsonType&, T&&) const [with BasicJsonType = nlohmann::basic_json<>; T = const int&; decltype (nlohmann::detail::from_json(j, forward<T>(val))) = void]’
../single_include/nlohmann/json.hpp:4867:30:   required from ‘static decltype ((nlohmann::{anonymous}::from_json(forward<BasicJsonType>(j), val), void())) nlohmann::adl_serializer<T, SFINAE>::from_json(BasicJsonType&&, TargetType&) [with BasicJsonType = const nlohmann::basic_json<>&; TargetType = const int; ValueType = const int; <template-parameter-1-2> = void; decltype ((nlohmann::{anonymous}::from_json(forward<BasicJsonType>(j), val), void())) = void]’
../single_include/nlohmann/json.hpp:20260:45:   required from ‘ValueType& nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType, JSONSerializer, BinaryType>::get_to(ValueType&) const [with ValueType = const int; typename std::enable_if<((! nlohmann::detail::is_basic_json<BasicJsonType>::value) && nlohmann::detail::has_from_json<nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType, JSONSerializer, BinaryType>, ValueType>::value), int>::type <anonymous> = 0; ObjectType = std::map; ArrayType = std::vector; StringType = std::__cxx11::basic_string<char>; BooleanType = bool; NumberIntegerType = long int; NumberUnsignedType = long unsigned int; NumberFloatType = double; AllocatorType = std::allocator; JSONSerializer = nlohmann::adl_serializer; BinaryType = std::vector<unsigned char>]’
../test/src/unit-udt_macro.cpp:238:9:   required from here
../single_include/nlohmann/json.hpp:4141:17: error: assignment of read-only reference ‘val’
 4141 |             val = static_cast<ArithmeticType>(*j.template get_ptr<const typename BasicJsonType::number_unsigned_t*>());
      |             ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../single_include/nlohmann/json.hpp:4146:17: error: assignment of read-only reference ‘val’
 4146 |             val = static_cast<ArithmeticType>(*j.template get_ptr<const typename BasicJsonType::number_integer_t*>());
      |             ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../single_include/nlohmann/json.hpp:4151:17: error: assignment of read-only reference ‘val’
 4151 |             val = static_cast<ArithmeticType>(*j.template get_ptr<const typename BasicJsonType::number_float_t*>());
      |             ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../single_include/nlohmann/json.hpp:4156:17: error: assignment of read-only reference ‘val’
 4156 |             val = static_cast<ArithmeticType>(*j.template get_ptr<const typename BasicJsonType::boolean_t*>());
      |             ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Output with g++ 9.3.0 with the patch:

/usr/bin/c++  -DDOCTEST_CONFIG_SUPER_FAST_ASSERTS -DJSON_DIAGNOSTICS=0 -DJSON_USE_IMPLICIT_CONVERSIONS=1 -Iinclude -I../test/thirdparty/doctest -I../test/thirdparty/fifo_map -I../single_include -Wno-deprecated -Wno-float-equal -Wno-deprecated-declarations -MD -MT test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o -MF test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o.d -o test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o -c ../test/src/unit-udt_macro.cpp
In file included from ../test/src/unit-udt_macro.cpp:34:
../single_include/nlohmann/json.hpp: In instantiation of ‘ValueType& nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType, JSONSerializer, BinaryType>::get_to(ValueType&) const [with ValueType = const int; typename std::enable_if<((! nlohmann::detail::is_basic_json<BasicJsonType>::value) && nlohmann::detail::has_from_json<nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType, JSONSerializer, BinaryType>, ValueType>::value), int>::type <anonymous> = 0; ObjectType = std::map; ArrayType = std::vector; StringType = std::__cxx11::basic_string<char>; BooleanType = bool; NumberIntegerType = long int; NumberUnsignedType = long unsigned int; NumberFloatType = double; AllocatorType = std::allocator; JSONSerializer = nlohmann::adl_serializer; BinaryType = std::vector<unsigned char>]’:
../test/src/unit-udt_macro.cpp:238:9:   required from here
../single_include/nlohmann/json.hpp:20259:23: error: static assertion failed: Cannot deserialize into constant fields
20259 |         static_assert(!std::is_const<ValueType>::value, "Cannot deserialize into constant fields");
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Output with clang++ 10.0.0 without the patch:

/usr/bin/clang++  -DDOCTEST_CONFIG_SUPER_FAST_ASSERTS -DJSON_DIAGNOSTICS=0 -DJSON_USE_IMPLICIT_CONVERSIONS=1 -Iinclude -I../test/thirdparty/doctest -I../test/thirdparty/fifo_map -I../single_include -Wno-deprecated -Wno-float-equal -MD -MT test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o -MF test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o.d -o test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o -c ../test/src/unit-udt_macro.cpp
In file included from ../test/src/unit-udt_macro.cpp:34:
../single_include/nlohmann/json.hpp:4141:17: error: cannot assign to variable 'val' with const-qualified type 'const int &'
            val = static_cast<ArithmeticType>(*j.template get_ptr<const typename BasicJsonType::number_unsigned_t*>());
            ~~~ ^
../single_include/nlohmann/json.hpp:4255:16: note: in instantiation of function template specialization 'nlohmann::detail::from_json<nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char>, bool, long, unsigned long, double, std::allocator, adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > >, const int, 0>' requested here
        return from_json(j, std::forward<T>(val));
               ^
../single_include/nlohmann/json.hpp:4867:9: note: in instantiation of function template specialization 'nlohmann::detail::from_json_fn::operator()<nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char>, bool, long, unsigned long, double, std::allocator, adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > >, const int &>' requested here
        ::nlohmann::from_json(std::forward<BasicJsonType>(j), val);
        ^
../single_include/nlohmann/json.hpp:20260:36: note: in instantiation of function template specialization 'nlohmann::adl_serializer<const int, void>::from_json<const nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char>, bool, long, unsigned long, double, std::allocator, adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > > &, const int>' requested here
        JSONSerializer<ValueType>::from_json(*this, v);
                                   ^
../test/src/unit-udt_macro.cpp:238:9: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char>, bool, long, unsigned long, double, std::allocator, adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > >::get_to<const int, 0>' requested here
        NLOHMANN_DEFINE_TYPE_INTRUSIVE(person_with_const_age, name, age)
        ^
        [~30 more lines]

Output with clang++ 10.0.0 with the patch:

/usr/bin/clang++  -DDOCTEST_CONFIG_SUPER_FAST_ASSERTS -DJSON_DIAGNOSTICS=0 -DJSON_USE_IMPLICIT_CONVERSIONS=1 -Iinclude -I../test/thirdparty/doctest -I../test/thirdparty/fifo_map -I../single_include -Wno-deprecated -Wno-float-equal -MD -MT test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o -MF test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o.d -o test/CMakeFiles/test-udt_macro.dir/src/unit-udt_macro.cpp.o -c ../test/src/unit-udt_macro.cpp
In file included from ../test/src/unit-udt_macro.cpp:34:
../single_include/nlohmann/json.hpp:20261:9: error: static_assert failed due to requirement '!std::is_const<const int>::value' "Cannot deserialize into constant fields"
        static_assert(!std::is_const<ValueType>::value, "Cannot deserialize into constant fields");
        ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../test/src/unit-udt_macro.cpp:238:9: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char>, bool, long, unsigned long, double, std::allocator, adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > >::get_to<const int, 0>' requested here
        NLOHMANN_DEFINE_TYPE_INTRUSIVE(person_with_const_age, name, age)
        ^
        [~50 more lines]

Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

Please don't

  • The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.7 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
  • Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for these kind of bugs). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
  • Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
  • Please do not open pull requests that address multiple issues.

@nlohmann
Copy link
Owner

We once had static asserts, but there was a reason to remove them, see #960. I know this situation different, but I am hesitant to merge right now.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.982% when pulling 2494932 on AlexanderLanin:const into 18a5f4c on nlohmann:develop.

@AlexanderLanin
Copy link
Author

Indeed "interesting". Especially since adding !std::is_const<ValueType> to enable_if / concepts is way less helpful. As mentioned above instead of "this is the correct function, but you can't call it", you get "by the way there was this one function, but nah... doesn't fit".

@stale
Copy link

stale bot commented Jan 9, 2022

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.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jan 9, 2022
@nlohmann nlohmann added review needed It would be great if someone could review the proposed changes. and removed state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated labels Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review needed It would be great if someone could review the proposed changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants