Enable `@edit @time 1+1`, fix #3377 #15282

dhoegh · 2016-02-28T20:16:28Z

This allows for writing ~~methods(:@time), edit(:@time) and~~ @edit @time 1+1 plus the equivalent for which and less. ~~methods can also check for macro methods as methods(:@time, Tuple{Any})~~. The @edit, @less and @which do also check for method specialization of the macro.

StefanKarpinski · 2016-02-28T23:49:36Z

💯

yuyichao · 2016-02-28T23:50:41Z

base/interactiveutil.jl

-    if !isa(ex, Expr)
+    if fcn in [:which, :less, :edit] && ex0.head == :macrocall
+        # special case @edit @time 1+2 to edit the macro
+        is_macro = true


Why is the special case needed?

The special case is needed because when the expand function called above expands the macro/insert its code into the expression, and hence the edit call will call edit on the expanded code. The special casing makes which, less and edit macro call the macrocall instead of the code the macro expands to. This enables the user to write: @edit @time 1+1, this will go to the @time macro's definition.

Is there a case you don't want to use operate on the macro?

My main concern is that this doesn't seem to be the way this should work. If it is somehow really necessary to have different behavior for different macros that use this function, it should be controlled explicitly with a optional/keyword argument to this function instead.

I understand your concern, but from my point of view it do not make sense to ask for @edit @random_macro max(1,1) and not point to the macro. As a macro can generate arbitrary code for you, and hence you do not know what you call @edit on.

dhoegh · 2016-02-29T18:49:27Z

I have pushed a new commit which adresses the comments. I did also find a bug, caused by wrong esc. I have addressed it and changed the tests to test it. Ping me when it is ready for a merge, then i will squas the commits.

dhoegh · 2016-02-29T20:00:47Z

I have squashed the commits.

JeffBezanson · 2016-02-29T20:18:49Z

base/reflection.jl

@@ -179,6 +194,10 @@ function methods(f::ANY,t::ANY)
    Any[m[3] for m in _methods(f,t,-1)]
 end
 function _methods(f::ANY,t::ANY,lim)
+    #special case Expr to allow methods(@time,Tuple{Any})
+    if _is_empty_macrocall(f)
+        f = _get_macro(current_module(),f.args[1])


This code is incorrect --- it makes it impossible for the system to query the methods of an Expr object. Exprs don't have any methods of course, but that just means this function must return an empty array.

I do not quite follow you. The meaning of the function is to get function object of the macro from the getfield expression. Do you have an alternative way of obtaining the result?

You can't do this in _methods. _methods(f, ...) must return the list of methods you could call by doing f(...) with the specific object f. The following (current) behavior is correct:

julia> e = Expr(:macrocall, symbol("@time")) :(@time) julia> e() ERROR: MethodError: objects of type Expr are not callable in eval(::Module, ::Any) at ./boot.jl:275 julia> methods(e) # 0 methods for generic function "(::Expr)":

JeffBezanson · 2016-02-29T20:25:11Z

This functionality is great, but we need to figure out a different way to provide it. This implementation works via a pun, where methods "lies" about what the answer is for certain Exprs.

dhoegh · 2016-02-29T20:33:40Z

If there was a way to obtain the function object of the macro, then it would be easier, to solve correctly. Because Base.(Symbol("@time")) feels awkward.

JeffBezanson · 2016-02-29T20:38:54Z

I think the best approach for now is to limit this to user-interface-level features like @edit and @which. We could even add @methods.

dhoegh · 2016-03-01T19:27:09Z

@JeffBezanson I have changed the PR to suite your comments. I have added @methods and @functionloc macro. The only hatch by adding @methods @boundscheck do not return all methods for @boundscheck but return empty as it check for a zero argument method of @boundscheck. Hence @methods and @which is strictly speaking returning the same thing. The only difference is the @methods return a Array{Any,1} with one element. An alternativ approach would be to make @methods return all methods and if an argument is given throw an error and pointing to @which.

m = getfield(current_module(), Symbol("@boundscheck"))
methods(m) == @methods @boundscheck
@methods @boundscheck 1 # throw an error.

dhoegh · 2016-03-02T16:41:45Z

I have added docs to @functionloc and edited docs for @edit, @less and @which. I have moved the docs inline. The commit do also removed the @methods macro as it provided almost the same as @which.

yuyichao · 2016-03-02T16:49:18Z

base/interactiveutil.jl

    ex = expand(ex0)
-    if !isa(ex, Expr)
+    if fcn in [:which, :less, :edit, :functionloc] && ex0.head == :macrocall


I think you should check isa(ex0, Expr) here.

Also, I still don't understand why are the special cases necessary here. Why do you want @code_lowered @time 1 + 2 to error instead of showing the ast?

I see where you are going now:). I have removed the special casing of @edit etc. The Ast displayed by @code_warntype is pretty ugly, but it displays.

julia> @code_warntype @time 1+1 Variables: #self#::Base.#@time ex::Expr ####args#787#8911::TUPLE Body: begin # util.jl, line 152: return (top(_expr))(:block,$(Expr(:copyast, :($(QuoteNode(:( # util.jl, line 153:)))))),$(Expr(:copyast, :($(QuoteNode(:(local stats = gc_num())))))),$(Expr(:copyast, :($(QuoteNode(:( # util.jl, line 154:)))))),$(Expr(:copyast, :($(QuoteNode(:(local elapsedtime = time_ns())))))),$(Expr(:copyast, :($(QuoteNode(:( # util.jl, line 155:)))))),(top(_expr))(:local,(top(_expr))(:(=),:val,(Core._expr)(:escape,ex::Expr)::ANY)::Expr)::Expr,$(Expr(:copyast, :($(QuoteNode(:( # util.jl, line 156:)))))),$(Expr(:copyast, :($(QuoteNode(:(elapsedtime = time_ns() - elapsedtime)))))),$(Expr(:copyast, :($(QuoteNode(:( # util.jl, line 157:)))))),$(Expr(:copyast, :($(QuoteNode(:(local diff = GC_Diff(gc_num(),stats))))))),$(Expr(:copyast, :($(QuoteNode(:( # util.jl, line 158:)))))),$(Expr(:copyast, :($(QuoteNode(:(time_print(elapsedtime,diff.allocd,diff.total_time,gc_alloc_count(diff)))))))),$(Expr(:copyast, :($(QuoteNode(:( # util.jl, line 160:)))))),:val)::Expr end::Expr

Trust me, it's not going to be "uglier" than the AST of this function (gen_call_with_extracted_types) itself.

dhoegh · 2016-03-02T18:32:50Z

I will do. I will squash the commit away, when I have updated genstdlib.jl

dhoegh · 2016-03-06T14:39:22Z

I have executed genstdlib.jl and squased the commits. This should be ready to merge if no further comment are made.

tkelman · 2016-03-07T00:33:23Z

base/interactiveutil.jl

+    @functionloc
+
+Applied to a function or macro call, it evaluates the arguments to the specified call, and
+return a tuple `(filename,line)` giving the location for the method that would be called for those arguments.


Thank you. I have fixed it and updated the PR.

dhoegh · 2016-03-13T12:34:01Z

Bump, I have rebased the PR to resolve merge conflicts.

dhoegh · 2016-03-23T18:13:32Z

Bump/any further comment's?

StefanKarpinski · 2016-03-23T22:30:31Z

@JeffBezanson, you're the one who had objections before, so please take a look and merge if it's ok.

dhoegh · 2016-03-31T17:17:37Z

Bump.

yuyichao · 2016-03-31T17:28:56Z

LGTM. @JeffBezanson ?

yuyichao · 2016-04-05T19:04:41Z

@dhoegh I think I'll merge this if you can rebase again and no one have further comment.

@time

…ctionloc @time 1+1` plus the equivalent for `@which`, `@edit` and `@less`. The macros check for method specialization of the macro and return the dispatched macro. The `@functionloc` macro is exported and the documentation is updated.

dhoegh · 2016-04-05T19:41:28Z

@yuyichao thank you. I have rebased the PR and it should be good to merge when the CI is finished.

yuyichao · 2016-04-05T20:53:46Z

.... Somehow the commit was merged but @github thinks the PR is still open and now with conflicts ..... Manually close since the merge is done....

@time

Enable `@edit @time 1+1`, fix JuliaLang#3377

dhoegh force-pushed the edit_macro branch from 9722bb4 to a4bf4ae Compare February 28, 2016 20:30

yuyichao reviewed Feb 28, 2016
View reviewed changes

dhoegh force-pushed the edit_macro branch from 64c8f2a to 9dae03b Compare February 29, 2016 19:59

JeffBezanson reviewed Feb 29, 2016
View reviewed changes

dhoegh force-pushed the edit_macro branch 2 times, most recently from ca9241e to 8ecd97f Compare March 1, 2016 19:10

dhoegh changed the title ~~Enable methods(:@time), fix #3377~~ Enable @edit @time 1+1, fix #3377 Mar 1, 2016

yuyichao reviewed Mar 2, 2016
View reviewed changes

dhoegh force-pushed the edit_macro branch 2 times, most recently from 9be0c68 to 6c4e05c Compare March 6, 2016 14:37

dhoegh force-pushed the edit_macro branch from 6c4e05c to 305c25a Compare March 6, 2016 17:03

tkelman reviewed Mar 7, 2016
View reviewed changes

dhoegh force-pushed the edit_macro branch 2 times, most recently from bb1e957 to 09507d7 Compare March 13, 2016 12:32

dhoegh force-pushed the edit_macro branch from 09507d7 to dd6b9ee Compare April 5, 2016 19:13

yuyichao closed this Apr 5, 2016

elsuizo pushed a commit to elsuizo/julia that referenced this pull request Apr 5, 2016

Merge pull request JuliaLang#15282 from dhoegh/edit_macro

adf800e

Enable `@edit @time 1+1`, fix JuliaLang#3377

dhoegh mentioned this pull request Jul 9, 2016

less() and edit() are not defined for macros #3377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `@edit @time 1+1`, fix #3377 #15282

Enable `@edit @time 1+1`, fix #3377 #15282

dhoegh commented Feb 28, 2016

StefanKarpinski commented Feb 28, 2016

yuyichao Feb 28, 2016

dhoegh Feb 29, 2016

yuyichao Feb 29, 2016

yuyichao Feb 29, 2016

dhoegh Feb 29, 2016

dhoegh commented Feb 29, 2016

dhoegh commented Feb 29, 2016

JeffBezanson Feb 29, 2016

dhoegh Feb 29, 2016

JeffBezanson Feb 29, 2016

JeffBezanson commented Feb 29, 2016

dhoegh commented Feb 29, 2016

JeffBezanson commented Feb 29, 2016

dhoegh commented Mar 1, 2016

dhoegh commented Mar 2, 2016

yuyichao Mar 2, 2016

dhoegh Mar 2, 2016

yuyichao Mar 2, 2016

dhoegh commented Mar 2, 2016

dhoegh commented Mar 6, 2016

tkelman Mar 7, 2016

dhoegh Mar 7, 2016

dhoegh commented Mar 13, 2016

dhoegh commented Mar 23, 2016

StefanKarpinski commented Mar 23, 2016

dhoegh commented Mar 31, 2016

yuyichao commented Mar 31, 2016

yuyichao commented Apr 5, 2016

dhoegh commented Apr 5, 2016

yuyichao commented Apr 5, 2016

Enable @edit @time 1+1, fix #3377 #15282

Enable @edit @time 1+1, fix #3377 #15282

Conversation

dhoegh commented Feb 28, 2016

StefanKarpinski commented Feb 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoegh commented Feb 29, 2016

dhoegh commented Feb 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Feb 29, 2016

dhoegh commented Feb 29, 2016

JeffBezanson commented Feb 29, 2016

dhoegh commented Mar 1, 2016

dhoegh commented Mar 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoegh commented Mar 2, 2016

dhoegh commented Mar 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoegh commented Mar 13, 2016

dhoegh commented Mar 23, 2016

StefanKarpinski commented Mar 23, 2016

dhoegh commented Mar 31, 2016

yuyichao commented Mar 31, 2016

yuyichao commented Apr 5, 2016

dhoegh commented Apr 5, 2016

yuyichao commented Apr 5, 2016

Enable `@edit @time 1+1`, fix #3377 #15282

Enable `@edit @time 1+1`, fix #3377 #15282