Fix conversion from empty json to std::optional<>

[karzhenkov]

Here is possible solution (or workaround) for issues in PR #2117, #2213.
The problem is caused by "wrong" user defined conversion selected by overload resolution. Compiler selects a converting constructor for std::optional<std::string>, while template <...> json::operator ValueType is needed. This behavoir is correct because json can be implicitly converted to std::string. The constructor is unaware about from_json function.
"Non-template" version of conversion operator presented here "adjusts" the resolution process. However, the desired result is achieved for copy initailization only. Direct initialization still selects the converting constructor (this probably cannot be changed).

[karzhenkov]

Is the following really needed?

...\test\src\unit-concepts.cpp(124): ERROR: CHECK( std::is_standard_layout<json>::value ) is NOT correct!

[nlohmann]

That assertion held true so far. If that changes now, this may indicate a breaking change.

[karzhenkov]

Modified json may have two (or more) base classes of the same type, and thus not to be StandardLayout. This may happen if there are identical types in argument pack for optional_converter_helper. Possible effects need to be explored.

[coveralls]

Coverage: 100.0%. Remained the same when pulling 14e3318 on karzhenkov:fix-conv-to-optional into 4c6cde7 on nlohmann:develop.

[karzhenkov]

There are no identical bases. Moreover, direct identical bases are not allowed.

[dota17]

Direct initialization still selects the converting constructor (this probably cannot be changed).

As i said in #2213, this is why an exception is raised when calling std::optional<std::string>(j_null). It is a problem of std::optional not this json library, i think.
IMO, we can add notes in documents to tell people that they should not use std::optional<std::string>(j_null) directly if it still raise an exception.

[dota17]

Weird. In my local env, the exception would not raise. By from CI online logs , it will get the exception.

[karzhenkov]

What is special in your local environment?

[dota17]

ubuntu 18.04
gcc 7.4
cmake 3.10.2

[karzhenkov]

Somewhat has changed since GCC 7.4
https://godbolt.org/z/4MfL8i

[karzhenkov]

There is probably a bug in GCC 7. When used with "-std=c++17" it may perform wrong overload resolution for user-defined conversion. In previous example and also here (with no std::optional involved) it behaves as if the implicit copy constructor is selected, whereas there is a more suitable template constructor. Other major releases of GCC (including older versions) prefer template constructor.

[nlohmann]

IMO, we can add notes in documents to tell people that they should not use std::optional<std::string>(j_null) directly if it still raise an exception.

I do not like this approach. I cannot believe this is an issue in std::optional. It feels like a bug in the library. If we cannot properly support a quite frequent type like std::optional<std::string>, then maybe we should not add support for std::optional to the library in the first place as this would be highly confusing.

[bhardwajs]

IMO, we can add notes in documents to tell people that they should not use std::optional<std::string>(j_null) directly if it still raise an exception.

I do not like this approach. I cannot believe this is an issue in std::optional. It feels like a bug in the library. If we cannot properly support a quite frequent type like std::optional<std::string>, then maybe we should not add support for std::optional to the library in the first place as this would be highly confusing.

Are there specific STL implementations where the issue with std::optional is surfacing? gcc, clang, and MSVC don't always have the same implementations.

[karzhenkov]

It seems the issue is caused by std::optional intented desing and language standard, not by implementation features.
Perhaps it would be better to disable implicit conversions from json to std::string, bool, etc. When doing so, the overload resolution will prefer conversion operator defined in json over converting constructor of std::optional. The conversion operator will call the proper from_json function. But, оf course, this is a breaking change.
On the other hand, it may be sufficient to 'disable' direct initialization by throwing an exception. A bit ugly, but maybe not so bad.

[karzhenkov]

MSVC has bad std::is_standard_layout. Here is a sample.
By the way, I see no reason why StandardLayoutType concept may be useful when using json.

[dota17]

What is more, std::optional<std::string>(j_null) still raises an exception.
I added logs to check SECTION("string") and bool, number. Unfortunally, I found that they still didn't call the expected method - from_json(basic_json, std::optional).

[karzhenkov]

Microsoft doesn't consider their implemenation wrong. Their story is about empty base optimization, backward binary compatibility and non-standard extension __declspec(empty_bases) which can be used to achieve standard behavior.

[stale]

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]

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]

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.

[nlohmann]

Same question as with #2117 - how to proceed here?

[karzhenkov]

Here is a "better" optional that probably allows to achieve the desired behavior. The nlohmann::optional class is derived from its standard counterpart and defines some constructors. The goal of these definitions is to change the preferred conversion performed by converting constructor. If the object being converted has operator optional<T>, this operator will be called by corresponding constructor. Other constructors are defined to resemble std::optional in any other initialization scenario.

[gregmarr]

@karzhenkov This PR is conflicting again. Can you update?
@nlohmann Anything besides the merge conflict still outstanding here?

[falbrechtskirchinger]

I'd like to evaluate this branch in one of my projects to make sure there aren't any ugly side effects. Afterward, I'll come back for a more in-depth review.

[falbrechtskirchinger]

Would it be better to just move the *_CONVERSION_WARNING macros into the #if JSON_HAS_CPP_17 block?
You're getting this warning because, in the C++11 build, the macros aren't used.

[karzhenkov]

I considered it, but I think it's hardly much better. I preferred a slightly more concise variant.

[karzhenkov]

It's not clear if std::optional has noexcept default constructor in GCC 8.1. Actually, it isn't declared noexcept there. This can be checked in the example at godbolt. However, after the local variable definition in the example is uncommented, std::optional default constructor looks noexcept. I guess, there are both library and compiler bugs.

[nlohmann]

FYI: I suppressed the warnings for non-explicit constructors in Codacy.

[karzhenkov]

The remaining problems are related to three-way comparisons in C++20. I hope to fix them when I have time.

[karzhenkov]

The problem boils down to the following example, which complies in C++17 but not in C++20:

#include <optional>

struct opt : std::optional<int> {};

bool operator == (const opt&, const opt&);
bool operator >= (const opt&, const opt&);

int main()
{
    sizeof(opt() == opt());
    sizeof(opt() >= opt());
}

If any of the operator declarations is removed along with the corresponding check, the code compiles regardless of the language standard.

[falbrechtskirchinger]

Digging a little bit deeper, the code triggers a recursion in __partially_ordered_with via std::operator <=>.
For std::optional, that recursion is broken via the constraint __is_optional_v.

Adding a specilization to std (for testing purposes only, of course!) makes the code compile without issue.

namespace std {
template<>
inline constexpr bool __is_optional_v<opt> = true;
}

How do we work around this?

[karzhenkov]

How do we work around this?

It looks like a defect in the C++20 standard (or maybe in the standard library implementation). I think it is impossible to correctly define comparisons for a class derived from std::optional in C++20. My initial approach was to adjust the class constructors, but that doesn't seem viable now. Another option is to completely redefine the class, which also has disadvantages. All functionality needs to be duplicated and maintained; the issues of the interchangeability needs to be solved.