Add support for deserialization of STL containers of non-default constructable types (fixes #2574).

[AnthonyVH]

This PR fixes the issue mentioned in #2574, i.e. the fact that std::array, std::pair, and std::tuple of non-default constructable types can't be deserialized.

A summary of what I did:

• Added an second from_json function to the default adl_serializer class. This second from_json returns an object, instead of taking an already existing object by reference. Expression SFINAE is used to disambiguate between the two function, so at most one should exist for any given type with which adl_serializer is constructed.

• Similarly I added a second operator() function to detail::from_json_fn. Again, due to SFINAE only one should only be available for any given type. Even if both are available, this should not be an issue, since the new function takes as its second argument an object nlohmann::detail::tag<T>. I.e. if you're manually calling functions with that object you're clearly actively trying to break something.

• The tag type mentioned above was added, since some method is required to disambiguate nlohmann::detail::from_json(j) for std::array, std::pair, and std::tuple. I'm using tag dispatching, that seems the simplest (only?) option.

• I've added some SFINAE to the existing deserialization code for the mentioned types, using std::is_default_constructible. While this seems OK for std::pair and std::tuple the existing code to deserialize (various types) of arrays had a lot of SFINAE checks, so it might be worthwhile to double check whether the checks on from_json(BasicJsonType && j, tag<std::array<T, N>> t) are sufficient.

• All of from_json's const BasicJsonType & arguments for the mentioned types were replaced with BasicJsonType && and std::forward was added. It's most likely unnecessary, but I don't see how it could hurt.

• Added extra checks to ensure that the JSON is an array to the std::pair and std::tuple deserialization code.

Any user-written specialization should still get precedence due the way I implemented the changes. I.e. if someone worked around this issue in their private code by specializing adl_serializer for e.g. std::array then that specialization will be used. Since an internal tag type is used for dispatching, there is no risk of this colliding with any existing code out there.

As mentioned I've added support for std::array, std::pair, and std::tuple. As far as I know these are the only STL containers which might not be default constructible due to the type they're templated on. In case I missed some types, it should be easy to add them by implementing T from_json (BasicJsonType && j, tag<T>) in detail/conversions/from_json.hpp.

Looking forward to feedback! This was definitely more involved than what I had in mind. Writing the code to deserialize to those "problematic" objects was actually the easy part 😁.

---

Pull request checklist

Read the Contribution Guidelines for detailed information.

• Changes are described in the pull request, or an existing issue is referenced.
• The test suite compiles and runs without error.
• Code coverage is 100%. Test cases can be added by editing the test suite.
• The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

[coveralls]

Coverage increased (+4.0e-05%) to 99.982% when pulling 2b86513 on AnthonyVH:non_default_constructable_stl_containers into 6f55193 on nlohmann:develop.

[nlohmann]

I would appreciate if @theodelrieu could have a look at this PR as well. They wrote the whole code for the to/from JSON conversions.

[theodelrieu]

I prefer to keep a single from_json overload in these cases, and forward to from_json_pair_impl, which use a priority_tag to ease the SFINAE pain, and to make it possible to add from_json_pair_impl overloads in the future.

Also, removing the SFINAE for overloads with detail::tag will allow default-constructible types to be passed as well, thus reducing code duplication (the first overload can call the second now, instead of copy-pasting)

[AnthonyVH]

What would the signature of this from_json function be? The second argument could be either std::pair<A1, A2> or identity_tag<std::pair<A1, A2>>, and I'm not seeing how to automatically deduce A1 and A2 when the second argument is e.g. PairRelatedType.

Or do you mean to have a single overload taking an std::pair and one taking identity_tag<std::pair> and then both of those dispatching to from_json_pair_impl? In that case, if the SFINAE is removed for the detail::tag overloads, then there will be two possible overloads available for adl_serializer to call (in case of default constructable types), so then that won't compile anymore due to ambiguity.

I'm probably just overlooking something...

[theodelrieu]

I mean something like this: https://github.com/theodelrieu/mgs/blob/master/codecs/include/mgs/codecs/basic_codec.hpp#L109

[AnthonyVH]

I've pushed an update which removes some code duplication and uses priority_tag to dispatch to the preferred function. Is this what you had in mind?

Note that adl_serializer now potentially has two from_json functions. This is e.g. the case for all default-constructable std::array, std::pair, and std::tuple. This doesn't cause ambiguity because json::get() uses SFINAE to select the "object returning" from_json() in that case (discarded get() vs chosen get()). This choice seems pretty deliberate, and looking at the code it's a logical choice due to possible bypassing at least 1 move construction (at least pre-C++17, although with optimizations there's probably no difference).

While doing some testing I did notice that the "chosen" get() gets called when deserializing a std::vector<float>. I found that quite surprising. I get how this adl_serializer::from_json() could now exist due to my code changes, but looking at the existing code it's not clear to me which detail::from_json would get called in that case. Just something I found odd which I figured was worth mentioning.

[theodelrieu]

Is this what you had in mind?

Yes, you can remove this line for the priority_tag<0> overload though:

enable_if_t < !std::is_default_constructible<std::array<T, N>>::value, int > = 0 >

This is the point of priority_tag<0>, it forces priority inside the overload set so you can just keep the is_default_constructible check for std::pair<A1, A2>.

Note that adl_serializer now potentially has two from_json functions.

IIRC (it's been a while since I've looked at the code), there is support for such overloads for adl_serializer specializations. I don't see why there would not be one for the base template indeed.

While doing some testing I did notice that the "chosen" get() gets called when deserializing a std::vector.

Seems like a change of behavior. To be honest, the template code is quite dusty, I've learned quite a lot since I wrote it, and it's quite hard to figure out all that's going on inside...

For instance, the different get() overloads would be a nice case for priority_tag. All the rules could be refined using emulated concepts.

[AnthonyVH]

I've updated some more code and fixed a potential issue with my std::pair deserialization. Is this OK?

[AnthonyVH]

I notice some regressions are failing to compile on older compilers. I'll try and take a look at them tonight. The errors seem to be in a rather unexpected place I must say, e.g.:

/usr/include/c++/4.9/bits/stl_pair.h: In instantiation of ‘constexpr std::pair<_T1, _T2>::pair() 
[with _T1 = NonDefaultConstructible; _T2 = NonDefaultConstructible]’:

/usr/include/c++/4.9/type_traits:806:28:   required by substitution of ‘template<class _Tp, class> 
static std::true_type std::__do_is_default_constructible_impl::__test(int)
[with _Tp = std::pair<NonDefaultConstructible, NonDefaultConstructible>; <template-parameter-1-2> = <missing>]’

[theodelrieu]

This is not how priority_tag is supposed to be used. You should have a single from_json overload and construct the tag with the highest value, like this:

template < typename BasicJsonType, typename ArrayType >
auto from_json(BasicJsonType&& j, ArrayType&& array)
-> decltype(from_json_inplace_array_impl(std::forward<BasicJsonType>(j),
                                         std::forward<ArrayType>(array), priority_tag<1> {}));

This gist shows quite nicely how to use it. Basically, it forces the overload resolution so you don't have to perform every SFINAE check in the world (like you had is_default_constructible and ! is_default_constructible)

[AnthonyVH]

Note that there's only 1 possible overload for from_json_inplace_array_impl(), hence 0 is the highest value in that case. Or am I missing something else?

The from_json_inplace_array_impl_base() is there because there's no index_sequence argument for from_json_inplace_array_impl(). I could add that and call make_index_sequence() from from_json(), but then any future from_json_inplace_array_impl() overload should have that as an argument as well, which didn't seem ideal. Hence the extra indirection.

I also looked into making this part of the from_json_array_impl() overloads, but there's SFINAE on its "entry-point" from_json() which prevents it from being called for non-default constructable types.

[theodelrieu]

Or am I missing something else?

No, it's me sorry. But in this case there is no need for priority_tag at all.

but there's SFINAE on its "entry-point" from_json() which prevents it from being called for non-default constructable types.

Just thought about it but non-default constructible types can very well use from_json(Json const&, T&) overloads, this happens when get_to is used for instance.

Therefore, the is_default_constructible check should just happen in get, and all functions that default-construct values before calling JSONSerializer<T>::from_json, no?

This would allow you to dispatch to from_json_array_impl and then use priority_tag.

It's been quite a while since I put my hands/eyes on the code, sorry for rusty review 😅

[AnthonyVH]

No, it's me sorry. But in this case there is no need for priority_tag at all.

True, I didn't have that there originally, but I gathered from one of your previous comments that you preferred this to make adding future overloads easier. I can remove it again if you prefer.

Therefore, the is_default_constructible check should just happen in get, and all functions that default-construct values before calling JSONSerializer::from_json, no?

Yes, that's true. Checking for default constructability in detail::from_json (e.g. the array version) shouldn't be necessary, since at that point you already have a reference to such an object, so it's already constructed.

Note that get() already has some checks using e.g. has_from_json and has_non_default_from_json. And those currently depend on default constructability of a type, since they depend the presence of certain signatures of from_json existing, and some of those in turn exist based on constructability of a type. Which as noted above actually isn't necessary...

So I guess the solution would be to get rid of constructability checks in from_json() and add an explicit std::is_default_constructible to get()?

I can take a look at it later this week. Although that might be overextending the scope of this PR. Or is that not an issue?

[theodelrieu]

True, I didn't have that there originally, but I gathered from one of your previous comments that you preferred this to make adding future overloads easier.

When there is at least two overloads, yes 😄

Although that might be overextending the scope of this PR.

I think it's a part of it, then you adding a get overload with that deals with non-default constructible types that do not have has_non_default_from_json should do the trick (it would check if from_json(json, identity_tag<> is valid).

Icing on the cake would be to refactor all those get overloads to use priority_tag.

[AnthonyVH]

When there is at least two overloads, yes 😄

Ok, I got ride of the redundant priority_tag and simplified some more code.

The LWG 2367 but mentioned below should also be fixed. Let's see what the regression says.

Also added some icing 😉.

[AnthonyVH]

Regarding the regressions which fail to compile: I need to look into it some more, but I did a quick search and it seems to be a language defect/STL implementation issue/whatever you want to call it. Basically std::is_default_constructible was not correctly implemented for std::pair and std::tuple. See LWG 2367.

The LWG report seems to indicate that this was only fixed for C++17, but clearly it got fixed sooner (maybe I'm just misreading the report). Here's some more info: https://stackoverflow.com/a/31438404. It seems it's been fixed in clang 3.6 and in gcc in a commit from 2015-06-30, though it's not clear what the earliest version was which contained this commit.

How do you want to handle this? I think the cleanest solution would be a "custom" is_default_constructible trait which dispatches to the STL one, except for std::pair and std::tuple. That should make the code functional for the problematic compilers as well. I guess the "custom" is_default_constructible could always skip the specializations for std::pair and std::tuple for e.g. C++17 and higher.

An alternative solution would be to keep this type of deserialization broken for older compilers by adding some #ifdefs around the regression and the new deserialization code. Of course in that case the library behavior depends on which compiler is used, which doesn't sound too great.

Btw, here's a live demo of the issue: https://godbolt.org/z/Kn4Krn.

[theodelrieu]

@AnthonyVH

I think the cleanest solution would be a "custom" is_default_constructible

Agreed, I've add this kind of issues in other projects, and I've reimplemented those everytime.

[AnthonyVH]

By the way, PR #2558 is going to create quite a few merge conflicts with my changes. Do you have any idea if that PR is going to be merged soon? If so, I can wait for it to be merged and fix the conflicts myself.

[AnthonyVH]

Lots of failures on older compilers 😞. Looking into it!

[AnthonyVH]

Progress 😄! Some tests still don't compile for GCC 4.8 on my PC, I'll try and take a look at that tomorrow evening.

[AnthonyVH]

Ok, it's going to take some more effort to work around a bunch of compiler bugs on older compilers. I see e.g. older MSVC versions throwing compile errors listed here: https://devblogs.microsoft.com/cppblog/expression-sfinae-improvements-in-vs-2015-update-3/.

[AnthonyVH]

I took another quick look and managed to fix the compile error. But first...

I can't make head or tails of the compile errors. In particular, it seems as if the get() function gets called for no reason at all. Here's part of the error message that I can't make sense of. This was generated by clang 3.9 when compiling with multiple headers. gcc 4.9 seems to do the exact same thing, the errors are just a lot less readable 😄.

.../include/nlohmann/json.hpp:3075:16: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char>, bool, long, unsigned long,
      double, std::allocator, adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > >::get_impl<std::initializer_list<nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char>, bool, long, unsigned
      long, double, std::allocator, adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > > >, 0>' requested here
        return get_impl<ValueType>(detail::priority_tag<4> {});
               ^
.../include/nlohmann/json.hpp:5584:20: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char>, bool, long, unsigned long,
      double, std::allocator, adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > >::insert_iterator<const nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char>, bool, long, unsigned long,
      double, std::allocator, adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > > &>' requested here
            return insert_iterator(pos, val);

I have no clue how it gets from insert_iterator() to get(). Or where that std::initializer_list suddenly comes from.

Either way, removing constexpr from get() fixes the compile errors in the old compilers I tested with. So I made it conditional for >= C++14. Is that acceptable? Or should I just remove it for all versions?

There's a bunch of constexpr functions (e.g. get_ptr(), which might get called by get_impl(), hence why I tagged get() as constexpr as well), but none of the json constructors seem to be constexpr, so I fail to see how any of these functions could ever have been called at compile time. Do you agree that removing constexpr from get() is not going to be an issue for anyone?

AFAIK, it should be legal to put constexpr there, at least if this StackOverflow answer is correct, which cites [dcl.constexpr]/7:

If the instantiated template specialization of a constexpr function template or member function of a class template would fail to satisfy the requirements for a constexpr function, that specialization is still a constexpr function, even though a call to such a function cannot appear in a constant expression.
If no specialization of the template would satisfy the requirements for a constexpr function when considered as a non-template function, the template is ill-formed, no diagnostic required.

That last sentence doesn't seem to apply here, since get_ptr() is constexpr, which gets called by get_impl(priority_tag<4>), which can get called by get() when called with a pointer type template argument. And perhaps more importantly, since there's no constexpr constructors, it should be impossible to call any member function in a constant expression.

Since newer compilers don't object to it, I'm guessing the errors here are due to either compiler bugs or changes in the spec. Unfortunately I have no clue what to search for, "conditional constexpr" doesn't seem to do the trick (that just returns a whole bunch of pages on if-constexpr).

[theodelrieu]

have no clue how it gets from insert_iterator() to get().

This rings a bell, I recall that basic_json types must be SFINAEd out.

Do you compile with JSON_USE_IMPLICIT_CONVERSIONS=False? If not, you should use it to know if the operator T() is the source of this trouble.

I highly recommend looking back at all the discussions in #958 to understand how such types can get there, and why get SFINAE checks must cover everything.

[AnthonyVH]

This rings a bell, I recall that basic_json types must be SFINAEd out.

I'm a little confused, because there was already an overload for get() to convert to basic_json. And I made it the 2nd most preferred overload, and it doesn't call from_json()...

Anyway, I'll take a look with implicit conversion disabled and see if that points to anything. Either way, the code seems to be fully functional on all tested compilers now (well, the complete regression hasn't finished yet, but I don't expect any issues given that multiple old compilers already passed the tests).

[AnthonyVH]

How do you want to proceed with this? Merge in the current version (i.e. with get() marked constexpr only for C++14 and up, see my comment earlier)? Or do you prefer this to be working with a constexpr get() in all versions?

I'm not sure if the compile errors I'm seeing with constexpr are due to the code, or due to language/compiler issues. Since the errors don't seem to point at the exact issue (e.g. function A() supposedly calling B() even though there's no mention of B() anywhere in A()), it might take a while before I figure out what's going on.

[theodelrieu]

@AnthonyVH

constexpr being broken in many old compilers (and practically unusable pre C++14), I'd go with merging now.

[nlohmann]

I made some small suggestions. Apart from this there are two open issues:

• A licensing issue (see comments).
• Is there a need to update the README file?

[nlohmann]

Can you elaborate whether you took code out of that commit? I'm asking because of licensing issues (GPLv2 vs. MIT).

[AnthonyVH]

I looked at the code to see what the "proper" fix looked like. However, that requires modifying std::pair/std::tuple. The code I added is more or less the same as the suggested one in the LWG defect report. Well, the logic is the same, it's not the exact same code. So I don't think there's any issue here.

It's also this report that mentions that this fix is incomplete:

The proposed resolution is incomplete, because it wouldn't work for cv-qualified objects of pair or for references of them during reference-initialization.

I don't think this is an issue, since detail::is_constructible is never called for e.g. std::pair<A, B> volatile nor for a reference to it. I did not add extra specializations to detail::is_constructible for volatile types or references. Currently only std::pair<A, B> and std::pair<A, B> const are specialized. Actually, I don't think that second one will ever be used. Do you prefer I add specializations for volatile, const volatile and references to all specialized types anyway (i.e. 6 extra specializations for std::pair and std::tuple each)?

[nlohmann]

This is not what I meant. I was asking whether the code following that comment was written by yourself (and could be MIT licensed in this project) or taken from a different source - e.g., the GCC source code which would be GPLv2 licensed.

[AnthonyVH]

No no, as I mentioned the code I added uses the same logic as the suggested solution mentioned in the LWG defect report. No code was copied from anywhere.

Maybe I should just remove the mention to the GCC commit?

[nlohmann]

Yes, please. Comments like this make FOSS auditors anxious...

[AnthonyVH]

Something to keep in mind next time 😄. I've removed the sentence and just pushed it. I guess that's the last required change then?

[AnthonyVH]

Is there a need to update the README file?

I don't think so. My changes only affect how deserialization happens internally. Since it currently doesn't explicitly mention the lack of support for deserialization of non default-constructable STL "containers", I don't think support for it should be called out?

[AnthonyVH]

Hm, not sure what's going on with AppVeyor. No progress in the last 14 hours, all jobs still in queued state...

[nlohmann]

Hm, not sure what's going on with AppVeyor. No progress in the last 14 hours, all jobs still in queued state...

AppVeyor is just really slow... I cancelled some redundant jobs. Stay tuned...

[AnthonyVH]

Seems like it went through 😄. Do you want me to change anything else or is this OK to merge?

[AnthonyVH]

I was wondering if anything is holding back this PR from being merged? I'm a bit worried that new commits to the develop branch might create merge conflicts.

[nlohmann]

@AnthonyVH Can you please update to the latest develop branch? I moved the CI away from Travis and fixes a lot of warnings. Then we can finally merge this.

[AnthonyVH]

Looks like the merge was successful 👍 . There were a bunch of merge conflicts in include/nlohmann/json.hpp which I was afraid of messing up, but all seems good.

All the conflicts were related to the get() and get_impl() functions which I had modified. As far as I could see, no actual code was modified by the changes in the upstream develop branch, only comments or annotations for e.g. clang-tidy.

[AnthonyVH]

@nlohmann Is there anything I can do to help speed this merge along?

[nlohmann]

@nlohmann Is there anything I can do to help speed this merge along?

I'll check tomorrow. Thanks for your patience!

[nlohmann]

The code looks fine - any update to the documentation required?

[AnthonyVH]

I don't think so, the current documentation doesn't mention this defect, so I don't see why it should be mentioned as explicitly being supported once this is merged.

[nlohmann]

Looks good to me.

[nlohmann]

Thanks a lot!

[AnthonyVH]

My pleasure! Great that this could be added to the library.