fix macroexpansion scope miscalculation #23247
Conversation
|
I'm hoping this also changes the behavior of the test at Line 784 in 131807c |
0b02f6f
to
9a336b1
Compare
test/parse.jl
Outdated
| @test_broken typeof(local_foo16096).name.module === M16096 | ||
| @test_broken typeof(local_foo16096).name.mt.module === M16096 | ||
| @test_broken getfield(M16096, typeof(local_foo16096).name.mt.name) === local_foo16096 | ||
| @test_broken !isdefined(@__MODULE__, typeof(local_foo16096).name.mt.name) |
There was a problem hiding this comment.
I actually don't think the current behavior is broken. I interpret the purpose of this macro as defining a method in the caller's module. If you consider e.g. the vectorize macros, if my module defines foo and I write @vectorize_1arg foo, I want it to add a new method in my module, not in Base.
There was a problem hiding this comment.
Hygiene means that you either need esc (so hygiene rules don't apply), or it'll gensym the name (so you'll define #1#foo#). I suppose either case is actually OK to me. I was thinking it might make more sense to not be creating a global with a gensym name – but the goal of this PR was only to restore the v0.6 behavior for these cases, not propose changes.
There was a problem hiding this comment.
No matter what the name is, the type introduced by this macro should be in the caller's module, not M16096. I'm not proposing a behavior change, just that this test should be inverted and test_broken should be changed to test.
There was a problem hiding this comment.
as defining a method in the caller's module
Also, fwiw, it's best to be precise in terminology, since this test has nothing to do with Method, but instead is testing for the properties of the function's type. Macroexpand (and expand in general) would require severe contortions (spliced in calls to eval) to affect that property (the module of a method). This instead is testing specifically for whether the macro added a new global bindings (and where).
There was a problem hiding this comment.
Types always have global bindings. Hygiene can only affect whether the function object has a global name, which is not the issue here.
src/macroexpand.scm
Outdated
| @@ -407,10 +407,30 @@ | |||
| (and (eq? (car e) '=) (length= e 3) | |||
| (eventually-call? (cadr e)))))) | |||
|
|
|||
| ;; count hygienic / escape pairs | |||
| ;; and fold together a list resulting from applying the function to | |||
| ;; and block at the same hygienic scope | |||
There was a problem hiding this comment.
is this supposed to say "applying the function to a block at the same hygienic scope" ?
|
For completeness, can we add a test specifically for the |
|
@stevengj Yes, that case was reduced to create the tests added to core.jl (including the |
|
Any idea whether this also fixes BenchmarkTools? |
|
Doesn't appear to. Was it intended to? |
previously, we were treating hygienic-scope as also introducing a new scope-block that was wrong (and inconsistent with previous behavior) also, we were failing to set the outermost flag when entering a hygienic-scope block, further compounding the above error fix #23239
9a336b1
to
bfe7b73
Compare
|
...I'll take that as a no then? |
|
I think @vtjnash mentioned elsewhere that this would be a partial fix, but not all the way. |
|
Okay thanks, I didn't see that. |
previously, we were treating hygienic-scope as also introducing a new scope-block
that was wrong (and inconsistent with previous behavior)
also, we were failing to set the outermost flag when entering a
hygienic-scope block, further compounding the above error
fix #23239