Test the copy constructor's iterative path in CI

The copy constructor descends into 128 levels before it finishes a value
without the call stack, so the iterative path is otherwise only reached
by the few tests that nest deeper than that.

JSON_NO_THREAD_LOCAL switches the descent off, which sends every value
down that path. Running the whole test suite that way covers it with
every object type, string type, allocator, and base class the suite
already exercises. The new ci_test_no_thread_local target does that; the
macro had no build coverage at all before.

Copying a nested value also has to carry over what the element-wise copy
constructor would have copied: the parents that JSON_DIAGNOSTICS relies
on, and the positions that JSON_DIAGNOSTIC_POSITIONS reports. Both are
now checked on either side of the descent bound, for objects and arrays.
Neither was tested before, and dropping either one makes the new tests
fail.

Also quantify what JSON_NO_THREAD_LOCAL costs a copy instead of calling
it "measurably slower".

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

Author: nlohmann

.github/workflows/ubuntu.yml

@@ -100,7 +100,7 @@ jobs:
     container: ubuntu:focal
     strategy:
       matrix:
-        target: [ci_cmake_flags, ci_test_diagnostics, ci_test_diagnostic_positions, ci_test_noexceptions, ci_test_noimplicitconversions, ci_test_legacycomparison, ci_test_noglobaludls]
+        target: [ci_cmake_flags, ci_test_diagnostics, ci_test_diagnostic_positions, ci_test_noexceptions, ci_test_noimplicitconversions, ci_test_legacycomparison, ci_test_noglobaludls, ci_test_no_thread_local]
     steps:
       - name: Install build-essential
         run: apt-get update ; apt-get install -y build-essential unzip wget git libssl-dev

cmake/ci.cmake

@@ -242,6 +242,25 @@ add_custom_target(ci_test_noglobaludls
     COMMENT "Compile and test with global UDLs disabled"
 )
 
+###############################################################################
+# Disable thread-local storage.
+###############################################################################
+
+# Without thread-local storage, the copy constructor cannot bound its descent
+# and copies every object and array without the call stack. That path is
+# otherwise only reached by values nested deeper than the bound, so this target
+# is what runs the whole test suite through it.
+add_custom_target(ci_test_no_thread_local
+    COMMAND ${CMAKE_COMMAND}
+    -DCMAKE_BUILD_TYPE=Debug -GNinja
+    -DJSON_BuildTests=ON
+    -DCMAKE_CXX_FLAGS=-DJSON_NO_THREAD_LOCAL
+    -S${PROJECT_SOURCE_DIR} -B${PROJECT_BINARY_DIR}/build_no_thread_local
+    COMMAND ${CMAKE_COMMAND} --build ${PROJECT_BINARY_DIR}/build_no_thread_local
+    COMMAND cd ${PROJECT_BINARY_DIR}/build_no_thread_local && ${CMAKE_CTEST_COMMAND} --parallel ${N} --output-on-failure
+    COMMENT "Compile and test without thread-local storage"
+)
+
 ###############################################################################
 # Coverage.
 ###############################################################################

docs/mkdocs/docs/api/macros/json_no_thread_local.md

@@ -14,7 +14,9 @@ shared between threads would be raced.
 
 Without that counter, no descent can be bounded safely, so objects and arrays are copied without the call stack right
 away. Copying keeps working exactly as it does otherwise - the same values come out, and deeply nested values are copied
-just as safely - but copying is measurably slower, as the containers no longer copy themselves.
+just as safely - but copying is slower, because the containers no longer copy themselves. Copying the benchmark
+documents takes 9% (`canada.json`) to 34% (`twitter.json`) longer; values built mostly from objects are affected the
+most.
 
 ## Default definition
 

tests/src/unit-diagnostic-positions.cpp

@@ -68,6 +68,61 @@ TEST_CASE("Better diagnostics with positions")
         CHECK(j.end_pos() == root.size());
     }
 
+    SECTION("copying keeps the positions of nested values (#5387)")
+    {
+        // Values nested deeper than the copy constructor's descent bound are
+        // copied without the call stack, on a path that has to carry the
+        // positions over itself; shallower ones copy their containers, which
+        // bring the positions along. Both sides of the bound are checked here.
+        const auto check_copy = [](std::size_t depth, bool objects)
+        {
+            CAPTURE(depth)
+            CAPTURE(objects)
+
+            const std::string open = objects ? R"({"a":)" : "[";
+            const std::string close = objects ? "}" : "]";
+
+            std::string text;
+            for (std::size_t i = 0; i < depth; ++i)
+            {
+                text += open;
+            }
+            text += "12";
+            for (std::size_t i = 0; i < depth; ++i)
+            {
+                text += close;
+            }
+
+            const json original = json::parse(text);
+            const json copy(original); // NOLINT(performance-unnecessary-copy-initialization)
+
+            const json* o = &original;
+            const json* c = &copy;
+            for (std::size_t level = 0; level <= depth; ++level)
+            {
+                CAPTURE(level)
+                REQUIRE(c->start_pos() == o->start_pos());
+                REQUIRE(c->end_pos() == o->end_pos());
+
+                if (level < depth)
+                {
+                    o = objects ? &o->at("a") : &o->at(0);
+                    c = objects ? &c->at("a") : &c->at(0);
+                }
+            }
+        };
+
+        const bool shapes[] = {false, true};
+        for (const bool objects : shapes)
+        {
+            check_copy(1, objects);
+            check_copy(127, objects);
+            check_copy(128, objects);
+            check_copy(129, objects);
+            check_copy(300, objects);
+        }
+    }
+
     SECTION("JSON patch add to primitive parent (#4292)")
     {
         // the JSON Patch "add" target /foo/bar/baz has a string parent

tests/src/unit-diagnostics.cpp

@@ -273,5 +273,62 @@ TEST_CASE("Regression tests for extended diagnostics")
         CHECK(j1["numbers"]["two"] == 2);
         CHECK(j1["string"] == "t");
     }
+
+    SECTION("Regression test for issue #5387 - copying keeps the parents of nested values")
+    {
+        // A value nested deeper than the copy constructor's descent bound is
+        // copied without the call stack. Every container that path creates has
+        // to have the parents of its children set, or the JSON Pointer in the
+        // diagnostic is cut short.
+        const std::size_t depth = 300;
+
+        SECTION("objects")
+        {
+            json j = "not a number";
+            std::string pointer;
+            for (std::size_t i = 0; i < depth; ++i)
+            {
+                j = json{{"a", j}};
+                pointer += "/a";
+            }
+
+            json const copy(j); // NOLINT(performance-unnecessary-copy-initialization)
+
+            const json* inner = &copy;
+            for (std::size_t i = 0; i < depth; ++i)
+            {
+                inner = &inner->at("a");
+            }
+
+            std::string const expected = "[json.exception.type_error.302] (" + pointer + ") type must be number, but is string";
+            int i = 0;
+            CHECK_THROWS_WITH_AS(i = inner->get<int>(), expected.c_str(), json::type_error);
+            CHECK(i == 0);
+        }
+
+        SECTION("arrays")
+        {
+            json j = "not a number";
+            std::string pointer;
+            for (std::size_t i = 0; i < depth; ++i)
+            {
+                j = json::array({j});
+                pointer += "/0";
+            }
+
+            json const copy(j); // NOLINT(performance-unnecessary-copy-initialization)
+
+            const json* inner = &copy;
+            for (std::size_t i = 0; i < depth; ++i)
+            {
+                inner = &inner->at(0);
+            }
+
+            std::string const expected = "[json.exception.type_error.302] (" + pointer + ") type must be number, but is string";
+            int i = 0;
+            CHECK_THROWS_WITH_AS(i = inner->get<int>(), expected.c_str(), json::type_error);
+            CHECK(i == 0);
+        }
+    }
 }