Skip to content

Fix type instability for exponent iterator with no variable #343

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix type instability for exponent iterator with no variable #343

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
13266b1
Fix type instability for exponent iterator with no variable
blegat Sep 2, 2025
4ee7576
Add tests
blegat Sep 3, 2025
388330e
Fix format
blegat Sep 4, 2025
51bb7f0
Switch to SizeUnknown
blegat Sep 4, 2025
ef785af
Ref to Cycle PR
blegat Sep 4, 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
29 changes: 21 additions & 8 deletions src/comparison.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,11 +459,6 @@ struct ExponentsIterator{M,D<:Union{Nothing,Int},O}
),
)
end
if length(object) == 0 && isnothing(maxdegree)
# Otherwise, it will incorrectly think that the iterator is infinite
# while it actually has zero elements
maxdegree = mindegree
end
return new{M,typeof(maxdegree),typeof(object)}(
object,
mindegree,
Expand All @@ -477,21 +472,39 @@ Base.eltype(::Type{ExponentsIterator{M,D,O}}) where {M,D,O} = O
function Base.IteratorSize(::Type{<:ExponentsIterator{M,Nothing}}) where {M}
return Base.IsInfinite()
end
function Base.IteratorSize(it::ExponentsIterator{M,Nothing}) where {M}
if isempty(it.object)
return Base.HasLength()
end
return Base.IsInfinite()
end
Copy link
Contributor

@nsajko nsajko Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Copy link
Member Author

@blegat blegat Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IteratorSize should be define on the type, not the value.

It is but if you look at the implementation of collect(::Any), it is calling IteratorSize on the value which then redirect to calling IteratorSize on the type so it this implementation actually passes all my tests checking consistency with collect etc...
So the only issue will be that users that call IteratorSize on the type in the case they created the exponent iterator with a list of 0 variables will get Base.IsInfinite while in fact the iterator only has one element. The fact that this gives IsInfinite will just prevent some code to work while it could have worked if it was finite but that code is already not working when there is more than 0 variables so I don't see a use case being broken by that. I actually need IsInfinite for the use with. Indeed, SizeUnknown would be a bit safer here but on the other hand, the case with zero variable is kind of a corner case and IsInfinite may also be understood as MaybeIsInfinite, not sure if there is any code that relies on it being actually infinite :/

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe the good compromise it to just return SizeUnknown when it's called on the type

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah this is exactly what's done for Iterators.Cycle, thanks for the link, this is exactly the same case, interesting

Copy link
Contributor

Choose a reason for hiding this comment

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

IteratorSize should be define on the type, not the value.

It is but if you look at the implementation [...]

  • 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 resistance to invalidation) for unrelated loaded packages. This is especially bad if the method is of an uncommon type.

function Base.IteratorSize(::Type{<:ExponentsIterator{M,Int}}) where {M}
return Base.HasLength()
end

function Base.length(it::ExponentsIterator{M,Int}) where {M}
if it.maxdegree < it.mindegree
function _length(it::ExponentsIterator, maxdegree)
if maxdegree < it.mindegree
return 0
end
len = binomial(nvariables(it) + it.maxdegree, nvariables(it))
len = binomial(nvariables(it) + maxdegree, nvariables(it))
if it.mindegree > 0
len -= binomial(nvariables(it) + it.mindegree - 1, nvariables(it))
end
return len
end

function Base.length(it::ExponentsIterator{M,Int}) where {M}
return _length(it, it.maxdegree)
end

function Base.length(it::ExponentsIterator{M,Nothing}) where {M}
if isempty(it.object)
return _length(it, it.mindegree)
else
error("The iterator is infinity because `maxdegree` is `nothing`.")
end
end

nvariables(it::ExponentsIterator) = length(it.object)

_last_lex_index(n, ::Type{LexOrder}) = n
Expand Down
5 changes: 5 additions & 0 deletions test/comparison.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ function test_errors()
"Ordering `$M` is not a valid ordering, use `Graded{$M}` instead.",
)
@test_throws err ExponentsIterator{M}([0], maxdegree = 2)
exps = ExponentsIterator{LexOrder}([0])
err = ErrorException(
"The iterator is infinity because `maxdegree` is `nothing`.",
)
@test_throws err length(exps)
end

function test_exponents_iterator()
Expand Down