Skip to content
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

Make Iterators.peel return nothing when itr empty #39607

Merged
merged 2 commits into from
Jun 5, 2021

Conversation

cmcaine
Copy link
Contributor

@cmcaine cmcaine commented Feb 11, 2021

Fixes #39569.

Old behaviour: peel throws BoundsError when itr is empty.

try
    h, t = Iterators.peel(itr)
    # do something with h and t
catch e
    if e isa BoundsError
        # itr was probably empty
    else
        rethrow()
    end
end

New behaviour: peel returns nothing when itr is empty.

x = Iterators.peel(itr)
if isnothing(x)
    # itr was empty
end
h, t = x

Motivation: head, tail... = itr is a fundamental operation for lots of recursive code, and it would be nice if the lazy version of that (peel) were more ergonomic to use. This PR makes it about as easy to use as iterate.

Impact: this is a bit backwards incompatible, but I did a code search on GitHub, and none of the 140 matches for Iterators.peel would be broken by this change because they all try to iterate the object returned by peel immediately, so they will still get a BoundsError if itr is empty.

Argument against: if isempty(itr) works well then this pattern is fine and works right now:

if isempty(itr)
    # itr was empty
else
    h, t = Iterators.peel(itr)
end

The issue is that isempty is not safe for all stateful iterators (see discussion at #27412).

Fixes JuliaLang#39569.

Old behaviour:
```jl
try
    h, t = Iterators.peel(itr)
    # do something with h and t
catch e
    if e isa BoundsError
        # itr was probably empty
    else
        rethrow()
    end
end
```

New behaviour:
```jl
x = Iterators.peel(itr)
if isnothing(x)
    # itr was empty
end
h, t = x
```
Comment on lines 852 to 858
@testset "Iterators.peel" begin
@test Iterators.peel([]) == nothing
@test Iterators.peel(1:10)[1] == 1
@test Iterators.peel(1:10)[2] |> collect == 2:10
@test Iterators.peel(x^2 for x in 2:4)[1] == 4
@test Iterators.peel(x^2 for x in 2:4)[2] |> collect == [9, 16]
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Grep didn't find any existing tests of Iterators.peel, so I added a few simple ones. I'm using the arrow syntax so the characters line up better, making it clearer when I'm calling with the same arguments.

@sostock
Copy link
Contributor

sostock commented Feb 15, 2021

You don’t need to use try block in the first version, you can simply check whether the iterator isempty before calling Iterators.peel. I don’t see an improvement in calling Iterators.peel and then isnothing (instead of isempty and then Iterators.peel).

@cmcaine
Copy link
Contributor Author

cmcaine commented Feb 15, 2021

That's a good point. My initial motivation for this PR was that isempty is unsafe for stateful iterators: the fallback implementation consumes a value from the iterator while testing if it is empty.

I have some other PRs that fix eachline and readeach, but there will be more around.

Anyway, I'd forgotten that those things were related by the time I wrote the commit message for this.

If we could fix the isempty fallback method in a backwards compatible way (we'd probably need to add a trait), then that would make this PR redundant, I think, but I'm not sure it can be fixed before V2.

@cmcaine
Copy link
Contributor Author

cmcaine commented Feb 28, 2021

Can triage give an opinion on this, please?

I've updated the first post to reflect all discussion on the original issue and this PR up to this point, so that's all that needs to be read.

@staticfloat staticfloat added the triage This should be discussed on a triage call label Mar 12, 2021
@mbauman mbauman added iteration Involves iteration or the iteration protocol and removed triage This should be discussed on a triage call labels Jun 3, 2021
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jun 3, 2021
@oscardssmith oscardssmith merged commit e0165b4 into JuliaLang:master Jun 5, 2021
aviatesk added a commit that referenced this pull request Jun 5, 2021
After #39607, `Iterators.peel` can return `nothing` and the return type
can be more inaccurate than before for certain iterative inputs where
inference doesn't understand its length, e.g.
`Iterators.peel(::<:AbstractString)`.

The following JET analysis shows an example of this type instability:
```julia
═════ 2 possible errors found ═════
┌ @ coreio.jl:4 Base.println(Core.tuple(Core.typeassert(Base.stdout, Base.IO)), xs...)
│┌ @ strings/io.jl:73 Base.print(Core.tuple(io), xs, Core.tuple("\n")...)
││┌ @ strings/io.jl:46 Base.print(io, x)
│││┌ @ show.jl:1283 Base.show_unquoted(Base.IOContext(io, Base.=>(:unquote_fallback, false)), ex, 0, -1)
││││┌ @ show.jl:1634 Base.show_unquoted_quote_expr(io, Base.getproperty(ex, :value), indent, prec, 0)
│││││┌ @ show.jl:1651 Base.isidentifier(sym)
││││││┌ @ show.jl:1339 Base.isidentifier(Base.string(s))
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 1)
││││││││┌ @ tuple.jl:92 Base.iterate(I)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Base.iterate(I::Nothing)
││││││││└───────────────
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 2, _3)
││││││││┌ @ tuple.jl:97 Base.iterate(I, state)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing, Int64}): Base.iterate(I::Nothing, state::Int64)
││││││││└───────────────
Core.Const(nothing)
```

This PR adds some manual annotations and seems to fix some regressions
within Base.
@aviatesk
Copy link
Member

aviatesk commented Jun 5, 2021

This change could lead to type inference inaccuracy for certain iterators that inference can't understand its length, e.g. peel(::<:AbstractString), and could possibly cause some invalidation risks.
Some of them will be addressed by #41094.

aviatesk added a commit that referenced this pull request Jun 5, 2021
After #39607, `Iterators.peel` can return `nothing` and the return type
can be more inaccurate than before for certain iterative inputs where
inference doesn't understand its length, e.g.
`Iterators.peel(::<:AbstractString)`.

The following JET analysis shows an example of this type instability:
```julia
═════ 2 possible errors found ═════
┌ @ coreio.jl:4 Base.println(Core.tuple(Core.typeassert(Base.stdout, Base.IO)), xs...)
│┌ @ strings/io.jl:73 Base.print(Core.tuple(io), xs, Core.tuple("\n")...)
││┌ @ strings/io.jl:46 Base.print(io, x)
│││┌ @ show.jl:1283 Base.show_unquoted(Base.IOContext(io, Base.=>(:unquote_fallback, false)), ex, 0, -1)
││││┌ @ show.jl:1634 Base.show_unquoted_quote_expr(io, Base.getproperty(ex, :value), indent, prec, 0)
│││││┌ @ show.jl:1651 Base.isidentifier(sym)
││││││┌ @ show.jl:1339 Base.isidentifier(Base.string(s))
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 1)
││││││││┌ @ tuple.jl:92 Base.iterate(I)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Base.iterate(I::Nothing)
││││││││└───────────────
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 2, _3)
││││││││┌ @ tuple.jl:97 Base.iterate(I, state)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing, Int64}): Base.iterate(I::Nothing, state::Int64)
││││││││└───────────────
Core.Const(nothing)
```

This PR adds code refactors or manual annotations and seems to fix
some of regressions within Base.
aviatesk added a commit that referenced this pull request Jun 7, 2021
After #39607, `Iterators.peel` can return `nothing` and the return type
can be more inaccurate than before for certain iterative inputs where
inference doesn't understand its length, e.g.
`Iterators.peel(::<:AbstractString)`.

The following JET analysis shows an example of this type instability:
```julia
═════ 2 possible errors found ═════
┌ @ coreio.jl:4 Base.println(Core.tuple(Core.typeassert(Base.stdout, Base.IO)), xs...)
│┌ @ strings/io.jl:73 Base.print(Core.tuple(io), xs, Core.tuple("\n")...)
││┌ @ strings/io.jl:46 Base.print(io, x)
│││┌ @ show.jl:1283 Base.show_unquoted(Base.IOContext(io, Base.=>(:unquote_fallback, false)), ex, 0, -1)
││││┌ @ show.jl:1634 Base.show_unquoted_quote_expr(io, Base.getproperty(ex, :value), indent, prec, 0)
│││││┌ @ show.jl:1651 Base.isidentifier(sym)
││││││┌ @ show.jl:1339 Base.isidentifier(Base.string(s))
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 1)
││││││││┌ @ tuple.jl:92 Base.iterate(I)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Base.iterate(I::Nothing)
││││││││└───────────────
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 2, _3)
││││││││┌ @ tuple.jl:97 Base.iterate(I, state)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing, Int64}): Base.iterate(I::Nothing, state::Int64)
││││││││└───────────────
Core.Const(nothing)
```

This PR adds code refactors or manual annotations and seems to fix
some of regressions within Base.
aviatesk added a commit that referenced this pull request Jun 7, 2021
After #39607, `Iterators.peel` can return `nothing` and the return type
can be more inaccurate than before for certain iterative inputs where
inference doesn't understand its length, e.g.
`Iterators.peel(::<:AbstractString)`.

The following JET analysis shows an example of this type instability:
```julia
═════ 2 possible errors found ═════
┌ @ coreio.jl:4 Base.println(Core.tuple(Core.typeassert(Base.stdout, Base.IO)), xs...)
│┌ @ strings/io.jl:73 Base.print(Core.tuple(io), xs, Core.tuple("\n")...)
││┌ @ strings/io.jl:46 Base.print(io, x)
│││┌ @ show.jl:1283 Base.show_unquoted(Base.IOContext(io, Base.=>(:unquote_fallback, false)), ex, 0, -1)
││││┌ @ show.jl:1634 Base.show_unquoted_quote_expr(io, Base.getproperty(ex, :value), indent, prec, 0)
│││││┌ @ show.jl:1651 Base.isidentifier(sym)
││││││┌ @ show.jl:1339 Base.isidentifier(Base.string(s))
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 1)
││││││││┌ @ tuple.jl:92 Base.iterate(I)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Base.iterate(I::Nothing)
││││││││└───────────────
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 2, _3)
││││││││┌ @ tuple.jl:97 Base.iterate(I, state)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing, Int64}): Base.iterate(I::Nothing, state::Int64)
││││││││└───────────────
Core.Const(nothing)
```

This PR adds code refactors or manual annotations and seems to fix
some of regressions within Base.
shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
…41094)

After JuliaLang#39607, `Iterators.peel` can return `nothing` and the return type
can be more inaccurate than before for certain iterative inputs where
inference doesn't understand its length, e.g.
`Iterators.peel(::<:AbstractString)`.

The following JET analysis shows an example of this type instability:
```julia
═════ 2 possible errors found ═════
┌ @ coreio.jl:4 Base.println(Core.tuple(Core.typeassert(Base.stdout, Base.IO)), xs...)
│┌ @ strings/io.jl:73 Base.print(Core.tuple(io), xs, Core.tuple("\n")...)
││┌ @ strings/io.jl:46 Base.print(io, x)
│││┌ @ show.jl:1283 Base.show_unquoted(Base.IOContext(io, Base.=>(:unquote_fallback, false)), ex, 0, -1)
││││┌ @ show.jl:1634 Base.show_unquoted_quote_expr(io, Base.getproperty(ex, :value), indent, prec, 0)
│││││┌ @ show.jl:1651 Base.isidentifier(sym)
││││││┌ @ show.jl:1339 Base.isidentifier(Base.string(s))
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 1)
││││││││┌ @ tuple.jl:92 Base.iterate(I)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Base.iterate(I::Nothing)
││││││││└───────────────
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 2, _3)
││││││││┌ @ tuple.jl:97 Base.iterate(I, state)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing, Int64}): Base.iterate(I::Nothing, state::Int64)
││││││││└───────────────
Core.Const(nothing)
```

This PR adds code refactors or manual annotations and seems to fix
some of regressions within Base.
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jun 18, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
…41094)

After JuliaLang#39607, `Iterators.peel` can return `nothing` and the return type
can be more inaccurate than before for certain iterative inputs where
inference doesn't understand its length, e.g.
`Iterators.peel(::<:AbstractString)`.

The following JET analysis shows an example of this type instability:
```julia
═════ 2 possible errors found ═════
┌ @ coreio.jl:4 Base.println(Core.tuple(Core.typeassert(Base.stdout, Base.IO)), xs...)
│┌ @ strings/io.jl:73 Base.print(Core.tuple(io), xs, Core.tuple("\n")...)
││┌ @ strings/io.jl:46 Base.print(io, x)
│││┌ @ show.jl:1283 Base.show_unquoted(Base.IOContext(io, Base.=>(:unquote_fallback, false)), ex, 0, -1)
││││┌ @ show.jl:1634 Base.show_unquoted_quote_expr(io, Base.getproperty(ex, :value), indent, prec, 0)
│││││┌ @ show.jl:1651 Base.isidentifier(sym)
││││││┌ @ show.jl:1339 Base.isidentifier(Base.string(s))
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 1)
││││││││┌ @ tuple.jl:92 Base.iterate(I)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Base.iterate(I::Nothing)
││││││││└───────────────
│││││││┌ @ show.jl:1335 Base.indexed_iterate(Base.Iterators.peel(s), 2, _3)
││││││││┌ @ tuple.jl:97 Base.iterate(I, state)
│││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing, Int64}): Base.iterate(I::Nothing, state::Int64)
││││││││└───────────────
Core.Const(nothing)
```

This PR adds code refactors or manual annotations and seems to fix
some of regressions within Base.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iteration Involves iteration or the iteration protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In v2, Iterators.peel could return nothing when iterator is empty
9 participants