expr_to_syntaxtree: Quoted symbol fixes (#78)

Fix #54.  QuoteNode was previously converted to `(quote <contents>)` which lowers to a runtime call.  
- For plain symbol contents, this was too dynamic for ccall to handle, and optimizable in any case.  
- For Expr contents, I don't think using non-interpolating quote was correct in the first place.
- For LineNumberNode contents, we wrap with a QuoteNode on the way out, so we need to unwrap it on the way in.

I still need to think more about what the various forms of quoting mean when Expr and SyntaxTree are mixed, but this is at least an upgrade in number of passing tests.  I've added some cases from things I broke along the way.

Other fixes:
- Bool isa Integer, so prevent `expr_to_syntaxtree` from giving booleans `K"Integer"` kind
- enable logging of failures from `expr_to_syntaxtree` in the core lowering hook

---------

Co-authored-by: Claire Foster <[email protected]>

Author: mlechu

src/compat.jl

@@ -25,7 +25,7 @@ function expr_to_syntaxtree(@nospecialize(e), lnn::Union{LineNumberNode, Nothing
         kind=Kind, syntax_flags=UInt16,
         source=SourceAttrType, var_id=Int, value=Any,
         name_val=String, is_toplevel_thunk=Bool,
-        scope_layer=LayerId)
+        scope_layer=LayerId, meta=CompileHints)
     expr_to_syntaxtree(graph, e, lnn)
 end
 
@@ -188,17 +188,23 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
     if isnothing(e)
         st_id = _insert_tree_node(graph, K"core", src; name_val="nothing")
         return st_id, src
+    elseif e isa LineNumberNode
+        # A LineNumberNode in value position evaluates to nothing
+        st_id = _insert_tree_node(graph, K"core", src; name_val="nothing")
+        return st_id, e
     elseif e isa Symbol
         st_id = _insert_tree_node(graph, K"Identifier", src; name_val=String(e))
         return st_id, src
-    elseif e isa QuoteNode && e.value isa Symbol
-        # Undo special handling from st->expr
-        return _insert_convert_expr(Expr(:quote, e.value), graph, src)
-    # elseif e isa QuoteNode
-    #     st_id = _insert_tree_node(graph, K"inert", src)
-    #     quote_child, _ = _insert_convert_expr(e.value, graph, src)
-    #     setchildren!(graph, st_id, NodeId[quote_child])
-    #     return st_id, src
+    elseif e isa QuoteNode
+        if e.value isa Symbol
+            return _insert_convert_expr(Expr(:quoted_symbol, e.value), graph, src)
+        elseif e.value isa Expr
+            return _insert_convert_expr(Expr(:inert, e.value), graph, src)
+        elseif e.value isa LineNumberNode
+            return _insert_tree_node(graph, K"Value", src; value=e.value), src
+        else
+            return _insert_convert_expr(e.value, graph, src)
+        end
     elseif e isa String
         st_id = _insert_tree_node(graph, K"string", src)
         id_inner = _insert_tree_node(graph, K"String", src; value=e)
@@ -207,7 +213,9 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
     elseif !(e isa Expr)
         # There are other kinds we could potentially back-convert (e.g. Float),
         # but Value should work fine.
-        st_k = e isa Integer ? K"Integer" : find_kind(string(typeof(e)))
+        st_k = e isa Bool ? K"Bool" :
+            e isa Integer ? K"Integer" :
+            find_kind(string(typeof(e)))
         st_id = _insert_tree_node(graph, isnothing(st_k) ? K"Value" : st_k, src; value=e)
         return st_id, src
     end
@@ -354,6 +362,7 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
             lam_args = Any[]
             lam_eqs = Any[]
             for a in a1.args
+                a isa LineNumberNode && continue
                 a isa Expr && a.head === :(=) ? push!(lam_eqs, a) : push!(lam_args, a)
             end
             !isempty(lam_eqs) && push!(lam_args, Expr(:parameters, lam_eqs...))
@@ -399,6 +408,10 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
         callargs = collect_expr_parameters(e.args[1], 2)
         if e.args[1].head === :macrocall
             st_k = K"macrocall"
+            if callargs[2] isa LineNumberNode
+                src = callargs[2]
+            end
+            deleteat!(callargs, 2)
             c1,c1_esc = unwrap_esc(callargs[1])
             callargs[1] = c1_esc(Expr(:MacroName, c1))
         else
@@ -442,12 +455,6 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
         st_k = K"latestworld_if_toplevel"
     elseif e.head === Symbol("hygienic-scope")
         st_k = K"hygienic_scope"
-    elseif e.head === :escape
-        if length(e.args) == 1 && unwrap_esc_(e.args[1]) isa LineNumberNode
-            # escape containing only a LineNumberNode will become empty and
-            # thus must be removed before lowering sees it.
-            st_k = K"TOMBSTONE"
-        end
     elseif e.head === :meta
         # Messy and undocumented.  Only sometimes we want a K"meta".
         @assert e.args[1] isa Symbol
@@ -549,25 +556,27 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
     if isnothing(child_exprs)
         return st_id, src
     else
-        st_child_ids, last_src = _insert_child_exprs(child_exprs, graph, src)
+        st_child_ids, last_src = _insert_child_exprs(e.head, child_exprs, graph, src)
         setchildren!(graph, st_id, st_child_ids)
         return st_id, last_src
     end
 end
 
-function _insert_child_exprs(child_exprs::Vector{Any}, graph::SyntaxGraph,
-                             src::SourceAttrType)
+function _insert_child_exprs(head::Symbol, child_exprs::Vector{Any},
+                             graph::SyntaxGraph, src::SourceAttrType)
     st_child_ids = NodeId[]
     last_src = src
-    for c in child_exprs
-        if c isa LineNumberNode
-            last_src = c
+    for (i, c) in enumerate(child_exprs)
+        c_unwrapped, _ = unwrap_esc(c)
+        # If c::LineNumberNode is anywhere in a block OR c is not in tail
+        # position, we don't need to insert `nothing` here
+        if c_unwrapped isa LineNumberNode && (head === :block || head === :toplevel && i != length(child_exprs))
+            last_src = c_unwrapped
         else
-            (c_id, c_src) = _insert_convert_expr(c, graph, last_src)
+            (c_id, last_src) = _insert_convert_expr(c, graph, last_src)
             if !isnothing(c_id)
                 push!(st_child_ids, c_id)
             end
-            last_src = something(c_src, src)
         end
     end
     return st_child_ids, last_src

src/hooks.jl

@@ -15,8 +15,9 @@ function core_lowering_hook(@nospecialize(code), mod::Module,
     file = file isa Ptr{UInt8} ? unsafe_string(file) : file
     line = !(line isa Int64) ? Int64(line) : line
 
-    st0 = code isa Expr ? expr_to_syntaxtree(code, LineNumberNode(line, file)) : code
+    local st0 = nothing
     try
+        st0 = code isa Expr ? expr_to_syntaxtree(code, LineNumberNode(line, file)) : code
         ctx1, st1 = expand_forms_1(  mod,  st0, true, world)
         ctx2, st2 = expand_forms_2(  ctx1, st1)
         ctx3, st3 = resolve_scopes(  ctx2, st2)

src/syntax_graph.jl

@@ -577,7 +577,9 @@ function JuliaSyntax._expr_leaf_val(ex::SyntaxTree, _...)
     name = get(ex, :name_val, nothing)
     if !isnothing(name)
         n = Symbol(name)
-        if hasattr(ex, :scope_layer)
+        if kind(ex) === K"Symbol"
+            return QuoteNode(n)
+        elseif hasattr(ex, :scope_layer)
             Expr(:scope_layer, n, ex.scope_layer)
         else
             n

test/compat.jl

@@ -608,7 +608,24 @@ const JL = JuliaLowering
                 "x"::K"Identifier"
             ]
 
-        @test JuliaLowering.expr_to_syntaxtree(Expr(:block, esc(LineNumberNode(1)))) ≈ @ast_ [K"block"]
+        @test JuliaLowering.expr_to_syntaxtree(Expr(:block, LineNumberNode(1))) ≈
+            @ast_ [K"block"]
+        @test JuliaLowering.expr_to_syntaxtree(Expr(:block, esc(LineNumberNode(1)))) ≈
+            @ast_ [K"block"]
+        @test JuliaLowering.expr_to_syntaxtree(Expr(:block, QuoteNode(LineNumberNode(1)))) ≈
+            @ast_ [K"block" LineNumberNode(1)::K"Value"]
+
+        # toplevel (and all other non-block forms) keep LineNumberNodes in value position
+        @test JuliaLowering.expr_to_syntaxtree(Expr(:toplevel, esc(LineNumberNode(1)))) ≈
+            @ast_ [K"toplevel"  [K"escape" "nothing"::K"core"]]
+        @test JuliaLowering.expr_to_syntaxtree(Expr(:toplevel, LineNumberNode(1))) ≈
+            @ast_ [K"toplevel" "nothing"::K"core"]
+        @test JuliaLowering.expr_to_syntaxtree(Expr(:toplevel, QuoteNode(LineNumberNode(1)))) ≈
+            @ast_ [K"toplevel" LineNumberNode(1)::K"Value"]
+        @test JuliaLowering.expr_to_syntaxtree(Expr(:call, :identity, LineNumberNode(1))) ≈
+            @ast_ [K"call" "identity"::K"Identifier" "nothing"::K"core"]
+        @test JuliaLowering.expr_to_syntaxtree(Expr(:call, :identity, QuoteNode(LineNumberNode(1)))) ≈
+            @ast_ [K"call" "identity"::K"Identifier" LineNumberNode(1)::K"Value"]
 
     end
 end

test/hooks.jl

@@ -20,21 +20,38 @@ const JL = JuliaLowering
     end
 
     if isdefined(Core, :_lower)
+        function jeval(str)
+            prog = parseall(Expr, str)
+            local out
+            try
+                JL.activate!()
+                out = Core.eval(test_mod, prog)
+            finally
+                JL.activate!(false)
+            end
+        end
         @testset "integration: `JuliaLowering.activate!`" begin
-            prog = parseall(Expr, "global asdf = 1")
-            JL.activate!()
-            out = Core.eval(test_mod, prog)
-            JL.activate!(false)
+            out = jeval("global asdf = 1")
             @test out === 1
             @test isdefined(test_mod, :asdf)
 
-            prog = parseall(Expr, "module M; x = 1; end")
-            JL.activate!()
-            out = Core.eval(test_mod, prog)
-            JL.activate!(false)
+            out = jeval("module M; x = 1; end")
             @test out isa Module
             @test isdefined(test_mod, :M)
             @test isdefined(test_mod.M, :x)
+
+            # Tricky cases with symbols
+            out = jeval("""module M
+                Base.@constprop :aggressive function f(x); x; end
+                const what = ccall(:jl_value_ptr, Ptr{Cvoid}, (Any,), Core.nothing)
+            end""")
+            @test out isa Module
+            @test isdefined(test_mod, :M)
+            @test isdefined(test_mod.M, :f)
+            @test isdefined(test_mod.M, :what)
+
+            # TODO: broken, commented to prevent error logging
+            # @test jeval("Base.@propagate_inbounds @inline meta_double_quote_issue(x) = x") isa Function
         end
     end
 end

test/macros.jl

@@ -346,6 +346,13 @@ end
     end
     isglobal_chk(1)
     """) === (false, false, false, false)
+
+    # @test appears to be the only macro in base to use :inert
+    test_result = JuliaLowering.include_string(test_mod, """
+    using Test
+    @test identity(123) === 123
+    """; expr_compat_mode=true)
+    @test test_result.value === true
 end
 
 end