Fix destroy() for ObjectTypes without reverse iteration

The non-recursive destroy walk from #5762 picked an object's last child
via object_t::rbegin() and std::prev(end()). Neither is available for
every ObjectType: no_key_compare_map in unit-custom-object-type.cpp has
no rbegin(), so develop no longer compiles that test, and hash maps such
as std::unordered_map only have forward iterators.

The walk can take an object's children in any order, as long as it
finds the same child again while the object is not modified in between.
So objects with bidirectional iterators keep using their last child
(O(1) to remove from vector-based maps like ordered_map), and objects
with forward-only iterators use begin() instead. No reverse iteration
or rbegin() is needed any more, and the walk stays allocation-free.

Adds a forward-only ObjectType to the tests, destroyed both with mixed
nesting and 100000 levels deep.

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

include/nlohmann/json.hpp

@@ -638,21 +638,49 @@ private:
             }
         }
 
-        static basic_json& last_child(basic_json& v)
+        // The walk in destroy_container() may take the children of a
+        // container in any order, as long as it picks the same child again
+        // while that container is not modified in between. Arrays and
+        // objects with bidirectional iterators (std::map, ordered_map, ...)
+        // use their last child, which a vector-based container can remove
+        // in O(1). ObjectType only needs forward iterators, though (e.g.
+        // std::unordered_map), so other objects use their first child.
+        template<typename ObjectType_>
+        static typename ObjectType_::iterator walk_child_it(ObjectType_& o, std::bidirectional_iterator_tag /*unused*/)
+        {
+            return std::prev(o.end());
+        }
+
+        template<typename ObjectType_>
+        static typename ObjectType_::iterator walk_child_it(ObjectType_& o, std::forward_iterator_tag /*unused*/)
+        {
+            return o.begin();
+        }
+
+        template<typename ObjectType_>
+        static typename ObjectType_::iterator walk_child_it(ObjectType_& o)
+        {
+            JSON_ASSERT(!o.empty());
+            return walk_child_it(o, typename std::iterator_traits<typename ObjectType_::iterator>::iterator_category());
+        }
+
+        // the child of a non-empty array/object v that the walk in
+        // destroy_container() continues with (see walk_child_it() above)
+        static basic_json& walk_child(basic_json& v)
         {
             if (v.m_data.m_type == value_t::array)
             {
                 return v.m_data.m_value.array->back();
             }
             JSON_ASSERT(v.m_data.m_type == value_t::object);
-            return v.m_data.m_value.object->rbegin()->second;
+            return walk_child_it(*v.m_data.m_value.object)->second;
         }
 
-        // removes the last child of a non-empty array/object v; this never
+        // removes walk_child(v) from a non-empty array/object v; this never
         // allocates, and since it is only ever called when that child is a
         // scalar or an already-empty array/object, destroying it never
         // recurses more than one level deep (see destroy() below)
-        static void pop_last_child(basic_json& v)
+        static void pop_walk_child(basic_json& v)
         {
             if (v.m_data.m_type == value_t::array)
             {
@@ -661,9 +689,7 @@ private:
             else
             {
                 JSON_ASSERT(v.m_data.m_type == value_t::object);
-                // erase() needs a forward iterator, so std::prev(end()) is
-                // used here rather than rbegin() (see last_child() above)
-                v.m_data.m_value.object->erase(std::prev(v.m_data.m_value.object->end()));
+                v.m_data.m_value.object->erase(walk_child_it(*v.m_data.m_value.object));
             }
         }
 
@@ -734,14 +760,17 @@ public:
             // bad_alloc, which would escape this noexcept destructor and
             // terminate the program (#5135).
             //
-            // Instead, walk down the "last child" chain, reversing links
-            // as we go: cur is the container currently being emptied,
-            // and prev is its parent (value_t::null when there is none).
-            // Each parent's last child slot doubles as storage for that
-            // parent's own parent link while we are below it, so no
-            // extra memory is needed. We only ever remove a child once
-            // it is a scalar or an empty array/object, which neither
-            // allocates nor recurses more than one level deep.
+            // Instead, walk down a chain of children (always the one
+            // walk_child() picks), reversing links as we go: cur is the
+            // container currently being emptied, and prev is its parent
+            // (value_t::null when there is none). Each parent's
+            // walk_child() slot doubles as storage for that parent's own
+            // parent link while we are below it, so no extra memory is
+            // needed; the parent is not modified meanwhile, so
+            // walk_child() finds that same slot again on the way up. We
+            // only ever remove a child once it is a scalar or an empty
+            // array/object, which neither allocates nor recurses more
+            // than one level deep.
             //
             // This json_value is not itself a basic_json, so the
             // top-level container is first moved into a local stand-in
@@ -767,11 +796,11 @@ public:
                     }
 
                     // ascend: detach the grandparent link from prev's
-                    // last slot, drop that (now null) slot, free cur
+                    // walk_child() slot, drop that (now null) slot, free cur
                     // (it is empty), then move up one level
                     basic_json gp;
-                    take(gp, last_child(prev));
-                    pop_last_child(prev);
+                    take(gp, walk_child(prev));
+                    pop_walk_child(prev);
 
                     free_container(cur);
 
@@ -780,21 +809,21 @@ public:
                     continue;
                 }
 
-                basic_json& cur_last_ref = last_child(cur);
+                basic_json& cur_child_ref = walk_child(cur);
 
-                if (has_no_children(cur_last_ref))
+                if (has_no_children(cur_child_ref))
                 {
                     // scalar, or already-empty array/object
-                    pop_last_child(cur);
+                    pop_walk_child(cur);
                     continue;
                 }
 
-                // descend into the non-empty last child, reversing the
+                // descend into the non-empty child, reversing the
                 // link: its slot takes over prev, and the child becomes
                 // the new cur
                 basic_json tmp;
-                take(tmp, cur_last_ref);
-                take(cur_last_ref, prev);
+                take(tmp, cur_child_ref);
+                take(cur_child_ref, prev);
                 take(prev, cur);
                 take(cur, tmp);
             }

single_include/nlohmann/json.hpp

@@ -27877,21 +27877,49 @@ private:
             }
         }
 
-        static basic_json& last_child(basic_json& v)
+        // The walk in destroy_container() may take the children of a
+        // container in any order, as long as it picks the same child again
+        // while that container is not modified in between. Arrays and
+        // objects with bidirectional iterators (std::map, ordered_map, ...)
+        // use their last child, which a vector-based container can remove
+        // in O(1). ObjectType only needs forward iterators, though (e.g.
+        // std::unordered_map), so other objects use their first child.
+        template<typename ObjectType_>
+        static typename ObjectType_::iterator walk_child_it(ObjectType_& o, std::bidirectional_iterator_tag /*unused*/)
+        {
+            return std::prev(o.end());
+        }
+
+        template<typename ObjectType_>
+        static typename ObjectType_::iterator walk_child_it(ObjectType_& o, std::forward_iterator_tag /*unused*/)
+        {
+            return o.begin();
+        }
+
+        template<typename ObjectType_>
+        static typename ObjectType_::iterator walk_child_it(ObjectType_& o)
+        {
+            JSON_ASSERT(!o.empty());
+            return walk_child_it(o, typename std::iterator_traits<typename ObjectType_::iterator>::iterator_category());
+        }
+
+        // the child of a non-empty array/object v that the walk in
+        // destroy_container() continues with (see walk_child_it() above)
+        static basic_json& walk_child(basic_json& v)
         {
             if (v.m_data.m_type == value_t::array)
             {
                 return v.m_data.m_value.array->back();
             }
             JSON_ASSERT(v.m_data.m_type == value_t::object);
-            return v.m_data.m_value.object->rbegin()->second;
+            return walk_child_it(*v.m_data.m_value.object)->second;
         }
 
-        // removes the last child of a non-empty array/object v; this never
+        // removes walk_child(v) from a non-empty array/object v; this never
         // allocates, and since it is only ever called when that child is a
         // scalar or an already-empty array/object, destroying it never
         // recurses more than one level deep (see destroy() below)
-        static void pop_last_child(basic_json& v)
+        static void pop_walk_child(basic_json& v)
         {
             if (v.m_data.m_type == value_t::array)
             {
@@ -27900,9 +27928,7 @@ private:
             else
             {
                 JSON_ASSERT(v.m_data.m_type == value_t::object);
-                // erase() needs a forward iterator, so std::prev(end()) is
-                // used here rather than rbegin() (see last_child() above)
-                v.m_data.m_value.object->erase(std::prev(v.m_data.m_value.object->end()));
+                v.m_data.m_value.object->erase(walk_child_it(*v.m_data.m_value.object));
             }
         }
 
@@ -27973,14 +27999,17 @@ public:
             // bad_alloc, which would escape this noexcept destructor and
             // terminate the program (#5135).
             //
-            // Instead, walk down the "last child" chain, reversing links
-            // as we go: cur is the container currently being emptied,
-            // and prev is its parent (value_t::null when there is none).
-            // Each parent's last child slot doubles as storage for that
-            // parent's own parent link while we are below it, so no
-            // extra memory is needed. We only ever remove a child once
-            // it is a scalar or an empty array/object, which neither
-            // allocates nor recurses more than one level deep.
+            // Instead, walk down a chain of children (always the one
+            // walk_child() picks), reversing links as we go: cur is the
+            // container currently being emptied, and prev is its parent
+            // (value_t::null when there is none). Each parent's
+            // walk_child() slot doubles as storage for that parent's own
+            // parent link while we are below it, so no extra memory is
+            // needed; the parent is not modified meanwhile, so
+            // walk_child() finds that same slot again on the way up. We
+            // only ever remove a child once it is a scalar or an empty
+            // array/object, which neither allocates nor recurses more
+            // than one level deep.
             //
             // This json_value is not itself a basic_json, so the
             // top-level container is first moved into a local stand-in
@@ -28006,11 +28035,11 @@ public:
                     }
 
                     // ascend: detach the grandparent link from prev's
-                    // last slot, drop that (now null) slot, free cur
+                    // walk_child() slot, drop that (now null) slot, free cur
                     // (it is empty), then move up one level
                     basic_json gp;
-                    take(gp, last_child(prev));
-                    pop_last_child(prev);
+                    take(gp, walk_child(prev));
+                    pop_walk_child(prev);
 
                     free_container(cur);
 
@@ -28019,21 +28048,21 @@ public:
                     continue;
                 }
 
-                basic_json& cur_last_ref = last_child(cur);
+                basic_json& cur_child_ref = walk_child(cur);
 
-                if (has_no_children(cur_last_ref))
+                if (has_no_children(cur_child_ref))
                 {
                     // scalar, or already-empty array/object
-                    pop_last_child(cur);
+                    pop_walk_child(cur);
                     continue;
                 }
 
-                // descend into the non-empty last child, reversing the
+                // descend into the non-empty child, reversing the
                 // link: its slot takes over prev, and the child becomes
                 // the new cur
                 basic_json tmp;
-                take(tmp, cur_last_ref);
-                take(cur_last_ref, prev);
+                take(tmp, cur_child_ref);
+                take(cur_child_ref, prev);
                 take(prev, cur);
                 take(cur, tmp);
             }

tests/src/unit-custom-object-type.cpp

@@ -10,7 +10,9 @@
 
 #include <nlohmann/json.hpp>
 
+#include <cstddef>
 #include <cstdint>
+#include <iterator>
 #include <map>
 #include <string>
 #include <type_traits>
@@ -196,6 +198,198 @@ struct void_erase_map : std::map<Key, T, Compare, Allocator>
 
 using void_erase_json = nlohmann::basic_json<void_erase_map>;
 
+// wraps an iterator, but only offers the LegacyForwardIterator operations,
+// like the iterators of std::unordered_map and other hash maps
+template<class BaseIterator>
+class forward_only_iterator
+{
+    BaseIterator m_it{};
+
+  public:
+    using iterator_category = std::forward_iterator_tag;
+    using value_type = typename std::iterator_traits<BaseIterator>::value_type;
+    using difference_type = typename std::iterator_traits<BaseIterator>::difference_type;
+    using pointer = typename std::iterator_traits<BaseIterator>::pointer;
+    using reference = typename std::iterator_traits<BaseIterator>::reference;
+
+    forward_only_iterator() = default;
+    explicit forward_only_iterator(BaseIterator it) : m_it(it) {}
+
+    BaseIterator base() const
+    {
+        return m_it;
+    }
+
+    reference operator*() const
+    {
+        return *m_it;
+    }
+    pointer operator->() const
+    {
+        return &*m_it;
+    }
+    forward_only_iterator& operator++()
+    {
+        ++m_it;
+        return *this;
+    }
+    forward_only_iterator operator++(int)
+    {
+        auto result = *this;
+        ++m_it;
+        return result;
+    }
+
+    friend bool operator==(const forward_only_iterator& lhs, const forward_only_iterator& rhs)
+    {
+        return lhs.m_it == rhs.m_it;
+    }
+    friend bool operator!=(const forward_only_iterator& lhs, const forward_only_iterator& rhs)
+    {
+        return lhs.m_it != rhs.m_it;
+    }
+};
+
+// An ObjectType whose iterators are forward-only, as those of hash maps are;
+// it has no rbegin() and its iterators no operator--. A hash map is not used
+// directly for the same reason as in no_key_compare_map above.
+template<class Key, class T, class Compare, class Allocator>
+class forward_only_map
+{
+    using map_t = std::map<Key, T, Compare, Allocator>;
+    map_t data;
+
+  public:
+    using key_type = typename map_t::key_type;
+    using mapped_type = typename map_t::mapped_type;
+    using value_type = typename map_t::value_type;
+    using size_type = typename map_t::size_type;
+    using allocator_type = typename map_t::allocator_type;
+    using iterator = forward_only_iterator<typename map_t::iterator>;
+    using const_iterator = forward_only_iterator<typename map_t::const_iterator>;
+
+    forward_only_map() noexcept(std::is_nothrow_default_constructible<map_t>::value) : data() {}
+
+    template<class InputIt>
+    forward_only_map(InputIt first, InputIt last) : data(first, last) {}
+
+    iterator begin() noexcept
+    {
+        return iterator(data.begin());
+    }
+    iterator end() noexcept
+    {
+        return iterator(data.end());
+    }
+    const_iterator begin() const noexcept
+    {
+        return const_iterator(data.begin());
+    }
+    const_iterator end() const noexcept
+    {
+        return const_iterator(data.end());
+    }
+    const_iterator cbegin() const noexcept
+    {
+        return const_iterator(data.cbegin());
+    }
+    const_iterator cend() const noexcept
+    {
+        return const_iterator(data.cend());
+    }
+
+    bool empty() const noexcept
+    {
+        return data.empty();
+    }
+    size_type size() const noexcept
+    {
+        return data.size();
+    }
+    size_type max_size() const noexcept
+    {
+        return data.max_size();
+    }
+    void clear() noexcept
+    {
+        data.clear();
+    }
+
+    iterator find(const key_type& key)
+    {
+        return iterator(data.find(key));
+    }
+    const_iterator find(const key_type& key) const
+    {
+        return const_iterator(data.find(key));
+    }
+    size_type count(const key_type& key) const
+    {
+        return data.count(key);
+    }
+
+    std::pair<iterator, bool> emplace(const key_type& key, const mapped_type& value)
+    {
+        const auto result = data.emplace(key, value);
+        return {iterator(result.first), result.second};
+    }
+
+    std::pair<iterator, bool> insert(const value_type& value)
+    {
+        const auto result = data.insert(value);
+        return {iterator(result.first), result.second};
+    }
+
+    template<class InputIt>
+    void insert(InputIt first, InputIt last)
+    {
+        data.insert(first, last);
+    }
+
+    mapped_type& operator[](const key_type& key)
+    {
+        return data[key];
+    }
+
+    mapped_type& at(const key_type& key)
+    {
+        return data.at(key);
+    }
+    const mapped_type& at(const key_type& key) const
+    {
+        return data.at(key);
+    }
+
+    iterator erase(iterator pos)
+    {
+        return iterator(data.erase(pos.base()));
+    }
+    iterator erase(iterator first, iterator last)
+    {
+        return iterator(data.erase(first.base(), last.base()));
+    }
+    size_type erase(const key_type& key)
+    {
+        return data.erase(key);
+    }
+
+    void swap(forward_only_map& other) noexcept(noexcept(data.swap(other.data)))
+    {
+        data.swap(other.data);
+    }
+
+    friend bool operator==(const forward_only_map& lhs, const forward_only_map& rhs)
+    {
+        return lhs.data == rhs.data;
+    }
+    friend bool operator<(const forward_only_map& lhs, const forward_only_map& rhs)
+    {
+        return lhs.data < rhs.data;
+    }
+};
+
+using forward_only_json = nlohmann::basic_json<forward_only_map>;
+
 } // namespace
 
 TEST_CASE("object type whose erase() returns void")
@@ -322,3 +516,46 @@ TEST_CASE("object type without key_compare")
     }
 }
 
+
+TEST_CASE("object type with forward-only iterators")
+{
+    CHECK(std::is_same<std::iterator_traits<forward_only_json::object_t::iterator>::iterator_category,
+          std::forward_iterator_tag>::value);
+
+    SECTION("destroying nested objects and arrays")
+    {
+        forward_only_json j;
+        j["a"] = 1;
+        j["b"]["c"] = "x";
+        j["b"]["d"] = forward_only_json::array();
+        j["b"]["d"].push_back(forward_only_json::object());
+        j["b"]["d"].push_back(true);
+        j["b"]["e"]["f"]["g"] = nullptr;
+        j["h"] = forward_only_json::object();
+        j["i"]["j"] = 2;
+
+        CHECK(j.size() == 4);
+        CHECK(j["b"].size() == 3);
+        CHECK(j["b"]["d"].size() == 2);
+        CHECK(j["b"]["e"]["f"]["g"].is_null());
+
+        CHECK(j.erase("b") == 1);
+        CHECK(j.size() == 3);
+        j = 42;
+        CHECK(j == 42);
+    }
+
+    SECTION("destroying a deeply nested object")
+    {
+        constexpr std::size_t depth = 100000;
+        forward_only_json j;
+        forward_only_json* cur = &j;
+        for (std::size_t i = 0; i < depth; ++i)
+        {
+            (*cur)["s"] = i;
+            cur = &(*cur)["o"];
+        }
+        CHECK(j["o"]["o"]["s"] == 2);
+        // destroyed at the end of scope without recursing per level
+    }
+}