Skip to content

Commit 822be59

Browse files
authored
Fix inconsistent behaviour for last(::Zip) with differing iterator size types (#59217)
This PR fixes two bugs relating to `last(::Zip)`, closing #58922. First: there is inconsistent behaviour for `last(::Zip)` with differing iterator size types (a mixture of known-size and not) are passed in the `Zip`, causing a `MethodError`. `last(::Zip)` works with finite zipped iterators of different lengths, but fails when one of them is an infinite iterator. In the case where there are a mixture of known-length (finite) and infinite iterators in the `Zip`, we can know their length statically. (I call these `Zip`s "finite-guarded," because the `Zip` is finite due to its finite component.) The desired behaviour is to match the behaviour of `last` with finite iterators of different lengths. Second: while standardising this behaviour, [a bug in `last(::Zip)` for `OffsetArray`s](#58922 (comment)) is fixed. The issue was subtle and didn't error, but produced the wrong answer (noticed by @adienes). We also add an explicit `ArgumentError` when `last` is called on a `Zip` whose size is unknown (good call by @Seelengrab). Previously this was a `MethodError` for `lastindex`, used by the [previous implementation of `last(::Zip)`](https://github.com/JuliaLang/julia/blob/80f7db8e51b2ba1dd21e913611c23a6d5b75ecab/base/iterators.jl#L476). This PR does **not** give support to any [functionality involving iterators of unknown size](https://github.com/JuliaLang/julia/pull/59217/files#discussion_r2256315847). This may be done in future.
1 parent 44fdede commit 822be59

File tree

2 files changed

+31
-3
lines changed

2 files changed

+31
-3
lines changed

base/iterators.jl

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -473,7 +473,8 @@ zip_iteratoreltype() = HasEltype()
473473
zip_iteratoreltype(a) = a
474474
zip_iteratoreltype(a, tail...) = and_iteratoreltype(a, zip_iteratoreltype(tail...))
475475

476-
last(z::Zip) = getindex.(z.is, minimum(Base.map(lastindex, z.is)))
476+
last(z::Zip) = nth(z, length(z))
477+
477478
function reverse(z::Zip)
478479
if !first(_zip_lengths_finite_equal(z.is))
479480
throw(ArgumentError("Cannot reverse zipped iterators of unknown, infinite, or unequal lengths"))
@@ -1700,6 +1701,9 @@ function _nth(::IteratorSize, itr, n::Integer)
17001701
y === nothing && throw(BoundsError(itr, n))
17011702
y[1]
17021703
end
1704+
1705+
_nth(::IteratorSize, z::Zip, n::Integer) = Base.map(nth(n), z.is)
1706+
17031707
"""
17041708
nth(n::Integer)
17051709

test/iterators.jl

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ using Random
55
using Base: IdentityUnitRange
66
using Dates: Date, Day
77

8+
isdefined(Main, :OffsetArrays) || @eval Main include("testhelpers/OffsetArrays.jl")
9+
using .Main.OffsetArrays
10+
811
@test (@inferred Base.IteratorSize(Any)) isa Base.SizeUnknown
912

1013
# zip and filter iterators
@@ -1141,7 +1144,6 @@ end
11411144
end
11421145

11431146
@testset "nth" begin
1144-
11451147
Z = Array{Int,0}(undef)
11461148
Z[] = 17
11471149
it_result_pairs = Dict(
@@ -1171,7 +1173,6 @@ end
11711173
(Iterators.cycle(((),)), 1000) => ()
11721174
)
11731175

1174-
11751176
@testset "iter: $IT" for (IT, n) in keys(it_result_pairs)
11761177
@test it_result_pairs[(IT, n)] == nth(IT, n)
11771178
@test_throws BoundsError nth(IT, -42)
@@ -1207,3 +1208,26 @@ end
12071208
@test Base.infer_return_type((Vector{Any},)) do xs
12081209
[x for x in xs if x isa Int]
12091210
end == Vector{Int}
1211+
1212+
@testset "issue #58922" begin
1213+
# `last` short circuits correctly
1214+
@test last(zip(1:10, 2:11)) == (10, 11) # same length
1215+
@test last(zip(1:3, 2:11)) == (3, 4) # different length
1216+
1217+
# Finite-guarded zip iterator: one iterator bounded and the other is not
1218+
@test last(zip(1:3, Iterators.countfrom(2))) == (3, 4)
1219+
@test last(zip(1:3, Iterators.cycle(('x', 'y')))) == (3, 'x')
1220+
@test last(zip(1:3, Iterators.repeated('x'))) == (3, 'x')
1221+
@test last(zip(OffsetArray(1:10, 2), OffsetArray(1:10, 3))) == (10, 10)
1222+
1223+
# Cannot statically know length of zipped iterator if any of its components are of
1224+
# unknown length
1225+
@test_throws MethodError last(zip(1:3, Iterators.filter(x -> x > 0, -5:5))) # (3, 3)
1226+
@test_throws MethodError last(zip(Iterators.filter(x -> x > 0, -5:5), 1:3)) # (3, 3)
1227+
@test_throws MethodError last(zip(1:10, Iterators.filter(x -> x > 0, -5:5))) # (5, 5)
1228+
1229+
# We also can't know the length of zipped iterators when all constituents are of an
1230+
# unknown length. In this test, the answer is (5, 4), but we can't know that without
1231+
# a greedy algorithm
1232+
@test_throws MethodError last(zip(Iterators.filter(x -> x > 0, -5:5), Iterators.filter(x -> x % 2 == 0, -5:5))) # (5, 4)
1233+
end

0 commit comments

Comments
 (0)