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

Conversion operator for nlohmann::json is not SFINAE friendly #1237

Closed
ldionne opened this issue Sep 12, 2018 · 3 comments
Closed

Conversion operator for nlohmann::json is not SFINAE friendly #1237

ldionne opened this issue Sep 12, 2018 · 3 comments
Assignees
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@ldionne
Copy link
Contributor

ldionne commented Sep 12, 2018

  • What is the issue you have?

nlohmann::json's conversion operator is mis-behaved. It pretends to be convertible to things that it is not actually convertible to, which causes errors when using generic libraries that rely on these checks.

  • Please describe the steps to reproduce the issue. Can you provide a small but working code example?

In the following program, the static_assert passes because std::is_convertible checks whether there's a conversion operator for the given type. However, when actually trying to instantiate the conversion operator, we fail because no viable get function can be found:

#include <nlohmann/json.hpp>
#include <type_traits>

struct MyType { };

static_assert(std::is_convertible<nlohmann::json, MyType>::value, "");
nlohmann::json f();
MyType foo = f();

The error message is

In file included from ../test/src/unit-json_conversion_operator.cpp:30:
../single_include/nlohmann/json.hpp:13914:16: error: no matching member function for call to 'get'
        return get<ValueType>();
               ^~~~~~~~~~~~~~
../test/src/unit-json_conversion_operator.cpp:37:14: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer>::operator MyType<MyType, 0>' requested here
MyType foo = f();
             ^
../single_include/nlohmann/json.hpp:13567:16: note: candidate template ignored: requirement 'std::is_same<typename std::remove_const<MyType>::type, basic_json_t>::value' was not satisfied [with BasicJsonType = MyType]
    basic_json get() const
               ^
../single_include/nlohmann/json.hpp:13590:19: note: candidate template ignored: requirement 'detail::is_basic_json<MyType>::value' was not satisfied [with BasicJsonType = MyType]
    BasicJsonType get() const
                  ^
../single_include/nlohmann/json.hpp:13640:15: note: candidate template ignored: requirement 'detail::has_from_json<basic_json_t, MyType>::value' was not satisfied [with ValueTypeCV = MyType, ValueType = MyType]
    ValueType get() const noexcept(noexcept(
              ^
../single_include/nlohmann/json.hpp:13691:15: note: candidate template ignored: requirement 'detail::has_non_default_from_json<basic_json_t, MyType>::value' was not satisfied [with ValueTypeCV = MyType, ValueType = MyType]
    ValueType get() const noexcept(noexcept(
              ^
../single_include/nlohmann/json.hpp:13728:17: note: candidate template ignored: requirement 'std::is_pointer<MyType>::value' was not satisfied [with PointerType = MyType]
    PointerType get() noexcept
                ^
../single_include/nlohmann/json.hpp:13740:33: note: candidate template ignored: requirement 'std::is_pointer<MyType>::value' was not satisfied [with PointerType = MyType]
    constexpr const PointerType get() const noexcept
                                ^
  • What is the expected behavior?

The static_assert should fail instead.

  • And what is the actual behavior instead?

The static_assert passes, lying about nlohmann::json being convertible to MyType.

All Clangs fail.

  • Did you use a released version of the library or the version from the develop branch?

Released version.

Yes.

@theodelrieu
Copy link
Contributor

Hello Louis,

I recently merged #1228 that improved the library's SFINAE-correctness, but I forgot about operator T().

There are lots of issues with operator T() described in detail in #958, but it should do the right thing in an unevaluated context anyway, I'll fix it.

theodelrieu added a commit to theodelrieu/json that referenced this issue Sep 12, 2018
* Make the conversion operator SFINAE correct.
* Workaround a GCC bug with some traits in type_traits.hpp
@theodelrieu theodelrieu mentioned this issue Sep 12, 2018
4 tasks
theodelrieu added a commit to theodelrieu/json that referenced this issue Sep 12, 2018
* Make the conversion operator SFINAE correct.
* Workaround a GCC bug with some traits in type_traits.hpp

The first bullet-point implies that every `get`/`get_ptr` be SFINAE
correct as well.
theodelrieu added a commit to theodelrieu/json that referenced this issue Sep 13, 2018
* Make the conversion operator SFINAE correct.
* Workaround a GCC bug with some traits in type_traits.hpp

The first bullet-point implies that every `get`/`get_ptr` be SFINAE
correct as well.
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Sep 17, 2018
@nlohmann
Copy link
Owner

@ldionne Could you check whether #1238 fixes the issue?

theodelrieu added a commit to theodelrieu/json that referenced this issue Sep 20, 2018
* Make the conversion operator SFINAE correct.
* Workaround a GCC bug with some traits in type_traits.hpp

The first bullet-point implies that every `get`/`get_ptr` be SFINAE
correct as well.
@theodelrieu
Copy link
Contributor

@nlohmann I've added a test in unit-udt.cpp with the same use-case, I believe it is safe to merge #1238.

nlohmann added a commit that referenced this issue Sep 29, 2018
@nlohmann nlohmann self-assigned this Sep 29, 2018
@nlohmann nlohmann added this to the Release 3.2.1 milestone Sep 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

3 participants