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

remove dependence on Julia internal Core.Compiler.return_type #127

Closed
wants to merge 5 commits into from
Closed

remove dependence on Julia internal Core.Compiler.return_type #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
958a72f
remove dependence on Julia internal `Core.Compiler.return_type`
palday Dec 28, 2023
bac632b
patch bump
palday Dec 28, 2023
11887c8
remove restriction
palday Dec 28, 2023
898a033
Problems with Indices{Union{}} on Julia 1.10
palday Dec 28, 2023
c83f300
more specific
palday Dec 28, 2023
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Manifest.toml

2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name = "Dictionaries"
uuid = "85a47980-9c8c-11e8-2b9f-f7ca1fa99fb4"
authors = ["Andy Ferris <ferris.andy@gmail.com>"]
version = "0.3.25"
version = "0.3.26"

[deps]
Indexing = "313cdc1a-70c2-5d6a-ae34-0150d3930a38"
Expand Down
14 changes: 14 additions & 0 deletions src/Dictionaries.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,20 @@ export dictionary, index, distinct, disjoint, isdictequal, filterview, sortkeys,
export issettable, isinsertable, set!, unset!
export istokenizable, tokentype, tokens, tokenized, gettoken, gettokenvalue, istokenassigned, settokenvalue!, gettoken!, deletetoken!, sharetokens

"""
return_type(f, types)

Find the return type of `f` called with `types`.

!!! note
Currently, this method depends on `code_typed`.
"""
function return_type(f, types)
return last(only(code_typed(f, types;
Copy link

Choose a reason for hiding this comment

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

This is worse than just calling Core.Compiler.return_type. It still trespasses on the internals of the Julia implementation, as it's not documented that last may be used here, and I guess it's slower than Core.Compiler.return_type.

Copy link
Author

Choose a reason for hiding this comment

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

@nsajko It's calling last on a Pair -- how is that undocumented behavior? It's also only calling public API (an function that appears in the manual). I guess I could also access the rettype property of the CodeInfo that's the first element of that pair, but I still need to access the pair.

It will almost definitely be slower than calling Core.Compiler.return_type, because I'm guessing that Core.Compiler.return_type is called somewhere in the implementation of code_typed.

Copy link

Choose a reason for hiding this comment

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

You're right, sorry.

optimize=false,
debuginfo=:none)))
end

include("AbstractDictionary.jl")
include("AbstractIndices.jl")

Expand Down
4 changes: 2 additions & 2 deletions src/Dictionary.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,8 @@
tmp = iterate(iter)
if tmp === nothing
IT = Base.@default_eltype(iter)
I = Core.Compiler.return_type(first, Tuple{IT})
T = Core.Compiler.return_type(last, Tuple{IT})
I = return_type(first, Tuple{IT})
T = return_type(last, Tuple{IT})

Check warning on line 224 in src/Dictionary.jl

View check run for this annotation

Codecov / codecov/patch

src/Dictionary.jl#L223-L224 

Added lines #L223 - L224 were not covered by tests
return Dictionary{I, T}()
end
(x, s) = tmp
Expand Down
10 changes: 9 additions & 1 deletion src/Indices.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,15 @@ function Indices(iter)
iter = collect(iter)
end

return Indices{eltype(iter)}(iter)
# This is necessary to make map'ing across an empty dictionary work
# In both Julia 1.9 and 1.10 eltype(::Tuple{}) == Union{}
# but in Julia 1.10, this causes problems downstream when Indices{Union{}}
# is used. For an empty iterator, we actually don't know what the
# True(tm) eltype is, so the top of the type hierarchy (Any) is
# just as reasonable as the bottom (Union{})
I = typeof(iter) == Tuple{} ? Any : eltype(iter)
Copy link

Choose a reason for hiding this comment

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

If you're constructing an Index from an empty tuple, you do not know what the original element type is, so Any IMHO makes as much sense as Union{}.

Small point: Any is technically correct (in a trivial sense), but less precise than Union{}. The empty union is the best possible (most accurate) type for the empty set of values. The possible breakage on the Julia side, however, is another thing...

Copy link

@nsajko nsajko Jan 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Small note: typeof(iter) == Tuple{} isn't very pretty/Julian. Here are two alternatives that are prettier in my opinion (although all of them seem equally performant):

f(it) = (typeof(it) == Tuple{}) ? Any : eltype(it)
g(it) = (it == ()) ? Any : eltype(it)
h(it) = (it isa Tuple{}) ? Any : eltype(it)

I vote for it == () (in case this change turns out to be necessary), seems like the nicest option.

EDIT: I think there's actually a style guide or something in Julia's manual that advises not to compare types with ==. In that light another option is typeof(it) <: Tuple{}, although the typeof is still unnecessary with that form.

Copy link
Owner

Choose a reason for hiding this comment

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

I believe it === () or it isa Tuple{} are the correct forms. (Note: the latter form might invoke a slower subtyping algorithm when the LHS isn't inferred to a concrete type and/or the RHS is an abstract type; the former is always quite straightforward).


return Indices{I}(iter)
end

function Indices{I}(iter) where {I}
Expand Down
4 changes: 2 additions & 2 deletions src/broadcast.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ _data(d::BroadcastedDictionary) = getfield(d, :data)
sharetokens = _sharetokens(dicts...)
I = keytype(dicts[1])
Ts = Base.Broadcast.eltypes(data)
T = Core.Compiler.return_type(f, Ts)
T = return_type(f, Ts)

return BroadcastedDictionary{I, T, typeof(f), typeof(data)}(f, data, sharetokens)
end
Expand Down Expand Up @@ -130,4 +130,4 @@ end
Base.Broadcast.materialize(d::BroadcastedDictionary) = copy(d)
Base.Broadcast.materialize!(out::AbstractDictionary, d::BroadcastedDictionary) = copyto!(out, d)

Base.Broadcast.materialize!(out::AbstractDictionary, bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{0}, Nothing, typeof(identity)}) = fill!(out, bc.args[1][])
Base.Broadcast.materialize!(out::AbstractDictionary, bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{0}, Nothing, typeof(identity)}) = fill!(out, bc.args[1][])
6 changes: 3 additions & 3 deletions src/map.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,13 @@
end

function Base.map(f, d::AbstractDictionary)
out = similar(d, Core.Compiler.return_type(f, Tuple{eltype(d)}))
out = similar(d, return_type(f, Tuple{eltype(d)}))
@inbounds map!(f, out, d)
return out
end

function Base.map(f, d::AbstractDictionary, ds::AbstractDictionary...)
out = similar(d, Core.Compiler.return_type(f, Tuple{eltype(d), map(eltype, ds)...}))
out = similar(d, return_type(f, Tuple{eltype(d), map(eltype, ds)...}))
@inbounds map!(f, out, d, ds...)
return out
end
Expand Down Expand Up @@ -146,7 +146,7 @@
if VERSION > v"1.6-"
function Iterators.map(f, d::AbstractDictionary)
I = keytype(d)
T = Core.Compiler.return_type(f, Tuple{eltype(d)}) # Base normally wouldn't invoke inference for something like this...
T = return_type(f, Tuple{eltype(d)}) # Base normally wouldn't invoke inference for something like this...

Check warning on line 149 in src/map.jl

View check run for this annotation

Codecov / codecov/patch

src/map.jl#L149 

Added line #L149 was not covered by tests
return MappedDictionary{I, T, typeof(f), Tuple{typeof(d)}}(f, (d,))
end
end
4 changes: 2 additions & 2 deletions test/map.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
@testset "map" begin
function _mapview(f, d::AbstractDictionary)
I = keytype(d)
T = Core.Compiler.return_type(f, Tuple{eltype(d)})
T = Dictionaries.return_type(f, Tuple{eltype(d)})

return MappedDictionary{I, T, typeof(f), Tuple{typeof(d)}}(f, (d,))
end
Expand All @@ -23,4 +23,4 @@

@test _mapview(iseven, d)::Dictionaries.MappedDictionary == dictionary([1=>false, 2=>false, 3=>true, 4=>true, 5=>false])
@test _mapview(isodd, d)::Dictionaries.MappedDictionary == dictionary([1=>true, 2=>true, 3=>false, 4=>false, 5=>true])
end
end
Loading