Iterators cannot be used with range-v3

[axic]

What is the issue you have?

(Apologies if this should have been a discussion item and not a bug report.)

I have a JSON object and would like to extract the keys (preferably as a set). (In jsoncpp this is accomplished with .getMemberNames())

Ideally this would work with range-v3, but the iterator of this library does not seem to be compatible:

1. json.items() | ranges::views::keys

/Projects/solidity/test/libsolidity/StandardCompiler.cpp:1255:39: error: invalid operands to binary expression ('const nlohmann::basic_json<>::value_type' (aka 'const nlohmann::basic_json<>') and 'const view_closure<ranges::views::keys_fn>')
                (optimizer["details"]["yulDetails"] | ranges::views::keys | ranges::to<set>) ==
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~

2. ranges::subrange(json.begin(), json.end()) || ranges::views::keys

/Projects/solidity/test/libsolidity/StandardCompiler.cpp:1255:4: error: no viable constructor or deduction guide for deduction of template arguments of 'subrange'
                (ranges::subrange(optimizer["details"]["yulDetails"].begin(), optimizer["details"]["yulDetails"].end()) | ranges::views::keys | ranges::to<set>) ==
                 ^

3. json.items() | ranges::views::transform([](auto&& _v) { return _v.key(); })

/Projects/solidity/test/libsolidity/StandardCompiler.cpp:1254:100: error: no member named 'key' in 'nlohmann::basic_json<>'
        auto x = optimizer["details"]["yulDetails"] |  ranges::views::transform([](auto&& _v) { return _v.key(); });
                                                                                                       ~~ ^

Please describe the steps to reproduce the issue.

As above ^^

Can you provide a small but working code example?

What is the expected behavior?

And what is the actual behavior instead?

Which compiler and operating system are you using?

• Compiler: gcc + clang
• Operating system: linux + macOS

Which version of the library did you use?

• latest release version 3.10.4
• other release - please state the version: 3.10.2
• the develop branch

If you experience a compilation error: can you compile and run the unit tests?

• yes
• no - please copy/paste the error message below

[gregmarr]

I did some digging here. The more easily resolved issues are that iteration_proxy_value and iteration_proxy have user-specified constructors and thus the default constructor, copy constructor, move constructor, move assignment operator and copy assignment operator are disabled.

They can all be defaulted, except for the copy assignment operators. For iteration_proxy that's because it has a reference to the container instead of a pointer to the container, so it can't be rebound. For iteration_proxy_value, it's due to const string_type empty_value;

Also, iteration_proxy_value needs the post-increment operator iteration_proxy_value operator++(int) const and a const version of operator*.

That makes it an input_range. For it to be used with a view, it needs to be a viewable_range and to fix that, I had to specialize ranges::enable_borrowed_range. I don't like that.

The major problem with using it for json.items() | ranges::views::keys is that requires that the dereferenced thing from the iterator have .first and .second data members. There's a possibility that ranges::elements<> could be customized for json. I've gone as far as I can for now.

This part works though:

auto items = j.items();
auto keys = items | ranges::views::transform([](auto&& _v) { return _v.key(); });

You can't do it with j.items() | because that returns a temporary and views won't bind to a temporary.

[gregmarr]

explicit iteration_proxy_value() {}
iteration_proxy_value(iteration_proxy_value const &) = default;
iteration_proxy_value(iteration_proxy_value &&) = default;

// THIS CAN'T BE DEFAULTED BECAUSE OF empty_str.  This could potentially be
// fixed by making that a static.  That would also get rid of potential lifetime
// issues due to the iterator proxy going away while someone was holding a reference
// to the string.  Not sure why key() returns const string& instead of string.
// Probably a performance thing. 
iteration_proxy_value &operator=(iteration_proxy_value const &v) {
    anchor = v.anchor;
    array_index = v.array_index;
    array_index_last = v.array_index_last;
    array_index_str = v.array_index_str;
    return *this;
}

iteration_proxy_value &operator=(iteration_proxy_value &&) = default; 

/// dereference operator (needed for range-based for)
iteration_proxy_value& operator*()
{
    return *this;
}

/// dereference operator (needed for range-based for)
iteration_proxy_value& operator*() const
{
    return *this;
}

/// increment operator (needed for range-based for)
iteration_proxy_value& operator++()
{
    ++anchor;
    ++array_index;

    return *this;
}

/// increment operator (needed for range-based for)
iteration_proxy_value operator++(int) const
{
    iteration_proxy_value v(*this);
    v++;
    return v;
}


explicit iteration_proxy() {}
iteration_proxy(iteration_proxy const &) = default;
iteration_proxy(iteration_proxy &&) = default;

// THIS IS WRONG.  THIS COPIES r's CONTAINER INTO THIS CONTAINER RATHER THAN CHANGING THE REFERENCED CONTAINER
// Fixing this requires something like changing the type of `container` to be a pointer.
iteration_proxy &operator=(iteration_proxy const &r) { container = r.container; return *this; }

iteration_proxy &operator=(iteration_proxy &&) = default;

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[nlohmann]

I will have a look at this soon. I hope we can fix iteration_proxy without breaking changes.

[davidstone]

operator* for iteration_proxy_value would be better defined as

const iteration_proxy_value& operator*() const
{
    return *this;
}

This makes the iterator const-dereferenceable, which is assumed for all iterators.

For empty_str, if you could assume C++17 or later, you would just make it static inline const string_type empty_str, but since you don't assume that your best bet is to fake it with

static const string_type & empty_str() {
    static const string_type value;
    return value;
}

This resolves some lifetime issues and means you don't have to store an extra empty string in each instance. If you are feeling really fancy, you can annotate it as

    [[clang::no_destroy]] static const string_type value;

See https://clang.llvm.org/docs/AttributeReference.html#no-destroy

The mutable data members are pretty weird, since it means your allegedly const functions aren't actually const when it comes to reasoning about thread safety. But since you support getting the key as a string representation of the array index for array members, there's not an easy way around this.

It's also a little strange that you cannot call begin or end on a const iteration_proxy. That's technically allowed by C++20 and range-v3, but it's an unnecessary restriction. Given that this is intended as a reference type to the json object, I would also mark begin and end as const.

[falbrechtskirchinger]

Should be fixed by #3332 (merged) if someone wants to verify.

Edit: Nevermind. @gregmarr tried it here and it's not. Kind of obvious. I forgot about the other iterator issues.