Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The code used to build with 3.10.2 but fails now #3804

Closed
2 tasks done
me21 opened this issue Oct 20, 2022 · 9 comments
Closed
2 tasks done

The code used to build with 3.10.2 but fails now #3804

me21 opened this issue Oct 20, 2022 · 9 comments
Labels
kind: bug solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)

Comments

@me21
Copy link

me21 commented Oct 20, 2022

Description

I could convert nlohmann::json to a std::variant implicitly with version 3.10.2, but with the latest version, I can't.

With the latest version, the code doesn't build.

I don't know whether it is this way for a reason (I apologize if so), or a regression.

Reproduction steps

Try to build the given code.

Expected vs. actual results

The code used to build with v3.10.2, but it doesn't with the tip of develop branch.

Minimal code example

nlohmann::json test_json;
std::variant<size_t, std::string> val{test_json};

Error messages

error: no matching function for call to 'std::variant<unsigned int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::variant(<brace-enclosed initializer list>)'
[build]                          std::variant<size_t, std::string> val{test_json};
[build]                                                                         ^

Compiler and operating system

Windows 10, GCC 8.3.0 for ARM

Library version

a3e6e26

Validation

@me21 me21 added the kind: bug label Oct 20, 2022
@me21
Copy link
Author

me21 commented Oct 20, 2022

Oh, I found this: #958
It's not recommended to use implicit conversions anyway. I'm sorry.

@me21 me21 closed this as completed Oct 20, 2022
@me21
Copy link
Author

me21 commented Oct 20, 2022

I'm sorry again, on the second thought you were going to remove the support for implicit conversions from JSON in the next major version, so I thought you might still like to look at this.

@me21 me21 reopened this Oct 20, 2022
@me21
Copy link
Author

me21 commented Oct 20, 2022

Okay, I tried to change the code to

nlohmann::json test_json;
std::variant<size_t, std::string> val = test_json.get<std::variant<size_t, std::string>>();

and I still get the error:

error: no matching function for call to 'nlohmann::json_abi_v3_11_2::basic_json<>::get<std::variant<unsigned int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >()'
[build]                          std::variant<size_t, std::string> val = test_json.get<std::variant<size_t, std::string>>();
[build]                                                                                                                   ^

It seems the explicit conversion is also broken.

@me21
Copy link
Author

me21 commented Oct 31, 2022

Based on the discussion in #1261, it was never assumed it should work and worked only by chance?.. I need to explicitly add variant support, correct?..
On another hand, that issue is about serialization, whereas I was interested in deserialization only. So I don't know if it should work, or the operation was never guaranteed.

@me21
Copy link
Author

me21 commented Oct 31, 2022

The breaking commit was a87c188.

@falbrechtskirchinger
Copy link
Contributor

I have to take a closer look to be sure, but here are my initial thoughts:
There's no built-in conversion for std::variant, meaning the compiler will look for any compatible conversion and won't find an unambiguous one.

I need to explicitly add variant support, correct?..

Yes, you need to provide your own to_json/from_json functions to implement the desired conversion behavior.

std::variant<size_t, std::string> val = test_json.get<std::variant<size_t, std::string>>();

This does not change the fact that there's no conversion available. The following lines should work instead:

std::variant<size_t, std::string> val = test_json.get<size_t>();
// or
std::variant<size_t, std::string> val = test_json.get<std::string>();

These calls can of course still fail at runtime, depending on the stored type.

@me21
Copy link
Author

me21 commented Apr 21, 2023

I understand, but the thing is that the code did build and run and work correctly with 3.10.2.
It's like it queried the stored type under the hood and called the correct nlohmann::json::get specialization.

@nlohmann
Copy link
Owner

It seems that it was a bug before to have supported an example with std::variant.

@nlohmann nlohmann added the solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) label Apr 22, 2023
@me21
Copy link
Author

me21 commented Apr 23, 2023

Did it break something? Because it is convenient, looks like a feature to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)
Projects
None yet
Development

No branches or pull requests

3 participants