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 to std::array with non-default constructable types fails #2574

Closed
2 of 5 tasks
AnthonyVH opened this issue Jan 8, 2021 · 10 comments · Fixed by #2576
Closed
2 of 5 tasks

Deserialization to std::array with non-default constructable types fails #2574

AnthonyVH opened this issue Jan 8, 2021 · 10 comments · Fixed by #2576
Assignees
Labels
kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@AnthonyVH
Copy link
Contributor

AnthonyVH commented Jan 8, 2021

Compilations fails when trying to convert an nlohmann::json object to an std::array<T, N>, where T does not have a default constructor.

What is the issue you have?

Compile error when trying to deserialize an std::array of non-default constructable types. However, deserialization from such types works fine if such a type is deserialized directly (i.e. not an std::array of it), or if e.g. an std::vector of such types is deserialized.

Hence it seems to me that this limitation is purely due to the way deserialization is implemented for std::array. Of course for e.g. std::vector this is not an issue anyway, since there you can constructor the std::vector first and then emplace_back() objects into it, which isn't possible for std::array in this case.

I haven't really looked into it, but I would think that using pack expansion (with most likely some extra helper structs thrown in) would allow creating the requested std::array in "one go" (i.e. no creating it beforehand and then assigning values to each location).

Please describe the steps to reproduce the issue.

Live example here: https://godbolt.org/z/qxP5aE

Can you provide a small but working code example?

#include <nlohmann/json.hpp>

#include <array>
#include <vector>

struct NoDefaultConstructor {
    explicit NoDefaultConstructor (int x) : x(x) { }
    int x;
};

namespace nlohmann {
    template <>
    struct adl_serializer<NoDefaultConstructor> {
        static NoDefaultConstructor from_json (json const & j) {
            return NoDefaultConstructor(j.get<int>());
        }
    };
}

int main  () {
    { // OK
        auto a = nlohmann::json(3);
        auto b = a.get<NoDefaultConstructor>();
    }

    { // OK
        auto a = nlohmann::json({ 1, 2 });
        auto b = a.get<std::vector<NoDefaultConstructor>>();
    }

    { // Does not compile.
        auto a = nlohmann::json({ 1, 2 });
        auto b = a.get<std::array<NoDefaultConstructor, 2>>();
    }
}

What is the expected behavior?

Code compiles and returns the requested std::array.

And what is the actual behavior instead?

nlohmann::json 3.7.1: Compile error that the function get<std::array<NoDefaultConstructor, 2>> does not exist:

JsonTest.C:33:57: error: no matching function for call to ‘nlohmann::basic_json<>::get<std::array<NoDefaultConstructor, 2> >()’
   33 |     auto b = a.get<std::array<NoDefaultConstructor, 2>>();

nlohmann::json 3.6.0: Static assert goes off complaining that get<T>() doesn't allow T to be non-default constructable.

Which compiler and operating system are you using?

  • Compiler: gcc 9.1.0 & clang 11.0.0
  • Operating system: Some flavor of linux

Which version of the library did you use?

  • latest release version 3.9.1
  • other release - please state the version: 3.6.0 (Godbolt, clang) and 3.7.1 (local, gcc)
  • 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
@nlohmann
Copy link
Owner

nlohmann commented Jan 8, 2021

Related: #2575

@nlohmann
Copy link
Owner

nlohmann commented Jan 8, 2021

The problem seems to be that std::array can only be default initializer or aggregate-initialized. Solutions like the one described in #2575 rely on default initialization which your type does not allow.

So it seems to be less a problem of the library, but more the question: how to initialize a std::array<NoDefaultConstructor>?

@AnthonyVH
Copy link
Contributor Author

AnthonyVH commented Jan 8, 2021

Issue #2575 seems wrong use of the library somehow? I don't see it being related to this at all, this is specifically about non-default constructible types.

Initializing an array of such types is not really a problem in general. You can use std::index_sequence together with pack expansion. See for example the code below (live example if you want to play with it):

#include <array>
#include <iostream>
#include <utility>
#include <vector>

struct NoDefConstructor {
    explicit NoDefConstructor (int x) : x(x) { }
    int x;
};

template <std::size_t... Is>
std::array<NoDefConstructor, sizeof...(Is)> to_array_impl (std::vector<NoDefConstructor> const & v,
        std::index_sequence<Is...>) {
    return { v.at(Is)... };
}

template <std::size_t N>
std::array<NoDefConstructor, N> to_array (std::vector<NoDefConstructor> const & v) {
    return to_array_impl(v, std::make_index_sequence<N>());
}

int main() {
    auto a = std::vector<NoDefConstructor>{ NoDefConstructor(7), NoDefConstructor(2) };
    auto b = to_array<2>(a);

    for (auto const & e : b) { std::cout << e.x << ' '; }
}

You could always use this way of constructing an array when deserializing. Although that would generate different code for each combination of T and N. You could get rid of that duplication in case of default constructable objects if you reuse one single function which you pass in begin & end iterators of the already constructed array. I haven't checked how it's currently done though.

@nlohmann
Copy link
Owner

nlohmann commented Jan 8, 2021

Ok, so we have a possible solution. Nice!

@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation state: waiting for PR labels Jan 8, 2021
@AnthonyVH
Copy link
Contributor Author

I'm working on a pull request. Should be finished soon.

@nlohmann
Copy link
Owner

nlohmann commented Jan 8, 2021

Thanks so much. Sorry for the confusion - with two issues related to std::array within a few minutes, I mistakenly thought they were related. Please also note we target C++11 - we have an implementation of index_sequence in include/nlohmann/detail/meta/cpp_future.hpp.

@AnthonyVH
Copy link
Contributor Author

Yes, I found those implementations. I've just been spending the past half hour staring at where to put the code :).

@AnthonyVH
Copy link
Contributor Author

By the way, this same issue happens for std::pair and std::tuple. Shall I make a separate bug for this, or do I just put it all in 1 single PR?

@nlohmann
Copy link
Owner

nlohmann commented Jan 8, 2021

Put them in the same. Thanks for checking!

@AnthonyVH
Copy link
Contributor Author

Ok, will do.

Might take a bit longer than expected. I'm struggling to find the best place to put this. It seems it belongs in a specialization of adl_serializer, but I need to specialize depending on the value of has_non_default_from_json. But that needs BasicJsonType as its first argument, which is not known when instantiating the adl_serializer...

AnthonyVH added a commit to AnthonyVH/json that referenced this issue Jan 8, 2021
nlohmann added a commit that referenced this issue Apr 25, 2021
…_containers

Add support for deserialization of STL containers of non-default constructable types (fixes #2574).
@nlohmann nlohmann self-assigned this Apr 25, 2021
@nlohmann nlohmann added this to the Release 3.9.2 milestone Apr 25, 2021
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
2 participants