Make IteratorSize(::Iterators.Cycle) not always IsInfinite add to docstring as example

[LilithHafner]

Fixes #53169
Fixes #43235

Deletes an "XXX: this is false if iterator ever becomes empty" comment

[martinholters]

Do we have other examples where IteratorSize(it) and IteratorSize(typeof(it)) (potentially) disagree? Makes me a bit uncomfortable.

And if it.xs is stateful, it seems that IsInfinite might turn out to be wrong, because isempty(it.xs) might become true.

[LilithHafner]

Do we have other examples where IteratorSize(it) and IteratorSize(typeof(it)) (potentially) disagree? Makes me a bit uncomfortable.

No, this is the first case, and changes the docs to suggest the possibility.

And if it.xs is stateful, it seems that IsInfinite might turn out to be wrong, because isempty(it.xs) might become true.

IMO that's because Iterators.cycle on stateful iterators is broken. It is explicitly documented to be infinite for non-empty arguments.

[blegat]

I am facing the same issue in JuliaAlgebra/MultivariatePolynomials.jl#343. There are probably many cases where an iterator is infinite except in degenerate cases. We could also document that when an iterator type says that it is IsInfinite, it means that it may be infinite and calling IteratorSize on an instance may (it could also be difficult to know in advance whether it is infinite, say for instance that you define an iterator for this iteration that stops when it reaches 1) distinguish whether it is infinite or not. These iterators are also said to be infinite but they may still end if the corresponding input closes I guess.

If we decide that IsInfinite only means "may be infinite", then the IteratorSize(::Type{Cycle}) should probably return IsInfinite in all cases, and never SizeUnknown.

[nsajko]

My thoughts regarding defining IteratorSize on instances of a type:

• I interpret the iteration interface like so:

  • Callers of IteratorSize(::Any) should be able to assume the call is "free", in the sense it gets constant folded and does not exist at run time when everything is concretely inferred.

  • The intention behind the IteratorSize(x) = IteratorSize(typeof(x)) is as a mere convenience for making it unnecessary to call typeof. So this method is supposed to be the only method of IteratorSize that accepts non-Type.

  • Callers should be able to assume IteratorSize(x) === IteratorSize(typeof(x)), if !isa(x, Type).

  • If one wants a trait like IteratorSize, but defined on instances, they should create a new function instead of adding methods to IteratorSize.

• Another point possibly worth considering is abstract inference: when the argument types to a call are not concretely known, Julia will possibly consider a set of more than one matching method. Thus adding a methods to a callable can negatively affect the code generation (and affinity to invalidation) for unrelated loaded packages. This is especially bad if the method is of an uncommon type.

[nsajko]

This PR includes:

• a necessary bugfix that should be backported: the changes for the IteratorSize(::Type{<:Cycle}) method
• several other changes that are:
  • significant
  • potentially controversial, even, dare I say, dubious
  • not backportable, I suppose

Would it be possible to separate this into multiple PRs?

[LilithHafner]

This PR is blocked by design decisions around how we want to handle iterators with interesting size/length availability characteristics.

Questions for triage:

• How do we handle iterator types that are typically infinite but could be empty in degenerate cases?
• More broadly, how do we handle iterators types that may or may not have size or length or finitude depending on their runtime contents?

[adienes]

on second look, it seems important that IsInfinite means really must be infinite, not "may" be infinite. otherwise I think we will see tons of SizeUnknown proliferation very quickly. for example on status quo:

• zip(::HasLength, ::IsInfinite) --> HasLength
• take(::IsInfinite, n) --> HasLength
• drop(::IsInfinite, n) --> IsInfinite

basically all the "passthroughs" like filter, enumerate, map etc.

ALL of these would no longer be able to determine IteratorSize based only on the types of their inputs and would have to return SizeUnknown.

was it considered yet to add a type parameter to Cycle{I,Empty} and IteratorSize(::Cycle{I,E}) = E ? HasLength() : IsInfinite() ?

[LilithHafner]

From Triage: IteratorSize(::Cycle) should be unknown. There's nothing else to be done here.

[LilithHafner]

We can't add a type parameter to track this because

• then cycle would no longer by type stable
• conceptually, emptiness of, say 1:0 is a property of the value, not the type
• this still wouldn't cover stateful iterators (though depending on the assumed API they are already broken with Cycle)

[vtjnash]

There seems to be a problem with CI with _min_length interpreting SizeUnknown as meaning there is a computable length 🙄 . I think we just make that case the same as IsInfinite?

[LilithHafner]

Ah, that _min_length example currently results in

julia> z = zip([1,2,5], Iterators.cycle(()))
zip([1, 2, 5], Base.Iterators.Cycle{Tuple{}}(()))

julia> length(z)
3

julia> collect(z)
3-element Vector{Union{}}:
 #undef
 #undef
 #undef

julia> z = zip([1,2,5], Iterators.cycle(Int[]))
zip([1, 2, 5], Base.Iterators.Cycle{Vector{Int64}}(Int64[]))

julia> collect(z)
3-element Vector{Tuple{Int64, Int64}}:
 (281473195310576, 281473195310672)
 (281473195310736, 281473195310800)
 (281473195310864, 281473164575856)

[LilithHafner]

Making length(::Zip) handle SizeUnkown as if it were SizeInfinite would preserve the aformentioned bug and introduce it in other SizeUnknown cases.

For the most part, I think this PR is fine and correct. If an unknown size is present, length(::Zip) calls length on that component, and if it returns successfully (which it may) , succeeds, otherwise fails. And Base.IteratorsSize(::Zip) will be Base.SizeUnknown if any component is unknown, so the caller has been advised that length may fail.

We could special case Cycle components of Zip to get this right (if isempty, return 0, otherwise treat as IsInfinite).

We could also have a way for length to return Inf (we could use typemax(Int) as a sentinel) in which case cycle and user defined iterators could opt into this behavior. Ideally that would be out of scope for this PR.

IMO the best course of action is to adjust the test in CI, run PkgEval, and merge as is.

[adienes]

I support special case Cycle components of Zip or merge as is, but not use typemax(Int) as a sentinel [for Inf]

[LilithHafner]

Looking at the actual CI failures

last(zip(1:3, Iterators.cycle(('x', 'y')))) == (3, 'x')

I find "merge as is" somewhat unacceptable. I'll go with special case Cycle components of Zip and see how that goes.

[JeffBezanson]

We should add methods for IteratorSize of Cycle of empty and non-empty tuples, which we know are empty and infinite, respectively.

[JeffBezanson]

Also arguably Zip should not try to call length on SizeUnknown iterators, but you get an error either way so it may not matter.