Add conversion from/to std::optional #2117

nlohmann · 2020-05-16T17:42:44Z

Closes #1749.

Use JSON_HAS_CPP_17 only after it has been defined

…/nlohmann/json into feature/optional

coveralls · 2020-05-16T23:18:55Z

Coverage remained the same at 100.0% when pulling 7ffd3ae on feature/optional into 68c3696 on develop.

…/nlohmann/json into feature/optional

dota17 · 2020-05-26T03:09:21Z

I am working for checking these CI tasks.

Restart the travis CI tasks, and then they would be fine.
This Appveyor CI task may be ok after we restart, it seems like a network error.

And about other failed Appveyor CI tasks, I find the difference is CXX_FLAGS=/permissive- /std:c++latest. Maybe it is the problem, and I would try to fix it.

nlohmann · 2020-05-26T03:44:13Z

I restarted all jobs.

nlohmann · 2020-05-26T11:08:40Z

The same AppVeyor jobs keep failing. Any ideas?

dota17 · 2020-05-27T03:08:02Z

More Info :
The std::optional code maybe not included. I had added logs in unit-conversions.cpp to check if the testcases of std::optional had been run:

    ....
    SECTION("traditional enum")
    {
        // check normal testcase had been run, result : print, run
        std::cout << "traditional enum" << std::endl; 
        ...
    }
#ifdef JSON_HAS_CPP_17
TEST_CASE("std::optional")
{
    SECTION("null")
    {
        // check the C++17 and testcase, reslut : not print, not run
        std::cout << "null" << std::endl; 
....

It seems that all successful jobs hadn't compile the std::optional code so that there are no compiler errors and the testcases hadn't been run. If compiled, maybe they would had same errors.

nlohmann · 2020-05-27T05:22:25Z

Yes, the <optional> tests are only executed in C++17 mode. Strangely, not all of them fail, but only a few of them.

dota17 · 2020-05-27T07:15:50Z

Yes, the tests are only executed in C++17 mode. Strangely, not all of them fail, but only a few of them.

I find that the <optional> tests are only executed in VS 2017 with CXX_FLAGS=/permissive- /std:c++latest. I think they should be also executed in VS2019, but didn't. Thus, some success may be fake because they didn't run the tests.

What's more, in VS 2017 with CXX_FLAGS=/permissive- /std:c++latest jobs, they didn't call the method from_json(const BasicJsonType& j, std::optional<T<& opt).

The actual behavior:

CHECK(std::optional<std::string>(j_string) == opt_string);  // call from_json(const BasicJsonType& j, ConstructibleStringType& s)
CHECK(std::optional<bool>(j_bool) == opt_bool); // call from_json(const BasicJsonType& j, typename BasicJsonType::boolean_t& b) 
CHECK(std::optional<int>(j_number) == opt_int); // call from_json(const BasicJsonType& j, ArithmeticType& val)

And the test SECTION("null") didn't call the expected method either.

We should ensure which envs/jobs would execute the <optional> tests.
It seems that the implement of from_json(const BasicJsonType& j, std::optional<T<& opt) doesn't work. We should fix it so that CHECK(std::optional<int>(j_number) == opt_int); calls the expected method. - I have no idea now.

nlohmann · 2020-05-27T08:54:44Z

Since I don't use MSVC myself, I cannot debug this further. I will drop this feature from 3.8.0 to proceed. Feel free to dig in, but I will not pursue this issue in the moment.

stale · 2020-07-11T02:52:13Z

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.

karzhenkov · 2020-08-18T09:22:19Z

I guess that some tests at Travis are successful due to a bug in GCC and Clang. Particularly, these compilers accept the following (while MSVC doesn't):

json/test/src/unit-conversions.cpp

Line 1764 in fedf82c

CHECK(std::vector<std::optional<int>>(j_array) == opt_array);

Consider the simplified example:

#include <vector>

struct J
{
    template <typename T> operator T () const;
};

void test()
{
    J j;
    std::vector<int> v(j);
}

With -std=c++14 both GCC and Clang correctly diagnose the ambiguity. Candidate functions are converting constructors:

vector<_Tp, _Alloc>::vector(vector<_Tp, _Alloc>&&);
vector<_Tp, _Alloc>::vector(const vector<_Tp, _Alloc>&);
vector<_Tp, _Alloc>::vector(const allocator_type&);
vector<_Tp, _Alloc>::vector(size_type, const allocator_type& = allocator_type());
vector<_Tp, _Alloc>::vector(initializer_list<_Tp>, const allocator_type& = allocator_type());

Note that specializations of J::operator T() const are not candidates for overload resolution.

When matching the first parameter against the initalizer of type J all the candidates require an implicit user-defined conversion,
and there is no best one among them. All applicable specializations of J::operator T() const provide exact match.

C++17 introduces a special case - when the initializer is prvalue of the same type as the object being initialized, no overload resolution is perfomed at all (and no conversion). We have different types, so this special case doesn't apply here; overload resolution should be performed as before.

However, with -std=c++17 both GCC and Clang erroneously select the second candidate from the list above. This bug was reported for GCC 7 and seems to be fixed in GCC 8, but arises again in GCC 9 and later.

Exploiting a compiler bug is not as good choise. Perhaps support for std::optional should be deferred until compilers become more mature. Other possibility is to find suitable workarounds. Interestingly, the workaround is needed for MSVC, which exhibits a better conformance here.

karzhenkov · 2020-08-19T18:47:09Z

The success at Travis has one more reason. Conversion tests for std::optional are not executed. There are three test cases in unit-conversions.cpp whereas job log contains only two of them. Test case for std::optional requires C++17 but is excluded despite of CXXFLAGS=-std=c++2a.

This can be explained by the fact that the desired С++ standard is specified twice: in top-level CMakeLists.txt (compile feature cxx_std_11) and through aforementioned CXXFLAGS in .travis.yml. CMake probably doesn't analyze user flags, so the compiler command line includes both options. The collision is resolved to C++11 (this is determined by order of options).

Problem can be fixed by using cmake -DCMAKE_CXX_STANDARD=... in Travis configuration instead of CXXFLAGS (somewhat like 7df758f from #2213).

nlohmann · 2020-08-20T09:46:03Z

Wow, thanks for the detailed analysis! What do you propose to do?

karzhenkov · 2020-08-20T12:00:38Z

I don't see any way to achieve desired conversion sequence for direct initialization when implicit conversions of json are enabled. We have no control over constructors of "external" target classes such as std::vector. Conversion operators defined in json are included in the candidate set only for copy initialization which uses a different syntax, e. g. std::vector<...> v = j instead of std::vector<...>(j).

The above only applies to "conforming" compilers, but each compiler vendor has their own approach to interpretation of standard. It will take time to reach a consensus on subtle topics (and on some trivial topics too). Anyway, it is dangerous to rely on the "new" overload resolution algorithm provided by GCC and Clang with -std=c++17. Language improvements should not change the meaning of the program. Meanwhile, the example discussed on Bugzilla compiles to executable that produces different output when compiled with -std=c++14 or -std=c++17. I suspect that there is a significant difference between GCC 8.3 and GCC 8.4 (the latter is used by Travis, but is not avaliaible at godbolt).

Maybe it is reasonable to support std::optional only with JSON_USE_IMPLICIT_CONVERSIONS defined as 0 for a while.

nlohmann · 2020-08-24T18:35:43Z

Maybe it is reasonable to support std::optional only with JSON_USE_IMPLICIT_CONVERSIONS defined as 0 for a while.

Yes, I think this is a reasonable approach.

stale · 2020-10-04T02:35:07Z

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.

…re/optional � Conflicts: � test/src/unit-conversions.cpp

karzhenkov · 2021-01-01T12:26:11Z

test/src/unit-conversions.cpp

+        std::optional<std::string> opt_null;
+
+        CHECK(json(opt_null) == j_null);
+        CHECK(std::optional<std::string>(j_null) == std::nullopt);


Initialization of optional<T> from empty json should be considered as a type_error:

Suggested change

CHECK(std::optional<std::string>(j_null) == std::nullopt);

CHECK_THROWS_AS(std::optional<std::string>(j_null), json::type_error&);

The reason is as follows. If json is implicitly convertible to T, then initialization of optional<T> from json almost necessarily goes through the "forwarding" constructor:

template <class U> optional(U&& value); // unrelated details omitted

This constructor provides the best parameter/argument match by value category, constness and a type. Its parameter binds directly to json in any form. The only "weakness" of the forwarding constructor is that it is a template. A conversion function that would be considered as a better candidate for overload resolution must be non-templated, but this is obviously unacceptable.

The forwarding constructor converts json to T (initializes the latter from the former) and stores the result, so the constructed optional<T> cannot be empty. Such a conversion from empty json to T is a type error (even if T itself is optional<X>, by induction).

The non-standard overload resolution performed by GCC and Clang when enabling C++17 does not change the outcome. The only way to achieve the "natural" behavior is to tune constructors of std::optional<T>, which is beyond the scope of the project.

There is also the usual way to explicitly convert json to optional<T>. The following will work:

Suggested change

CHECK(std::optional<std::string>(j_null) == std::nullopt);

CHECK(j_null.get<std::optional<std::string>>() == std::nullopt);

Essentially, there is nothing new here. @dota17 proposed the same in #2213.
However, this approach (treat such initialization as type_error) introduces some inconsistency:

std::optional<int> opt1 = json().get<std::optional<int>>(); // ok: std::nullopt std::optional<int> opt2 = std::optional<int>(json()); // throws type_error std::optional<int> opt3 = json(); // throws type_error

Perhaps it makes sense to throw type_error in the first case too?
The "natural" conversion could be explicitly implemented by dedicated member function:

template <typename T> std::optional<T> get_optional() const;

Moreover, as it turns out, this dedicated member function is the only meaningful thing nlohmann::basic_json can provide to support std::optional. Overloads of get<std::optional<T>> should probably be disabled to prevent confusion.

I think the behavior is counter-intuitive. JSON's null is usually considered as "unset" value (in contrast to an empty value). Therefore, I would expect null to convert to std::nullopt of any optional type.

karzhenkov · 2021-01-01T12:31:21Z

test/src/unit-conversions.cpp

+        std::vector<std::optional<int>> opt_array = {{1, 2, std::nullopt}};
+
+        CHECK(json(opt_array) == j_array);
+        CHECK(std::vector<std::optional<int>>(j_array) == opt_array);


Direct initialization of vector<...> is not suitable here:

Suggested change

CHECK(std::vector<std::optional<int>>(j_array) == opt_array);

CHECK(j_array.get<std::vector<std::optional<int>>>() == opt_array);

Such kind of initialization of vector<...> has not been tested even with simple item types and actually leads to a compile error for the reason considered earlier. For example, the following will not compile unless using GCC or Clang with C++17 enabled (see https://godbolt.org/z/oeooPG):

json j({1, 2, 3}); std::vector<int> v1 = j; // Copy initialization: ok auto v2 = std::vector<int>(j); // Direct initialization: compile error

karzhenkov · 2021-01-01T12:31:24Z

test/src/unit-conversions.cpp

+        std::map<std::string, std::optional<int>> opt_object {{"one", 1}, {"two", 2}, {"zero", std::nullopt}};
+
+        CHECK(json(opt_object) == j_object);
+        CHECK(std::map<std::string, std::optional<int>>(j_object) == opt_object);


Direct initialization of map<...> has the same trouble as with vector<...> above:

Suggested change

CHECK(std::map<std::string, std::optional<int>>(j_object) == opt_object);

CHECK(j_object.get<std::map<std::string, std::optional<int>>>() == opt_object);

.travis.yml

include/nlohmann/detail/macro_scope.hpp

…re/optional

UndarkAido · 2021-01-14T07:24:15Z

I don't know exactly how this is being implemented but my ideal implementation would be for a field not in the from_json to be interpreted as std::nullopt and for a std::nullopt to not appear in the to_json result.

(I'm trying to parse object from Discord's API and there's lots of optional fields. Right now I can't use the NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE because the fields just aren't there to parse.)

nlohmann · 2021-01-23T10:35:36Z

I'm not sure how to proceed with this PR. Any ideas?

karzhenkov · 2021-01-31T19:09:49Z

The primary issue we have is the conversion of null json values to std::optional<T>. To fix the counter-intuitive behavior observed here (the cases that throw type_error) the "improvement" of std::optional is needed. Perhaps it is worth to consider a "better" version of optional as a part of this library (see #2229). Otherwise, we will have to adopt the existing counter-intuitive behavior, whether with or without this PR.

fredemmott · 2021-02-15T21:57:02Z

I don't think this can be directly resolved without edge cases as 'may be null', and 'may be omitted' are distinct concepts in JSON, but people expect std::optional<> to work with both - and they have conflicting requirements.

I think these need to be separate types in C++ - perhaps a new nlohmann::nullable<T> type which acts essentially the same way as std::optional<> except for JSON encoding:

a nullable field is mandatory, but may be null
an optional field may be entirely missing
an optional field may not be null, unless it's an std::optional<nlohmann::nullable<T>>

This would be similar to:

foo ?: T (optional) vs foo: T|null (nullable) in TypeScript with strictNullChecks enabled
shape(?'foo' => T) (optional) vs shape('foo' => ?T) (nullable) in Hack

stale · 2022-01-09T02:24:40Z

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.

fsandhei · 2023-04-24T13:15:46Z

Is there any updates on this?

nlohmann · 2023-04-24T13:29:57Z

Unfortunately not - any help welcome!

fsandhei · 2023-05-07T14:14:39Z

If I may ask, what is the current blocker for this to go in to the library?

I see that the issues with overload resolutions taken up by @karzhenkov seem to still not be addressed by GCC/Clang.

From what I understand there is still also discussions about the case of initializing a std::optional from an empty JSON object.

Also the issues with implicit conversions hell with conversion operators / constructors seem avoidable (?) through JSON_USE_IMPLICIT_CONVERSIONS = 0

I would like to try to contribute to this to help, but I have to admit I'm unsure what I can help with.

nlohmann · 2023-05-07T15:03:29Z

The branch first needs to be rebased to the current develop branch.

nlohmann and others added 8 commits November 23, 2019 00:21

🚧 conversions for std::optional

df30a0e

🏁 fix <optional> inclusion

1359c56

💚 overwork tests

713038f

Use JSON_HAS_CPP_17 only after it has been defined

1b846c9

Merge pull request #1872 from remedi/feature/optional

a2fea87

Use JSON_HAS_CPP_17 only after it has been defined

⚗️ set CXXFLAGS

6b31375

Merge branches 'develop' and 'feature/optional' of https://github.com…

71c80cc

…/nlohmann/json into feature/optional

✅ update tests

007c6c4

nlohmann added the release item: ✨ new feature label May 16, 2020

nlohmann added this to the Release 3.8.0 milestone May 16, 2020

nlohmann self-assigned this May 16, 2020

nlohmann added 2 commits May 17, 2020 13:58

🏁 include right <optional> header

f25bf31

Merge branches 'develop' and 'feature/optional' of https://github.com…

b61d563

…/nlohmann/json into feature/optional

nlohmann mentioned this pull request May 20, 2020

[C++17] Allow std::optional to convert to nlohmann::json #1749

Closed

nlohmann removed this from the Release 3.8.0 milestone May 27, 2020

nlohmann mentioned this pull request Jun 4, 2020

/Zc:__cplusplus leads to C2416 #1695

Closed

nlohmann added the state: help needed the issue needs help to proceed label Jun 8, 2020

This was referenced Jun 24, 2020

fix problems in PR2117 dota17/json#16

Open

Fix problems in PR2117 #2213

Closed

json constructor does not support std::optional #2214

Closed

karzhenkov mentioned this pull request Jun 28, 2020

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

Draft

karzhenkov mentioned this pull request Aug 20, 2020

add travis task : compile and test under C++17 #2241

Closed

karzhenkov mentioned this pull request Aug 21, 2020

Allow to use string_view from std::experimental #2364

Closed

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 Oct 4, 2020

nlohmann removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Oct 5, 2020

nlohmann added 2 commits December 21, 2020 20:59

Merge branch 'develop' of https://github.com/nlohmann/json into featu…

5e55158

…re/optional � Conflicts: � test/src/unit-conversions.cpp

Merge branch 'develop' of https://github.com/nlohmann/json into featu…

ebb63bd

…re/optional � Conflicts: � test/src/unit-conversions.cpp

karzhenkov suggested changes Jan 1, 2021

View reviewed changes

karzhenkov added a commit to karzhenkov/json that referenced this pull request Jan 1, 2021

Reset to current state of PR nlohmann#2117 (ebb63bd)

44d05ab

nlohmann added 3 commits January 3, 2021 20:10

♻️ do not include experimental headers

f1913fe

🔥 remove useless line

384442e

Merge branch 'develop' of https://github.com/nlohmann/json into featu…

7ffd3ae

…re/optional

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

stale bot removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Apr 24, 2023

nlohmann added the please rebase Please rebase your branch to origin/develop label May 7, 2023

fsandhei mentioned this pull request May 20, 2023

feat: Rebase feature/optional to develop #4036

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conversion from/to std::optional #2117

Add conversion from/to std::optional #2117

nlohmann commented May 16, 2020

coveralls commented May 16, 2020 •

edited

dota17 commented May 26, 2020

nlohmann commented May 26, 2020

nlohmann commented May 26, 2020

dota17 commented May 27, 2020 •

edited

nlohmann commented May 27, 2020

dota17 commented May 27, 2020

nlohmann commented May 27, 2020

stale bot commented Jul 11, 2020

karzhenkov commented Aug 18, 2020 •

edited

karzhenkov commented Aug 19, 2020 •

edited

nlohmann commented Aug 20, 2020

karzhenkov commented Aug 20, 2020 •

edited

nlohmann commented Aug 24, 2020

stale bot commented Oct 4, 2020

karzhenkov Jan 1, 2021

karzhenkov Jan 1, 2021 •

edited

karzhenkov Jan 2, 2021 •

edited

karzhenkov Jan 2, 2021

nlohmann Jan 8, 2021

karzhenkov Jan 1, 2021

karzhenkov Jan 1, 2021

UndarkAido commented Jan 14, 2021

nlohmann commented Jan 23, 2021

karzhenkov commented Jan 31, 2021

fredemmott commented Feb 15, 2021 •

edited

stale bot commented Jan 9, 2022

fsandhei commented Apr 24, 2023

nlohmann commented Apr 24, 2023

fsandhei commented May 7, 2023 •

edited

nlohmann commented May 7, 2023

	CHECK(std::optional<std::string>(j_null) == std::nullopt);
	CHECK_THROWS_AS(std::optional<std::string>(j_null), json::type_error&);

	CHECK(std::optional<std::string>(j_null) == std::nullopt);
	CHECK(j_null.get<std::optional<std::string>>() == std::nullopt);

	CHECK(std::vector<std::optional<int>>(j_array) == opt_array);
	CHECK(j_array.get<std::vector<std::optional<int>>>() == opt_array);

	CHECK(std::map<std::string, std::optional<int>>(j_object) == opt_object);
	CHECK(j_object.get<std::map<std::string, std::optional<int>>>() == opt_object);

Add conversion from/to std::optional #2117

Are you sure you want to change the base?

Add conversion from/to std::optional #2117

Conversation

nlohmann commented May 16, 2020

coveralls commented May 16, 2020 • edited

dota17 commented May 26, 2020

nlohmann commented May 26, 2020

nlohmann commented May 26, 2020

dota17 commented May 27, 2020 • edited

nlohmann commented May 27, 2020

dota17 commented May 27, 2020

nlohmann commented May 27, 2020

stale bot commented Jul 11, 2020

karzhenkov commented Aug 18, 2020 • edited

karzhenkov commented Aug 19, 2020 • edited

nlohmann commented Aug 20, 2020

karzhenkov commented Aug 20, 2020 • edited

nlohmann commented Aug 24, 2020

stale bot commented Oct 4, 2020

karzhenkov Jan 1, 2021

Choose a reason for hiding this comment

karzhenkov Jan 1, 2021 • edited

Choose a reason for hiding this comment

karzhenkov Jan 2, 2021 • edited

Choose a reason for hiding this comment

karzhenkov Jan 2, 2021

Choose a reason for hiding this comment

nlohmann Jan 8, 2021

Choose a reason for hiding this comment

karzhenkov Jan 1, 2021

Choose a reason for hiding this comment

karzhenkov Jan 1, 2021

Choose a reason for hiding this comment

UndarkAido commented Jan 14, 2021

nlohmann commented Jan 23, 2021

karzhenkov commented Jan 31, 2021

fredemmott commented Feb 15, 2021 • edited

stale bot commented Jan 9, 2022

fsandhei commented Apr 24, 2023

nlohmann commented Apr 24, 2023

fsandhei commented May 7, 2023 • edited

nlohmann commented May 7, 2023

coveralls commented May 16, 2020 •

edited

dota17 commented May 27, 2020 •

edited

karzhenkov commented Aug 18, 2020 •

edited

karzhenkov commented Aug 19, 2020 •

edited

karzhenkov commented Aug 20, 2020 •

edited

karzhenkov Jan 1, 2021 •

edited

karzhenkov Jan 2, 2021 •

edited

fredemmott commented Feb 15, 2021 •

edited

fsandhei commented May 7, 2023 •

edited