Suggestions from code review

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

Author: mlechu

src/compat.jl

@@ -1,10 +1,10 @@
 const JS = JuliaSyntax
 
-function _insert_tree_node(graph::SyntaxGraph, k::JS.Kind, src::SourceAttrType,
+function _insert_tree_node(graph::SyntaxGraph, k::Kind, src::SourceAttrType,
                            flags::UInt16=0x0000; attrs...)
     id = newnode!(graph)
     sethead!(graph, id, k)
-    setflags!(graph, id, flags)
+    flags !== 0 && setflags!(graph, id, flags)
     setattr!(graph, id; source=src, attrs...)
     return id
 end
@@ -19,17 +19,17 @@ Expr-producing macros.  Always prefer re-parsing source text over using this.
 
 Supports parsed and/or macro-expanded exprs, but not lowered exprs
 """
-function expr_to_syntaxtree(@nospecialize(e),
-                            lnn::Union{LineNumberNode, Nothing}=nothing,
-                            ctx=nothing)
-    graph = if isnothing(ctx)
-        ensure_attributes!(SyntaxGraph(),
-                           kind=Kind, syntax_flags=UInt16,
-                           source=SourceAttrType, var_id=Int, value=Any,
-                           name_val=String, is_toplevel_thunk=Bool)
-    else
-        syntax_graph(ctx)
-    end
+function expr_to_syntaxtree(@nospecialize(e), lnn::Union{LineNumberNode, Nothing}=nothing)
+    graph = ensure_attributes!(
+        SyntaxGraph(),
+        kind=Kind, syntax_flags=UInt16,
+        source=SourceAttrType, var_id=Int, value=Any,
+        name_val=String, is_toplevel_thunk=Bool)
+    expr_to_syntaxtree(graph, e, lnn)
+end
+
+function expr_to_syntaxtree(ctx, @nospecialize(e), lnn::Union{LineNumberNode, Nothing})
+    graph = syntax_graph(ctx)
     toplevel_src = if isnothing(lnn)
         # Provenance sinkhole for all nodes until we hit a linenode
         dummy_src = SourceRef(
@@ -39,7 +39,7 @@ function expr_to_syntaxtree(@nospecialize(e),
     else
         lnn
     end
-    st_id, _ = _insert_convert_expr(e, graph, toplevel_src)
+    st_id = _insert_convert_expr(e, graph, toplevel_src)
     out = SyntaxTree(graph, st_id)
     return out
 end
@@ -109,15 +109,23 @@ function expr_parameters(p::Expr, pos::Int)
     return nothing
 end
 
-"Unwrap (usually block) if it has only one non-linenode child"
-function maybe_strip_block(b::Expr)
+"""
+If `b` (usually a block) has exactly one non-LineNumberNode argument, unwrap it.
+"""
+function maybe_unwrap_arg(b::Expr)
     e1 = findfirst(c -> !isa(c, LineNumberNode), b.args)
     isnothing(e1) && return b
     e2 = findfirst(c -> !isa(c, LineNumberNode), b.args[e1+1:end])
     !isnothing(e2) && return b
     return b.args[e1]
 end
 
+function maybe_extract_lnn(b, default)
+    !(b isa Expr) && return b
+    lnn_i = findfirst(a->isa(a, LineNumberNode), b.args)
+    return isnothing(lnn_i) ? default : b.args[lnn_i]
+end
+
 # Get kind by string if exists.  TODO relies on internals
 function find_kind(s::String)
     out = get(JS._kind_str_to_int, s, nothing)
@@ -130,19 +138,9 @@ function is_dotted_operator(s::AbstractString)
         JS.is_operator(something(find_kind(s[2:end]), K"None"))
 end
 
-# Expr doesn't record whether or not var"x" was used on x, so just assume one
-# was used for any invalid identifier, but lose the information otherwise.
-function _insert_var_str(child::NodeId, graph::SyntaxGraph, src::SourceAttrType)
-    var_id = _insert_tree_node(graph, K"var", src)
-    setchildren!(graph, var_id, [child])
-    setflags!(graph, child, JS.RAW_STRING_FLAG)
-    setflags!(graph, var_id, JS.NON_TERMINAL_FLAG)
-    return (var_id, src)
-end
-
-function is_call_expr(e)
+function is_eventually_call(e)
     return e isa Expr && (e.head === :call ||
-        e.head in (:where, :(::)) && is_call_expr(e.args[1]))
+        e.head in (:where, :(::)) && is_eventually_call(e.args[1]))
 end
 
 """
@@ -157,15 +155,10 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
     # Non-expr types
     if isnothing(e)
         st_id = _insert_tree_node(graph, K"core", src; name_val="nothing")
-        return st_id, src
+        return st_id
     elseif e isa Symbol
         st_id = _insert_tree_node(graph, K"Identifier", src; name_val=String(e))
-        if !Base.isoperator(e) && !Base.is_valid_identifier(e)
-            return _insert_var_str(st_id, graph, src)
-        end
-        return st_id, src
-    elseif e isa LineNumberNode
-        return nothing, e
+        return st_id
     elseif e isa QuoteNode && e.value isa Symbol
         # Undo special handling from st->expr
         return _insert_convert_expr(Expr(:quote, e.value), graph, src)
@@ -175,23 +168,24 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
     #     setchildren!(graph, st_id, NodeId[quote_child])
     #     return st_id, src
     elseif e isa String
-        st_id = _insert_tree_node(graph, K"string", src, JS.NON_TERMINAL_FLAG)
+        st_id = _insert_tree_node(graph, K"string", src)
         id_inner = _insert_tree_node(graph, K"String", src; value=e)
         setchildren!(graph, st_id, [id_inner])
-        return st_id, src
+        return st_id
     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_id = _insert_tree_node(graph, isnothing(st_k) ? K"Value" : st_k, src; value=e)
-        return st_id, src
+        return st_id
     end
 
     #---------------------------------------------------------------------------
     # `e` is an expr.  In many cases, it suffices to
     # - guess that the kind name is the same as the expr head
     # - add no syntax flags or attrs
     # - map e.args to syntax tree children one-to-one
+    e::Expr
     nargs = length(e.args)
     maybe_kind = find_kind(string(e.head))
     st_k = isnothing(maybe_kind) ? K"None" : maybe_kind
@@ -214,7 +208,7 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
             st_k = K"op="
             op = s[1:end-1]
         end
-        child_exprs = [e.args[1], Symbol(op), e.args[2]]
+        child_exprs = Any[e.args[1], Symbol(op), e.args[2]]
     elseif e.head === :comparison
         for i = 2:2:length(child_exprs)
             op = child_exprs[i]
@@ -228,7 +222,9 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
         @assert nargs >= 2
         a1 = e.args[1]
         child_exprs = collect_expr_parameters(e, 3)
-        # macrocall has a linenode "argument" here. should we set src?
+        if child_exprs[2] isa LineNumberNode
+            src = child_exprs[2]
+        end
         deleteat!(child_exprs, 2)
         if a1 isa Symbol
             child_exprs[1] = Expr(:MacroName, a1)
@@ -244,13 +240,15 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
             elseif a1.name === Symbol("@big_str")
             end
         elseif a1 isa Function
+            # pass
         else
             error("Unknown macrocall form $(sprint(dump, e))")
+            @assert false
         end
     elseif e.head === Symbol("'")
         @assert nargs === 1
         st_k = K"call"
-        child_exprs = [e.head, e.args[1]]
+        child_exprs = Any[e.head, e.args[1]]
     elseif e.head === :. && nargs === 2
         a2 = e.args[2]
         if a2 isa Expr && a2.head === :tuple
@@ -265,11 +263,11 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
         end
     elseif e.head === :for
         @assert nargs === 2
-        child_exprs = Expr[_to_iterspec(Any[e.args[1]]), e.args[2]]
+        child_exprs = Any[_to_iterspec(Any[e.args[1]]), e.args[2]]
     elseif e.head === :where
         @assert nargs >= 2
         if !(e.args[2] isa Expr && e.args[2].head === :braces)
-            child_exprs = [e.args[1], Expr(:braces, e.args[2:end]...)]
+            child_exprs = Any[e.args[1], Expr(:braces, e.args[2:end]...)]
         end
     elseif e.head in (:tuple, :vect, :braces)
         child_exprs = collect_expr_parameters(e, 1)
@@ -311,8 +309,8 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
         push!(child_exprs, _to_iterspec(next.args[2:end]))
         pushfirst!(child_exprs, next.args[1])
     elseif e.head === :ncat || e.head === :nrow
-        st_flags |= JS.set_numeric_flags(e.args[1])
-        child_exprs = child_exprs[2:end]
+        dim = popfirst!(child_exprs)
+        st_flags |= JS.set_numeric_flags(dim)
     elseif e.head === :typed_ncat
         st_flags |= JS.set_numeric_flags(e.args[2])
         deleteat!(child_exprs, 2)
@@ -330,7 +328,8 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
         elseif !(e.args[1] isa Expr && (e.args[1].head in (:tuple, :where)))
             child_exprs[1] = Expr(:tuple, e.args[1])
         end
-        child_exprs[2] = maybe_strip_block(e.args[2])
+        src = maybe_extract_lnn(e.args[2], src)
+        child_exprs[2] = maybe_unwrap_arg(e.args[2])
     elseif e.head === :call
         child_exprs = collect_expr_parameters(e, 2)
         a1 = child_exprs[1]
@@ -342,32 +341,37 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
                 child_exprs[1] = Symbol(a1s[2:end])
             end
         end
+    elseif e.head === :function
+        if nargs >= 2
+            src = maybe_extract_lnn(e.args[2], src)
+        end
     elseif e.head === :(=)
-        if is_call_expr(e.args[1])
+        if is_eventually_call(e.args[1])
             st_k = K"function"
             st_flags |= JS.SHORT_FORM_FUNCTION_FLAG
-            child_exprs[2] = maybe_strip_block(child_exprs[2])
+            src = maybe_extract_lnn(e.args[2], src)
+            child_exprs[2] = maybe_unwrap_arg(e.args[2])
         end
     elseif e.head === :module
         @assert nargs === 3
         if !e.args[1]
             st_flags |= JS.BARE_MODULE_FLAG
         end
-        child_exprs = [e.args[2], e.args[3]]
+        child_exprs = Any[e.args[2], e.args[3]]
     elseif e.head === :do
         # Expr:
         # (do (call f args...) (-> (tuple lam_args...) (block ...)))
         # SyntaxTree:
         # (call f args... (do (tuple lam_args...) (block ...)))
         callargs = collect_expr_parameters(e.args[1], 2)
         fname = string(callargs[1])
-        if fname[1] === '@'
+        if e.args[1].head === :macrocall
             st_k = K"macrocall"
             callargs[1] = Expr(:MacroName, callargs[1])
         else
             st_k = K"call"
         end
-        child_exprs = [callargs..., Expr(:do_lambda, e.args[2].args...)]
+        child_exprs = Any[callargs..., Expr(:do_lambda, e.args[2].args...)]
     elseif e.head === :let
         if nargs >= 1 && !(e.args[1] isa Expr && e.args[1].head === :block)
             child_exprs[1] = Expr(:block, e.args[1])
@@ -384,7 +388,7 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
         st_k = K"="
     elseif e.head in (:local, :global) && nargs > 1
         # Possible normalization
-        # child_exprs = [Expr(:tuple, child_exprs...)]
+        # child_exprs = Any[Expr(:tuple, child_exprs...)]
     elseif e.head === :error
         # Zero-child errors from parsing are leaf nodes.  We could change this
         # upstream for consistency.
@@ -411,13 +415,13 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
                 # Kick the can down the road (should only be simple atoms?)
                 child_exprs = map(c->Expr(:meta, :nospecialize, c), child_exprs[2:end])
             else
-                st_id, src = _insert_convert_expr(e.args[2], graph, src)
+                st_id = _insert_convert_expr(e.args[2], graph, src)
                 setmeta!(SyntaxTree(graph, st_id); nospecialize=true)
-                return st_id, src
+                return st_id
             end
         else
             @assert nargs === 1
-            child_exprs[1] = Expr(:sym_not_identifier, e.args[1])
+            child_exprs[1] = Expr(:quoted_symbol, e.args[1])
         end
     elseif e.head === :symbolicgoto || e.head === :symboliclabel
         @assert nargs === 1
@@ -427,15 +431,16 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
     elseif e.head === :inline || e.head === :noinline
         @assert nargs === 1 && e.args[1] isa Bool
         # TODO: JuliaLowering doesn't accept this (non-:meta) form yet
-        return nothing, src
+        st_k = K"TOMBSTONE"
+        child_exprs = nothing
     elseif e.head === :core
         @assert nargs === 1
         @assert e.args[1] isa Symbol
         st_attrs[:name_val] = string(e.args[1])
         child_exprs = nothing
     elseif e.head === :islocal || e.head === :isglobal
         st_k = K"extension"
-        child_exprs = [Expr(:sym_not_identifier, e.head), e.args[1]]
+        child_exprs = [Expr(:quoted_symbol, e.head), e.args[1]]
     elseif e.head === :block && nargs >= 1 &&
         e.args[1] isa Expr && e.args[1].head === :softscope
         # (block (softscope true) ex) produced with every REPL prompt.
@@ -454,15 +459,12 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
         mac_name = string(e.args[1])
         mac_name = mac_name == "@__dot__" ? "@." : mac_name
         st_id = _insert_tree_node(graph, K"MacroName", src, st_flags; name_val=mac_name)
-        if !Base.is_valid_identifier(mac_name[2:end])
-            return _insert_var_str(st_id, graph, src)
-        end
-        return st_id, src
+        return st_id
     elseif e.head === :catch_var_placeholder
         st_k = K"Placeholder"
         st_attrs[:name_val] = ""
         child_exprs = nothing
-    elseif e.head === :sym_not_identifier
+    elseif e.head === :quoted_symbol
         st_k = K"Symbol"
         st_attrs[:name_val] = String(e.args[1])
         child_exprs = nothing
@@ -482,24 +484,25 @@ function _insert_convert_expr(@nospecialize(e), graph::SyntaxGraph, src::SourceA
     # child_exprs === nothing means we want a leaf.  Note that setchildren! with
     # an empty list makes a node non-leaf.
     if isnothing(child_exprs)
-        return st_id, src
+        return st_id
     else
-        st_flags |= JS.NON_TERMINAL_FLAG
         setflags!(graph, st_id, st_flags)
-        st_child_ids, last_src = _insert_child_expr(child_exprs, graph, src)
+        st_child_ids = _insert_child_expr(child_exprs, graph, src)
         setchildren!(graph, st_id, st_child_ids)
-        return st_id, last_src
+        return st_id
     end
 end
 
 function _insert_child_expr(child_exprs::Vector{Any}, graph::SyntaxGraph,
-                               src::SourceAttrType)
+                            src::SourceAttrType)
     st_child_ids = NodeId[]
     last_src = src
     for c in child_exprs
-        (c_id, c_src) = _insert_convert_expr(c, graph, last_src)
-        isnothing(c_id) || push!(st_child_ids, c_id)
-        last_src = something(c_src, src)
+        if c isa LineNumberNode
+            last_src = c
+        else
+            push!(st_child_ids, _insert_convert_expr(c, graph, last_src))
+        end
     end
-    return st_child_ids, last_src
+    return st_child_ids
 end

src/syntax_graph.jl

@@ -146,9 +146,7 @@ function sethead!(graph, id::NodeId, k::Kind)
 end
 
 function setflags!(graph, id::NodeId, f::UInt16)
-    if f != 0
-        graph.syntax_flags[id] = f
-    end
+    graph.syntax_flags[id] = f
 end
 
 function _convert_nodes(graph::SyntaxGraph, node::SyntaxNode)
@@ -427,6 +425,8 @@ end
 
 attrsummary(name, value) = string(name)
 attrsummary(name, value::Number) = "$name=$value"
+attrsummary(name, value::String) = "$name=\"$(Base.escape_string(value))\""
+attrsummary(name, value::Symbol) = "$name=$value"
 
 function _value_string(ex)
     k = kind(ex)
@@ -476,9 +476,10 @@ function _show_syntax_tree(io, ex, indent, show_kinds)
         treestr = treestr*" :: "*string(kind(ex))
     end
 
-    std_attrs = Set([:name_val,:value,:kind,:syntax_flags,:source,:var_id])
+    std_attrs = Set([:kind])
     attrstr = join([attrsummary(n, getproperty(ex, n))
                     for n in attrnames(ex) if n ∉ std_attrs], ",")
+    attrstr = "$(lpad(ex._id, 3)) [$(lpad(byte_range(ex).start, 4)):$(lpad(byte_range(ex).stop, 4))] $attrstr"
     treestr = string(rpad(treestr, 60), " │ $attrstr")
 
     println(io, treestr)