Allow outside catchers to see full stacktrace information of macro expansion

[aviatesk]

MacroExpansionError has good information about error location, but does not preserve information about the error raised by user macros beyond the error message itself. Such information is very useful for tooling like language servers, and stacktraces are particularly important.

This commit adds a stacktrace::Vector{Base.StackTraces.StackFrame} field to MacroExpansionError. New macro definitions still call the MacroExpansionError(ex::SyntaxTree, msg::AbstractString; position=:all) constructor, which internally calls stacktrace(...), so the user-facing interface remains unchanged.
Additionally, scrub_expand_macro_stacktrace is implemented to automatically trim information about JL internal functions that are not useful to users.

[c42f]

In the current design with lowering throwing errors, I find it a little odd to store the backtrace inside the exception itself, as that's also available at the catch site outside of JuliaLowering.

But I'm ok with this because we should probably move to an error propagation model where we don't throw any exception from lowering, but instead propagate errors via AST nodes of kind K"error" (allowing for partial lowering of broken ASTs). And in that case we will need something similar to what you've done here.

[c42f]

Oh, please also add a test of both catch error code paths before merging :)

(I added you as a collaborator so you can merge this when it's ready)

[aviatesk]

Made this change for the following reasons (along with the corresponding change on runtests.jl):

• To prevent name collisions with other test files
• To enable test/macros.jl to be executed as a standalone script from the command line

Having said that, this creates a different style from other test files, so if you don't like this, I'd be happy to revert it.

[c42f]

I don't really mind this change. But if you first include("test/utils.jl") most (all?) of the non-IR test/blah.jl should be able to be included individually at the REPL? This is how I run tests though it's admittedly slightly clunky.

[aviatesk]

In the current design with lowering throwing errors, I find it a little odd to store the backtrace inside the exception itself, as that's also available at the catch site outside of JuliaLowering.

Hmm, that's certainly true. If we change throw to rethrow here

JuliaLowering.jl/src/macro_expansion.jl

Lines 155 to 166 in 2dd53bf

	expanded = try
	# TODO: Allow invoking old-style macros for compat
	invokelatest(macfunc, macro_args...)
	catch exc
	if exc isa MacroExpansionError
	# Add context to the error.
	# TODO: Using rethrow() is kinda ugh. Is there a way to avoid it?
	rethrow(MacroExpansionError(mctx, exc.ex, exc.msg, exc.position, exc.stacktrace))
	else
	throw(MacroExpansionError(mctx, ex, "Error expanding macro", :all, scrub_expand_macro_stacktrace(stacktrace(catch_backtrace()))))
	end
	end

, it would be possible to obtain the stacktrace information from invokelatest(macfunc, macro_args...) even when caught outside. That might be cleaner and simpler implementation-wise, although we may end up implementing a similar stacktrace-capturing mechanism in the future anyway. Your comment just above also mentions wanting to avoid rethrow(), so I'd like to ask for your judgment on this.

[aviatesk]

That might be cleaner and simpler implementation-wise, although we may end up implementing a similar stacktrace-capturing mechanism in the future anyway.

I've implemented a way to avoid capturing stack traces by using rethrow: fbdca4d.
This works fine for my use case. Personally, I find this simpler, as it means we don't have to overthink the state of MacroExpansionError. So if we don't see any issues with using rethrow, I'd like to go with this.

[aviatesk]

Let's proceed with using rethrow for now. If necessary, we can revert the 2nd commit later.

[topolarity]

Let's proceed with using rethrow for now. If necessary, we can revert the 2nd commit later.

Just as a heads up, we've been exploring this kind of use case for loading in Base too: JuliaLang/julia#55816 (comment)

I need to find a minute to re-visit my stack trace changes so that it's possible to do what that comment talks about it, but the general idea is to be able to efficiently record a "marker" in the catch backtrace, so that you can slice out the relevant bits later

(without having to eagerly unwind and then pattern match on the symbolized stack trace)

[c42f]

Yeah. I think we'll resolve this by just not throwing at all here, once we move to propagating errors differently.

[c42f]

I've implemented a way to avoid capturing stack traces by using rethrow: fbdca4d.
This works fine for my use case. Personally, I find this simpler, as it means we don't have to overthink the state of MacroExpansionError. So if we don't see any issues with using rethrow, I'd like to go with this.

I think this is ok for now. As I noted above, I think we'll just delete the rethrow() entirely at some point by capturing the error and emitting it as a K"error" AST node. But that's a fairly big project since all of lowering needs to be aware of how to propagate such nodes non-destructively (rather than immediately throwing)