converting json to vector of type with templated constructor

[dvirtz]

Bug Report

The following code fails to compile when uncommenting line 16:

#include <nlohmann/json.hpp>

namespace nl = nlohmann;

struct S {
  S() = default;
  template<typename T> S(const T& t) {}
};

void from_json(const nl::json& j, S& s) {
}

int main() {
  nl::json j;
  auto s = j.get<S>();
  //auto v = j.get<std::vector<S>>();
};

The error I get is

json.hpp(1360): error C2338: could not find from_json() method in T's namespace

The cause seems to stem from the check for std::is_convertible<BasicJsonType, typename CompatibleArrayType::value_type>::value on void from_json(const BasicJsonType& j, CompatibleArrayType& arr).

Since you do not convert directly from json to the array value but call get() I think this check can be changed to allow this to compile.

Compiled on VS 15.6.0 preview 2.0
Library version 3.0.1

[theodelrieu]

I'm looking at this. Adding explicit to your constructor fixes the issue, but I'll dig a bit more to see where and why it blows up.

Please note that you're exposing yourself to HUGE problems with a non-explicit templated constructor, without any constraints (enable_if conditions).

This library is confronted to this issue, since basic_json also has a non-explicit templated constructor (with constraints though, even if they're not perfect).

Here is an example with your type S:

// innocent-looking operator==
bool operator==(S, S) { return true; }

// innocent type
struct Innocent {};

Innocent t;

// Compiles... and call operator==(S, S)
t == t;

[theodelrieu]

The culprit is indeed the is_convertible check, I added the following method (following this cppreference page)

Here is the output:

t.cpp:14:19: error: conversion from 'nlohmann::basic_json<std::map, std::vector, std::__1::basic_string, bool, long long, unsigned long long, double, std::allocator,
adl_serializer>' to 'S' is ambiguous
T test() { return std::declval(); }
^~~~~~~~~~~~~~~~~
t.cpp:24:3: note: in instantiation of function template specialization 'test<nlohmann::basic_json<std::map, std::vector, std::__1::basic_string, bool, long long,
unsigned long long, double, std::allocator, adl_serializer>, S>' requested here
test<nl::json, S>();
^
develop/json.hpp:3053:5: note: candidate function [with ValueType = S, $1 = 0]
operator ValueType() const
^
t.cpp:7:25: note: candidate constructor [with T = nlohmann::basic_json<std::map, std::vector, std::__1::basic_string, bool, long long, unsigned long long, double,
std::allocator, adl_serializer>]
template S(const T &t) {}

This ambiguity is what prevent your code from working, I will open a PR to replace the is_convertible to is_constructible.

Thanks for the report!

EDIT:

It'll take a bit more time, there is more stuff to fix than what I thought

[dvirtz]

Thanks. The actual type is from a library so I can't change the constructor. I'm not sure is_constructible is appropriate here. I think you just need to check that get<T>() is valid.

…

On 19 Jan 2018 12:09 pm, "Théo DELRIEU" ***@***.***> wrote: The culprit is indeed the is_convertible check, I added the following method (following this cppreference <http://en.cppreference.com/w/cpp/types/is_convertible> page) Here is the output: t.cpp:14:19: error: conversion from 'nlohmann::basic_json<std::map, std::vector, std::__1::basic_string, bool, long long, unsigned long long, double, std::allocator, adl_serializer>' to 'S' is ambiguous T test() { return std::declval(); } ^~~~~~~~~~~~~~~~~ t.cpp:24:3: note: in instantiation of function template specialization 'test<nlohmann::basic_json<std::map, std::vector, std::__1::basic_string, bool, long long, unsigned long long, double, std::allocator, adl_serializer>, S>' requested here test<nl::json, S>(); ^ develop/json.hpp:3053:5: note: candidate function [with ValueType = S, $1 = 0] operator ValueType() const ^ t.cpp:7:25: note: candidate constructor [with T = nlohmann::basic_json<std::map, std::vector, std::__1::basic_string, bool, long long, unsigned long long, double, std::allocator, adl_serializer>] template S(const T &t) {} This ambiguity is what prevent your code from working, I will open a PR to replace the is_convertible to is_constructible. Thanks for the report! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#924 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHTojR_ePuofeCR6m7uAB3ejqCjb8C74ks5tMGnSgaJpZM4Rjs5L> .

[theodelrieu]

Yeah I was wrong, I realized that all those constraints made no sense whatsoever, I shall fix them when I have some free time.

Meanwhile, I would suggest you completely remove the line with the is_convertible check.

Also, if you cannot change the code of the library you use, I would suggest to open an issue :)

[nlohmann]

Any news on this?

[theodelrieu]

I had no time to work on it yet, it is linked with another issue I've found which I will open as soon as I can.

[nlohmann]

Ok, no worries.

[nlohmann]

@theodelrieu Any idea on how to proceed with this issue?

[theodelrieu]

IIRC this was related to #958, I'll check tomorrow the compiler errors to be sure.

[nlohmann]

With PR #969 and the code in #924 (comment) compiles without issues.