implicit conversion of return json to std::optional no longer implicit

[risa2000]

Description

When playing with current develop branch I noticed that part of my code no longer compiles. An example for reproduction follows below.

Tested on Windows with both MSVC (VS2022) and clang-cl, compiled with C++20 defined.

The fix seems easy, simply changing the return statement to:

...
        return std::optional<json>(valRoot.at(CFG_PROP_DEFAULT));
...

The code compiled fine in v3.12.0, so I guess it may be related to recent changes, maybe in #4742?

Not sure if this is a bug, or intended behavior.

Reproduction steps

Build the sample code.

Expected vs. actual results

The code should compile?

Minimal code example

#include <nlohmann/json.hpp>

#include <optional>

using json = nlohmann::json;
inline constexpr const char* CFG_PROP_DEFAULT = "default";

std::optional<json> GetValue(const json& valRoot)
{
    if (valRoot.contains(CFG_PROP_DEFAULT)) {
        return valRoot.at(CFG_PROP_DEFAULT);
    }
    return std::nullopt;
}

int main(int argc, char* argv[])
{
    json jval{};
    auto result = GetValue(jval);

    return 0;
}

Error messages

>------ Build All started: Project: json2_test, Configuration: x64_Debug_xClang-x64_Debug_xClang ------
  [1/2] Building CXX object CMakeFiles\json2_test.dir\json2_test.cpp.obj
  FAILED: CMakeFiles/json2_test.dir/json2_test.cpp.obj 
  D:\Dev\LLVM\bin\clang-cl.exe  /nologo -TP  -imsvcD:\Work_OSS\_risa2000\nlohmann_json\include --target=amd64-pc-windows-msvc -fdiagnostics-absolute-paths  /DWIN32 /D_WINDOWS /GR /EHsc /Zi /Ob0 /Od /RTC1 -std:c++20 -MDd /showIncludes /FoCMakeFiles\json2_test.dir\json2_test.cpp.obj /FdCMakeFiles\json2_test.dir\ -c -- D:\Work\vsprojects\_tests\json2_test\json2_test.cpp
D:\Work\vsprojects\_tests\json2_test\json2_test.cpp(11,16): error : conversion from 'const value_type' (aka 'const nlohmann::basic_json<>') to 'std::optional<json>' (aka 'optional<basic_json<>>') is ambiguous
     11 |         return valRoot.at(CFG_PROP_DEFAULT);
        |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
  C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.44.35207\include\optional(270,54): note: candidate constructor [with _Ty2 = const nlohmann::basic_json<> &, $1 = 0]
    270 |     constexpr explicit(!is_convertible_v<_Ty2, _Ty>) optional(_Ty2&& _Right)
        |                                                      ^
  D:\Work_OSS\_risa2000\nlohmann_json\include\nlohmann\json.hpp(1924,55): note: candidate function [with ValueType = std::optional<nlohmann::basic_json<>>, $1 = 0]
   1924 |                                         JSON_EXPLICIT operator ValueType() const
        |                                                       ^
  1 error generated.
  ninja: build stopped: subcommand failed.

Build All failed.

Compiler and operating system

Windows, compilers Microsoft (R) C/C++ Optimizing Compiler Version 19.44.35222 for x64 (VS2022), clang version 21.1.8

Library version

develop

Validation

• The bug also occurs if the latest version from the develop branch is used.
• I can successfully compile and run the unit tests.

[gregmarr]

What is the type of valRoot?

[risa2000]

I am not sure what do you mean - it is const nlohmann::json& parameter of GetValue function.

[gregmarr]

Sorry, missed it when reading the code.

[gregmarr]

I would say this part of the reason why the implicit conversions are bad, it makes things ambiguous. If you set JSON_USE_IMPLICIT_CONVERSIONS to 0, it should fix it, but it may break other parts of your code that use implicit conversions.

[risa2000]

I guess I am not understanding the idea behind the "implicit conversions" correctly.

nlohmann::json::at should return reference or const_reference type (supposedly the latter in this particular case).
Then I would expect the return value will be std::optional instantiated with the aforementioned const_reference, so forcing a copy ctor on the reference.

How the implicit conversions for nlohmann::json could lead to an ambiguity at this point, when the return type does not need any conversion at all? (std::optional encapsulates plain nlohmann::json)

And why this ambiguity is fixed by instantiating the exactly same type (std::optional<nlohmann::json>) explicitly?

Could it be a "bug" in std::optional implementation in Microsoft C++ std library?

EDIT: On the other hand, it worked as expected in v3.12.0.

[gregmarr]

error : conversion from 'const nlohmann::basic_json<>' to 'std::optional<basic_json<>>') is ambiguous
note: candidate constructor [with _Ty2 = const nlohmann::basic_json<> &, $1 = 0]
      constexpr explicit(!is_convertible_v<_Ty2, _Ty>) optional(_Ty2&& _Right)
note: candidate function [with ValueType = std::optional<nlohmann::basic_json<>>, $1 = 0]
                                          JSON_EXPLICIT operator ValueType() const

So std::optional<basic_json> knows how to construct itself from const basic_json &, and basic_json knows how to construct a std::optional<basic_json> from itself using an implicit conversion operator. The compiler can't tell which of those two is a better match. By making one of those two methods disappear, by making the conversion operator explicit, it's no longer ambiguous.

EDIT: On the other hand, it worked as expected in v3.12.0.

In 3.12.0, the support for implicit conversion to std::optional<> was partially disabled. I think that's why it worked.

@nlohmann It might be worth disabling that from_json when T is a basic_json type. I can think of no reason why creating a std::optional<basic_json> from basic_json should go through that function.

json/include/nlohmann/detail/conversions/from_json.hpp

Lines 52 to 53 in 6c03aaf

	template<typename BasicJsonType, typename T>
	void from_json(const BasicJsonType& j, std::optional<T>& opt)

[nlohmann]

@gregmarr You mean replacing

template<typename BasicJsonType, typename T> void from_json(const BasicJsonType& j, std::optional<T>& opt)

with

template <typename BasicJsonType, typename T,
          typename std::enable_if<!nlohmann::detail::is_basic_json<T>::value, int>::type = 0>
void from_json(const BasicJsonType& j, std::optional<T>& opt)

?

[gregmarr]

Yes, I think that would work.

[nlohmann]

Assuming the CI does not find an issue, #5052 should fix the issue. Please take a look!

[risa2000]

@nlohmann Works for me.