nlohmann
Fix conversion from empty json to std::optional<>
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).
Is the following really needed?
...\test\src\unit-concepts.cpp(124): ERROR: CHECK( std::is_standard_layout<json>::value ) is NOT correct!
That assertion held true so far. If that changes now, this may indicate a breaking change.
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.
Coverage: 100.0%. Remained the same when pulling 14e33189295399e9ead4d08309b1d1b21f4040d0 on karzhenkov:fix-conv-to-optional into 4c6cde72e533158e044252718c013a48bcff346c on nlohmann:develop.
There are no identical bases. Moreover, direct identical bases are not allowed.
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.
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.
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 likestd::optional<std::string>, then maybe we should not add support forstd::optionalto 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.
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.
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.
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).
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[bot] avatar](https://avatars.githubusercontent.com/in/1724?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
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.
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.
Rebased, but there is a test failure. Codacy test is probably excessively strict. It requires single-argument constructors to be explicit, but even std::optional doesn't meet this. The implicit conversions that the standard implementation allows are also allowed for nlohmann::optional. In fact, one of constructors in question is conditionally explicit, which is also replicated. Is there a possibility to relax codacy requirements?
Here is also a simple example to consider.
Don't worry about Codacity - there is also another test failing: ci_single_binaries fails, because header optional.hpp is not self-contained and seems not to include <optional>.
I hope the includes are arranged properly now. However, ci_single_binaries is cancelled. This is propably caused by wrong test environment, not by actual problem in the code. The test log contains the following:
/bin/sh: 1: SCAN_BUILD_TOOL-NOTFOUND: not found
Oh, I had to make some changes to the CI. See #2981. I will merge this soon. Sorry for this!
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.
This doesn't seem to be fixing the truly optional case because there's no change to NLOHMANN_JSON_FROM macro.
Trying to fill this data structure:
struct KVPair {
std::string name;
std::optional<std::string> value;
};
NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE(KVPair, name, value)
will throw in this case:
json j = json::parse("{ "name" : "foo" }");
KVPair kvp;
j.get_to(kvp); // << throws here, because "value" is not in json at all
Because NLOHMANN_JSON_FROM uses at operator, it'll throw, while I expected that it'll return null-json (the one for which json.is_null() returns true) so that get_to can properly call from_json for std::optional overload.
@jarni-ua You are correct that this doesn't address that particular case. This just addresses using a std::optional<T> as the value half of that. It accepts and produces this format:
json::parse("{ \"name\" : \"foo\", \"value\": null }");
@nlohmann Anything still holding this up? There is a codacy warning about two constructors not being explicit in the optional wrapper class, but they're not explicit in std::optional either.
@karzhenkov This PR is conflicting again. Can you update? @nlohmann Anything besides the merge conflict still outstanding here?
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.
The remaining problems are related to three-way comparisons in C++20. I hope to fix them when I have time.
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.
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?
