Iterator doesn't satisfy std::incrementable because post-increment may change constness

[falbrechtskirchinger]

The following code fails to compile:

nlohmann::json j = nlohmann::json::array({"1", "2", "3", "4"});
auto it = std::ranges::lower_bound(j, "2");

Here are the relevant excerpts from the compiler output with -fconcepts-diagnostics-depth=2:

error: no match for call to ‘(const std::ranges::__lower_bound_fn) (nlohmann::json&, const char [2])’
  388 |         auto it = std::ranges::lower_bound(j, "2");

iterator_concepts.h:611:10:   in requirements with ‘_Iter __i’ [with _Iter = nlohmann::detail::iter_impl<nlohmann::basic_json<...> >]
iterator_concepts.h:611:34: note: ‘(__i ++)’ does not satisfy return-type-requirement, because
  611 |       && requires(_Iter __i) { { __i++ } -> same_as<_Iter>; };

concepts:57:15:   required for the satisfaction of ‘__same_as<_Tp, _Up>’ [with _Tp = const nlohmann::detail::iter_impl<nlohmann::basic_json<...> >; _Up = nlohmann::detail::iter_impl<nlohmann::basic_json<...> >]
concepts:57:32: note: the expression ‘is_same_v<_Tp, _Up> [with _Tp = const nlohmann::detail::iter_impl<nlohmann::basic_json<...> >; _Up = nlohmann::detail::iter_impl<nlohmann::basic_json<...> >]’ evaluated to ‘false’

The compiler is telling us that the post-increment operator is adding const to the return type, which violates the requirements of the std::incrementable iterator concept.

Looking in json.hpp we find:

    /*!
    @brief post-increment (it++)
    @pre The iterator is initialized; i.e. `m_object != nullptr`.
    */
    iter_impl const operator++(int) // NOLINT(readability-const-return-type)
    {
        auto result = *this;
        ++(*this);
        return result;
    }

Remove const from the return type and the introductory code snippet compiles just fine.

Someone more familiar with the code base will have to decide whether that is an appropriate fix.

Which version of the library did you use?

• latest release version 3.10.5
• other release - please state the version: ___
• the develop branch

[gregmarr]

Those const qualifiers came in here, but it's not clear why. It just says effective c++ items. #858
The author @mattismyname is showing no activity on GitHub after this PR.

[falbrechtskirchinger]

In light of @gregmarr's findings, I'm going ahead with a PR. Just waiting for the unit tests to finish, before I submit.

[falbrechtskirchinger]

Static analysis on the PR reveals the motivation for the const qualifier and cites a CERT guideline (cert-dcl21-cpp).

error: overloaded 'operator++' returns a non-constant object instead of a constant object type [cert-dcl21-cpp,-warnings-as-errors]

These guidelines are no longer accessible:

The CERT C++ Coding Standard does not currently expose any recommendations; all C++ recommendations have been removed (moved to The Void section) due to quality concerns pending further review and development.

Further investigations lead me to: https://stackoverflow.com/questions/52871026/overloaded-operator-returns-a-non-const-and-clang-tidy-complains

lvalue ref-qualifying the operators seems to be the solution. I'll make the change and we'll see if it breaks any tests and silences clang-tidy.

[gregmarr]

I'm worried that adding the lvalue qualifiers would be a breaking change. Granted, it is unlikely to be what the user intended, but it's still something that would compile before and won't now.

[falbrechtskirchinger]

I disagree. The behavior should be exactly the same.
For example, I just tried the following with and without PR #3332:

  nlohmann::json j({1, 2, 3, 4});
  auto it = j.begin();
  auto val = *(it++);
  auto val2 = *((it++)++);

Line 3 is (hopefully obviously) unaffected and line 4 fails with virtually the same compiler output (referring to the second use of post-increment):

error: passing ‘const nlohmann::detail::iter_impl<nlohmann::basic_json<> >’ as ‘this’ argument discards qualifiers [-fpermissive]
   50 |   auto val2 = *((it++)++);
   
error: passing ‘nlohmann::detail::iter_impl<nlohmann::basic_json<> >’ as ‘this’ argument discards qualifiers [-fpermissive]
   50 |   auto val2 = *((it++)++);

Note the const qualifier in the first error message.

The const qualifier on the return type and the lvalue reference qualifier on the member function serve the exact same purpose. Prevent the user from accidentally calling non-const member functions, and thereby mutating a temporary object, which is undefined behavior,
Line 3 demonstrates calling a const member function (operator *() const), line 4 calls either an unqualified (before PR #3332) or lvalue ref-qualified (after PR #3332) post-increment operator.

Put differently, both qualifiers "break" code that would have compiled before #858, but no additional code should fail to compile after #3332. Both prevent accidental use of UB.

Also, isn't that what unit tests are for? 😉

[falbrechtskirchinger]

I did come up with something. This works before #3332 but not after:

auto &it2 = it++;

Compiler output after #3332:

error: cannot bind non-const lvalue reference of type ‘nlohmann::detail::iter_impl<nlohmann::basic_json<> >&’ to an rvalue of type ‘nlohmann::detail::iter_impl<nlohmann::basic_json<> >’
  53 |   auto &it2 = it++;

Dangling reference prevented.

[gregmarr]

What about this?

auto const &it2 = it++;

Just want to make sure that we don't break people unnecessarily.

[falbrechtskirchinger]

I've tested all permutations: auto, const auto, auto &, const auto &, auto && (i. e. everything but const auto && which isn't useful).
As stated, auto & is the only one which fails to compile and does so in a case that would've previously resulted in UB at runtime.

Stepping back for a moment. What prompted me filing this issue, was the inability to use basic_json as a std::ranges::range in std::ranges algorithms. I hope we can agree that not inter operating with a pretty big new standard library feature (all be it somewhat handicapped until C++23) needs to be addressed.
We can either do so by removing const from the return type, making it easier for people to shoot themselves in the foot, or add lvalue reference qualifiers to the member functions while enhancing existing protections against dangling references. I vote for the latter. The GNU standard library opted for no `const, no lvalue ref. But they also need to remain backwards-compatible with pre-C++11.

I'm fully open to doing more testing if anyone want's to suggest scenarios and maybe add more unit tests accordingly.
Otherwise, #3332 is now passing all CI checks and I do consider it ready for a final review and merge.