Construct a scalar member in place; only a throwing value goes through a temporary

Creating the value first and moving it over the member is needed only when
creating it can throw (a string, a binary value or a container). A scalar
or null is again constructed in place, without the extra move and destroy
per member (and, with JSON_DIAGNOSTICS, without the extra set_parents()).

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

docs/mkdocs/docs/api/basic_json/insert.md

@@ -65,17 +65,7 @@ the same index, which is now a different value.
 
 ## Exception safety
 
-1. Strong guarantee: if an exception is thrown, there are no changes in the JSON value.
-2. Basic guarantee: if an exception is thrown while the elements are copied (for example, `std::bad_alloc`), the JSON
-   value may be partially modified, but it stays a valid array.
-3. Basic guarantee: if an exception is thrown while the elements are copied (for example, `std::bad_alloc`), the JSON
-   value may be partially modified, but it stays a valid array.
-4. Basic guarantee: if an exception is thrown while the elements are copied (for example, `std::bad_alloc`), the JSON
-   value may be partially modified, but it stays a valid array.
-5. Basic guarantee: if an exception is thrown while the members are copied (for example, `std::bad_alloc`), the JSON
-   value may be partially modified, but it stays a valid object.
-
-The exceptions listed below are thrown before the JSON value is changed.
+Strong exception safety: if an exception occurs, the original value stays intact.
 
 ## Exceptions
 

include/nlohmann/detail/input/json_sax.hpp

@@ -1303,7 +1303,7 @@ class json_sax_dom_parser : public json_sax_dom_error_state
         {
             // a new member is null and holds nothing: construct the value
             // over it instead of destroying and swapping
-            ::new (static_cast<void*>(&member)) BasicJsonType(std::forward<Value>(v));
+            construct_member(member, std::forward<Value>(v), std::is_nothrow_constructible < BasicJsonType, Value && > {});
         }
         else
         {
@@ -1312,6 +1312,25 @@ class json_sax_dom_parser : public json_sax_dom_error_state
         }
     }
 
+    /// a scalar cannot throw: construct it in place
+    template<typename Value>
+    JSON_HEDLEY_ALWAYS_INLINE
+    static void construct_member(BasicJsonType& member, Value&& v, std::true_type /*nothrow*/)
+    {
+        ::new (static_cast<void*>(&member)) BasicJsonType(std::forward<Value>(v));
+    }
+
+    /// creating a container or a string can throw, and a constructor that was
+    /// interrupted must not have touched the member (#5985): create the value
+    /// first, then move it over the member (the move constructor is noexcept)
+    template<typename Value>
+    JSON_HEDLEY_ALWAYS_INLINE
+    static void construct_member(BasicJsonType& member, Value&& v, std::false_type /*may throw*/)
+    {
+        BasicJsonType value(std::forward<Value>(v));
+        ::new (static_cast<void*>(&member)) BasicJsonType(std::move(value));
+    }
+
     /// a custom base class may care about the lifetime of the value
     template<typename Value>
     static void set_member(BasicJsonType& member, Value&& v, std::false_type /*custom base*/)

include/nlohmann/json.hpp

@@ -4914,30 +4914,6 @@ public:
         return {it, res.second};
     }
 
-  private:
-    /// Sets the parents of a value when it goes out of scope, also when an
-    /// exception is thrown: a container insertion that throws may already
-    /// have added elements, whose parents must be set for the value to stay
-    /// valid (basic exception guarantee). Does nothing without JSON_DIAGNOSTICS.
-    class set_parents_on_exit
-    {
-      public:
-        explicit set_parents_on_exit(basic_json* v) noexcept : m_value(v) {}
-        set_parents_on_exit(const set_parents_on_exit&) = delete;
-        set_parents_on_exit& operator=(const set_parents_on_exit&) = delete;
-        set_parents_on_exit(set_parents_on_exit&&) = delete;
-        set_parents_on_exit& operator=(set_parents_on_exit&&) = delete;
-
-        ~set_parents_on_exit()
-        {
-            m_value->set_parents();
-        }
-
-      private:
-        basic_json* m_value;
-    };
-
-  public:
     /// Helper for insertion of an iterator
     /// @note: This uses std::distance to support GCC 4.8,
     ///        see https://github.com/nlohmann/json/pull/1257
@@ -4947,7 +4923,6 @@ public:
         iterator result(this);
         JSON_ASSERT(m_data.m_value.array != nullptr);
 
-        const set_parents_on_exit parents(this);
         auto insert_pos = std::distance(m_data.m_value.array->begin(), pos.m_it.array_iterator);
         m_data.m_value.array->insert(pos.m_it.array_iterator, std::forward<Args>(args)...);
         result.m_it.array_iterator = m_data.m_value.array->begin() + insert_pos;
@@ -4956,6 +4931,7 @@ public:
         // result.m_it.array_iterator = m_data.m_value.array->insert(pos.m_it.array_iterator, cnt, val);
         // but the return value of insert is missing in GCC 4.8, so it is written this way instead.
 
+        set_parents();
         return result;
     }
 
@@ -4975,16 +4951,8 @@ public:
             throw_iterator_from_other_value(this);
         }
 
-        // vector::insert has no effects if copying a single element to the
-        // end throws; elsewhere, it may already have shifted the elements, so
-        // copy first and insert by move, which cannot throw but by allocating
-        if (pos.m_it.array_iterator == m_data.m_value.array->end())
-        {
-            return insert_iterator(pos, val);
-        }
-
-        basic_json tmp(val);
-        return insert_iterator(pos, std::move(tmp));
+        // insert to array and return iterator
+        return insert_iterator(pos, val);
     }
 
     /// @brief inserts element into array
@@ -5116,8 +5084,8 @@ public:
             detail::throw_literal<invalid_iterator>(detail::exception_id::iterator_from_other_value, "iterators first and last must point to objects", this);
         }
 
-        const set_parents_on_exit parents(this);
         m_data.m_value.object->insert(first.m_it.object_iterator, last.m_it.object_iterator);
+        set_parents();
     }
 
     /// @brief updates a JSON object from another object, overwriting existing keys

single_include/nlohmann/json.hpp

@@ -16948,7 +16948,7 @@ class json_sax_dom_parser : public json_sax_dom_error_state
         {
             // a new member is null and holds nothing: construct the value
             // over it instead of destroying and swapping
-            ::new (static_cast<void*>(&member)) BasicJsonType(std::forward<Value>(v));
+            construct_member(member, std::forward<Value>(v), std::is_nothrow_constructible < BasicJsonType, Value && > {});
         }
         else
         {
@@ -16957,6 +16957,25 @@ class json_sax_dom_parser : public json_sax_dom_error_state
         }
     }
 
+    /// a scalar cannot throw: construct it in place
+    template<typename Value>
+    JSON_HEDLEY_ALWAYS_INLINE
+    static void construct_member(BasicJsonType& member, Value&& v, std::true_type /*nothrow*/)
+    {
+        ::new (static_cast<void*>(&member)) BasicJsonType(std::forward<Value>(v));
+    }
+
+    /// creating a container or a string can throw, and a constructor that was
+    /// interrupted must not have touched the member (#5985): create the value
+    /// first, then move it over the member (the move constructor is noexcept)
+    template<typename Value>
+    JSON_HEDLEY_ALWAYS_INLINE
+    static void construct_member(BasicJsonType& member, Value&& v, std::false_type /*may throw*/)
+    {
+        BasicJsonType value(std::forward<Value>(v));
+        ::new (static_cast<void*>(&member)) BasicJsonType(std::move(value));
+    }
+
     /// a custom base class may care about the lifetime of the value
     template<typename Value>
     static void set_member(BasicJsonType& member, Value&& v, std::false_type /*custom base*/)
@@ -42706,30 +42725,6 @@ public:
         return {it, res.second};
     }
 
-  private:
-    /// Sets the parents of a value when it goes out of scope, also when an
-    /// exception is thrown: a container insertion that throws may already
-    /// have added elements, whose parents must be set for the value to stay
-    /// valid (basic exception guarantee). Does nothing without JSON_DIAGNOSTICS.
-    class set_parents_on_exit
-    {
-      public:
-        explicit set_parents_on_exit(basic_json* v) noexcept : m_value(v) {}
-        set_parents_on_exit(const set_parents_on_exit&) = delete;
-        set_parents_on_exit& operator=(const set_parents_on_exit&) = delete;
-        set_parents_on_exit(set_parents_on_exit&&) = delete;
-        set_parents_on_exit& operator=(set_parents_on_exit&&) = delete;
-
-        ~set_parents_on_exit()
-        {
-            m_value->set_parents();
-        }
-
-      private:
-        basic_json* m_value;
-    };
-
-  public:
     /// Helper for insertion of an iterator
     /// @note: This uses std::distance to support GCC 4.8,
     ///        see https://github.com/nlohmann/json/pull/1257
@@ -42739,7 +42734,6 @@ public:
         iterator result(this);
         JSON_ASSERT(m_data.m_value.array != nullptr);
 
-        const set_parents_on_exit parents(this);
         auto insert_pos = std::distance(m_data.m_value.array->begin(), pos.m_it.array_iterator);
         m_data.m_value.array->insert(pos.m_it.array_iterator, std::forward<Args>(args)...);
         result.m_it.array_iterator = m_data.m_value.array->begin() + insert_pos;
@@ -42748,6 +42742,7 @@ public:
         // result.m_it.array_iterator = m_data.m_value.array->insert(pos.m_it.array_iterator, cnt, val);
         // but the return value of insert is missing in GCC 4.8, so it is written this way instead.
 
+        set_parents();
         return result;
     }
 
@@ -42767,16 +42762,8 @@ public:
             throw_iterator_from_other_value(this);
         }
 
-        // vector::insert has no effects if copying a single element to the
-        // end throws; elsewhere, it may already have shifted the elements, so
-        // copy first and insert by move, which cannot throw but by allocating
-        if (pos.m_it.array_iterator == m_data.m_value.array->end())
-        {
-            return insert_iterator(pos, val);
-        }
-
-        basic_json tmp(val);
-        return insert_iterator(pos, std::move(tmp));
+        // insert to array and return iterator
+        return insert_iterator(pos, val);
     }
 
     /// @brief inserts element into array
@@ -42908,8 +42895,8 @@ public:
             detail::throw_literal<invalid_iterator>(detail::exception_id::iterator_from_other_value, "iterators first and last must point to objects", this);
         }
 
-        const set_parents_on_exit parents(this);
         m_data.m_value.object->insert(first.m_it.object_iterator, last.m_it.object_iterator);
+        set_parents();
     }
 
     /// @brief updates a JSON object from another object, overwriting existing keys

tests/src/unit-allocator.cpp

@@ -12,7 +12,7 @@
 #include <nlohmann/json.hpp>
 using nlohmann::json;
 
-#include <functional>
+#include <limits>
 #include <valarray>
 #if JSON_HAS_RANGES
     #include <ranges>
@@ -791,7 +791,7 @@ TEST_CASE("a failed allocation leaves the value unchanged")
 #if JSON_HAS_RANGES && !defined(__MINGW32__)
         const std::vector<int> numbers = {1, 2};
         next_construct_fails = true;
-        CHECK_THROWS_AS(nlohmann::to_json(j, numbers | std::views::filter([](int /*unused*/)
+        CHECK_THROWS_AS(nlohmann::to_json(j, numbers | std::views::filter([](int /*unused*/) noexcept
         {
             return true;
         })), std::bad_alloc&);
@@ -806,221 +806,171 @@ TEST_CASE("a failed allocation leaves the value unchanged")
 }
 #endif
 
-// the no-exceptions CI job has no std::bad_alloc to catch; with iterator
-// debugging (MSVC STL debug builds) containers allocate inside noexcept
-// functions, so failing an allocation there terminates the program
-#if !defined(JSON_NOEXCEPTION) && !(defined(_ITERATOR_DEBUG_LEVEL) && _ITERATOR_DEBUG_LEVEL > 0)
+#if !defined(JSON_NOEXCEPTION)
 namespace
 {
-// whether every child of j names j as its parent (always true without
-// JSON_DIAGNOSTICS, where no parents are stored)
-template<class BasicJsonType>
-bool children_name_their_parent(const BasicJsonType& j)
-{
-#if JSON_DIAGNOSTICS
-    if (j.is_array())
-    {
-        for (const auto& element : *j.m_data.m_value.array)
-        {
-            if (element.m_parent != &j)
-            {
-                return false;
-            }
-        }
-    }
-    else if (j.is_object())
-    {
-        for (const auto& element : *j.m_data.m_value.object)
-        {
-            if (element.second.m_parent != &j)
-            {
-                return false;
-            }
-        }
-    }
-#else
-    static_cast<void>(j);
-#endif
-    return true;
-}
+// the allocation numbered sweep_fail_at (counted from 1) throws; 0 never fails
+std::size_t sweep_allocations = 0;
+std::size_t sweep_fail_at = 0;
+std::ptrdiff_t sweep_live = 0;
 
-enum class guarantee
+template<class T>
+struct sweep_allocator
 {
-    basic, // the value stays valid and keeps its type
-    strong // the value is unchanged
+    using value_type = T;
+
+    sweep_allocator() = default;
+    template<class U> sweep_allocator(const sweep_allocator<U>& /*unused*/) noexcept {}
+
+    T* allocate(std::size_t n)
+    {
+        if (sweep_fail_at != 0 && ++sweep_allocations == sweep_fail_at)
+        {
+            throw std::bad_alloc();
+        }
+        ++sweep_live;
+        return std::allocator<T>().allocate(n);
+    }
+
+    void deallocate(T* p, std::size_t n) noexcept
+    {
+        --sweep_live;
+        std::allocator<T>().deallocate(p, n);
+    }
+
+    // GCC 4.8's std::map calls these on the allocator directly instead of
+    // going through std::allocator_traits
+    std::size_t max_size() const noexcept
+    {
+        return (std::numeric_limits<std::size_t>::max)() / sizeof(T);
+    }
+
+    template<class U, class... Args>
+    void construct(U* p, Args&& ... args) noexcept(noexcept(::new (static_cast<void*>(p)) U(std::forward<Args>(args)...)))
+    {
+        ::new (static_cast<void*>(p)) U(std::forward<Args>(args)...);
+    }
+
+    template<class U>
+    void destroy(U* p) noexcept
+    {
+        static_cast<void>(p); // unused for trivially destructible types
+        p->~U();
+    }
+
+    template <class U>
+    struct rebind
+    {
+        using other = sweep_allocator<U>;
+    };
+
+    bool operator==(const sweep_allocator& /*unused*/) const noexcept
+    {
+        return true;
+    }
+    bool operator!=(const sweep_allocator& /*unused*/) const noexcept
+    {
+        return false;
+    }
 };
 
-// Runs op on a fresh value make() with the 0th, 1st, 2nd, ... allocation made
-// by the library failing in turn, until op completes. Every failed attempt
-// must have thrown std::bad_alloc and left the value valid, with all parent
-// pointers intact (copying a value asserts the invariant), and for the strong
-// guarantee exactly as it was.
-template<class BasicJsonType>
-void check_exception_guarantee(guarantee g,
-                               const std::function<BasicJsonType()>& make,
-                               const std::function<void(BasicJsonType&)>& op)
+// run make() with the allocation numbered k failing, for k = 1, 2, ...: every
+// failure must be a std::bad_alloc and must not leave anything behind, and
+// the first k without a failure must yield the value made without failures
+template<class BasicJsonType, class Make>
+void sweep_allocations_of(const Make& make)
 {
-    fail_at_alloc_call = -1;
+    sweep_fail_at = 0;
+    const BasicJsonType expected = make();
+    const std::ptrdiff_t live_before = sweep_live;
 
-    bool completed = false;
     std::size_t failures = 0;
-    for (std::size_t n = 0; !completed; ++n)
+    for (std::size_t k = 1; k < 10000; ++k)
     {
-        CAPTURE(n)
-        REQUIRE(n < 1000);
-
-        BasicJsonType j = make();
-        const std::string before = j.dump();
-        const auto type_before = j.type();
-
-        alloc_call_count = 0;
-        fail_at_alloc_call = static_cast<long>(n);
+        sweep_allocations = 0;
+        sweep_fail_at = k;
         try
         {
-            op(j);
-            completed = true;
+            const BasicJsonType result = make();
+            sweep_fail_at = 0;
+            CHECK(result == expected);
+            break;
         }
         catch (const std::bad_alloc&)
         {
+            sweep_fail_at = 0;
             ++failures;
         }
-        fail_at_alloc_call = -1;
-
-        if (!completed)
-        {
-            CHECK((j.type() == type_before));
-            CHECK(children_name_their_parent(j));
-            const BasicJsonType copy(j); // NOLINT(performance-unnecessary-copy-initialization): the copy is the point, it allocates
-            CHECK(copy.dump() == j.dump());
-            if (g == guarantee::strong)
-            {
-                CHECK(j.dump() == before);
-            }
-        }
-    }
-
-    // the operation allocates, so at least one attempt must have failed
-    CHECK(failures > 0);
-}
-
-template<class BasicJsonType>
-void check_insert_exception_guarantees()
-{
-    using json_t = BasicJsonType;
-    using make_t = std::function<json_t()>;
-
-    // strings of this length do not fit into the small string buffer, so
-    // copying them allocates
-    const json_t val = std::string(64, 'x');
-    const json_t source = json_t::array({std::string(64, 'p'), std::string(64, 'q'), std::string(64, 'r')});
-
-    for (const bool spare_capacity :
-            {
-                false, true
-            })
-    {
-        CAPTURE(spare_capacity)
-
-        // [ "aaa...", "bbb...", "ccc..." ], with a spare slot if requested
-        const make_t make = [spare_capacity]()
-        {
-            json_t j = json_t::array({std::string(64, 'a'), std::string(64, 'b'), std::string(64, 'c')});
-            if (spare_capacity)
-            {
-                j.push_back(std::string(64, 'd'));
-                j.erase(3);
-            }
-            return json_t(std::move(j));
-        };
-
-        for (const bool at_end :
-                {
-                    false, true
-                })
-        {
-            CAPTURE(at_end)
-
-            SECTION("insert(pos, const basic_json&)")
-            {
-                check_exception_guarantee<json_t>(guarantee::strong, make, [&](json_t& j)
-                {
-                    j.insert(at_end ? j.end() : j.begin() + 1, val);
-                });
-            }
-
-            SECTION("insert(pos, basic_json&&)")
-            {
-                check_exception_guarantee<json_t>(guarantee::strong, make, [&](json_t& j)
-                {
-                    j.insert(at_end ? j.end() : j.begin() + 1, json_t(val));
-                });
-            }
-
-            SECTION("insert(pos, size_type, const basic_json&)")
-            {
-                check_exception_guarantee<json_t>(guarantee::basic, make, [&](json_t& j)
-                {
-                    j.insert(at_end ? j.end() : j.begin() + 1, 3, val);
-                });
-            }
-
-            SECTION("insert(pos, const_iterator, const_iterator)")
-            {
-                check_exception_guarantee<json_t>(guarantee::basic, make, [&](json_t& j)
-                {
-                    j.insert(at_end ? j.end() : j.begin() + 1, source.begin(), source.end());
-                });
-            }
-
-            SECTION("insert(pos, initializer_list_t)")
-            {
-                check_exception_guarantee<json_t>(guarantee::basic, make, [&](json_t& j)
-                {
-                    j.insert(at_end ? j.end() : j.begin() + 1, {val, val});
-                });
-            }
-        }
-    }
-
-    SECTION("insert(const_iterator, const_iterator) for objects")
-    {
-        const json_t members = {{"m", std::string(64, 'm')}, {"n", std::string(64, 'n')}, {"o", std::string(64, 'o')}};
-
-        SECTION("no member exists yet")
-        {
-            check_exception_guarantee<json_t>(guarantee::basic, []()
-            {
-                return json_t({{"a", 1}});
-            }, [&](json_t& j)
-            {
-                j.insert(members.begin(), members.end());
-            });
-        }
-
-        SECTION("some members exist already")
-        {
-            check_exception_guarantee<json_t>(guarantee::basic, []()
-            {
-                return json_t({{"a", 1}, {"n", std::string(64, 'x')}});
-            }, [&](json_t& j)
-            {
-                j.insert(members.begin(), members.end());
-            });
-        }
+        CHECK(sweep_live == live_before);
     }
+    sweep_fail_at = 0;
+    CHECK(failures > 10);
+    CHECK(sweep_live == live_before);
 }
 } // namespace
 
-TEST_CASE("insert() provides its documented exception guarantees (#5986)")
+TEST_CASE("parse() survives a failing allocation at every point (#5985)")
 {
-    SECTION("std::map-backed object_t")
+    // strings beyond the small string buffer, nested objects and arrays
+    const std::string text = R"({"a":"a long string that does not fit into the small string buffer1","b":[1,2,[3,{"k":"another string that is longer than any small string buffer"}]],"c":{"x":1,"y":{"z":[]},"w":"yet another string that does not fit into a small buffer"},"d":1,"b":[[],{}]})";
+
+    using sweep_json = nlohmann::json::with_allocator_t<sweep_allocator>;
+    using sweep_ordered_json = nlohmann::ordered_json::with_allocator_t<sweep_allocator>;
+
+    SECTION("json")
     {
-        check_insert_exception_guarantees<nlohmann::json::with_allocator_t<nth_alloc_fails_allocator>>();
+        sweep_allocations_of<sweep_json>([&]
+        {
+            return sweep_json::parse(text);
+        });
     }
 
-    SECTION("ordered_map-backed object_t")
+    SECTION("ordered_json")
     {
-        check_insert_exception_guarantees<nlohmann::ordered_json::with_allocator_t<nth_alloc_fails_allocator>>();
+        sweep_allocations_of<sweep_ordered_json>([&]
+        {
+            return sweep_ordered_json::parse(text);
+        });
+    }
+
+    // With iterator debugging (MSVC STL debug builds, also used by clang-cl),
+    // containers allocate a debug proxy through the allocator inside their
+    // noexcept move constructors, so failing that allocation terminates the
+    // program instead of throwing std::bad_alloc. Nothing to check there.
+#if !(defined(_ITERATOR_DEBUG_LEVEL) && _ITERATOR_DEBUG_LEVEL > 0)
+    SECTION("parser callback")
+    {
+        const auto keep_all = [](int /*depth*/, sweep_json::parse_event_t /*event*/, sweep_json& /*parsed*/) noexcept
+        {
+            return true;
+        };
+        sweep_allocations_of<sweep_json>([&]
+        {
+            return sweep_json::parse(text, keep_all);
+        });
+
+        const auto drop_d = [](int /*depth*/, sweep_json::parse_event_t event, sweep_json & parsed)
+        {
+            return event != sweep_json::parse_event_t::key || parsed != "d";
+        };
+        sweep_allocations_of<sweep_json>([&]
+        {
+            return sweep_json::parse(text, drop_d);
+        });
+    }
+#endif
+
+    SECTION("sax_parse into an existing value")
+    {
+        // a failed parse leaves the target in a state that is safe to destroy
+        using sax_t = nlohmann::detail::json_sax_dom_parser<sweep_json, nlohmann::detail::iterator_input_adapter<const char*>>;
+        sweep_allocations_of<sweep_json>([&]() -> sweep_json
+        {
+            sweep_json value;
+            sax_t sax(value, false);
+            static_cast<void>(sweep_json::sax_parse(text, &sax));
+            return value;
+        });
     }
 }
 #endif

tests/src/unit-json_view_builder.cpp

@@ -692,7 +692,7 @@ TEST_CASE("json_view builder: fraction digits")
             check_same(number);
             check_same("[" + number + "]");
             std::string pair;
-            pair.reserve((number.size() * 2) + 3);
+            pair.reserve(number.size() * 2 + 3);
             pair += '[';
             pair += number;
             pair += ',';

tests/src/unit-json_view_positions.cpp

@@ -521,8 +521,7 @@ TEST_CASE("json_view exceptions name the path and the bytes of the value")
         std::string text = "7";
         for (int level = 0; level < 40; ++level)
         {
-            text.insert(0, R"([[1, [2]], )");
-            text += R"(, {"k": [3, {"l": [4]}]}, [5, [6, [7]]]])";
+            text = R"([[1, [2]], )" + text + R"(, {"k": [3, {"l": [4]}]}, [5, [6, [7]]]])";
         }
         CAPTURE(text)
         const json j = json::parse(text);