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.

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

more precise eltype for some Generator subtypes #56899

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+61 −2
Open

more precise eltype for some Generator subtypes #56899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f73e7d2
more precise `eltype` for some `Generator` subtypes
nsajko Dec 24, 2024
b399dd9
don't touch `IteratorEltype`, avoid breaking common assumptions
nsajko Dec 24, 2024
22b80b8
fix some tests
nsajko Dec 24, 2024
634307b
fix bugs, thanks to mcabbott
nsajko Dec 24, 2024
ed9aac0
slightly tighten up the `getindex` case
nsajko Dec 24, 2024
3767190
leave out the tightening from this PR, with TODO note
nsajko Dec 24, 2024
7237c22
deduplicate
nsajko Dec 24, 2024
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
1 change: 1 addition & 0 deletions base/Base_compiler.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ include("bool.jl")
include("number.jl")
include("int.jl")
include("operators.jl")
include("generator_eltype.jl")
include("pointer.jl")
include("refvalue.jl")
include("cmem.jl")
Expand Down
25 changes: 25 additions & 0 deletions base/generator_eltype.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
function eltype(::Type{Generator{A, typeof(identity)}}) where {A}
eltype(A)
end
Comment on lines +1 to +3
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL that x for x in A is noticed by the parser, and produces this type:

julia> (x for x in 1:3)
Base.Generator{UnitRange{Int64}, typeof(identity)}(identity, 1:3)

julia> (identity(x) for x in 1:3)
Base.Generator{UnitRange{Int64}, var"#13#14"}(var"#13#14"(), 1:3)

(Was true on Julia 1.6, not true on 1.0.)

I wonder what would break if this instead returned A, i.e. we made this (x for x in 1:3) === 1:3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I discovered the same thing while working on the PR, and wonder the same thing, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I know what would break. map(f, x) is implemented as something like collect(Iterators.map(f, x)). This relies on the fact that Generator has EltypeUnknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lots of code assumes that, e.g., map(identity, x) will return a collection with more precise eltype than x has. This is what relies on Generator having EltypeUnknown.


function eltype(::Type{Generator{A, Fix1{typeof(getindex), B}}}) where {A, B}
if B <: Type
# TODO: theoretically we could be more precise here and return a subtype
# of `AbstractVector`. The problem is that several packages do dubious
# punning of `getindex`. See
# https://github.com/mcabbott/AxisKeys.jl/issues/163
Any
else
let a = eltype(A), b = keytype(B)
if (a == b) || (a <: Integer >: b)
valtype(B)
else
Any
end
end
end
end

function eltype(::Type{Generator{A, Fix1{typeof(getfield), B}}} where {A}) where {B}
typejoin(fieldtypes(B)...)
end
35 changes: 34 additions & 1 deletion test/iterators.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1015,6 +1015,39 @@ end
@test last(Iterators.filter(iseven, (Iterators.map(identity, 1:3)))) == 2
end

@testset "`eltype` for `Generator` involving `Fix` and `getindex`/`getfield` (issue #41519)" begin
@testset "correct `eltype`" begin
for (f, i) ∈ (
(Base.Fix1(getindex, Int), [3, 7]),
(Base.Fix1(getindex, [3, 7]), [[1 2], [2 1]]),
)
r = map(f, i)
t = Iterators.map(f, i)
@test eltype(r) <: @inferred eltype(t)
@test (@inferred Base.IteratorEltype(t)) isa Base.IteratorEltype
end
end
@testset "precise `eltype`" begin
for (f, i) ∈ (
(identity, [3, 7]),
(Base.Fix1(getindex, [3, 7]), 1:2),
(Base.Fix1(getindex, Dict("a" => 3, "b" => 7)), ["a", "b"]),
(Base.Fix1(getfield, (; a = 3, b = 7)), 1:2),
(Base.Fix1(getfield, (; a = 3, b = 7)), [:a, :b]),
)
r = map(f, i)
t = Iterators.map(f, i)
@test eltype(r) == @inferred eltype(t)
@test (@inferred Base.IteratorEltype(t)) isa Base.IteratorEltype
end
end
end

@testset "issue #48448" begin
it = union(i for i in 1:5)
@test Int == @inferred eltype(it)
end

@testset "isempty and isdone for Generators" begin
itr = eachline(IOBuffer("foo\n"))
gen = (x for x in itr)
Expand Down Expand Up @@ -1049,7 +1082,7 @@ end
end
end

let itr = (i for i in 1:9) # Base.eltype == Any
let itr = ((x -> x)(i) for i in 1:9) # Base.eltype == Any
@test first(Iterators.partition(itr, 3)) isa Vector{Any}
@test collect(zip(repeat([Iterators.Stateful(itr)], 3)...)) == [(1, 2, 3), (4, 5, 6), (7, 8, 9)]
end
Expand Down
2 changes: 1 addition & 1 deletion test/missing.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ end
@test collect(x) == [1, 2, 4]
@test collect(x) isa Vector{Int}

x = skipmissing(v for v in [missing, 1, missing, 2, 4])
x = skipmissing((x -> x)(v) for v in [missing, 1, missing, 2, 4])
@test eltype(x) === Any
@test collect(x) == [1, 2, 4]
@test collect(x) isa Vector{Int}
Expand Down
Loading