Skip to content

Commit

Permalink
reverse(zip(its...)) now checks lengths of constituent iterators fo…
Browse files Browse the repository at this point in the history
…r equality (#50435)

fixes #45085, fixes
#25583, as per suggested by
@Moelf

Despite the fact that this might cause code to fail that previously
worked, I think this is a bugfix not a breaking change.

In particular, `Iterators.reverse` says:
> Given an iterator itr, then reverse(itr) is an iterator over the same
collection but in the reverse order

And in many cases, the previous behavior of `reverse(::Zip)` would _not_
return "the same collection"
  • Loading branch information
adienes authored Oct 26, 2023
1 parent 75881a9 commit 342d718
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 1 deletion.
23 changes: 22 additions & 1 deletion base/iterators.jl
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,22 @@ function _zip_min_length(is)
end
end
_zip_min_length(is::Tuple{}) = nothing

# For a collection of iterators `is`, returns a tuple (b, n), where
# `b` is true when every component of `is` has a statically-known finite
# length and all such lengths are equal. Otherwise, `b` is false.
# `n` is an implementation detail, and will be the `length` of the first
# iterator if it is statically-known and finite. Otherwise, `n` is `nothing`.
function _zip_lengths_finite_equal(is)
i = is[1]
if IteratorSize(i) isa Union{IsInfinite, SizeUnknown}
return (false, nothing)
else
b, n = _zip_lengths_finite_equal(tail(is))
return (b && (n === nothing || n == length(i)), length(i))
end
end
_zip_lengths_finite_equal(is::Tuple{}) = (true, nothing)
size(z::Zip) = _promote_tuple_shape(Base.map(size, z.is)...)
axes(z::Zip) = _promote_tuple_shape(Base.map(axes, z.is)...)
_promote_tuple_shape((a,)::Tuple{OneTo}, (b,)::Tuple{OneTo}) = (intersect(a, b),)
Expand Down Expand Up @@ -468,8 +484,13 @@ zip_iteratoreltype() = HasEltype()
zip_iteratoreltype(a) = a
zip_iteratoreltype(a, tail...) = and_iteratoreltype(a, zip_iteratoreltype(tail...))

reverse(z::Zip) = Zip(Base.map(reverse, z.is)) # n.b. we assume all iterators are the same length
last(z::Zip) = getindex.(z.is, minimum(Base.map(lastindex, z.is)))
function reverse(z::Zip)
if !first(_zip_lengths_finite_equal(z.is))
throw(ArgumentError("Cannot reverse zipped iterators of unknown, infinite, or unequal lengths"))
end
Zip(Base.map(reverse, z.is))
end

# filter

Expand Down
4 changes: 4 additions & 0 deletions test/iterators.jl
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ using Dates: Date, Day
# issue #4718
@test collect(Iterators.filter(x->x[1], zip([true, false, true, false],"abcd"))) == [(true,'a'),(true,'c')]

# issue #45085
@test_throws ArgumentError Iterators.reverse(zip("abc", "abcd"))
@test_throws ArgumentError Iterators.reverse(zip("abc", Iterators.cycle("ab")))

let z = zip(1:2)
@test size(z) == (2,)
@test collect(z) == [(1,), (2,)]
Expand Down

0 comments on commit 342d718

Please sign in to comment.