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.

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

pass macro call location to macro-function (rebase) #21746

Merged
merged 3 commits into from
May 25, 2017

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented May 8, 2017

rebase of #13339
fixes #9577
replaces #9579
replaces #20895

  • merge and tag Compat
  • merge and tag Documenter
  • re-enable building docs
  • add to NEWs and docs on macros

@vtjnash vtjnash force-pushed the jn/ihn/macrocall_pass_loc branch from 7aac52f to 64a60be Compare May 8, 2017 17:58
@Keno
Copy link
Member

Keno commented May 8, 2017

I can haz column info as well?

@vtjnash
Copy link
Member Author

vtjnash commented May 8, 2017

The parser doesn't have any, afaik. I think getting line number ranges would be the next step.

@Keno
Copy link
Member

Keno commented May 8, 2017

#9579 has code for column tracking in the parser. I don't really care about line ranges since that's easy to compute for string macros.

@vtjnash
Copy link
Member Author

vtjnash commented May 8, 2017

Would you recommend calling this __source__ or __location__ instead then, and update the LineNumberNode object to be a general LocationNode (with possible future fields line, column, file, line_end, and column_end)?

@Keno
Copy link
Member

Keno commented May 8, 2017

That sounds like a good idea.

@vtjnash
Copy link
Member Author

vtjnash commented May 8, 2017

This breaks Documenter because it tries to pattern-match the macrocall expression to see if it has zero-arguments or no arguments specified, and will need the following patch:

diff --git a/src/DocSystem.jl b/src/DocSystem.jl
index e3b89c3f..333feabe 100644
--- a/src/DocSystem.jl
+++ b/src/DocSystem.jl
@@ -88,7 +88,7 @@ binding(m::Module, λ::Any) = binding(λ)
 
 function signature(x, str::AbstractString)
     ts = Base.Docs.signature(x)
-    (Meta.isexpr(x, :macrocall, 1) && !endswith(strip(str), "()")) ? :(Union{}) : ts
+    (Meta.isexpr(x, :macrocall, 2) && !endswith(strip(str), "()")) ? :(Union{}) : ts
 end
 if VERSION < v"0.5.0-dev"
     Base.Docs.signature(::Any) = :(Union{})
diff --git a/src/Utilities/Utilities.jl b/src/Utilities/Utilities.jl
index c4ce0a28..6c477443 100644
--- a/src/Utilities/Utilities.jl
+++ b/src/Utilities/Utilities.jl
@@ -238,7 +238,7 @@ Returns a expression that, when evaluated, returns an [`Object`](@ref) represent
 function object(ex::Union{Symbol, Expr}, str::AbstractString)
     binding   = Expr(:call, Binding, splitexpr(Docs.namify(ex))...)
     signature = Base.Docs.signature(ex)
-    isexpr(ex, :macrocall, 1) && !endswith(str, "()") && (signature = :(Union{}))
+    isexpr(ex, :macrocall, 2) && !endswith(str, "()") && (signature = :(Union{}))
     Expr(:call, Object, binding, signature)
 end
 
@@ -275,7 +275,7 @@ if VERSION < v"0.5-"
     end
 else
     function docs(ex::Union{Symbol, Expr}, str::AbstractString)
-        isexpr(ex, :macrocall, 1) && !endswith(rstrip(str), "()") && (ex = quot(ex))
+        isexpr(ex, :macrocall, 2) && !endswith(rstrip(str), "()") && (ex = quot(ex))
         :(Base.Docs.@doc $ex)
     end
 end

@vtjnash vtjnash force-pushed the jn/ihn/macrocall_pass_loc branch from 64a60be to 10afb8f Compare May 8, 2017 18:56
vtjnash added a commit to JuliaLang/Compat.jl that referenced this pull request May 10, 2017
vtjnash added a commit to JuliaLang/Compat.jl that referenced this pull request May 10, 2017
vtjnash added a commit to vtjnash/Documenter.jl that referenced this pull request May 10, 2017
vtjnash added a commit to vtjnash/Documenter.jl that referenced this pull request May 10, 2017
vtjnash added a commit to vtjnash/Documenter.jl that referenced this pull request May 10, 2017
vtjnash added a commit to JuliaLang/Compat.jl that referenced this pull request May 10, 2017
@vtjnash vtjnash force-pushed the jn/ihn/macrocall_pass_loc branch from 10afb8f to 1796a25 Compare May 10, 2017 04:39
@tkelman
Copy link
Contributor

tkelman commented May 10, 2017

Does this need devdocs updates for AST changes?

@vtjnash vtjnash force-pushed the jn/ihn/macrocall_pass_loc branch 2 times, most recently from 8c00bc2 to 1a42609 Compare May 10, 2017 05:22
@@ -1247,6 +1247,7 @@ export
# parser internal
@__FILE__,
@__DIR__,
@__LINE__,
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to be added to stdlib doc index to be included in the rendered docs

@vtjnash vtjnash force-pushed the jn/ihn/macrocall_pass_loc branch 2 times, most recently from 49686d3 to eb75d1f Compare May 10, 2017 17:43
vtjnash added a commit to JuliaLang/Compat.jl that referenced this pull request May 12, 2017
Compat flag for adding source location argument to macros (JuliaLang/julia#21746)
vtjnash added a commit to vtjnash/Documenter.jl that referenced this pull request May 15, 2017
mortenpi pushed a commit to JuliaDocs/Documenter.jl that referenced this pull request May 15, 2017
mortenpi pushed a commit to JuliaDocs/Documenter.jl that referenced this pull request May 15, 2017
@vtjnash vtjnash force-pushed the jn/ihn/macrocall_pass_loc branch from eb75d1f to 93c1f33 Compare May 19, 2017 20:00
@vtjnash vtjnash force-pushed the jn/ihn/macrocall_pass_loc branch 3 times, most recently from eaeda8b to 4269a3e Compare May 22, 2017 15:37
@vtjnash vtjnash changed the title RFC: pass macro call location to macro-function (rebase) pass macro call location to macro-function (rebase) May 22, 2017
@@ -708,6 +729,32 @@ julia> foo()

This kind of manipulation of variables should be used judiciously, but is occasionally quite handy.

Getting the hygiene rules correct can be a formidable challenge,
Copy link
Contributor

Choose a reason for hiding this comment

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

end sentence with a period not a comma

@vtjnash vtjnash force-pushed the jn/ihn/macrocall_pass_loc branch from 4269a3e to 54a185b Compare May 22, 2017 17:23
@vtjnash
Copy link
Member Author

vtjnash commented May 23, 2017

For users needing compatible parsing, note that (@__LINE__) will work.

NEWS.md Outdated
rather than from the task-local `SOURCE_PATH` global when it was expanded.

* All macros receive an extra argument `__source__::LineNumberNode` which describes the
parser location in the source file for the `@` of the the macro call.
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicated "the"

vtjnash and others added 3 commits May 24, 2017 14:53
…ll macros

also emit an explicit push_loc in @generated functions
rather than depending on the existence of a LineNumberNode
and other lowering heuristics to produce it
@vtjnash vtjnash force-pushed the jn/ihn/macrocall_pass_loc branch from af3ab07 to 6b05e37 Compare May 24, 2017 19:01
@vtjnash vtjnash merged commit 9e3318c into master May 25, 2017
@vtjnash vtjnash deleted the jn/ihn/macrocall_pass_loc branch May 25, 2017 19:37
@StefanKarpinski
Copy link
Member

#9577, #13339, #21746

@@ -1162,6 +1162,7 @@
(error "macros cannot accept keyword arguments"))
(expand-forms
`(function (call ,(symbol (string #\@ (cadr (cadr e))))
(|::| __source__ (core LineNumberNode))
Copy link
Member

Choose a reason for hiding this comment

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

Ah, it's suddenly clear how this approach could work at all. Very clever!

end
@test (@emit_LINE) == ((@__LINE__) - 3, @__LINE__)
@test @nested_LINE_expansion() == ((@__LINE__() - 4, @__LINE__() - 12), @__LINE__())
@test @nested_LINE_expansion2() == ((@__LINE__() - 5, @__LINE__() - 9), @__LINE__())
Copy link
Member

Choose a reason for hiding this comment

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

@vtjnash @ihnorton with this approach, it looks like there's no way to create a wrapper macro which should be transparent to @LINE, in the same way that non-block quote doesn't generate normal line number nodes.

In MicroLogging I've got @info, @warn implemented as trivial wrappers around an @logmsg macro which has all the guts of the logging implementation, and I was hoping to have @LINE expanded inside @logmsg. Of course, for the particular purposes of my package I can work around this limitation, but I'd rather not have to.

Do you see a viable way to allow such wrapper macros using the __source__ approach you've implemented here?

One probably ugly possibility would be to have access to a Expr(:macrocall_with_source, ...) (better name needed!) where the user can pass the __source__ argument explicitly, bypassing the expansion step?

Copy link
Member Author

Choose a reason for hiding this comment

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

From inside one macro, I think both
return Expr(:macrocall, Symbol("@macroname"), __source__, args...)
or equivalently
m = :(@macroname args...); m.args[1] = __source__; return m
should work.

Copy link
Member

@c42f c42f Jun 13, 2017

Choose a reason for hiding this comment

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

Ah, of course. Thanks - this does make a lot of sense. (The reason I ask is that I'm having odd LLVM compile errors trying to build julia-0.7 right now, so I haven't been able to just try the code myself.)

This seems to be both the benefit and downside of the solution here: a benefit, because the __source__ should be clearly visible and non-magical in the AST. This is great. A downside, because it'll disturb any code which processes Expr(:macrocall, ...) (which quite honestly probably isn't a whole lot of user code).

Copy link
Contributor

Choose a reason for hiding this comment

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

The Expr(:macrocall, ...) change broke multiple packages on nightly. A deprecation should really be put in.

Copy link
Member

@c42f c42f Jun 13, 2017

Choose a reason for hiding this comment

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

Hah, wouldn't have been a problem with #20895 :-P

How could a deprecation be made to work? I assume this broke packages which were doing moderately "interesting" AST manipulations. Those would just be manipulating the surface syntax via an Expr which happened to have head==:macrocall so there's nowhere to insert any kind of depwarn, is there?

I could imagine adding some kind of AST pattern matching tool inside Compat, and encouraging people to use that when interacting with ASTs containing macros? Could be a lot easier on users than the minimal flag in https://github.com/JuliaLang/Compat.jl/pull/355/files

Copy link
Contributor

Choose a reason for hiding this comment

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

there've been a few similar deprecations put in at the lowering level for macro writers who construct obsolete AST forms, instead of hard-breaking them

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so you're referring to packages which construct an Expr(:macrocall, args...) explicitly? In that case, would it be sufficient to detect when the first argument isn't a line number node, and add an extra dummy __source__ argument just before calling invoke-julia-macro?

There's another case which could cause breakage, as @a 1 now parses to include the value of the __source__ explicitly in the AST. Packages which attempt to process this AST into something else are presumably going to be surprised, when the Expr ends up with 1 in the args[3] slot rather than args[2]?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Function for getting source location inside string macros
7 participants