fix some macro expansion scope bugs

[vtjnash]

This fixes macro hygiene for cases where a macro returns a call to another macro. It's actually a bit like doing the inverse of #10940.

[tkelman]

this looks strange, what is this doing?

[vtjnash]

running eval in a new workspace

[tkelman]

is that any different than

workspace()
@eval Main using Test22101

?

[vtjnash]

Core.Main, and no difference, those would name the same module

[tkelman]

Is this the only reason you made workspace() return the module or is there some other motivating reason for that?

[vtjnash]

If the test passes, they were the same. If not, it may or may not be a bug.

[tkelman]

How could it not pass if it is returning Core.Main ? This change seems unnecessary and unrelated.

[vtjnash]

As I said, if it passes, they might be the equivalent. If not, it might be a bug.

[tkelman]

You've made it return Core.Main - if functions suddenly returned something other than what they say they do, we'd have bigger problems?

[vtjnash]

Perhaps. But note that these expressions are run in different scopes.

[JeffBezanson]

Is this change actually necessary? I'm worried that resolution of identifiers in macro arguments could depend on the number of layers of macros involved.

[vtjnash]

Yes. Previously we did _apply(macrocall, map(resolvescope, args...)...), but only if the macro was called recursively. Now this does resolvescope(_apply(macrocall, args...)).

This also means that now I'm essentially just deferring scope resolution until the aptly named resolve-scopes function, instead of having a second implementation of it in this file.

The downside of this is that it means macro authors may need to fix their usages of recursive macros to reflect the correct scope rules here.

[JeffBezanson]

The behavior I think we want is for DocsTest to resolve to the call environment no matter what the macros do (unless they very specifically mangle the identifier to something else).

[JeffBezanson]

Edit: this turns out to be needed because @repl explicitly looks up everything in Main; that confused me.

[JeffBezanson]

I generally think #10940 is a good idea, and we should do it. Could we just revive that instead? Since I like #10940, I'm worried that I might not like the inverse of it :)

[vtjnash]

It's the inverse, but not actually the reverse (or something like that. yeah, it's confusing). IIUC, with this fix, implementing that PR simply means that we need to automatically wrap all arguments to macros in Expr(:esc) (but rename it Expr(:hygienic) and only apply it to symbols).

[JeffBezanson]

If I remember, one thing I liked about #10940 is that it basically leaves macro arguments as they are now, and instead wraps symbols inside quoted code in macros with Expr(:hygienic). That strikes me as more user-friendly.

[vtjnash]

is that it basically leaves macro arguments as they are now

If my recollection of my rebase attempt serves though, it actually couldn't leave them alone. It sometimes mangled them into GlobalRef, unless it guesses that resolve-scopes would have eventually made it a local symbol under the assumption that any further macro expansion will use the symbol as a local variable in the current scope and won't introduce any new scopes (e.g. basically the same assumptions that the code makes now).

Calling this "the inverse" was perhaps too tongue-in-cheek, since I meant it was approaching the fix to a particular bug from the other direction (a bug that the implementation in #10940 sadly doesn't actually end up fixing completely, I think it just reduced it in severity).

But anyways, wrapping quoting / interpolation instead of wrapping the arguments is another possible avenue to implementing #10940 that becomes much easier and more correct with this PR. In this PR, I called that operation Expr(:hygienic-scope). The requisite lowering changes would be to make @hygienic quote $expr end (or equivalently quote $expr end when written inside a macro body) to produce:
Expr(:hygienic-scope, Expr(:quote, Expr(:begin, Expr(:esc, expr))))
(the macro itself then would not imply any hygienic-scope operation – the return value would just be used directly).
I agree, that does sound like the more user-friendly approach.

[StefanKarpinski]

This change is breaking everything and needs to be reverted or fixed ASAP.