Default initialized iterators are not comparable
piyooshm opened this issue · 6 comments
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
- The bug also occurs if the latest version from the
developbranch is used. - I can successfully compile and run the unit tests.
I understand the issue, and this seems to be a bigger change:
- Right now, we demand iterators to be initialized before comparing. This is the reason for the assertion you reported.
- We also require the iterators to compare to be long to the same container. If this is not the case, we throw an
invalid_iteratorexception.
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?
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?
Thanks for the input. I will have a look.
Should also do the same for operator<, except return true, as if they are equal, the first is not less than the second.
json/include/nlohmann/detail/iterators/iter_impl.hpp
Lines 514 to 524 in a97041a
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.