Make IteratorSize(::Iterators.Cycle) not always IsInfinite and document why

[LilithHafner]

Fixes #53169

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?

[nsajko]

A refinement, to improve precision:

Suggested change

	IteratorSize(I) === IsInfinite() ? IsInfinite() : SizeUnknown()
	c = IteratorSize(I)
	((c === IsInfinite()) || (c === HasShape{0}())) ? IsInfinite() : SizeUnknown()

The idea is that there are two cases when we know the number of elements is not zero (and thus infinite):

• when IteratorSize(I) === IsInfinite(), already accounted for, just listing it for completeness
• when IteratorSize(I) === HasShape{0}(), because a zero-dimensional shape implies a length of exactly one