last now broken for custom iterators on master

[thofma]

On 1.0 - 1.7, the following is working

julia> convergents(fmpz[1, 2])
Nemo.ConvergentsIterator{fmpz, fmpq}(fmpz[1, 2])

julia> last(convergents(fmpz[1, 2]))
3//2

On recent master, due to #42943, this is broken and we have the following regression:

ulia> convergents(fmpz[1, 2])
Nemo.ConvergentsIterator{fmpz, fmpq}(fmpz[1, 2])

julia> last(convergents(fmpz[1, 2]))
ERROR: MethodError: no method matching iterate(::Base.Iterators.Reverse{Nemo.ConvergentsIterator{fmpz, fmpq}})
Closest candidates are:
  iterate(::Union{LinRange, StepRangeLen}) at ~/repositories/julia/usr/share/julia/base/range.jl:870
  iterate(::Union{LinRange, StepRangeLen}, ::Integer) at ~/repositories/julia/usr/share/julia/base/range.jl:870
  iterate(::T) where T<:Union{Base.KeySet{<:Any, <:Dict}, Base.ValueIterator{<:Dict}} at ~/repositories/julia/usr/share/julia/base/dict.jl:695
  ...
Stacktrace:
 [1] first(itr::Base.Iterators.Reverse{Nemo.ConvergentsIterator{fmpz, fmpq}})
   @ Base ./abstractarray.jl:424
 [2] last(a::Nemo.ConvergentsIterator{fmpz, fmpq})
   @ Base ./abstractarray.jl:483
 [3] top-level scope
   @ REPL[5]:1

It does not really matter what the type here is. It is a custom iterator satisfying the iterator interface.

Why was last broken for iterators? I thought we are not breaking user code (which does not rely on internals) before 2.0 :)

[Seelengrab]

Relevant usercode: https://github.com/Nemocas/Nemo.jl/blob/a35f2a5a6fc5f5a9455ef85af8020afb1b8c8b2f/src/gaussiannumbers/continued_fraction.jl#L353-L393

This interaction only worked because you've also defined getindex for your iterator, relying on the undocumented dependence of last on getindex. As getindex is not part of the iterator interface, I'm not sure this should have been expected to be stable/relied on. Would you rather have last consume your iterator by default?

Still, I can see how it's technically breaking behavior - cc @MasonProtter

[vtjnash]

A possible fixed proposed, by marking it an explicit AbstractVector subtype. Previously, it was defined as last(x) = x[lastindex(x)], so it actually previously worked because it defined a value for the lastindex which then was used with getindex to get the last value.

[thofma]

Hm, sure there was an "undocumented dependence", but does that mean unless a function explicitly states which function it will call or which interface the objects should satisfy, it can not be relied upon? Which functions are supposed to keep working between minor version bumps? I would like to avoid this in the future, but it is not yet clear to me how.

Edit: I understand that I can easily fix it. It is more a question of old versions now being broken between different julia (minor) versions, although no "unsafe" things were done.

[Seelengrab]

I don't know, I just think that last shouldn't have promised magic to happen with just lastindex (or mention it at all) 🤷 IMO the question is more about what to do about documentation bugs, which I personally consider this to be. It's really unfortunate that the fallback definition ::Any assumed indexable, documented that and now we wouldn't be able to enable the behavior for all iterables until 2.0. This is really the place where more traits would have been useful, because then you wouldn't have to rely on documentation not having bugs (please don't chastise me for opening this can of worms here :D).

[JeffBezanson]

That characterization of the problem is right, but in the absence of a general solution we need to (1) make good guesses about what methods might be implemented, and (2) try not to break things.

My view is that iteration is inherently sequential, so last and reverse really do not fit naturally. The reverse iterator interface is fairly obscure, so it doesn't strike me as the best choice of method to assume. Of course not every iterator supports indexing either, but indexing seems closer to the mark for something that has a last element.

[MasonProtter]

Couldn't we solve this by making the default method for iterating Iterators.reverse use indexing?

[JeffBezanson]

Yeah that sounds promising.

[tkf]

#42991 was indeed a breaking change because it changed the specification. Whenever you edit the docstring, you change the API. Unless you can show that the new version is a superset of the old version, it is breaking.

Implementing the default Iterator.reverse to use indexing does seem to be able to define a new API that supports the old behavior.