Default initialized iterators are not comparable

[piyooshm]

Description

I have a use-case where I iterate over collections in a generic manner (using templates). However, iteration over basic_json fails due to the statement:

        JSON_ASSERT(m_object != nullptr);

Before asserting for non-null object, shouldn't we first check of other.m_object is null too? If both are null, the operator should return true before the assertion.

Reproduction steps

nlohmann::json::iterator{} == nlohmann::json::iterator{}

Expected vs. actual results

Expected: Should return true of m_object is nullptr for both objects

Actual: Assertion fails

Minimal code example

No response

Error messages

No response

Compiler and operating system

gcc 9.3

Library version

3.11.3

Validation

• [X] The bug also occurs if the latest version from the develop branch is used.
• [X] I can successfully compile and run the unit tests.

[nlohmann]

I understand the issue, and this seems to be a bigger change:

1. Right now, we demand iterators to be initialized before comparing. This is the reason for the assertion you reported.
2. We also require the iterators to compare to be long to the same container. If this is not the case, we throw an invalid_iterator exception.

I am not sure why we introduced these requirements in the first place, but I fear that changing them now would be a breaking change which cannot be introduced in the 3.x release scheme. Any ideas on this?

[piyooshm]

Currently, this prevents use of JSON iterator from being used like other STL iterators in certain use-cases since the JSON iterator violates the LegacyForwardIterator equality domain as described in https://en.cppreference.com/w/cpp/named_req/ForwardIterator#:~:text=However%2C%20value,same%20empty%20sequence.

Looking at the code, it seems like the requirement may have been introduced mainly to guard the m_object-dependent implementation code following right after the asserts. So, all I'm suggesting is to return true in the equality comparison operator, in one special case, when both iterators are value-initialized (i.e. initialized using default constructor, causing m_object to be null). That case can also be seen as if both have "equal" null containers that are same. The check would continue to throw as earlier if only one of the two iterators has a null m_object. Furthermore, all other operations on the iter_impl will retain their existing behavior (and assertions) that may require them to have non-null m_object. The suggested change is specifically in the equality comparison operator to have a conforming equality domain and would be:

    template < typename IterImpl, detail::enable_if_t < (std::is_same<IterImpl, iter_impl>::value || std::is_same<IterImpl, other_iter_impl>::value), std::nullptr_t > = nullptr >
    bool operator==(const IterImpl& other) const
    {
        // if objects are not the same, the comparison is undefined
        if (JSON_HEDLEY_UNLIKELY(m_object != other.m_object))
        {
            JSON_THROW(invalid_iterator::create(212, "cannot compare iterators of different containers", m_object));
        }

+        if (m_object == nullptr)
+        {
+            return true;
+        }
-
-        JSON_ASSERT(m_object != nullptr);

        switch (m_object->m_data.m_type)
        {

It may be worth trying the change and seeing which unit tests break. I wouldn't expect any unit tests to break except for a test that explicitly tries to compare two value-initialized iterators. Do you see other possible scenarios where comparison of two value-initialized iterators may break the existing code? Can you provide any such examples to help me understand the concern/severity a little better?

[nlohmann]

Thanks for the input. I will have a look.

[gregmarr]

Should also do the same for operator<, except return true, as if they are equal, the first is not less than the second. https://github.com/nlohmann/json/blob/a97041a98fc7436cda3b28fb6e353071829d3f02/include/nlohmann/detail/iterators/iter_impl.hpp#L514-L524

[gregmarr]

I am not sure why we introduced these requirements in the first place, but I fear that changing them now would be a breaking change which cannot be introduced in the 3.x release scheme. Any ideas on this?

This will change an assert in debug and undefined behavior in release (dereferencing a nullptr) into well-defined behavior. I would say that this is not a breaking change.

[nlohmann]

I opened a PR: #4512 - please take a look.