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

define at-invoke macro #38438

Merged
merged 1 commit into from
Dec 25, 2020
Merged

define at-invoke macro #38438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 23 additions & 1 deletion base/util.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,23 @@ function _kwdef!(blk, params_args, call_args)
blk
end

"""
@invoke f(arg::T, ...; kwargs...)

Provides a convenient way to call [`invoke`](@ref);
`@invoke f(arg1::T1, arg2::T2; kwargs...)` will be expanded into `invoke(f, Tuple{T1,T2}, arg1, arg2; kwargs...)`.
When an argument's type annotation is omitted, it's specified as `Any` argument, e.g.
Copy link
Member

@stevengj stevengj Dec 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

My inclination is that an omitted type annotation should be equivalent to arg::typeof(arg), not arg::Any.

That is, it should be like passing the argument as-is, so that @invoke f(x,y) dispatches like an ordinary f(x,y) call.

Copy link
Member Author

@aviatesk aviatesk Dec 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Fair.

Why I didn't introduce arg::typeof(arg) pattern was it may lead to slower dynamic method lookup when used carelessly:

obvious benchmark

f(::Char) = return
f(::Int) = return
f(::Float64) = return

# w/o `arg::typeof(arg)`, it may be encouraged to statically resolve all the `invoke`
@btime let
    n = 10^3
    for _ in 1:n
        a = rand(Any[1,1.,'1'])
        if isa(a, Char)
            invoke(f, Tuple{Char}, a)
        elseif isa(a, Int)
            invoke(f, Tuple{Int}, a)
        elseif isa(a, Float64)
            invoke(f, Tuple{Float64}, a)
        end
    end
end # => 36.807 μs (1000 allocations: 109.38 KiB)

# with `arg::typeof(a)`, we may want to write just simply as
@btime let
    n = 10^3
    for _ in 1:n
        a = rand(Any[1,1.,'1'])
        invoke(f, Tuple{typeof(a)}, a)
    end
end # => 226.022 μs (4000 allocations: 281.25 KiB)

Well, I'm fine either way; arg::typeof(arg) is certainly intuitive, and we can assume @invoke is used carefully (it's so much reflective, anyway),

Copy link
Member Author

Choose a reason for hiding this comment

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

And, with some cases like below, the current behavior might look more "intuitive"

julia> f(a) = :Any
f (generic function with 1 methods)

julia> f(a::Int) = :Int
f (generic function with 2 methods)

julia> let
           a = 1
           @invoke f(a) # which is more intuitive ?
       end

Copy link
Member

@stevengj stevengj Dec 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I find it more intuitive that unspecified argument types correspond to "ordinary" dispatch rules.

(In many cases there won't even be an Any method.)

Copy link
Member

Choose a reason for hiding this comment

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

I agree that unspecified types meaning x::typeof(x) seems more useful than x::Any, but using typeof here might lead to some unexpected edge cases, for example f(Int) dispatching to f(::DataType) instead of f(::Type{Int}). Ideally we'd use Core.Typeof instead, but IIRC that has some performance gotchas.

Copy link
Member

Choose a reason for hiding this comment

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

@JeffBezanson, what is the best way to fall back to “ordinary” dispatch here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is merged and out in the wild, but I'd like to express support for applying ordinary dispatch rules to arguments without specified types within @invoke. Here's an example of why, distilled from a real-world use case:

abstract type Foo end
struct Bar <: Foo end

f(x::Foo, y) = 1
f(x::Foo, ::Nothing) = 2
f(x::Bar, y) = 3
f(x::Bar, ::Nothing) = 4

x = Bar()
Base.@invoke f(x::Foo, nothing)  # should return 2, returns 1

I think consistent dispatch is more important than avoiding dynamic dispatch here. Unless there's some subtlety I don't understand, the dispatch of @invoke f(x::Foo, y) would only have to be dynamic when the type of y can't be inferred, in which case f(x, y) and any other dispatch on the type of y would also be dynamic, so there's not much lost. (However, I know nothing about the gotchas with Core.Typeof. Suppose the type of x is not inferred: is invoke(f, Tuple{Core.Typeof(x)}, x) much slower than f(x)?)

Copy link
Member

Choose a reason for hiding this comment

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

I fully agree that this current behavior is undesirable, arguably even buggy. We should really consider changing this.

`@invoke f(arg1::T, arg2)` will be expanded into `invoke(f, Tuple{T,Any}, arg1, arg2)`.
"""
macro invoke(ex)
f, args, kwargs = destructure_callex(ex)
arg2typs = map(args) do x
is_expr(x, :(::)) ? (x.args...,) : (x, GlobalRef(Core, :Any))
end
args, argtypes = first.(arg2typs), last.(arg2typs)
return esc(:($(GlobalRef(Core, :invoke))($(f), Tuple{$(argtypes...)}, $(args...); $(kwargs...))))
end

"""
@invokelatest f(args...; kwargs...)

Expand All @@ -523,6 +540,11 @@ Provides a convenient way to call [`Base.invokelatest`](@ref).
`Base.invokelatest(f, args...; kwargs...)`.
"""
macro invokelatest(ex)
f, args, kwargs = destructure_callex(ex)
return esc(:($(GlobalRef(Base, :invokelatest))($(f), $(args...); $(kwargs...))))
end

function destructure_callex(ex)
is_expr(ex, :call) || throw(ArgumentError("a call expression f(args...; kwargs...) should be given"))

f = first(ex.args)
Expand All @@ -538,7 +560,7 @@ macro invokelatest(ex)
end
end

esc(:($(GlobalRef(Base, :invokelatest))($(f), $(args...); $(kwargs...))))
return f, args, kwargs
end

# testing
Expand Down
1 change: 1 addition & 0 deletions doc/src/base/base.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ Core.Function
Base.hasmethod
Core.applicable
Core.invoke
Base.@invoke
Base.invokelatest
Base.@invokelatest
new
Expand Down
40 changes: 40 additions & 0 deletions test/misc.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,46 @@ let foo() = begin
@test bar() == 1
end

@testset "@invoke macro" begin
# test against `invoke` doc example
let
f(x::Real) = x^2
f(x::Integer) = 1 + Base.@invoke f(x::Real)
@test f(2) == 5
end

let
f1(::Integer) = Integer
f1(::Real) = Real;
f2(x::Real) = _f2(x)
_f2(::Integer) = Integer
_f2(_) = Real
@test f1(1) === Integer
@test f2(1) === Integer
@test Base.@invoke(f1(1::Real)) === Real
@test Base.@invoke(f2(1::Real)) === Integer
end

# when argment's type annotation is omitted, it should be specified as `Any`
let
f(_) = Any
f(x::Integer) = Integer
@test f(1) === Integer
@test Base.@invoke(f(1::Any)) === Any
@test Base.@invoke(f(1)) === Any
end

# handle keyword arguments correctly
let
f(a; kw1 = nothing, kw2 = nothing) = a + max(kw1, kw2)
f(::Integer; kwargs...) = error("don't call me")

@test_throws Exception f(1; kw1 = 1, kw2 = 2)
@test 3 == Base.@invoke f(1::Any; kw1 = 1, kw2 = 2)
@test 3 == Base.@invoke f(1; kw1 = 1, kw2 = 2)
end
end

# Endian tests
# For now, we only support little endian.
# Add an `Sys.ARCH` test for big endian when/if we add support for that.
Expand Down