Add scope layer for macro arguments of normally-quoted AST fragments (JuliaLang/JuliaLowering.jl#109)

Macros may pull apart an expression (eg, a module expression or the
right hand side of a `.` expression) or quote that expression, and we
should keep track of the scope where this originated. A particular
example is the `@eval` macro. Consider

```
let name = :x
    @eval A.$name
end
```

In this case the right hand side of `.` would normally be quoted (as a
plain symbol) but in the case of `@eval` an extra `quote` is added
around the expression to make the `name` variable valid unquoted code
after quote expansion.

In general, macros may pull apart or rearrange what's passed to them, so
we can't make the assumption that normally-inert syntax passed to them
should go without a scope layer.

To fix this, this change adds a scope layer to all ASTs passed to
macros. After macro expansion is done, we can then remove the layer from
any AST we know is definitely inert to prevent it from interfering with
future lowering passes over that quoted code.

This helps but isn't a full solution - see JuliaLang/JuliaLowering.jl#111 for further work.

Author: c42f

JuliaLowering/src/eval.jl

@@ -461,6 +461,11 @@ end
     _eval(mod, iter)
 end
 
+# Version of eval() taking `Expr` (or Expr tree leaves of any type)
+function eval(mod::Module, ex; opts...)
+    eval(mod, expr_to_syntaxtree(ex); opts...)
+end
+
 if VERSION >= v"1.13.0-DEV.1199" # https://github.com/JuliaLang/julia/pull/59604
 
 function _eval(mod, iter)

JuliaLowering/src/macro_expansion.jl

@@ -189,13 +189,9 @@ end
 function set_macro_arg_hygiene(ctx, ex, layer_ids, layer_idx)
     k = kind(ex)
     scope_layer = get(ex, :scope_layer, layer_ids[layer_idx])
-    if k == K"module" || k == K"toplevel" || k == K"inert"
-        makenode(ctx, ex, ex, children(ex);
-                 scope_layer=scope_layer)
-    elseif k == K"."
-        makenode(ctx, ex, ex, set_macro_arg_hygiene(ctx, ex[1], layer_ids, layer_idx), ex[2],
-                 scope_layer=scope_layer)
-    elseif !is_leaf(ex)
+    if is_leaf(ex)
+        makeleaf(ctx, ex, ex; scope_layer=scope_layer)
+    else
         inner_layer_idx = layer_idx
         if k == K"escape"
             inner_layer_idx = layer_idx - 1
@@ -210,8 +206,6 @@ function set_macro_arg_hygiene(ctx, ex, layer_ids, layer_idx)
         end
         mapchildren(e->set_macro_arg_hygiene(ctx, e, layer_ids, inner_layer_idx),
                     ctx, ex; scope_layer=scope_layer)
-    else
-        makeleaf(ctx, ex, ex; scope_layer=scope_layer)
     end
 end
 
@@ -359,6 +353,20 @@ function append_sourceref(ctx, ex, secondary_prov)
     end
 end
 
+function remove_scope_layer!(ex)
+    if !is_leaf(ex)
+        for c in children(ex)
+            remove_scope_layer!(c)
+        end
+    end
+    deleteattr!(ex, :scope_layer)
+    ex
+end
+
+function remove_scope_layer(ctx, ex)
+    remove_scope_layer!(copy_ast(ctx, ex))
+end
+
 """
 Lowering pass 1
 
@@ -441,7 +449,12 @@ function expand_forms_1(ctx::MacroExpansionContext, ex::SyntaxTree)
     elseif k == K"macrocall"
         expand_macro(ctx, ex)
     elseif k == K"module" || k == K"toplevel" || k == K"inert"
-        ex
+        # Remove scope layer information from any inert syntax which survives
+        # macro expansion so that it doesn't contaminate lowering passes which
+        # are later run against the quoted code. TODO: This works as a first
+        # approximation but is incorrect in general. We need to revisit such
+        # "deferred hygiene" situations (see https://github.com/c42f/JuliaLowering.jl/issues/111)
+        remove_scope_layer(ctx, ex)
     elseif k == K"." && numchildren(ex) == 2
         # Handle quoted property access like `x.:(foo)` or `Core.:(!==)`
         # Unwrap the quote to get the identifier before expansion

JuliaLowering/src/syntax_graph.jl

@@ -155,6 +155,10 @@ function setattr!(graph::SyntaxGraph, id; attrs...)
     end
 end
 
+function deleteattr!(graph::SyntaxGraph, id::NodeId, name::Symbol)
+    delete!(getattr(graph, name), id)
+end
+
 function Base.getproperty(graph::SyntaxGraph, name::Symbol)
     # TODO: Remove access to internals?
     name === :edge_ranges && return getfield(graph, :edge_ranges)
@@ -294,6 +298,10 @@ function setattr!(ex::SyntaxTree; attrs...)
     setattr!(ex._graph, ex._id; attrs...)
 end
 
+function deleteattr!(ex::SyntaxTree, name::Symbol)
+    deleteattr!(ex._graph, ex._id, name)
+end
+
 # JuliaSyntax tree API
 
 function JuliaSyntax.is_leaf(ex::SyntaxTree)

JuliaLowering/test/macros.jl

@@ -1,6 +1,8 @@
 @testset "macro tests" begin
 
 test_mod = Module(:macro_test)
+Base.eval(test_mod, :(const var"@ast" = $(JuliaLowering.var"@ast")))
+Base.eval(test_mod, :(const var"@K_str" = $(JuliaLowering.var"@K_str")))
 
 JuliaLowering.include_string(test_mod, raw"""
 module M
@@ -406,4 +408,74 @@ end
 
 end
 
+@testset "scope layers for normally-inert ASTs" begin
+    # Right hand side of `.`
+    @test JuliaLowering.include_string(test_mod, raw"""
+    let x = :(hi)
+        :(A.$x)
+    end
+    """) ≈ @ast_ [K"."
+        "A"::K"Identifier"
+        "hi"::K"Identifier"
+    ]
+    # module
+    @test JuliaLowering.include_string(test_mod, raw"""
+    let x = :(AA)
+        :(module $x
+        end
+        )
+    end
+    """) ≈ @ast_ [K"module"
+        "AA"::K"Identifier"
+        [K"block"
+        ]
+    ]
+
+    # In macro expansion, require that expressions passed in as macro
+    # *arguments* get the lexical scope of the calling context, even for the
+    # `x` in `M.$x` where the right hand side of `.` is normally quoted.
+    @test JuliaLowering.include_string(test_mod, raw"""
+        let x = :(someglobal)
+            @eval M.$x
+        end
+    """) == "global in module M"
+
+    JuliaLowering.include_string(test_mod, raw"""
+        let y = 101
+            @eval module AA
+                x = $y
+            end
+        end
+    """)
+    @test test_mod.AA.x == 101
+
+    # "Deferred hygiene" in macros which emit quoted code currently doesn't
+    # work as might be expected.
+    #
+    # The old macro system also doesn't handle this - here's the equivalent
+    # implementation
+    # macro make_quoted_code(init, y)
+    #     QuoteNode(:(let
+    #         x = "inner x"
+    #         $(esc(init))
+    #         ($(esc(y)), x)
+    #     end))
+    # end
+    #
+    # TODO: The following should throw an error rather than producing a
+    # surprising value, or work "as expected" whatever that is!
+    JuliaLowering.include_string(test_mod, raw"""
+    macro make_quoted_code(init, y)
+        q = :(let
+            x = "inner x"
+            $init
+            ($y, x)
+        end)
+        @ast q q [K"inert" q]
+    end
+    """)
+    code = JuliaLowering.include_string(test_mod, """@make_quoted_code(x="outer x", x)""")
+    @test_broken JuliaLowering.eval(test_mod, code) == ("outer x", "inner x")
+end
+
 end

JuliaLowering/test/quoting_ir.jl

@@ -31,6 +31,19 @@ end
 3   (call JuliaLowering.interpolate_ast SyntaxTree (inert (block (quote (block (call-i ($ ($ x)) + 1))))) %₂)
 4   (return %₃)
 
+########################################
+# Symbols on `.` right hand side need to be scoped correctly
+let x = 1
+    :(A.$x)
+end
+#---------------------
+1   1
+2   (= slot₁/x %₁)
+3   slot₁/x
+4   (call core.tuple %₃)
+5   (call JuliaLowering.interpolate_ast SyntaxTree (inert (. A ($ x))) %₄)
+6   (return %₅)
+
 ########################################
 # Error: Double escape
 quote