Skip to content

Fix inconsistent behaviour for last(::Zip) with differing iterator size types #59217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 19, 2025
Merged

Fix inconsistent behaviour for last(::Zip) with differing iterator size types #59217

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
655ef5a
Add tests for asymmetric finite-guarded zips
jakewilliami Aug 5, 2025
9856827
Support `last` for finite-guarded zip iterators
jakewilliami Aug 6, 2025
193b6fc
Explicit throw on `last(::Zip)` when size unknown
jakewilliami Aug 6, 2025
da76f8c
Dispatch internal nth method on IteratorSize
jakewilliami Aug 19, 2025
54ee79f
Remove throw for iterator of unknown size in last(::Zip)
jakewilliami Aug 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion base/iterators.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,12 @@ zip_iteratoreltype() = HasEltype()
zip_iteratoreltype(a) = a
zip_iteratoreltype(a, tail...) = and_iteratoreltype(a, zip_iteratoreltype(tail...))

last(z::Zip) = getindex.(z.is, minimum(Base.map(lastindex, z.is)))
function last(z::Zip)
IteratorSize(z) == SizeUnknown() &&
throw(ArgumentError("Cannot get last element of zipped iterators of undefined lengths"))
return nth(z, length(z))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing
getindex.(z.is, minimum(Base.map(lastindex, z.is)))
to
nth(z, length(z))
is a clear win and an easy merge. Thanks!

end

function reverse(z::Zip)
if !first(_zip_lengths_finite_equal(z.is))
throw(ArgumentError("Cannot reverse zipped iterators of unknown, infinite, or unequal lengths"))
Expand Down Expand Up @@ -1700,6 +1705,9 @@ function _nth(::IteratorSize, itr, n::Integer)
y === nothing && throw(BoundsError(itr, n))
y[1]
end

_nth(::Union{HasShape, HasLength}, z::Zip, n::Integer) = Base.map(nth(n), z.is)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about SizeUnknown?

Copy link
Member

@adienes adienes Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will hit the generic fallback above that just calls iterate n times; _nth specializations exist only to be fast paths

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch. I didn't consider that case. This does indeed fail on iterators of unknown size. For example,

Iterators.filter(x -> x > 0, -5:5)

As humans, it's obvious that this is synonymous to 1:5, but it doesn't implement length.

We might need use _zip_lengths_finite_equal (we explored using internal Iterator length functions in #58922 but initially decided against it). Do you think this is sufficient (CC @adienes)?

function last(z::Zip)
    n = last(_zip_lengths_finite_equal(z.is))
    return nth(z, n)
end

Is it worth keeping the length implementation for iterators whose size is known?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that will fail if we have two iterators of unknown size in the Zip, e.g.,

zip(Iterators.filter(x -> x > 0, -5:5), Iterators.filter(x -> x % 2 == 0, -5:5))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how last can know the length of the Zip if all constituent iterators have SizeUnknown. Do we explicitly error in this case?

nth supports iterators whose size are unknown. It would be nice to support it in last but I don't see any efficient way to get the length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying @Seelengrab. I agree, I think we don't properly handle the second option. What do you think of 193b6fc? Explicitly throwing an ArgumentError seems better than letting it throw a MethodError when trying to compute length.

Copy link
Contributor

@Seelengrab Seelengrab Aug 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. I'm not even sure SizeUnknown of a constituent iterator is handled well for IteratorSize of a Zip. Like I said, if there is any HasLength being zipped over, there is at least an upper limit, but length cannot accurately report that since the SizeUnknown may finish first. Yet, it is definitely still possible to return the last zipped element, since you can just iterate up to the length of that HasLength. I can provide a nice MWE showing what I mean later today.

SizeUnknown is really a special case that needs to be treated differently on a per-case basis, so I'm not sure just erroring is nice from a principal POV. It's probably fine for this PR though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

julia> struct Collatz
           start::Int
       end

julia> Base.eltype(::Type{Collatz}) = Int

julia> Base.IteratorSize(::Type{Collatz}) = Base.SizeUnknown()

julia> Base.iterate(c::Collatz) = (c.start, c.start)

julia> function Base.iterate(c::Collatz, last::Int)
           isone(last) && return nothing
           next = if iseven(last)
               last ÷ 2
           else
               3*last + 1
           end
           (next, next)
       end

julia> zip(1:10, Collatz(3)) |> collect
8-element Vector{Tuple{Int64, Int64}}:
 (1, 3)
 (2, 10)
 (3, 5)
 (4, 16)
 (5, 8)
 (6, 4)
 (7, 2)
 (8, 1)

This is an iterator that just produces the Collatz sequence for a given number. This can do anything from being shorter than the other zipped iterator or longer than the zipped iterator (or loop infinitely on its own, who knows!). Still, if it does finish for a given number, we can definitely zip this with a 1:n range and use last on it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_nth(::Union{HasShape, HasLength}, z::Zip, n::Integer) = Base.map(nth(n), z.is)
_nth(::IteratorSize, z::Zip, n::Integer) = Base.map(nth(n), z.is)

and defer to the component iterators.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in da76f8c 🙂


"""
nth(n::Integer)

Expand Down
28 changes: 26 additions & 2 deletions test/iterators.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ using Random
using Base: IdentityUnitRange
using Dates: Date, Day

isdefined(Main, :OffsetArrays) || @eval Main include("testhelpers/OffsetArrays.jl")
using .Main.OffsetArrays

@test (@inferred Base.IteratorSize(Any)) isa Base.SizeUnknown

# zip and filter iterators
Expand Down Expand Up @@ -1140,7 +1143,6 @@ end
end

@testset "nth" begin

Z = Array{Int,0}(undef)
Z[] = 17
it_result_pairs = Dict(
Expand Down Expand Up @@ -1170,7 +1172,6 @@ end
(Iterators.cycle(((),)), 1000) => ()
)


@testset "iter: $IT" for (IT, n) in keys(it_result_pairs)
@test it_result_pairs[(IT, n)] == nth(IT, n)
@test_throws BoundsError nth(IT, -42)
Expand Down Expand Up @@ -1206,3 +1207,26 @@ end
@test Base.infer_return_type((Vector{Any},)) do xs
[x for x in xs if x isa Int]
end == Vector{Int}

@testset "issue #58922" begin
# `last` short circuits correctly
@test last(zip(1:10, 2:11)) == (10, 11) # same length
@test last(zip(1:3, 2:11)) == (3, 4) # different length

# Finite-guarded zip iterator: one iterator bounded and the other is not
@test last(zip(1:3, Iterators.countfrom(2))) == (3, 4)
@test last(zip(1:3, Iterators.cycle(('x', 'y')))) == (3, 'x')
@test last(zip(1:3, Iterators.repeated('x'))) == (3, 'x')
@test last(zip(OffsetArray(1:10, 2), OffsetArray(1:10, 3))) == (10, 10)

# Cannot statically know length of zipped iterator if any of its components are of
# unknown length
@test_throws ArgumentError last(zip(1:3, Iterators.filter(x -> x > 0, -5:5))) # (3, 3)
@test_throws ArgumentError last(zip(Iterators.filter(x -> x > 0, -5:5), 1:3)) # (3, 3)
@test_throws ArgumentError last(zip(1:10, Iterators.filter(x -> x > 0, -5:5))) # (5, 5)

# We also can't know the length of zipped iterators when all constituents are of an
# unknown length. In this test, the answer is (5, 4), but we can't know that without
# a greedy algorithm
@test_throws ArgumentError last(zip(Iterators.filter(x -> x > 0, -5:5), Iterators.filter(x -> x % 2 == 0, -5:5))) # (5, 4)
end