make last work on reversible any collection

[MasonProtter]

As suggested by @simeonschaub in #42943 (comment), this changes the default fallback of last(x) from x[end] to first(Iterators.reverse(x)), which allows it to work on any collection that supports being reversed, rather than just collections that have indexing.

Before:

julia> last((x for x in 1:3))
ERROR: MethodError: no method matching lastindex(::Base.Generator{UnitRange{Int64}, typeof(identity)}

After:

julia> last((x for x in 1:3))
3

julia> last((x for x in 1:3 if iseven(x)))
2

Fixes #42943

[simeonschaub]

Will need to delete

julia/base/iterators.jl

Line 106 in aca020b

first(r::Reverse) = last(r.itr) # and the last shall be first

[MasonProtter]

Ah of course. What would be an appropriate replacement for this? I suppose we could use

first(r::Reverse) = foldl((_, x) -> x, r.itr)

I think if itr supports reverse then folding over it should be fair game, but maybe not. Or should it just do something like iterate(r)[1]?

[simeonschaub]

I think that line can just be deleted, so it falls back to the generic first implementation, i.e. basically the latter. Should probably double-check though that this doesn't cause any regressions.

[MasonProtter]

Okay, let's see what happens

[simeonschaub]

Ah, I guess you might have to keep the old version for AbstractVector for bootstrapping purposes.

[MasonProtter]

Any reason to not keep it for AbstractArray generally?

[simeonschaub]

That's probably fine, although I don't think we generally assume all AbstractArrays support linear indexing.

[simeonschaub]

LGTM

[vtjnash]

We might need to put this back, with a special attempt not to form an infinite recursion, but let's wait for PkgEval to determine that.

[bkamins]

@MasonProtter This PR has broken DataFrames.jl tests.

I assume the reason is it indirectly introduced triggering of the following:

julia> iterate(Iterators.reverse((a=1, b=2)))
ERROR: MethodError: no method matching iterate(::Base.Iterators.Reverse{NamedTuple{(:a, :b), Tuple{Int64, Int64}}})

I have not looked at the issue in detail but probably adding:

iterate(r::Reverse{<:NamedTuple}, i::Int = length(r.itr)) = i < 1 ? nothing : (r.itr[i], i-1)

should be enough.

[JeffBezanson]

This could definitely break a lot of things, since getindex methods are much more common than methods for Iterators.Reverse, which is slightly obscure.

[JeffBezanson]

I think this can be for AbstractArray.

[MasonProtter]

I did that above, but @simeonschaub mentioned that since there are abstract arrays that don't support linear indexing, this might be the better default. Can give AbstractArray a try though and see how it goes

[vtjnash]

IIRC, all arrays support linear indexing, but it just isn't required to be the most efficient index

[JeffBezanson]

An array can also have a lastindex that isn't necessarily an integer.

[vtjnash]

I thought so too, but lastindex is actually defined to always return an integer.

[MasonProtter]

This could definitely break a lot of things, since getindex methods are much more common than methods for Iterators.Reverse, which is slightly obscure.

In that case, would it make sense to have an interate(::Reverse) fallback method that uses indexing?