diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index 500fcddf2..0023d75c2 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -1006,19 +1006,39 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec using copy_scratch_value_t = std::pair; using copy_scratch_t = std::vector>; - /// @brief copy everything of @a src into @a dst but its type and value - static void copy_metadata(const basic_json& src, basic_json& dst) - { - // a custom base class is only required to be copy-constructible and - // move-assignable, so the copy has to go through a temporary - static_cast(dst) = json_base_class_t(static_cast(src)); + /// @brief tag selecting the constructor below; used only to build the + /// elements of a deep copy (@ref copy_array_level, @ref copy_object_level) + struct copy_construct_tag {}; + public: + /*! + @brief construct a null value whose base class - and, with @ref + JSON_DIAGNOSTIC_POSITIONS, positions - are copied from @a src + + Copy-constructing @ref json_base_class_t here, rather than default- + constructing the element and assigning its base class afterwards, means + that copying a @ref basic_json only ever requires a copy-constructible + base class, and never a move-assignable one as well. + + @note this constructor has to be public: @ref copy_array_level and + @ref copy_object_level reach it through @ref array_t's or @ref + object_t's own emplace_back(), which constructs the element from + outside @ref basic_json and so cannot call a private constructor. + @ref copy_construct_tag is private, though, and nothing in the + public interface hands out a value of it, so outside code can still + never name it to call this constructor itself. + */ + basic_json(copy_construct_tag /*unused*/, const basic_json& src) + : json_base_class_t(src) #if JSON_DIAGNOSTIC_POSITIONS - dst.start_position = src.start_position; - dst.end_position = src.end_position; + , start_position(src.start_position) + , end_position(src.end_position) #endif + { } + private: + /*! @brief copy the value of @a src into @a dst, which must not be structured @@ -1081,8 +1101,11 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } /*! - @brief copy everything of @a src into the null value @a dst but the children + @brief finish the copy @a dst of @a src that a @ref copy_construct_tag + constructor started, other than the children of an object or array + @a dst already has @a src's base class and, with @ref + JSON_DIAGNOSTIC_POSITIONS, positions; only its value is still missing. Objects and arrays are not copied here; they are appended to @a worklist to be created later by @ref copy_iteratively. Until that happens, @a dst remains a null value, so that a partially built copy can be destroyed at any point @@ -1090,8 +1113,6 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec */ static void copy_shallow(const basic_json& src, basic_json& dst, copy_worklist_t& worklist) { - copy_metadata(src, dst); - if (src.m_data.m_type == value_t::object || src.m_data.m_type == value_t::array) { // defer: dst stays a null value until its container exists @@ -1112,13 +1133,17 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec { const array_t& src_array = *src.m_data.m_value.array; - // create all elements up front: growing the array afterwards could - // invalidate the pointers that are handed to the worklist; resize() - // rather than the fill constructor, because not every array type - // provides the latter (e.g., ones without a matching allocator-aware - // fill constructor) dst.m_data.m_value.array = create(); - dst.m_data.m_value.array->resize(src_array.size()); + + // create every element - its base class already copy-constructed from + // its counterpart in src, via the copy_construct_tag constructor - + // before any of their addresses are handed to worklist below: growing + // the array while that is going on could reallocate it and invalidate + // addresses taken from an earlier iteration + for (const auto& src_element : src_array) + { + dst.m_data.m_value.array->emplace_back(copy_construct_tag{}, src_element); + } auto dst_it = dst.m_data.m_value.array->begin(); for (auto src_it = src_array.cbegin(); src_it != src_array.cend(); ++src_it, ++dst_it) @@ -1136,12 +1161,14 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec // build the complete key skeleton and hand it to the object's range // constructor: adding the keys one by one would be quadratic for object - // types that are backed by a vector, such as nlohmann::ordered_map + // types that are backed by a vector, such as nlohmann::ordered_map; each + // value's base class is already copy-constructed from its counterpart + // in src, via the copy_construct_tag constructor scratch.clear(); scratch.reserve(src_object.size()); for (const auto& element : src_object) { - scratch.emplace_back(element.first, basic_json()); + scratch.emplace_back(element.first, basic_json(copy_construct_tag{}, element.second)); } dst.m_data.m_value.object = create(std::make_move_iterator(scratch.begin()), diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 576498738..64fff1697 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -27087,19 +27087,39 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec using copy_scratch_value_t = std::pair; using copy_scratch_t = std::vector>; - /// @brief copy everything of @a src into @a dst but its type and value - static void copy_metadata(const basic_json& src, basic_json& dst) - { - // a custom base class is only required to be copy-constructible and - // move-assignable, so the copy has to go through a temporary - static_cast(dst) = json_base_class_t(static_cast(src)); + /// @brief tag selecting the constructor below; used only to build the + /// elements of a deep copy (@ref copy_array_level, @ref copy_object_level) + struct copy_construct_tag {}; + public: + /*! + @brief construct a null value whose base class - and, with @ref + JSON_DIAGNOSTIC_POSITIONS, positions - are copied from @a src + + Copy-constructing @ref json_base_class_t here, rather than default- + constructing the element and assigning its base class afterwards, means + that copying a @ref basic_json only ever requires a copy-constructible + base class, and never a move-assignable one as well. + + @note this constructor has to be public: @ref copy_array_level and + @ref copy_object_level reach it through @ref array_t's or @ref + object_t's own emplace_back(), which constructs the element from + outside @ref basic_json and so cannot call a private constructor. + @ref copy_construct_tag is private, though, and nothing in the + public interface hands out a value of it, so outside code can still + never name it to call this constructor itself. + */ + basic_json(copy_construct_tag /*unused*/, const basic_json& src) + : json_base_class_t(src) #if JSON_DIAGNOSTIC_POSITIONS - dst.start_position = src.start_position; - dst.end_position = src.end_position; + , start_position(src.start_position) + , end_position(src.end_position) #endif + { } + private: + /*! @brief copy the value of @a src into @a dst, which must not be structured @@ -27162,8 +27182,11 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } /*! - @brief copy everything of @a src into the null value @a dst but the children + @brief finish the copy @a dst of @a src that a @ref copy_construct_tag + constructor started, other than the children of an object or array + @a dst already has @a src's base class and, with @ref + JSON_DIAGNOSTIC_POSITIONS, positions; only its value is still missing. Objects and arrays are not copied here; they are appended to @a worklist to be created later by @ref copy_iteratively. Until that happens, @a dst remains a null value, so that a partially built copy can be destroyed at any point @@ -27171,8 +27194,6 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec */ static void copy_shallow(const basic_json& src, basic_json& dst, copy_worklist_t& worklist) { - copy_metadata(src, dst); - if (src.m_data.m_type == value_t::object || src.m_data.m_type == value_t::array) { // defer: dst stays a null value until its container exists @@ -27193,13 +27214,17 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec { const array_t& src_array = *src.m_data.m_value.array; - // create all elements up front: growing the array afterwards could - // invalidate the pointers that are handed to the worklist; resize() - // rather than the fill constructor, because not every array type - // provides the latter (e.g., ones without a matching allocator-aware - // fill constructor) dst.m_data.m_value.array = create(); - dst.m_data.m_value.array->resize(src_array.size()); + + // create every element - its base class already copy-constructed from + // its counterpart in src, via the copy_construct_tag constructor - + // before any of their addresses are handed to worklist below: growing + // the array while that is going on could reallocate it and invalidate + // addresses taken from an earlier iteration + for (const auto& src_element : src_array) + { + dst.m_data.m_value.array->emplace_back(copy_construct_tag{}, src_element); + } auto dst_it = dst.m_data.m_value.array->begin(); for (auto src_it = src_array.cbegin(); src_it != src_array.cend(); ++src_it, ++dst_it) @@ -27217,12 +27242,14 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec // build the complete key skeleton and hand it to the object's range // constructor: adding the keys one by one would be quadratic for object - // types that are backed by a vector, such as nlohmann::ordered_map + // types that are backed by a vector, such as nlohmann::ordered_map; each + // value's base class is already copy-constructed from its counterpart + // in src, via the copy_construct_tag constructor scratch.clear(); scratch.reserve(src_object.size()); for (const auto& element : src_object) { - scratch.emplace_back(element.first, basic_json()); + scratch.emplace_back(element.first, basic_json(copy_construct_tag{}, element.second)); } dst.m_data.m_value.object = create(std::make_move_iterator(scratch.begin()), diff --git a/tests/src/unit-custom-base-class.cpp b/tests/src/unit-custom-base-class.cpp index 7dab5c576..e21506603 100644 --- a/tests/src/unit-custom-base-class.cpp +++ b/tests/src/unit-custom-base-class.cpp @@ -333,3 +333,73 @@ TEST_CASE("JSON Visit Node") ); CHECK(expected.empty()); } + +// A custom base class with a const member: copy-constructible (initializing a +// const member works fine), but not copy-/move-assignable (assigning one does +// not). Used to check that copy construction never requires more than that. +struct const_member_base +{ + const int id = 7; // NOLINT(misc-non-private-member-variables-in-classes) +}; + +using json_with_const_base = nlohmann::basic_json < + std::map, + std::vector, + std::string, + bool, + std::int64_t, + std::uint64_t, + double, + std::allocator, + nlohmann::adl_serializer, + std::vector, + const_member_base + >; + +// build an array nested @a depth levels deep, with the innermost value 1; +// every level is constructed (never assigned), since const_member_base does +// not support assignment +static json_with_const_base make_nested_array(std::size_t depth) +{ + if (depth == 0) + { + return json_with_const_base(1); + } + return json_with_const_base::array({make_nested_array(depth - 1)}); +} + +TEST_CASE("Regression test for issue #5674 - copy construction must not require an assignable base class") +{ + SECTION("depth 0") + { + // as in the original bug report: copy construction only, no assignment + const json_with_const_base j = {1, 2}; + const json_with_const_base copy = j; // NOLINT(performance-unnecessary-copy-initialization) + + CHECK(copy.size() == 2); + CHECK(copy.id == 7); + } + + SECTION("nested deeper than the copy constructor's descent bound") + { + // beyond nesting_depth_limit() (128) levels, the copy constructor + // copies without the call stack (copy_iteratively / copy_array_level), + // which used to assign the base class of every element it created + const std::size_t depth = 300; + + const json_with_const_base j = make_nested_array(depth); + const json_with_const_base copy = j; // NOLINT(performance-unnecessary-copy-initialization) + + const json_with_const_base* c = © + for (std::size_t level = 0; level <= depth; ++level) + { + CAPTURE(level) + REQUIRE(c->id == 7); + if (level < depth) + { + c = &c->at(0); + } + } + CHECK(*c == 1); + } +}