Fix macro hygiene when calling the macro in the same module.

[yuyichao]

There is a surprising number of base code that relies on the wrong behavior....

One broken test/code that need to be fixed on this commit is the MacroGlobalFunction test. The test doesn't work if the macro is used outside the module so I think the fix is correct. However, should the macro expander detect this case and avoid modifying the function name that is defined to be global already?

Closes #14893

c.c. @JeffBezanson about the test mentioned above and since the code was added a long time ago (605bc65) I can't really find a explanation why the check is necessary.

[yuyichao]

Also, the NULL check was added in 7de4648#diff-ee48e36e4d097c33660f778b57339696L97. From the code it seems that module should never be NULL and when I put an abort there it never triggers when running the whole test. Why is the NULL check there?

[JeffBezanson]

The NULL check might be left over from a case that used to happen during bootstrap.

[yuyichao]

OK, I comment out that code (replaced it with an assert) and updated the comment in macroexpand.scm. Let's see if the CI is happy about it...

[JeffBezanson]

should the macro expander detect this case and avoid modifying the function name that is defined to be global already?

No, it should apply normal hygiene rules, but this is going to require some further fixes. For example in 0.4:

julia> module Foo
               macro m()
                quote
                 global x
                 x = 2
                end
               end
            end
Foo

julia> macroexpand(:(@Foo.m))
quote  # none, line 4:
    global Foo.x # none, line 5:
    Foo.x = 2
end

This is correct, but gets a invalid syntax in "global" declaration error. On master there seems to be a regression, since we get

julia> macroexpand(:(@Foo.m))
quote  # none, line 4:
    global x # none, line 5:
    #2#x = 2
end

I can see why this regression was not caught, since the code doesn't work either way.

Your change to ast.c is fine, but changing the test should not have been necessary. We'd need the macro expansion to return a more permissive kind of module reference that lets you define a new function using the syntax M.f(x) = x. Currently that gives f not defined. As it is, this change will break code that does something like

macro m()
    quote
        global f
        f(x) = 2
    end
end
@m

which would include any module that uses macros to define its own functions. This might be too breaking.

[yuyichao]

What do you think is the best solution for this? Allowing assign to m.x (including method definition) if m is current module?

[JeffBezanson]

Maybe we should just go ahead and allow mod.x = y generally.

[yuyichao]

I'm also thinking maybe we can avoid replacing f with M.f if there's global f? This way we can make sure :(global a; a = 1) behaves the same as :(global a = 1)?

[JeffBezanson]

I think we will still need to return M.f to make sure the correct module is used.

[yuyichao]

the correct module is used.

Currently if a macro returns :(global a = 1) it creates a global variable a in the calling module and if :(global a; a = 1) it is an error. I find it a little weird that this two have different behaviors....

[yuyichao]

This PR should be ready now. Before someone restarted AppVeyor there was a timeout (that was not just hanging after the tests are done) possibly due to hitting the swap or something similar.

This still have different behavior for global a = 1 vs global a; a = 1 in a macro from a different module (the first one defines a in caller module and the second defines a in macro defining module).

[vtjnash]

I wrote a more complete diff for this file while attempting to do something else:
https://gist.github.com/vtjnash/3775a58db1efe7489e5922ad1eb117f3

[vtjnash]

the handling in codegen.cpp also needs to be updated for this case

[vtjnash]

it should be valid to lower this line to (= ,lhs ,rr). that should save us from needing to change the meaning of setfield. it looks like it should be handled correctly since #10403

[yuyichao]

What commit are you talking about? #10403 is an issue not a PR and all commits referenced in that issue are older than this one and I'm pretty sure $(GlobalRef(...)) = ... is not handled anywhere when I wrote this.

[yuyichao]

Rebased on top of master and Jameson's interpreter.c change. CI passed.

Also moved the handling of GlobalRef assignments fully to the runtime/JIT (needed to fix defined-julia-global) so setfield! on the module doesn't need any change anymore.

There's still a difference between global a; a = 1 and global a = 1 but that's not new and is a related but different issue so it doesn't have to be addressed in this PR.

[tkelman]

This could use a NEWS mention given how commonly macros were hitting this.