Copy-construct the base class of a deep copy's elements, not assign it

The bounded-descent copy added by #5389 built the elements of a deep copy
(nested past the 128-level bound) by default-constructing them and then
having copy_metadata() assign their base class afterwards. That assignment
is only instantiated for values nested past the bound, but being called
from copy_structured() at all meant it was compiled for every copy, so a
CustomBaseClass that is copy-constructible but not move-assignable (for
example one with a const data member) no longer let its basic_json be
copy-constructed, at any depth.

copy_array_level() and copy_object_level() now build each element with a
private-tag-selected constructor that copy-constructs the base class (and,
under JSON_DIAGNOSTIC_POSITIONS, copies the positions) directly, the same
way the copy constructor already builds elements within the 128-level
bound. Copying a basic_json is therefore back to requiring only a
copy-constructible base class, as documented and as it was before #5389;
copy assignment is unchanged and still requires an assignable one.

Fixes #5674.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

docs/mkdocs/docs/api/basic_json/parse.md

@@ -88,12 +88,7 @@ Strong guarantee: if an exception is thrown, there are no changes in the JSON va
 ## Exceptions
 
 - Throws [`parse_error.101`](../../home/exceptions.md#jsonexceptionparse_error101) in case of an unexpected token, or
-  empty input like a null `FILE*` or `char*` pointer, or an `std::istream` without a stream buffer
-  (`#!cpp i.rdbuf() == nullptr`, for instance `#!cpp std::istream(nullptr)`).
-- If reading from an `std::istream` reaches the end of the input and `eofbit` is part of the stream's
-  [`exceptions()`](https://en.cppreference.com/w/cpp/io/basic_ios/exceptions) mask, the `std::ios_base::failure`
-  thrown by the stream itself propagates instead of a `parse_error`, the same as it would for the standard library's
-  own extraction operators.
+  empty input like a null `FILE*` or `char*` pointer.
 
 ## Complexity
 
@@ -259,8 +254,6 @@ outside of a string, invalid) byte; see the [FAQ entry](../../home/faq.md#nul-by
 - Extended overload (2) to accept heterogeneous iterator+sentinel pairs (C++20 ranges support) in version 3.13.0.
 - `JSON_STRICT_NUL_HANDLING` added in version 3.13.0 to optionally reject a NUL byte in the input instead of treating
   it as end of input; planned to become the default in version 4.0.0.
-- Extended empty-input detection to also cover an `std::istream` without a stream buffer, and fixed a crash
-  (`std::terminate`) when parsing from an `std::istream` with `eofbit` in its exception mask, in version 3.13.0.
 
 !!! warning "Deprecation"
 

docs/mkdocs/docs/api/operator_gtgt.md

@@ -20,12 +20,7 @@ the stream `i`
 
 ## Exceptions
 
-- Throws [`parse_error.101`](../home/exceptions.md#jsonexceptionparse_error101) in case of an unexpected token, or if
-  `i` has no stream buffer (`#!cpp i.rdbuf() == nullptr`, for instance `#!cpp std::istream(nullptr)`).
-- If reading from `i` reaches the end of the input and `eofbit` is part of `i`'s
-  [`exceptions()`](https://en.cppreference.com/w/cpp/io/basic_ios/exceptions) mask, the `std::ios_base::failure`
-  thrown by `i` itself propagates instead of a `parse_error`, the same as it would for the standard library's own
-  extraction operators.
+- Throws [`parse_error.101`](../home/exceptions.md#jsonexceptionparse_error101) in case of an unexpected token.
 
 ## Complexity
 
@@ -123,5 +118,3 @@ being read.
   it as end of input; planned to become the default in version 4.0.0.
 - `JSON_PRECISE_STREAM_POSITION` added in version 3.13.0 to optionally leave the character that terminates a number in
   the stream; planned to become the default in version 4.0.0.
-- Fixed a null pointer dereference for an `std::istream` without a stream buffer (now throws `parse_error.101`), and a
-  crash (`std::terminate`) when `i` has `eofbit` in its exception mask, in version 3.13.0.

include/nlohmann/detail/input/input_adapters.hpp

@@ -106,13 +106,7 @@ class input_stream_adapter
             // was given back with release_lookahead()
             commit_lookahead();
 #endif
-            // only call clear() if there is something to clear: it throws
-            // std::ios_base::failure if the stream has exceptions() enabled
-            // for a state bit that remains set, and a destructor must not throw
-            if ((is->rdstate() & ~std::ios::eofbit) != 0)
-            {
-                is->clear(is->rdstate() & std::ios::eofbit);
-            }
+            is->clear(is->rdstate() & std::ios::eofbit);
         }
     }
 
@@ -817,16 +811,12 @@ inline file_input_adapter input_adapter(std::FILE* file)
 
 inline input_stream_adapter input_adapter(std::istream& stream)
 {
-    if (stream.rdbuf() == nullptr)
-    {
-        JSON_THROW(parse_error::create(101, 0, "attempting to parse an empty input; check that your input string or stream contains the expected JSON", nullptr));
-    }
     return input_stream_adapter(stream);
 }
 
 inline input_stream_adapter input_adapter(std::istream&& stream)
 {
-    return input_adapter(stream);
+    return input_stream_adapter(stream);
 }
 #endif  // JSON_NO_IO
 

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<typename object_t::key_type, basic_json>;
     using copy_scratch_t = std::vector<copy_scratch_value_t, AllocatorType<copy_scratch_value_t>>;
 
-    /// @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<json_base_class_t&>(dst) = json_base_class_t(static_cast<const json_base_class_t&>(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<array_t>();
-        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<object_t>(std::make_move_iterator(scratch.begin()),

single_include/nlohmann/json.hpp

@@ -7650,13 +7650,7 @@ class input_stream_adapter
             // was given back with release_lookahead()
             commit_lookahead();
 #endif
-            // only call clear() if there is something to clear: it throws
-            // std::ios_base::failure if the stream has exceptions() enabled
-            // for a state bit that remains set, and a destructor must not throw
-            if ((is->rdstate() & ~std::ios::eofbit) != 0)
-            {
-                is->clear(is->rdstate() & std::ios::eofbit);
-            }
+            is->clear(is->rdstate() & std::ios::eofbit);
         }
     }
 
@@ -8361,16 +8355,12 @@ inline file_input_adapter input_adapter(std::FILE* file)
 
 inline input_stream_adapter input_adapter(std::istream& stream)
 {
-    if (stream.rdbuf() == nullptr)
-    {
-        JSON_THROW(parse_error::create(101, 0, "attempting to parse an empty input; check that your input string or stream contains the expected JSON", nullptr));
-    }
     return input_stream_adapter(stream);
 }
 
 inline input_stream_adapter input_adapter(std::istream&& stream)
 {
-    return input_adapter(stream);
+    return input_stream_adapter(stream);
 }
 #endif  // JSON_NO_IO
 
@@ -27097,19 +27087,39 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     using copy_scratch_value_t = std::pair<typename object_t::key_type, basic_json>;
     using copy_scratch_t = std::vector<copy_scratch_value_t, AllocatorType<copy_scratch_value_t>>;
 
-    /// @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<json_base_class_t&>(dst) = json_base_class_t(static_cast<const json_base_class_t&>(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
 
@@ -27172,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
@@ -27181,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
@@ -27203,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<array_t>();
-        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)
@@ -27227,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<object_t>(std::make_move_iterator(scratch.begin()),

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<std::uint8_t>,
+                             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 = &copy;
+        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);
+    }
+}

tests/src/unit-deserialization.cpp

@@ -388,37 +388,6 @@ TEST_CASE("deserialization")
             }));
         }
 
-        SECTION("stream with eofbit in its exception mask (issue #5646)")
-        {
-            // reaching EOF while parsing a value that fills the whole input
-            // (e.g., a number, or any value under strict parsing) makes
-            // get_character() call std::istream::clear() to record eofbit;
-            // with eofbit in the exception mask, that clear() itself throws
-            // std::ios_base::failure - it must propagate to the caller instead
-            // of ~input_stream_adapter() throwing a second exception while the
-            // first is still unwinding, which would call std::terminate
-            json _;
-
-            std::istringstream is1("1");
-            is1.exceptions(std::ios::eofbit);
-            CHECK_THROWS_AS(_ = json::parse(is1), std::ios_base::failure&);
-
-            // the same holds for the common std::ifstream::exceptions(failbit |
-            // badbit | eofbit) pattern, because only eofbit ends up set
-            std::istringstream is2("1");
-            is2.exceptions(std::ios::failbit | std::ios::badbit | std::ios::eofbit);
-            CHECK_THROWS_AS(_ = json::parse(is2), std::ios_base::failure&);
-        }
-
-        SECTION("stream without a streambuf (issue #5646)")
-        {
-            // std::istream(nullptr) has badbit set and rdbuf() == nullptr;
-            // get_character() must not dereference that null streambuf
-            std::istream is(nullptr);
-            json _;
-            CHECK_THROWS_WITH_AS(_ = json::parse(is), "[json.exception.parse_error.101] parse error: attempting to parse an empty input; check that your input string or stream contains the expected JSON", json::parse_error&);
-        }
-
         SECTION("string")
         {
             json::string_t const s = R"(["foo",1,2,3,false,{"one":1})";

tests/src/unit-precise-stream-position.cpp

@@ -234,33 +234,4 @@ TEST_CASE("JSON_PRECISE_STREAM_POSITION")
         CHECK(j == json(1));
         CHECK(remaining(is) == "true");
     }
-
-    SECTION("stream with eofbit in its exception mask (issue #5646)")
-    {
-        // with JSON_PRECISE_STREAM_POSITION, get_character() peeks via
-        // sb->sgetc() rather than consuming via sb->sbumpc(), but it still
-        // calls std::istream::clear() to record eofbit once the streambuf is
-        // exhausted; with eofbit in the exception mask, that clear() itself
-        // throws std::ios_base::failure, which must propagate to the caller
-        // instead of ~input_stream_adapter() throwing a second exception
-        // while the first is still unwinding (which would call std::terminate)
-        json _;
-
-        std::istringstream is1("1");
-        is1.exceptions(std::ios::eofbit);
-        CHECK_THROWS_AS(_ = json::parse(is1), std::ios_base::failure&);
-
-        std::istringstream is2("1");
-        is2.exceptions(std::ios::failbit | std::ios::badbit | std::ios::eofbit);
-        CHECK_THROWS_AS(_ = json::parse(is2), std::ios_base::failure&);
-    }
-
-    SECTION("stream without a streambuf (issue #5646)")
-    {
-        // std::istream(nullptr) has badbit set and rdbuf() == nullptr;
-        // get_character() must not dereference that null streambuf
-        std::istream is(nullptr);
-        json _;
-        CHECK_THROWS_WITH_AS(_ = json::parse(is), "[json.exception.parse_error.101] parse error: attempting to parse an empty input; check that your input string or stream contains the expected JSON", json::parse_error&);
-    }
 }