-
-
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
Add callsuper macro. #20131
Add callsuper macro. #20131
Conversation
Feel free to bikeshed names. |
Fixes #18252.
658f936
to
236fda2
Compare
kwargs = Any[] | ||
blk = quote end | ||
for arg in args | ||
if isa(arg,Expr) && arg.head == :kw |
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.
This is not entirely correct. A few cases that should be handled and possibly tested are. (These are the reason I started but didn't finish the pr yet...)
-
Evaluation order
Currently
:parameters
are always evaluated the first. It's not necessarily the best choice but this should be consisent with normal calls. -
:parameters
can have...
-
:parameters
can have non:kw
in it -
positional arguments can also have
...
-
if
parameter
only have...
s and all of them are empty, the non-kw version should be called.
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.
what non-:kw
can a :parameters
have other than ...
?
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.
Any, iterable objects
julia> f(;kws...) = kws
f (generic function with 1 method)
julia> j = [:j, :k]
2-element Array{Symbol,1}:
:j
:k
julia> f(;:a=>:b, (:c, :d), [:e, :f], (:g, :h, :i), j)
5-element Array{Any,1}:
(:a,:b)
(:c,:d)
(:e,:f)
(:g,:h)
(:j,:k)
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.
@yuyichao, wouldn't be easier all around if you just did this PR? There was a similar back and forth when I proposed a possible implementation
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.
Maybe. I started an implementation but haven't finished it yet (collecting data of what input should/can be supported and what generic fallback path I can use).....
push!(vals, val) | ||
push!(types, typ) | ||
if isa(arg,Expr) && arg.head == :(::) && length(arg.args) == 2 | ||
push!(blk.args, :($typ = $(esc(arg.args[2])))) |
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.
Also, val
should be evaluated before the type.
end | ||
end | ||
if isempty(kwargs) | ||
push!(blk.args, :(invoke($(esc(fname)), Tuple{$(types...)}, $(vals...)))) |
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 fname
should be evaluated before the arguments.
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.
Actually, fname
evaluation is a bit weird:
julia> f(args...; kwargs...) = (args,kwargs)
f (generic function with 1 method)
julia> (println("f"); f)((println("a"); "a"); [:y,(println("b"); "b")])
b
f
f
a
(("a",),Any[(:y,"b")])
julia> (println("f"); f)((println("a"); "a"); [:y => (println("b"); "b")]...)
b
f
a
f
f
(("a",),Any[(:y,"b")])
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.
This is a lowering bug. The lowering code should emit a temporary variable to hold f
if it cannot proof that evaluating f
is "pure".
I like the name, @simonbyrne. Thanks for pushing this forward – it's a much nicer interface than bare |
IIUC, this doesn't fully fix #7045 since it doesn't fix the case of using |
This would be a really nice feature, is anyone still working on it? |
If I understand correctly this is superseded by #38438. Let me know if I misunderstood. |
Indeed it is. |
Fixes #18252 and #7045.