Code review comments

Co-authored-by: Cody Tapscott <topolarity@tapscott.me>

Author: mlechu

JuliaLowering/src/bindings.jl

@@ -5,7 +5,7 @@ mutable struct BindingInfo
     const id::IdTag                 # Unique integer identifying this binding
     const name::String
     const kind::Symbol              # :local :global :argument :static_parameter
-    const node_id::Int              # ID of associated K"BindingId" node in the syntax graph
+    const node_id::Int              # ID of some K"BindingId" node in the syntax graph
     const mod::Union{Nothing,Module} # Set when `kind === :global`
     type::Union{Nothing,SyntaxTree} # Type, for bindings declared like x::T = 10
     is_const::Bool            # Constant, cannot be reassigned
@@ -47,23 +47,23 @@ end
 
 function Base.show(io::IO, binfo::BindingInfo)
     print(io, "BindingInfo(", binfo.id,
-          ", name=", repr(binfo.name),
+          ", ", repr(binfo.name),
           ", ", repr(binfo.kind),
-          ", node=", binfo.node_id)
+          ", ", binfo.node_id)
     !isnothing(binfo.mod)    && print(io, ", mod=", binfo.mod)
     !isnothing(binfo.type)   && print(io, ", type=", binfo.type)
-    binfo.is_const           && print(io, ", is_const")
-    binfo.is_ssa             && print(io, ", is_ssa")
-    binfo.is_internal        && print(io, ", is_internal")
-    binfo.is_ambiguous_local && print(io, ", is_ambiguous_local")
-    binfo.is_nospecialize    && print(io, ", is_nospecialize")
-    binfo.is_read            && print(io, ", is_read")
-    binfo.is_called          && print(io, ", is_called")
-    binfo.is_assigned        && print(io, ", is_assigned")
-    binfo.is_assigned_once   && print(io, ", is_assigned_once")
-    binfo.is_captured        && print(io, ", is_captured")
-    binfo.is_always_defined  && print(io, ", is_always_defined")
-    binfo.is_used_undef      && print(io, ", is_used_undef")
+    binfo.is_const           && print(io, ", is_const=true")
+    binfo.is_ssa             && print(io, ", is_ssa=true")
+    binfo.is_internal        && print(io, ", is_internal=true")
+    binfo.is_ambiguous_local && print(io, ", is_ambiguous_local=true")
+    binfo.is_nospecialize    && print(io, ", is_nospecialize=true")
+    binfo.is_read            && print(io, ", is_read=true")
+    binfo.is_called          && print(io, ", is_called=true")
+    binfo.is_assigned        && print(io, ", is_assigned=true")
+    binfo.is_assigned_once   && print(io, ", is_assigned_once=true")
+    binfo.is_captured        && print(io, ", is_captured=true")
+    binfo.is_always_defined  && print(io, ", is_always_defined=true")
+    binfo.is_used_undef      && print(io, ", is_used_undef=true")
     print(io, ")")
 end
 
@@ -158,9 +158,7 @@ function binding_ex(ctx::AbstractLoweringContext, b::BindingInfo)
 end
 binding_ex(ctx, id::IdTag) = binding_ex(ctx, get_binding(ctx, id))
 
-# One lambda's variables.  TODO: It might be easier to use scope ID as a lambda
-# ID and give BindingInfo a field noting which lambda it belongs to.  This could
-# probably just be a Set{IdTag} of captures.
+# One lambda's variables
 struct LambdaBindings
     # Binding ID of #self#
     self::IdTag
@@ -169,6 +167,10 @@ struct LambdaBindings
     # A map from every referenced local binding ID to whether the local is
     # captured (true) or native to this lambda (false).  References in inner
     # lambdas count: `inner.locals_capt[id]` implies `haskey(locals_capt, id)`
+    # TODO: If we use scope ID as a lambda ID and give BindingInfo a field
+    # noting which lambda it belongs to, we could just make this a BitSet of
+    # vars present, where we tell if a binding is captured by comparing
+    # this.scope_id with the BindingInfo's scope_id.
     locals_capt::Dict{IdTag,Bool}
 end
 

JuliaLowering/src/scope_analysis.jl

@@ -95,8 +95,6 @@ enclosing_lambda(ctx, scope::ScopeInfo) = ctx.scopes[scope.lambda_id]
 parent(ctx, scope::ScopeInfo) = is_top_scope(scope) ? nothing :
     ctx.scopes[scope.parent_id]
 
-enclosing_lambda(ctx, lb::LambdaBindings) = ctx.scopes[lb.scope_id]
-
 _var_str(v) = v === :local ? "local variable" :
     v === :global ? "global variable" :
     v === :argument ? "argument" :
@@ -112,7 +110,7 @@ function maybe_declare_in_scope!(ctx, scope::ScopeInfo, ex, new_k::Symbol)
     if kind(ex) === K"BindingId"
         bid = ex.var_id
         @assert get_binding(ctx, bid).kind === new_k
-        record_lambda_var!(ctx, scope, get_binding(ctx, bid), false)
+        record_lambda_var!(ctx, scope, get_binding(ctx, bid), capt=false)
         return bid
     elseif kind(ex) === K"Placeholder"
         return nothing
@@ -130,7 +128,7 @@ function maybe_declare_in_scope!(ctx, scope::ScopeInfo, ex, new_k::Symbol)
     elseif old_k === new_k
         (new_k === :global || new_k === :local) && return bid
         throw(LoweringError(ex, "function $(_var_str(new_k)) name not unique"))
-    # See note in test/scopes.jl
+    # See note in test/scopes.jl: "globals may overlap args or sparams"
     # elseif new_k === :global && old_k in (:argument, :static_parameter)
     #     declare_in_scope!(ctx, scope, ex, :global)
     else
@@ -140,26 +138,27 @@ function maybe_declare_in_scope!(ctx, scope::ScopeInfo, ex, new_k::Symbol)
     end
 end
 
+# globals are added to both `scope` and the top scope
 function declare_in_scope!(ctx, scope::ScopeInfo, ex, bk::Symbol; kws...)
     nk = NameKey(ex)
     if bk === :global
-        home_scope = top_scope(ctx)
+        declaration_scope = top_scope(ctx)
         mod = ctx.scope_layers[ex.scope_layer].mod
     else
-        home_scope = scope
+        declaration_scope = scope
         mod = nothing
     end
     b = _new_binding(ctx, ex, nk.name, bk; mod, kws...)
-    home_scope.vars[nk] = b.id
+    declaration_scope.vars[nk] = b.id
     scope.vars[nk] = b.id
     @assert !haskey(enclosing_lambda(ctx, scope).locals_capt, b.id)
-    record_lambda_var!(ctx, scope, b, false)
+    record_lambda_var!(ctx, scope, b, capt=false)
     return b.id
 end
 
 # If `b` is local and not yet recorded in the lambda bindings, mark it as
 # `capt`.  Also, (if `capt==true`), add it to any parent lambdas.
-function record_lambda_var!(ctx, scope::Union{ScopeInfo, LambdaBindings}, b, capt)
+function record_lambda_var!(ctx, scope::ScopeInfo, b; capt)
     if b.kind === :global || b.is_ssa
         return
     end
@@ -169,7 +168,7 @@ function record_lambda_var!(ctx, scope::Union{ScopeInfo, LambdaBindings}, b, cap
         b.is_captured = capt
         s2 = parent(ctx, lam)
         if capt && !isnothing(s2)
-            record_lambda_var!(ctx, s2, b, true)
+            record_lambda_var!(ctx, s2, b, capt=true)
         end
     end
 end
@@ -262,7 +261,9 @@ function enter_scope!(ctx, ex)
     #---------------------------------------------------------------------------
     # Find assignment targets, possibly introducing implicit locals and globals
     for (bid, node_id) in sort!(collect(scope.binding_assignments))
-        # mutable nameless bindings may be introduced in desugaring
+        # Mutable nameless bindings may be introduced in desugaring.  These
+        # should be capturable, and may be local to the nearest lambda or
+        # global.  Desugaring should ensure these are never used undef.
         maybe_declare_in_scope!(ctx, scope, SyntaxTree(ctx.graph, node_id),
                                 get_binding(ctx, bid).kind)
     end
@@ -302,7 +303,7 @@ function enter_scope!(ctx, ex)
             throw(LoweringError(ex, "cannot overwrite a static parameter"))
         elseif b.kind === :local || b.kind === :argument
             # unambiguous assignment to existing variable
-            record_lambda_var!(ctx, scope, b, true)
+            record_lambda_var!(ctx, scope, b, capt=true)
         end
     end
 
@@ -320,8 +321,10 @@ function add_local_decls!(ctx, stmts, srcref, scope)
     end
 end
 
-function _resolve_scopes(ctx, ex::SyntaxTree, @nospecialize(scope))
+function _resolve_scopes(ctx, ex::SyntaxTree,
+                         @nospecialize(scope::Union{Nothing, ScopeInfo}))
     k = kind(ex)
+    @assert scope isa ScopeInfo || k === K"lambda"
     if k == K"Identifier"
         b = resolve_name(ctx, ex)
         # Unresolved names are assumed global
@@ -330,10 +333,10 @@ function _resolve_scopes(ctx, ex::SyntaxTree, @nospecialize(scope))
             b = get_binding(ctx, gid)
         end
         # Locals not present in the current lambda need capturing
-        record_lambda_var!(ctx, scope, b, true)
+        record_lambda_var!(ctx, scope, b, capt=true)
         newleaf(ctx, ex, K"BindingId", b.id)
     elseif k === K"BindingId"
-        record_lambda_var!(ctx, scope, get_binding(ctx, ex), true)
+        record_lambda_var!(ctx, scope, get_binding(ctx, ex), capt=true)
         ex
     elseif k == K"softscope"
         makeleaf(ctx, ex, K"TOMBSTONE")
@@ -494,6 +497,8 @@ function _resolve_scopes(ctx, ex::SyntaxTree, @nospecialize(scope))
         @assert kind(resolved[1]) === K"BindingId"
         if get_binding(ctx, resolved[1].var_id).kind === :local
             throw(LoweringError(ex, "unsupported `const` declaration on local variable"))
+        elseif !is_top_scope(enclosing_lambda(ctx, scope))
+            throw(LoweringError(ex, "unsupported `const` inside function"))
         end
         resolved
     elseif k == K"assign_or_constdecl_if_global"
@@ -590,7 +595,8 @@ function analyze_variables!(ctx, ex)
         b.is_read = true
         # The type of typed locals is invisible in the previous pass,
         # but is filled in here.
-        record_lambda_var!(ctx, ctx.lambda_bindings, b, true)
+        scope = ctx.scopes[ctx.lambda_bindings.scope_id]
+        record_lambda_var!(ctx, scope, b, capt=true)
         @assert b.kind === :global || b.is_ssa || haskey(ctx.lambda_bindings.locals_capt, b.id)
     elseif k == K"Identifier"
         @assert false
@@ -611,7 +617,8 @@ function analyze_variables!(ctx, ex)
         if kind(lhs) != K"Placeholder"
             b = get_binding(ctx, lhs)
             add_assign!(b)
-            record_lambda_var!(ctx, ctx.lambda_bindings, b, true)
+            scope = ctx.scopes[ctx.lambda_bindings.scope_id]
+            record_lambda_var!(ctx, scope, b, capt=true)
             if !isnothing(b.type)
                 # Assignments introduce a variable's type later during closure
                 # conversion, but we must model that explicitly here.

JuliaLowering/test/decls_ir.jl

@@ -238,6 +238,14 @@ let
 #        └────┘ ── unsupported `const` declaration on local variable
 end
 
+########################################
+# Error: Const not supported in function scope
+function (); global g; const g = 1; end
+#---------------------
+LoweringError:
+function (); global g; const g = 1; end
+#                           └────┘ ── unsupported `const` inside function
+
 ########################################
 # Type decl on function argument
 function f(x)