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

Deserialization: if class is_constructible from std::string wrong from_json overload is being selected, compilation failed #3171

Closed
4 of 5 tasks
BakhmutR opened this issue Dec 2, 2021 · 5 comments · Fixed by #3427
Assignees
Labels
kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@BakhmutR
Copy link

BakhmutR commented Dec 2, 2021

When some hierarchy exists and base class has overload for from_json function, but if child is constructible from string compilation failing because compiler selecting overload void from_json(const BasicJsonType& j, ConstructibleStringType& s) over function that was prepared for base class void from_json(const json& j, Base& b)

What is the issue you have?

As described above, and looks like that on previous release versions it was working as expected. (Code compiles and deserialization was correct).
If add specific from_json overload for derived class, everything is working as expected.

Bug can be reproduced on latest develop branch, checked from 3.9.1 to 3.10.3 code compiles as expected.

Looks like this is regression commit.

Please describe the steps to reproduce the issue.

Can you provide a small but working code example?

https://godbolt.org/z/e75a67oTj

What is the expected behavior?

Compilation should be successful, base class overload should be selected over overload for classes that can be constructed from string.

And what is the actual behavior instead?

Compilation failed.
But if specific from_json overload for derived class was added, everything is working as expected.

Which compiler and operating system are you using?

  • Compiler: clang version 11.0.0, clang version 9.0.0, g++ 11.2.0
  • Operating system: Ubuntu 18.04

Which version of the library did you use?

  • latest release version 3.10.4
  • other release - please state the version: 3.9.1 - 3.10.3
  • the develop branch

If you experience a compilation error: can you compile and run the unit tests?

  • yes
  • no - please copy/paste the error message below
opt/compiler-explorer/libs/nlohmann_json/trunk/single_include/nlohmann/json.hpp:4042:7: error: no viable overloaded '='
    s = *j.template get_ptr<const typename BasicJsonType::string_t*>();
    ~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/compiler-explorer/libs/nlohmann_json/trunk/single_include/nlohmann/json.hpp:4401:16: note: in instantiation of function template specialization 'nlohmann::detail::from_json<nlohmann::basic_json<>, B, 0>' requested here
        return from_json(j, std::forward<T>(val));
               ^
/opt/compiler-explorer/libs/nlohmann_json/trunk/single_include/nlohmann/json.hpp:5060:9: note: in instantiation of function template specialization 'nlohmann::detail::from_json_fn::operator()<nlohmann::basic_json<>, B &>' requested here
        ::nlohmann::from_json(std::forward<BasicJsonType>(j), val);
        ^
/opt/compiler-explorer/libs/nlohmann_json/trunk/single_include/nlohmann/json.hpp:20583:36: note: in instantiation of function template specialization 'nlohmann::adl_serializer<B>::from_json<const nlohmann::basic_json<> &, B>' requested here
        JSONSerializer<ValueType>::from_json(*this, ret);
                                   ^
/opt/compiler-explorer/libs/nlohmann_json/trunk/single_include/nlohmann/json.hpp:20726:16: note: in instantiation of function template specialization 'nlohmann::basic_json<>::get_impl<B, 0>' requested here
        return get_impl<ValueType>(detail::priority_tag<4> {});
               ^
<source>:17:8: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'const nlohmann::basic_json<>::string_t' (aka 'const std::basic_string<char>') to 'const B' for 1st argument
struct B : public A
@stale
Copy link

stale bot commented Jan 9, 2022

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 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
@DatCaptainHorse
Copy link

Bad bot! I've come across the same issue.

@stale 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 Jan 12, 2022
@nickanthony-dgl
Copy link

Same issue here with Eigen::Matrix

@falbrechtskirchinger
Copy link
Contributor

0e694b4 strikes again. There are now several bugs related to this commit.

@falbrechtskirchinger
Copy link
Contributor

falbrechtskirchinger commented Apr 6, 2022

I have a fix and am working on unit tests.

@nickanthony-dgl Could you share a minimal example demonstrating your issue? I'm not sure how it's related given we're talking about classes constructible from string.
Edit: I assume your case involves std::vector? Similar but different part of the code.
Edit 2: Nevermind. Saw your comment in #3267 and understand your issue.

falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue Apr 6, 2022
Constrain from_json() overload for ConstructibleStringType to not accept
json_ref and require assignability.

Re-enable C++14 tests on Clang <4.0.

Fixes nlohmann#3171, nlohmann#3312, nlohmann#3384.
Maybe fixes nlohmann#3267.
falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue Apr 6, 2022
Constrain from_json() overload for ConstructibleStringType to not accept
json_ref and require it to be assignable from basic_json::string_t.

Re-enable C++14 tests on Clang <4.0.

Fixes nlohmann#3171.
Fixes nlohmann#3267.
Fixes nlohmann#3312.
Fixes nlohmann#3384.
falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue Apr 7, 2022
Constrain from_json() overload for StringType to not accept json_ref and
require it to be assignable, instead of constructible, from
basic_json::string_t.

Re-enable C++14 tests on Clang <4.0.

Fixes nlohmann#3171.
Fixes nlohmann#3267.
Fixes nlohmann#3312.
Fixes nlohmann#3384.
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Apr 7, 2022
@nlohmann nlohmann added this to the Release 3.11.0 milestone Apr 8, 2022
@nlohmann nlohmann self-assigned this Apr 8, 2022
nlohmann pushed a commit that referenced this issue Apr 8, 2022
Constrain from_json() overload for StringType to not accept json_ref and
require it to be assignable, instead of constructible, from
basic_json::string_t.

Re-enable C++14 tests on Clang <4.0.

Fixes #3171.
Fixes #3267.
Fixes #3312.
Fixes #3384.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
5 participants