Avoid macro-expand recursion into Expr(:toplevel, ...)#53515
Avoid macro-expand recursion into Expr(:toplevel, ...)#53515Keno merged 7 commits intoJuliaLang:masterfrom
Expr(:toplevel, ...)#53515Conversation
The following is currently an error:
```
julia> module MyMacroModule
macro mymacro end
end
Main.MyMacroModule
julia> macro MyMacroModule.mymacro()
1
end
ERROR: syntax: invalid macro definition around REPL[2]:1
Stacktrace:
[1] top-level scope
@ REPL[2]:1
```
Discussing with Jeff, we didn't think there was any good reason
not to allow this, just a missing case in lowering. It's probably
not particularly useful (unlike the corresponding case for functions
that is used all the time), but it came up in writing a test case for #53515.
The following is currently an error:
```
julia> module MyMacroModule
macro mymacro end
end
Main.MyMacroModule
julia> macro MyMacroModule.mymacro()
1
end
ERROR: syntax: invalid macro definition around REPL[2]:1
Stacktrace:
[1] top-level scope
@ REPL[2]:1
```
Discussing with Jeff, we didn't think there was any good reason
not to allow this, just a missing case in lowering. It's probably
not particularly useful (unlike the corresponding case for functions
that is used all the time), but it came up in writing a test case for #53515.
|
Alright, so @JeffBezanson and I had about a two hour long discussion on how to fix the doc macro case here. To recap, the problem is that and the outer One particular example of a macro that uses this facility is Now, after this change, macroexpand no longer recurses into the toplevel Some options considered and discarded:
In particular, this would allow us to avoid @JeffBezanson didn't like this option because he thought that the scope of application should be lexical, not dynamic, so in particular: Should not be able to document
What we ultimately settled on is a variant of the last one that works as follows: If the |
The following is currently an error:
```
julia> module MyMacroModule
macro mymacro end
end
Main.MyMacroModule
julia> macro MyMacroModule.mymacro()
1
end
ERROR: syntax: invalid macro definition around REPL[2]:1
Stacktrace:
[1] top-level scope
@ REPL[2]:1
```
Discussing with Jeff, we didn't think there was any good reason not to
allow this, just a missing case in lowering. It's probably not
particularly useful (unlike the corresponding case for functions that is
used all the time), but it came up in writing a test case for #53515.
Keno
force-pushed
the
ct/macroexpand-no-toplevel-recursion
branch
from
e1662aa to
6647682
Compare
Keno
changed the title
Expr(:toplevel, ...)Expr(:toplevel, ...)
Keno
added
the
needs pkgeval
|
I have implemented the above proposed solution. I believe this should be ready to go, but we should pkgeval it, to see if there's any other impact. |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
Lots of failures, but most are PrecompileTools and SciMLBase. PrecompileTools has this, which got broken on this PR: SciMLBase has: which I haven't investigated yet, but probably just needs a small tweak to the doc macro support. |
…aLang#53535) The following is currently an error: ``` julia> module MyMacroModule macro mymacro end end Main.MyMacroModule julia> macro MyMacroModule.mymacro() 1 end ERROR: syntax: invalid macro definition around REPL[2]:1 Stacktrace: [1] top-level scope @ REPL[2]:1 ``` Discussing with Jeff, we didn't think there was any good reason not to allow this, just a missing case in lowering. It's probably not particularly useful (unlike the corresponding case for functions that is used all the time), but it came up in writing a test case for JuliaLang#53515.
|
Looks like the EnumX is an implementation issue with the way that the and ends like this: but that ignores and skips over the |
Keno
force-pushed
the
ct/macroexpand-no-toplevel-recursion
branch
from
1f80f17 to
e3c79f3
Compare
|
Alright, so to follow up, the offending pattern was removed from PrecompileTools in (JuliaLang/PrecompileTools.jl#36 - registration pending at JuliaRegistries/General#102567) and there's a larger discussion around the confusing macroexpand semantics in #53667. For the EnumX issue, I've updated this PR to not strip the hygenic-scope blocks, which fixes that case. Once the PrecompileTools registration is merged, I intend to re-run PkgEval. |
|
@nanosoldier |
|
I think the expr copy to the matching meta doc might need to add some escape nodes matching the number of new scopes it entered, if any, and if the doc expression isn't trivial (eg a literal string as it usually is)? The change to docm though looks like it was simple enough, which is great to see |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
LLVM-dependent failures are a different macro hygiene bug: #53673. I have a fix, which I'll push here also, so we can re-run pkgeval against that. |
|
@nanosoldier |
I've tried a bunch of examples, but I haven't yet been able to come up with a case that's actually failing. Do you have one in mind? |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
Those syntax errors seem related to the change here: julia> using Test; Test.@testset for x in y; end
(type-error car cons ())
unexpected error: #0 (caddr (escape (= x y)))
#1 (resolve-letlike-assign
(escape #0=(= x y)) #1=(#0#
(false) (arr . |#1#arr|) (default_rng_orig . |#2#default_rng_orig|)
(tls_seed_orig . |#3#tls_seed_orig|) (finish_errored . |#4#finish_errored|)
(ts . |#5#ts|) (first_iteration . |#6#first_iteration|))
(#0# . #1#) #<julia: Test> ((line 1 |REPL[2]|)) () #f) |
Keno
force-pushed
the
ct/macroexpand-no-toplevel-recursion
branch
from
6ae6fd5 to
494420c
Compare
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
CSyntax issue reduces to: |
|
@nanosoldier |
|
The package evaluation job you requested has completed - no new issues were detected. |
This will be necessary for us to push down the `(hygienic-scope ...)` correctly later.
Keno
force-pushed
the
ct/macroexpand-no-toplevel-recursion
branch
from
1d73b36 to
465955f
Compare
|
Alright, this is good to go. We've fixed all the pkgeval issues. @vtjnash still has some concerns about |
Keno
removed
the
needs pkgeval
The following is currently an error:
```
julia> module MyMacroModule
macro mymacro end
end
Main.MyMacroModule
julia> macro MyMacroModule.mymacro()
1
end
ERROR: syntax: invalid macro definition around REPL[2]:1
Stacktrace:
[1] top-level scope
@ REPL[2]:1
```
Discussing with Jeff, we didn't think there was any good reason not to
allow this, just a missing case in lowering. It's probably not
particularly useful (unlike the corresponding case for functions that is
used all the time), but it came up in writing a test case for #53515.
Here's an example output from macroexpand:
```
Expr
head: Symbol thunk
args: Array{Any}((1,))
1: Core.CodeInfo
code: Array{Any}((2,))
1: Expr
head: Symbol toplevel
args: Array{Any}((17,))
1: Expr
head: Symbol hygienic-scope
args: Array{Any}((3,))
1: LineNumberNode
2: Module Base.Enums
3: LineNumberNode
2: Expr
head: Symbol hygienic-scope
args: Array{Any}((3,))
1: Expr
2: Module Base.Enums
3: LineNumberNode
3: Expr
head: Symbol hygienic-scope
args: Array{Any}((3,))
1: LineNumberNode
2: Module Base.Enums
3: LineNumberNode
4: Expr
head: Symbol hygienic-scope
args: Array{Any}((3,))
1: Expr
2: Module Base.Enums
3: LineNumberNode
...
```
Currently fails during bootstrap with:
```
LoadError("sysimg.jl", 3, LoadError("Base.jl", 542, ErrorException("cannot document the following expression:\n\n#= mpfr.jl:65 =# @enum MPFRRoundingMode begin\n #= mpfr.jl:66 =#\n MPFRRoundNearest\n #= mpfr.jl:67 =#\n MPFRRoundToZero\n #= mpfr.jl:68 =#\n MPFRRoundUp\n #= mpfr.jl:69 =#\n MPFRRoundDown\n #= mpfr.jl:70 =#\n MPFRRoundFromZero\n #= mpfr.jl:71 =#\n MPFRRoundFaithful\n end\n\n'@enum' not documentable. See 'Base.@__doc__' docs for details.\n")))
```
Perhaps we can do better than wrapping each `Expr(:toplevel, ...)` arg
individually, or I should be filtering out the LineNumberNodes?
---------
Co-authored-by: Keno Fischer <keno@juliacomputing.com>
Co-authored-by: Keno Fischer <keno@juliahub.com>
…aLang#53535) The following is currently an error: ``` julia> module MyMacroModule macro mymacro end end Main.MyMacroModule julia> macro MyMacroModule.mymacro() 1 end ERROR: syntax: invalid macro definition around REPL[2]:1 Stacktrace: [1] top-level scope @ REPL[2]:1 ``` Discussing with Jeff, we didn't think there was any good reason not to allow this, just a missing case in lowering. It's probably not particularly useful (unlike the corresponding case for functions that is used all the time), but it came up in writing a test case for JuliaLang#53515.
4 participants
Here's an example output from macroexpand:
Currently fails during bootstrap with:
Perhaps we can do better than wrapping each
Expr(:toplevel, ...)arg individually, or I should be filtering out the LineNumberNodes?