Skip to content

specify that Iterators.rest must be given a valid state #58962

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.

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

Merged
merged 2 commits into from
Jul 12, 2025

Conversation

adienes
Copy link
Member

@adienes adienes commented Jul 10, 2025

currently Iterators.rest(1:2, 3) creates an infinite loop. after this PR it would be an ArgumentError

docs only now

@adienes adienes added bugfix This change fixes an existing bug iteration Involves iteration or the iteration protocol labels Jul 10, 2025
@KristofferC
Copy link
Member

Tangentially, but it is kind of ugly how this wants an explicit state passed in (and how the docstring uses the function). I don't think the value of the state has any meaning outside how it should be used in the iteration protocol, or?

@adienes
Copy link
Member Author

adienes commented Jul 10, 2025

I agree, and as far as I can see it's the only iterator with an API like that. I guess it's to draw a distinction with Iterators.drop

but anyway that's probably not changeable, so I just wanted to make it harder to hit infinite loops

@KristofferC
Copy link
Member

Maybe it is worth adding to the docstring that you should have received that state from an iterate call (and update the example)?

@adienes adienes force-pushed the ordinal_range_iterate branch from 64a3183 to 3c65d85 Compare July 10, 2025 21:58
@adienes
Copy link
Member Author

adienes commented Jul 11, 2025

is the fact that rest(1:4, 6) errors but rest([1,2,3,4], 6) does not acceptable? per the docstring, this is an illegal state so the output is undefined (not in the fancy sense, just that the semantics are not specified).

but pretty much any implementation of _iteratorstate_invalid will contain false negatives as it is simply impossible in the general case to query if an iteration state is valid

@mbauman
Copy link
Member

mbauman commented Jul 11, 2025

Maybe it is worth adding to the docstring that you should have received that state from an iterate call (and update the example)?

This is the way. But stronger: s/should/must/.

It's not really different from iterate(1:0, 1000) itself. If you give that something that's not actually a state from iterate, you may have a bad time. The point of rest is to allow you to swap back from a manual sequence of iterates to a for loop or to pass to another iterator function.

I honestly don't think it's worth trying to protect against this case, particularly because ::OrdinalRanges can (and do!) define their own iterate methods and may use different meaning for their own state. So this is worse than incomplete: it isn't correct.

Ideally, iterators would always return nothing or error for invalid states. But not all do, and it's not a promise we make.

@adienes adienes force-pushed the ordinal_range_iterate branch from 3c65d85 to aeb2a77 Compare July 11, 2025 16:15
@adienes adienes force-pushed the ordinal_range_iterate branch from aeb2a77 to 4bc0006 Compare July 11, 2025 16:15
@adienes adienes changed the title fail Rest construction with invalid state specify that Iterators.rest must be given a valid state Jul 11, 2025
@adienes adienes added docs This change adds or pertains to documentation and removed bugfix This change fixes an existing bug labels Jul 11, 2025
@adienes
Copy link
Member Author

adienes commented Jul 11, 2025

ok, fair enough. I've amended the PR to just make the docstring a bit more emphatic about the state validity then

@mbauman
Copy link
Member

mbauman commented Jul 11, 2025

I can't quite reach into the example to suggest a change there, but let's also have that example use a state it got from val, state = iterate([1,2,3,4])

@KristofferC KristofferC added the merge me PR is reviewed. Merge when all tests are passing label Jul 11, 2025
@adienes adienes merged commit 3f9ba7f into JuliaLang:master Jul 12, 2025
8 checks passed
@adienes adienes deleted the ordinal_range_iterate branch July 12, 2025 12:27
@adienes adienes removed the merge me PR is reviewed. Merge when all tests are passing label Jul 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation iteration Involves iteration or the iteration protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants