WIP: Caller location for macros

[c42f]

As discussed in #9577 (see also #9579 and #13339), sometimes it would be really useful to have the line number and file of the location a macro was called. Personally I ran across a related problem trying to make a prototype for a new logging system (wip at JuliaLang/Juleps#30, see also c42f/MicroLogging.jl#1) where I want the source line of each log message to be extracted at compilation time. The current version of @__LINE__ doesn't do what we want here, as it's expanded immediately by the parser and gives the line number of the macro body rather than the calling code.

In this WIP, I've implemented a @__LOCATION__ macro, which is expanded to the line of the calling code with a rule at macro expansion time. This uses the newly available line numbers in the AST so I think it's fairly non disruptive I've moved the behavior of the @__LINE__ macro from the parser to macro expansion time, and expanded it to the line of code where the macro was called from, when inside a macro expansion. As a side effect, the special parsing of @__LINE__ was dropped, which means it parses the same as every other macro. The same machinery also allows us to solve #9577 in a non disruptive way by adding the line number of the macro caller to thread local state in a way that macros can get at macro expansion time without changing the macro calling convention. So I just went ahead and did that too.

demo.jl:

macro info(msg)
    @show current_location()
    quote
        println("INFO $(@__FILE__):$(@__LOCATION__): $($msg)")
    end
end

@info "<- line in macro.  Line at caller is $(@__LINE__)"

julia> include("demo.jl")
current_location() = 8
INFO /home/cfoster/src/julia-git/demo.jl:8: <- line in macro.  Line at caller is 8

julia> @info "blah"
current_location() = 1
INFO nothing:1: blah

Now for the WIP bit - I'd love some feedback on the design and implementation here:

Design

• I'm not happy with having three distinct mechanisms for talking about lines (ie, @__LINE__, @__LOCATION__, and current_location()). I'm inclined to tweak this so that @__LINE__ takes over the @__LOCATION__ functionality, as emitting @__LINE__ inside a macro AST is currently not very useful done. Also naming bikeshedding, etc. I'm not attached to any naming used here.

• As you see above, the REPL still gives @__FILE__ as nothing, despite the parser seeing a filename of REPL[N]. I'd like to refactor @__FILE__ to use the same mechanism that I'm using for @__LOCATION__ here - is this reasonable? I'm confused by the current method by which @__FILE__ is expanded.

Implementation

• Despite reading a fair bit of code, I still don't think I understand the various mechanisms by which the global jl_lineno may be set and reset. Especially given the various code paths by which statements are parsed and executed in the repl vs from file vs whatever else. I'm very inclined to refactor things by removing the global entirely, and forcing all line/filename information to flow via the ASTs, but that's for another PR, I think (advice very welcome).

• I'm not quite sure how reasonable my use of with-bindings is or whether it would be preferable to pass current-lineno explicitly (@ihnorton can I join the scheme riff-raff club? :-) )

Oh, by the way @JeffBezanson, having this in at least three languages is... a strangely winning mixture of fun and frustration :-)

[c42f]

Closed as I opened this prematurely, sorry for the noise.

[vtjnash]

+1. See also #13339

[c42f]

Ah excellent, thanks @vtjnash for pointing that out.

I found about four other issues mentioning this problem, including an old PR from Keno, but missed #13339. Not sure why, perhaps I just missed it in the github search.

I'll open a new PR for this, with an actual description and discussion of the possible design variations, unless people would prefer me just to reopen this one.

[tkelman]

less noise to keep using this one. just be sure to reopen before pushing anything else to the branch or github might prevent it from being reopened

[c42f]

Sure, no problems, I've reopened it, this time with a proper description.

[c42f]

Hmm, test failures seem unrelated, cf. #20906

[yuyichao]

Other than making this thread local, shouldn't this just be Int(unsafe_load(cglobal(:jl_lineno, Cint)))?

[c42f]

No, but good question. ptls->current_lineno is not really a replacement for jl_lineno, but rather a place to stash the line number of the macro caller. (The only place it's set is inside fl_invoke_julia_macro.) As such I've probably named it badly, what if I rename it as macro_caller_lineno or something?

Side question - is there an approved way to load thread local state from julia without creating a new C function specifically for the purpose? I didn't see one.

[c42f]

I went ahead and put the @__LOCATION__ behavior into @__LINE__ as I think that makes more sense for the end user. Easy to change back if necessary.

[StefanKarpinski]

Are the parens in @__LINE__() + 1 necessary after this change in order to not parse it as @__LINE__(+,1) or @__LINE__(+1)? Did this PR change that parsing?

I'm also unclear on what it means to include the location information in @__LINE__. The tests and docs on the PR currently don't clarify it for me.

[c42f]

@__LINE__ now parse just like any other macro, so in @__LINE__ + 1 the + parses as a unary operator and you get the same as @__LINE__(+ 1).

I'm also unclear on what it means to include the location information in @LINE

What I did in the last two patches was remove the __LOCATION__ macro entirely, and change the behavior of __LINE__ instead. This is no functional change, except in the case __LINE__ is emitted from a macro body. As discussed in the PR docs, I wasn't sure it made sense to have yet another mechanism for referring to line numbers when the current __LINE__ behavior is not very useful inside a macro.

[c42f]

@StefanKarpinski I've updated the description to hopefully be a bit clearer, and added some tests at the top of test/loading.jl for the behavior implemented so far.

I'm not a fan of the fact I had to update the __LINE__ usage to add parens everywhere, even though it's now consistent with other macro parsing. I've gone for consistency, but we could leave the special parsing rules in place if desired.

[ihnorton]

Exciting, would be great to get this issue resolved!

• Does this work for nested expansions? I don't have a counter-example handy right now, but that was tricky when I looked at this.
• map-expand-with-lineno looks kind of expensive (but my sense of flisp performance is fairly limited!). IIRC, macro expansion is in the lowering path everywhere in base now due to how doc strings were implemented, so might be good to double-check that there isn't a compiler perf hit here.

[c42f]

Does this work for nested expansions?

Great question, I should add a test case for this. The behavior implemented here is that @__LINE__ will expand to the location of the calling macro a single level up, provided the caller has line exprs generated as part of its AST:

macro a()
    quote
        @__LINE__
    end
end

macro b()
    line_a_called_inside_b = @__LINE__() + 2
    quote
        (@a(), $line_a_called_inside_b)
    end
end

@show @b

# Which shows
@b() = (12, 12)

With changes to the map-expand-with-lineno code it would be possible to make available the full "stack trace" of macro expansions, though I'm not sure this would be a good idea :-)

Expanding only a single level won't be the right thing in all circumstances, but it works well for macros which occur inside other macros taking an entire scope (@inline, @doc, @compat, etc)

I don't have a feel for performance impact yet, needs some testing I guess. I'm not quite sure that @doc is something to worry about specifically - all code needs to be mapped through julia-expand-macros regardless of whether it's wrapped in a macro or not.

[c42f]

Tests now refactored for clarity, with cases added to test the expansion of @__LINE__ in nested macro expansions.

An interesting benefit of expanding @__LINE__ using the AST is that macros now have control over whether they're visible to a nested @__LINE__ expansion, by controlling whether line ast nodes are emitted in the returned AST (see tests at the top of test/loading.jl).

[c42f]

Any more comments on this one? Is it clear enough what the use cases for these changes are? I think this addresses the core of what's needed for #9577, #9579 and #13339?

I'm keen to move along implementing any further changes people think necessary to turn this from a WIP into something ready for production. Things I'm planning to do:

• I think current_location() probably needs a rename for clarity. What about macro_caller_line()?
• @ihnorton mentioned performance of the expansion code. Is there a particular benchmark I should be running for this?
• I'll dig further into the implementation questions in the description, but I'd really appreciate some high level advice. It will take a while to understand the compiler well enough that I think I get it in broad overview.

[yuyichao]

Replaced

[vtjnash]

Thanks c42f for the PR. I appreciated the perspective on the alternate approach, and used your tests in my PR.

[c42f]

Ah, it's frustrating that github doesn't send notifications when issues are linked. Otherwise I'd have reviewed #21746 a month ago when it was created..

[c42f]

By the way guys, if it was obvious that I was going in completely the wrong direction with this one it would have been good to know a little sooner. Just a quick and clear "Hey, don't go too far down the wrong path, I really think we're going to solve this with the approach in #13339" would have been good.

Now that I've dived into the system, it's much easier to appreciate the way the new parser and expansion step from #13339 work together to pass the necessary information without breaking user code. So I learned a lot and this wasn't wasted time, but perhaps could have been spent a little more fruitfully elsewhere, with just a little nudge ;-)

[ihnorton]

@c42f sorry for lack of further comment on my part. I didn't like the single-level limitation, and that made the approach "feel" brittle, but I wasn't sure I fully understood the approach (nor could take the time to do at the point when I was commenting). I decided to leave constructive feedback because it was still an improvement!

[c42f]

That's ok, luckily I'm not too attached to the particular implementation details - really I just want the problem solved. It's a little disappointing to see a bit of the effort go to waste, but mainly I'm happy that the problem is mostly solved by #21746. I am disappointed that one of the pieces of functionality implemented here (and tested in the tests) didn't seem to make its way into the version now in master. (see #21746 (comment) ).

I didn't like the single-level limitation, and that made the approach "feel" brittle

But this is constructive feedback! "I don't like this PR because of 'X'" is a perfectly valid thing to say - it opens the discussion about why things were implemented the way they are, or sheds light on why the approach is bad or good.

I don't think there's anything single level about what I did here, by the way (it supports arbitrary nesting of expansions, and allows macros to control whether they're visible to @LINE or not). A big upside of what's here was that it didn't change the AST at all, since it utilizes the usual line number nodes emitted by the parser, instead of introducing a whole new mechanism.

[c42f]

I am disappointed that one of the pieces of functionality implemented here (and tested in the tests) didn't seem to make its way into the version now in master

Luckily it looks like I'm wrong about this. In which case, awesome, I'm quite happy with what's in master. It's breakage in the AST layout which could have been avoided, but with the upside of being more explicit, which seems like a good balance.