Do not instantiate a hash map with an incomplete basic_json in the tests

object_t is probed for key_compare inside the definition of basic_json, so
it is instantiated while basic_json is still incomplete. Whether a hash map
survives that depends on the standard library: libstdc++ 9 needs the size of
the mapped type to instantiate std::unordered_map's node type and rejects
the adapter, which broke the GCC 9 builds.

The test now derives its no-key_compare object type from std::map -- which
does cope -- and shadows the inherited key_compare member type with an
entity that is not a type, so the library's probe finds none, exactly as for
a hash map. The unflatten() order-independence checks in unit-json_pointer
already cover the behaviour that the unordered object type was there for.
The limitation is documented for std::unordered_map.

Also address two Clang-Tidy findings the earlier commits introduced:
erase_from_object() declares its iterator with auto, and at(size_type) checks
the type first and then falls through to the return instead of throwing from
an else branch.

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

docs/mkdocs/docs/features/types/template_parameters.md

@@ -148,6 +148,11 @@ struct unordered_map_object
 using unordered_json = nlohmann::basic_json<unordered_map_object>;
 ```
 
+Whether `#!cpp std::unordered_map` can be instantiated at all depends on the standard library: `object_t` is formed
+while `basic_json` is still incomplete (see the warning above), and libstdc++ 9 needs the size of the mapped type to
+instantiate the hash map's node type, so the adapter does not compile there. Newer libstdc++ versions, and the hash
+maps listed below, do not have that problem.
+
 The adapter above works verbatim for Abseil's, Boost's, `phmap`'s and `gtl`'s hash maps, which all place the hash
 function third and take a `#!cpp std::pair<const Key, T>` allocator fifth. Two need a different adapter:
 
@@ -208,7 +213,7 @@ The library does not sort or de-duplicate keys itself; the behavior described in
 | [`nlohmann::ordered_map`](../../api/ordered_map.md)                                                           | used by [`ordered_json`](../../api/ordered_json.md); keeps insertion order |
 | [`nlohmann::fifo_map`](https://github.com/nlohmann/fifo_map)                                                  | keeps insertion order; adapter puts `fifo_map_compare` in the comparator slot |
 | `boost::container::map`, `boost::container::flat_map`                                                         | no adapter needed                                                        |
-| `#!cpp std::unordered_map`                                                                                    | through the adapter above                                                |
+| `#!cpp std::unordered_map`                                                                                    | through the adapter above; not with libstdc++ 9, see the note            |
 | `boost::unordered_map`, `boost::unordered_flat_map`, `boost::unordered_node_map`                              | through the adapter above                                                |
 | `absl::flat_hash_map`, `absl::node_hash_map`                                                                  | through the adapter above; `flat_hash_map` moves mapped values on rehash  |
 | `phmap::flat_hash_map`, `phmap::node_hash_map`, `gtl::flat_hash_map`                                          | through the adapter above                                                |

include/nlohmann/json.hpp

@@ -798,7 +798,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
                    std::is_void<decltype(std::declval<object_t&>().erase(std::declval<It>()))>::value, int > = 0 >
     typename object_t::iterator erase_from_object(It pos)
     {
-        typename object_t::iterator next = std::next(pos);
+        auto next = std::next(pos);
         m_data.m_value.object->erase(pos);
         return next;
     }
@@ -2063,18 +2063,17 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     reference at(size_type idx)
     {
         // at only works for arrays
-        if (JSON_HEDLEY_LIKELY(is_array()))
+        if (JSON_HEDLEY_UNLIKELY(!is_array()))
-        {
-            if (JSON_HEDLEY_UNLIKELY(idx >= m_data.m_value.array->size()))
-            {
-                JSON_THROW(out_of_range::create(401, detail::concat("array index ", std::to_string(idx), " is out of range"), this));
-            }
-            return set_parent((*m_data.m_value.array)[idx]);
-        }
-        else
         {
             JSON_THROW(type_error::create(304, detail::concat("cannot use at() with ", type_name()), this));
         }
+
+        if (JSON_HEDLEY_UNLIKELY(idx >= m_data.m_value.array->size()))
+        {
+            JSON_THROW(out_of_range::create(401, detail::concat("array index ", std::to_string(idx), " is out of range"), this));
+        }
+
+        return set_parent((*m_data.m_value.array)[idx]);
     }
 
     /// @brief access specified array element with bounds checking
@@ -2082,18 +2081,17 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     const_reference at(size_type idx) const
     {
         // at only works for arrays
-        if (JSON_HEDLEY_LIKELY(is_array()))
+        if (JSON_HEDLEY_UNLIKELY(!is_array()))
-        {
-            if (JSON_HEDLEY_UNLIKELY(idx >= m_data.m_value.array->size()))
-            {
-                JSON_THROW(out_of_range::create(401, detail::concat("array index ", std::to_string(idx), " is out of range"), this));
-            }
-            return (*m_data.m_value.array)[idx];
-        }
-        else
         {
             JSON_THROW(type_error::create(304, detail::concat("cannot use at() with ", type_name()), this));
         }
+
+        if (JSON_HEDLEY_UNLIKELY(idx >= m_data.m_value.array->size()))
+        {
+            JSON_THROW(out_of_range::create(401, detail::concat("array index ", std::to_string(idx), " is out of range"), this));
+        }
+
+        return (*m_data.m_value.array)[idx];
     }
 
     /// @brief access specified object element with bounds checking

single_include/nlohmann/json.hpp

@@ -22264,7 +22264,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
                    std::is_void<decltype(std::declval<object_t&>().erase(std::declval<It>()))>::value, int > = 0 >
     typename object_t::iterator erase_from_object(It pos)
     {
-        typename object_t::iterator next = std::next(pos);
+        auto next = std::next(pos);
         m_data.m_value.object->erase(pos);
         return next;
     }
@@ -23529,18 +23529,17 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     reference at(size_type idx)
     {
         // at only works for arrays
-        if (JSON_HEDLEY_LIKELY(is_array()))
+        if (JSON_HEDLEY_UNLIKELY(!is_array()))
-        {
-            if (JSON_HEDLEY_UNLIKELY(idx >= m_data.m_value.array->size()))
-            {
-                JSON_THROW(out_of_range::create(401, detail::concat("array index ", std::to_string(idx), " is out of range"), this));
-            }
-            return set_parent((*m_data.m_value.array)[idx]);
-        }
-        else
         {
             JSON_THROW(type_error::create(304, detail::concat("cannot use at() with ", type_name()), this));
         }
+
+        if (JSON_HEDLEY_UNLIKELY(idx >= m_data.m_value.array->size()))
+        {
+            JSON_THROW(out_of_range::create(401, detail::concat("array index ", std::to_string(idx), " is out of range"), this));
+        }
+
+        return set_parent((*m_data.m_value.array)[idx]);
     }
 
     /// @brief access specified array element with bounds checking
@@ -23548,18 +23547,17 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     const_reference at(size_type idx) const
     {
         // at only works for arrays
-        if (JSON_HEDLEY_LIKELY(is_array()))
+        if (JSON_HEDLEY_UNLIKELY(!is_array()))
-        {
-            if (JSON_HEDLEY_UNLIKELY(idx >= m_data.m_value.array->size()))
-            {
-                JSON_THROW(out_of_range::create(401, detail::concat("array index ", std::to_string(idx), " is out of range"), this));
-            }
-            return (*m_data.m_value.array)[idx];
-        }
-        else
         {
             JSON_THROW(type_error::create(304, detail::concat("cannot use at() with ", type_name()), this));
         }
+
+        if (JSON_HEDLEY_UNLIKELY(idx >= m_data.m_value.array->size()))
+        {
+            JSON_THROW(out_of_range::create(401, detail::concat("array index ", std::to_string(idx), " is out of range"), this));
+        }
+
+        return (*m_data.m_value.array)[idx];
     }
 
     /// @brief access specified object element with bounds checking

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

@@ -11,29 +11,36 @@
 #include <nlohmann/json.hpp>
 
 #include <cstdint>
-#include <functional>
 #include <map>
 #include <string>
 #include <type_traits>
-#include <unordered_map>
 #include <utility>
 #include <vector>
 
 namespace
 {
 
-// An ObjectType that does *not* define a key_compare member type. It adapts
+// An ObjectType that does *not* define a key_compare member type, which is
-// std::unordered_map to the template argument order expected by basic_json,
+// what every hash map looks like to the library.
-// where the third argument is a comparator rather than a hash function.
+//
-template<class Key, class T, class IgnoredCompare, class Allocator>
+// A hash map is deliberately not used here: object_t is probed for
-struct unordered_map_object
+// key_compare inside the definition of basic_json, that is, while basic_json
-    : std::unordered_map<Key, T, std::hash<Key>, std::equal_to<Key>, Allocator>
+// is still an incomplete type, and whether a hash map can be instantiated
+// with an incomplete mapped type depends on the standard library (libstdc++ 9
+// needs the size of the mapped type for its node type and rejects it). So the
+// object type is built from std::map, and the inherited key_compare member
+// type is shadowed by an entity that is not a type -- the library's probe
+// then finds no type, exactly as for a hash map.
+template<class Key, class T, class Compare, class Allocator>
+struct no_key_compare_map : std::map<Key, T, Compare, Allocator>
 {
-    using base_t = std::unordered_map<Key, T, std::hash<Key>, std::equal_to<Key>, Allocator>;
+    using base_t = std::map<Key, T, Compare, Allocator>;
     using base_t::base_t;
+
+    enum { key_compare }; // shadows base_t::key_compare, which is a type
 };
 
-using unordered_json = nlohmann::basic_json<unordered_map_object>;
+using no_key_compare_json = nlohmann::basic_json<no_key_compare_map>;
 
 // An ObjectType whose erase(iterator) returns void rather than the following
 // iterator, as for instance Abseil's hash maps do
@@ -109,8 +116,8 @@ TEST_CASE("object type without key_compare")
 {
     SECTION("object_comparator_t falls back to default_object_comparator_t")
     {
-        CHECK(std::is_same < unordered_json::object_comparator_t,
+        CHECK(std::is_same < no_key_compare_json::object_comparator_t,
-              unordered_json::default_object_comparator_t >::value);
+              no_key_compare_json::default_object_comparator_t >::value);
     }
 
     SECTION("object types defining key_compare are unaffected")
@@ -123,7 +130,7 @@ TEST_CASE("object type without key_compare")
 
     SECTION("creating and accessing values")
     {
-        unordered_json j;
+        no_key_compare_json j;
         j["one"] = 1;
         j["two"] = "zwei";
         j["three"]["nested"] = true;
@@ -142,39 +149,38 @@ TEST_CASE("object type without key_compare")
 
     SECTION("serialization and deserialization")
     {
-        const auto j = unordered_json::parse(R"({"a":[1,2,3],"b":{"c":null}})");
+        const auto j = no_key_compare_json::parse(R"({"a":[1,2,3],"b":{"c":null}})");
         CHECK(j["a"].size() == 3);
         CHECK(j["a"][2] == 3);
         CHECK(j["b"]["c"].is_null());
-        CHECK(unordered_json::parse(j.dump()) == j);
+        CHECK(no_key_compare_json::parse(j.dump()) == j);
     }
 
     SECTION("binary formats")
     {
-        const auto j = unordered_json::parse(R"({"a":[1,2,3],"b":"x"})");
+        const auto j = no_key_compare_json::parse(R"({"a":[1,2,3],"b":"x"})");
-        CHECK(unordered_json::from_cbor(unordered_json::to_cbor(j)) == j);
+        CHECK(no_key_compare_json::from_cbor(no_key_compare_json::to_cbor(j)) == j);
-        CHECK(unordered_json::from_msgpack(unordered_json::to_msgpack(j)) == j);
+        CHECK(no_key_compare_json::from_msgpack(no_key_compare_json::to_msgpack(j)) == j);
     }
 
-    SECTION("flatten and unflatten do not depend on the iteration order")
+    SECTION("flatten and unflatten")
     {
-        // the flattened object is iterated in an unspecified order, so
+        // "o" has a key that looks like an array index, so unflatten() must
-        // unflatten() must not decide between array and object based on
+        // not turn it into an array
-        // whichever reference token it happens to see first
+        const auto j = no_key_compare_json::parse(
-        const auto j = unordered_json::parse(
                            R"({"c":[1,2,3],"d":{"e":"s"},"n":[[0,1],[2]],"o":{"2":"x"}})");
         CHECK(j.flatten().unflatten() == j);
     }
 
     SECTION("conversion to and from nlohmann::json")
     {
-        const auto j = unordered_json::parse(R"({"a":1,"b":[true,null]})");
+        const auto j = no_key_compare_json::parse(R"({"a":1,"b":[true,null]})");
         const nlohmann::json converted(j);
 
         CHECK(converted.is_object());
         CHECK(converted["a"] == 1);
         CHECK(converted["b"][0] == true);
         CHECK(converted["b"][1].is_null());
-        CHECK(unordered_json(converted) == j);
+        CHECK(no_key_compare_json(converted) == j);
     }
 }