Skip to content

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

Merged
nlohmann merged 2 commits into
developfrom
claude/copy-without-assignable-base-5674
Oct 4, 2026
Merged

nlohmann merged 2 commits into
developfrom
claude/copy-without-assignable-base-5674

Conversation

@nlohmann

Copy link
Copy Markdown
Owner

Summary

The bounded-descent deep copy added by #5389 builds the elements of a copy nested
past the 128-level bound by default-constructing them and then having
copy_metadata() assign their base class afterwards. That assignment path is
only reached at runtime for values nested past the bound, but since it is
called (unconditionally in the source) from copy_structured(), which every
copy constructor call compiles, the assignment gets instantiated for every
copy regardless of depth. A CustomBaseClass that is copy-constructible but
not move-assignable (for example one with a const data member) therefore
could no longer be copied at all, even at the top level.

Changes

  • copy_array_level() and copy_object_level() now build each element with a
    new private-tag-selected constructor, basic_json(copy_construct_tag, const basic_json&),
    that copy-constructs the base class directly (and, under
    JSON_DIAGNOSTIC_POSITIONS, copies the positions), the same way the regular
    copy constructor already builds elements within the 128-level bound.
  • copy_shallow() no longer calls the now-removed copy_metadata(); it only
    fills in the leaf value or defers to the worklist, since the base class and
    positions are already set by the constructor above.
  • The constructor is public (an allocator's emplace_back()-driven
    construction happens outside basic_json's own member functions and so
    cannot call a private one), but its tag type stays a private nested type,
    so outside code can never obtain a value of it to call the constructor.
  • Regenerated single_include/nlohmann/json.hpp.

Tests

Added a regression test to tests/src/unit-custom-base-class.cpp using a
custom base class with a const member (copy-constructible, not
copy-/move-assignable), copied both at the top level and nested 300 levels
deep (past the 128-level bound), checking the base class's data is preserved
at every level. Built the new test once against develop's pre-fix headers to
confirm it fails to compile there with the exact error from the issue, and
against the fixed headers to confirm it compiles and passes.

Also built and ran the issue's own reproduction program against the fixed
headers (prints 2 7, matching headers from 56b3ee566^), and
tests/src/unit-diagnostics.cpp / tests/src/unit-diagnostic-positions.cpp
(including with JSON_DIAGNOSTICS=0) to confirm JSON_DIAGNOSTICS parent
pointers and JSON_DIAGNOSTIC_POSITIONS metadata are still copied correctly.

Configurations run: clang++ with -std=c++11 and -std=c++17,
-fsanitize=address,undefined, -Wall -Wextra -Werror; and g++ 16 with
-Weffc++ -Wnoexcept (both CI-relevant flags noted for this kind of change).

Public API

No breaking changes. This restores the behavior documented for
CustomBaseClass (copy construction requires only a copy-constructible base
class) and in place before #5389; copy assignment is unchanged and still
requires an assignable base class.

Fixes #5674


This PR was written by Claude Code on behalf of @nlohmann.

🤖 Generated with Claude Code

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>
@nlohmann nlohmann added the review needed It would be great if someone could review the proposed changes. label Sep 30, 2026
Conflicts:
- include/nlohmann/json.hpp: copy_array_level() keeps develop's #5721
  "set the array type only once the container exists" and then builds the
  elements with the PR's copy_construct_tag emplace_back loop
- single_include/nlohmann/json.hpp: regenerated with make amalgamate

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

Copy link
Copy Markdown
Owner Author

Merged develop (8366d86). Conflict in copy_array_level(): develop's #5721 now sets the array type right after create<array_t>(), and this PR replaced resize() with the copy_construct_tag emplace_back loop. The merge keeps both: create the array, set value_t::array, then emplace the elements. single_include regenerated.

— posted by Claude Code on behalf of @nlohmann

@nlohmann nlohmann added 🚀 ready to merge Ready to merge - just waiting for CI to complete. and removed review needed It would be great if someone could review the proposed changes. labels Oct 4, 2026
@nlohmann nlohmann added this to the Release 3.13.0 milestone Oct 4, 2026
@nlohmann
nlohmann merged commit 49cd427 into develop Oct 4, 2026
44 of 163 checks passed
@nlohmann
nlohmann deleted the claude/copy-without-assignable-base-5674 branch October 4, 2026 09:46
nlohmann added a commit that referenced this pull request Oct 4, 2026
- binary_reader: rename the error_handler constructor parameter, which
  shadowed the member (-Wshadow, -Wshadow-field-in-constructor; #5746)
- basic_json(copy_construct_tag, ...): declare it noexcept when copying
  the base class is (GCC 16 -Wnoexcept; #5690)
- the scalar-on-left legacy comparison operators: noexcept only when
  converting the scalar is, like their member counterparts (#5682, #5751)
- compare_leaves: use std::is_eq/is_lt/is_gt instead of comparing a
  std::partial_ordering with 0 (-Wzero-as-null-pointer-constant; #5686)
- serializer: silence MSVC C4127 for the EnsureAscii template parameter
  (#5741, #5746)
- clang-tidy: return the sanitized reference in binary_writer, take the
  key of ordered_map::find_impl by const reference (#5727), and mark the
  switches over parse_array_index (#5728)
- ordered_map: keep <memory> for std::allocator (IWYU)

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann added a commit that referenced this pull request Oct 4, 2026
* Keep the serializer conversion for objects whose keys cannot be converted

#5591 added a test converting nlohmann::json into a basic_json whose
string type cannot be constructed from std::string. That instantiates
convert_iteratively(), whose members.emplace_back(next.key(), ...) needs
exactly that key conversion, and broke the build of unit-alt-string.
Dispatch on the key's constructibility and leave such conversions to the
serializers, as the levels above the nesting bound already do (#3425).

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

* Fix the remaining CI failures on develop

- unit-wstring: with a 16-bit wchar_t (Windows), a lone surrogate is
  reported as the ill-formed byte 0xFF since #5704; the std::wstring
  expectations still had the previous <U+0000>.
- ci_single_binaries: json_literals.hpp (#5610) and json.hpp include each
  other on purpose, and IWYU, not following the cycle, asks to replace
  json.hpp with json_fwd.hpp. Report its findings without failing the
  build, as already done for json.hpp.

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

* Fix the library warnings and noexcept specifications from the merged PRs

- binary_reader: rename the error_handler constructor parameter, which
  shadowed the member (-Wshadow, -Wshadow-field-in-constructor; #5746)
- basic_json(copy_construct_tag, ...): declare it noexcept when copying
  the base class is (GCC 16 -Wnoexcept; #5690)
- the scalar-on-left legacy comparison operators: noexcept only when
  converting the scalar is, like their member counterparts (#5682, #5751)
- compare_leaves: use std::is_eq/is_lt/is_gt instead of comparing a
  std::partial_ordering with 0 (-Wzero-as-null-pointer-constant; #5686)
- serializer: silence MSVC C4127 for the EnsureAscii template parameter
  (#5741, #5746)
- clang-tidy: return the sanitized reference in binary_writer, take the
  key of ordered_map::find_impl by const reference (#5727), and mark the
  switches over parse_array_index (#5728)
- ordered_map: keep <memory> for std::allocator (IWYU)

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

* Split unit-conversions.cpp so MinGW can link it

clang 18 with the MinGW linker failed to link test-conversions_cpp17
("relocation truncated to fit: IMAGE_REL_AMD64_REL32"). As windows.yml
recommends, keep the objects small by splitting the test file.

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

* Fix the tests added by the merged PRs for all CI configurations

- discard the results of dump() and from_*() in CHECK_THROWS with
  utils::ignore_return_value (GCC -Werror=unused-result)
- give unit-bson's huge_string_t a default constructor (MSVC C2512,
  GCC 5, clang 3.5)
- unit-disabled_exceptions: use the literals namespace when the global
  UDLs are off (ci_test_noglobaludls; #5700)
- unit-binary_utf8_strict: expect the JSON pointer prefix with
  JSON_DIAGNOSTICS (#5741)
- skip the tests that rely on exceptions under JSON_NOEXCEPTION
  (#5678, #5732)
- clang-tidy and clang -Werror: static test data, CAPTURE(...);,
  const-correctness, use-after-move alias, unused conversion operator,
  a missing <iterator> include

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

* Title the macro examples and add JSON_STRICT_BINARY_UTF8 to the docset

The documentation style check requires "Example: ..." titles on pages with several examples (#5741, #5591) and a docset entry for every macro page.

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

* Regenerate BUILD.bazel and nlohmann_json.natvis

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

#5746 added detail/output/error_handler.hpp and #5741 the json_abi_sbu8 ABI tag.

* Install libidn11 for the CMake 3.5.0 binary in ci_cmake_flags

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

#5733 moved ci_cmake_options from ubuntu:focal to ubuntu:24.04, which no longer ships libidn.so.11; the CMake 3.5.0 release binary links against it, so every ci_cmake_flags run has failed since. Install focal's libidn11 package for that matrix entry only.

* Suppress Infer's false STACK_VARIABLE_ADDRESS_ESCAPE in get_impl

get_impl() returns its local by value. A test added by the merged PRs instantiates it with a type Infer misreads, so ci_infer reported the 2021 code for the first time.

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

---------

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L 🚀 ready to merge Ready to merge - just waiting for CI to complete. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Copy-constructing basic_json now requires an assignable CustomBaseClass (compile regression)

2 participants