json
json copied to clipboard
nlohmann
Add conversion from/to std::optional
Coverage remained the same at 100.0% when pulling 7ffd3ae400d5d42d8b1a5538b8ef505a8de6d6c9 on feature/optional into 68c36963826671d3f3ba157222430109ef932bac on develop.
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.
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.
Yes, the <optional> tests are only executed in C++17 mode. Strangely, not all of them fail, but only a few of them.
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 thatCHECK(std::optional<int>(j_number) == opt_int);calls the expected method. - I have no idea now.
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.
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}
Just wanted to point out that I selectively added this to the 3.9.0 single header and was able to build with visual studio 16.6.4 with no issues.
@kix-mcotton Thanks for reporting! I updated the branch to the latest develop version and will give it another try.
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): https://github.com/nlohmann/json/blob/fedf82c904af5560b47a922b69b23fead01afa23/test/src/unit-conversions.cpp#L1764
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.
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 7df758f41f97a43d1b562579bd260fee94187b9d from #2213).
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.
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.
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.
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.)
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.
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) vsfoo: T|null(nullable) in TypeScript withstrictNullChecksenabledshape(?'foo' => T)(optional) vsshape('foo' => ?T)(nullable) in Hack
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.
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.