-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
define at-invoke
macro
#38438
define at-invoke
macro
#38438
Conversation
and these macros should be removed once JuliaLang/julia#38438 gets merged
c1c9d1d
to
da0cb43
Compare
17e2c06
to
305eb6f
Compare
bump, what do you think on this, @JeffBezanson @StefanKarpinski ? |
4688d88
to
305ef91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- provides easier syntax to call `Core.invoke`, e.g. `@invoke f(a::Integer)` will be expanded into `invoke(f, Tuple{Integer}, a)` - when type annotation is omitted, the argument type is specified as `Any` Built on top of JuliaLang#37971
I rebased this branch against the current master. The single failed test seems unrelated to this PR:
|
|
||
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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),
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
?)
There was a problem hiding this comment.
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.
- provides easier syntax to call `Core.invoke`, e.g. `@invoke f(a::Integer)` will be expanded into `invoke(f, Tuple{Integer}, a)` - when type annotation is omitted, the argument type is specified as `Any` Built on top of JuliaLang#37971
Core.invoke
, e.g.@invoke f(a::Integer)
will be expanded intoinvoke(f, Tuple{Integer}, a)
Any
Built on top of #37971 (i.e. the current diff includes changes for
@invokelatest
); I would like to merge that PR first, and then I will rebase this PR against that.