Tests for all variable scope conflicts + make duplicate destructured …

…args an error

Here we introduce a `meta` attribute rather than - or perhaps in
addition to - the `K"meta"` kind and use it to tag local variables which
derived from function argument destructuring.

We use this to make it an error to have duplicate destructured argument
names. This is technically breaking, but probably only a good thing -
without this users will silently have the intial duplicate argument
names overwritten with the result of the last destructuring assignment.

Also add tests for the various variable scope conflict errors:
argument/local, static-parameter/local, local/global etc.

src/desugaring.jl

@@ -1171,24 +1171,30 @@ end
 #   (x::T, (y::U, z))
 #   strip out stmts = (local x) (decl x T) (local x) (decl y U) (local z)
 #   and return (x, (y, z))
-function strip_decls!(ctx, stmts, declkind, declkind2, ex)
+function strip_decls!(ctx, stmts, declkind, declkind2, declmeta, ex)
     k = kind(ex)
     if k == K"Identifier"
-        push!(stmts, makenode(ctx, ex, declkind, ex))
+        if !isnothing(declmeta)
+            push!(stmts, makenode(ctx, ex, declkind, ex; meta=declmeta))
+        else
+            push!(stmts, makenode(ctx, ex, declkind, ex))
+        end
         if !isnothing(declkind2)
             push!(stmts, makenode(ctx, ex, declkind2, ex))
         end
         ex
+    elseif k == K"Placeholder"
+        ex
     elseif k == K"::"
         @chk numchildren(ex) == 2
         name = ex[1]
         @chk kind(name) == K"Identifier"
         push!(stmts, makenode(ctx, ex, K"decl", name, ex[2]))
-        strip_decls!(ctx, stmts, declkind, declkind2, ex[1])
+        strip_decls!(ctx, stmts, declkind, declkind2, declmeta, ex[1])
     elseif k == K"tuple" || k == K"parameters"
         cs = SyntaxList(ctx)
         for e in children(ex)
-            push!(cs, strip_decls!(ctx, stmts, declkind, declkind2, e))
+            push!(cs, strip_decls!(ctx, stmts, declkind, declkind2, declmeta, e))
         end
         makenode(ctx, ex, k, cs)
     end
@@ -1199,6 +1205,7 @@ end
 # global x::T = 1   ==> (block (global x) (decl x T) (x = 1))
 function expand_decls(ctx, ex)
     declkind = kind(ex)
+    declmeta = get(ex, :meta, nothing)
     if numchildren(ex) == 1 && kind(ex[1]) ∈ KSet"const global local"
         declkind2 = kind(ex[1])
         bindings = children(ex[1])
@@ -1211,13 +1218,13 @@ function expand_decls(ctx, ex)
         kb = kind(binding)
         if is_prec_assignment(kb)
             @chk numchildren(binding) == 2
-            lhs = strip_decls!(ctx, stmts, declkind, declkind2, binding[1])
+            lhs = strip_decls!(ctx, stmts, declkind, declkind2, declmeta, binding[1])
             push!(stmts, @ast ctx binding [kb lhs binding[2]])
         elseif is_sym_decl(binding)
             if declkind == K"const" || declkind2 == K"const"
                 throw(LoweringError(ex, "expected assignment after `const`"))
             end
-            strip_decls!(ctx, stmts, declkind, declkind2, binding)
+            strip_decls!(ctx, stmts, declkind, declkind2, declmeta, binding)
         else
             throw(LoweringError(ex, "invalid syntax in variable declaration"))
         end
@@ -1461,9 +1468,8 @@ function expand_function_def(ctx, ex, docs, rewrite_call=identity, rewrite_body=
             if kind(aname) == K"tuple"
                 # Argument destructuring
                 n = new_mutable_var(ctx, aname, "destructured_arg_$i"; kind=:argument)
-                # TODO: Tag these destructured locals somehow so we can trigger
-                # the "function argument name not unique" error if they're repeated?
-                push!(body_stmts, @ast ctx aname [K"local" [K"=" aname n]])
+                push!(body_stmts, @ast ctx aname [K"local"(meta=CompileHints(:is_destructured_arg, true))
+                                                  [K"=" aname n]])
                 aname = n
             end
             push!(arg_names, aname)
@@ -2624,7 +2630,7 @@ function expand_forms_2(ctx::DesugaringContext, ex::SyntaxTree, docs=nothing)
             # Don't recurse when already simplified - `local x`, etc
             ex
         else
-            expand_forms_2(ctx, expand_decls(ctx, ex)) # FIXME
+            expand_forms_2(ctx, expand_decls(ctx, ex))
         end
     elseif k == K"where"
         expand_forms_2(ctx, expand_wheres(ctx, ex))

src/macro_expansion.jl

@@ -14,6 +14,13 @@ struct ScopeLayer
     is_macro_expansion::Bool # FIXME
 end
 
+# Type for `meta` attribute, to replace `Expr(:meta)`.
+# It's unclear how much flexibility we need here - is a dict good, or could we
+# just use a struct? Likely this will be sparse. Alternatively we could just
+# use individual attributes but those aren't easy to add on an ad-hoc basis in
+# the middle of a pass.
+const CompileHints = Base.ImmutableDict{Symbol,Any}
+
 struct MacroExpansionContext{GraphType} <: AbstractLoweringContext
     graph::GraphType
     bindings::Bindings
@@ -250,7 +257,8 @@ function expand_forms_1(mod::Module, ex::SyntaxTree)
     graph = ensure_attributes(syntax_graph(ex),
                               var_id=IdTag,
                               scope_layer=LayerId,
-                              __macro_ctx__=Nothing)
+                              __macro_ctx__=Nothing,
+                              meta=CompileHints)
     layers = ScopeLayer[ScopeLayer(1, mod, false)]
     ctx = MacroExpansionContext(graph, Bindings(), layers, layers[1])
     ex2 = expand_forms_1(ctx, reparent(ctx, ex))

src/scope_analysis.jl

@@ -45,7 +45,9 @@ struct NameKey
 end
 
 #-------------------------------------------------------------------------------
-function _find_scope_vars!(assignments, locals, globals, used_names, used_bindings, alias_bindings, ex)
+_insert_if_not_present!(dict, key, val) = get!(dict, key, val)
+
+function _find_scope_vars!(assignments, locals, destructured_args, globals, used_names, used_bindings, alias_bindings, ex)
     k = kind(ex)
     if k == K"Identifier"
         push!(used_names, NameKey(ex))
@@ -57,19 +59,23 @@ function _find_scope_vars!(assignments, locals, globals, used_names, used_bindin
             k in KSet"scope_block lambda module toplevel"
         return
     elseif k == K"local" || k == K"local_def"
-        get!(locals, NameKey(ex[1]), ex)
+        if (meta = get(ex, :meta, nothing); !isnothing(meta) && get(meta, :is_destructured_arg, false))
+            push!(destructured_args, ex[1])
+        else
+            _insert_if_not_present!(locals, NameKey(ex[1]), ex)
+        end
     elseif k == K"global"
-        get!(globals, NameKey(ex[1]), ex)
+        _insert_if_not_present!(globals, NameKey(ex[1]), ex)
     # elseif k == K"method" TODO static parameters
     elseif k == K"="
         v = decl_var(ex[1])
         if !(kind(v) in KSet"BindingId globalref Placeholder")
-            get!(assignments, NameKey(v), v)
+            _insert_if_not_present!(assignments, NameKey(v), v)
         end
-        _find_scope_vars!(assignments, locals, globals, used_names, used_bindings, alias_bindings, ex[2])
+        _find_scope_vars!(assignments, locals, destructured_args, globals, used_names, used_bindings, alias_bindings, ex[2])
     else
         for e in children(ex)
-            _find_scope_vars!(assignments, locals, globals, used_names, used_bindings, alias_bindings, e)
+            _find_scope_vars!(assignments, locals, destructured_args, globals, used_names, used_bindings, alias_bindings, e)
         end
     end
 end
@@ -82,12 +88,13 @@ function find_scope_vars(ex)
     ExT = typeof(ex)
     assignments = Dict{NameKey,ExT}()
     locals = Dict{NameKey,ExT}()
+    destructured_args = Vector{ExT}()
     globals = Dict{NameKey,ExT}()
     used_names = Set{NameKey}()
     used_bindings = Set{IdTag}()
     alias_bindings = Vector{Pair{NameKey,IdTag}}()
     for e in children(ex)
-        _find_scope_vars!(assignments, locals, globals, used_names, used_bindings, alias_bindings, e)
+        _find_scope_vars!(assignments, locals, destructured_args, globals, used_names, used_bindings, alias_bindings, e)
     end
 
     # Sort by key so that id generation is deterministic
@@ -97,7 +104,7 @@ function find_scope_vars(ex)
     used_names = sort(collect(used_names))
     used_bindings = sort(collect(used_bindings))
 
-    return assignments, locals, globals, used_names, used_bindings, alias_bindings
+    return assignments, locals, destructured_args, globals, used_names, used_bindings, alias_bindings
 end
 
 function Base.isless(a::NameKey, b::NameKey)
@@ -196,6 +203,27 @@ function init_binding(ctx, varkey::NameKey, kind::Symbol, is_ambiguous_local=fal
     id
 end
 
+# Add lambda arguments and static parameters
+function add_lambda_args(ctx, var_ids, args, args_kind)
+    for arg in args
+        ka = kind(arg)
+        if ka == K"Identifier"
+            varkey = NameKey(arg)
+            if haskey(var_ids, varkey)
+                vk = lookup_binding(ctx, var_ids[varkey]).kind
+                _is_arg(k) = k == :argument || k == :local
+                msg = _is_arg(vk) && _is_arg(args_kind) ? "function argument name not unique"         :
+                      vk == :static_parameter && args_kind == :static_parameter ? "function static parameter name not unique" :
+                      "static parameter name not distinct from function argument"
+                throw(LoweringError(arg, msg))
+            end
+            var_ids[varkey] = init_binding(ctx, varkey, args_kind)
+        elseif ka != K"BindingId" && ka != K"Placeholder"
+            throw(LoweringError(arg, "Unexpected lambda arg kind"))
+        end
+    end
+end
+
 # Analyze identifier usage within a scope, adding all newly discovered
 # identifiers to ctx.bindings and returning a lookup table from identifier
 # names to their variable IDs
@@ -206,42 +234,22 @@ function analyze_scope(ctx, ex, scope_type, is_toplevel_global_scope=false,
     in_toplevel_thunk = is_toplevel_global_scope ||
         (!is_outer_lambda_scope && parentscope.in_toplevel_thunk)
 
-    assignments, locals, globals, used, used_bindings, alias_bindings = find_scope_vars(ex)
+    assignments, locals, destructured_args, globals,
+        used, used_bindings, alias_bindings = find_scope_vars(ex)
 
     # Create new lookup table for variables in this scope which differ from the
     # parent scope.
     var_ids = Dict{NameKey,IdTag}()
 
-    # Add lambda arguments and static parameters
-    function add_lambda_args(args, var_kind)
-        for arg in args
-            ka = kind(arg)
-            if ka == K"Identifier"
-                varkey = NameKey(arg)
-                if haskey(var_ids, varkey)
-                    vk = lookup_binding(ctx, var_ids[varkey]).kind
-                    msg = vk == :argument         && var_kind == vk ? "function argument name not unique"         :
-                          vk == :static_parameter && var_kind == vk ? "function static parameter name not unique" :
-                          "static parameter name not distinct from function argument"
-                    throw(LoweringError(arg, msg))
-                end
-                var_ids[varkey] = init_binding(ctx, varkey, var_kind)
-            elseif ka != K"BindingId" && ka != K"Placeholder"
-                throw(LoweringError(a, "Unexpected lambda arg kind"))
-            end
-        end
-    end
     if !isnothing(lambda_args)
-        add_lambda_args(lambda_args, :argument)
-        add_lambda_args(lambda_static_parameters, :static_parameter)
+        add_lambda_args(ctx, var_ids, lambda_args, :argument)
+        add_lambda_args(ctx, var_ids, lambda_static_parameters, :static_parameter)
+        add_lambda_args(ctx, var_ids, destructured_args, :local)
     end
 
-    global_keys = Set(first(g) for g in globals)
     # Add explicit locals
     for (varkey,e) in locals
-        if varkey in global_keys
-            throw(LoweringError(e, "Variable `$(varkey.name)` declared both local and global"))
-        elseif haskey(var_ids, varkey)
+        if haskey(var_ids, varkey)
             vk = lookup_binding(ctx, var_ids[varkey]).kind
             if vk === :argument && is_outer_lambda_scope
                 throw(LoweringError(e, "local variable name `$(varkey.name)` conflicts with an argument"))
@@ -258,7 +266,9 @@ function analyze_scope(ctx, ex, scope_type, is_toplevel_global_scope=false,
     for (varkey,e) in globals
         if haskey(var_ids, varkey)
             vk = lookup_binding(ctx, var_ids[varkey]).kind
-            if vk === :argument && is_outer_lambda_scope
+            if vk === :local
+                throw(LoweringError(e, "Variable `$(varkey.name)` declared both local and global"))
+            elseif vk === :argument && is_outer_lambda_scope
                 throw(LoweringError(e, "global variable name `$(varkey.name)` conflicts with an argument"))
             elseif vk === :static_parameter
                 throw(LoweringError(e, "global variable name `$(varkey.name)` conflicts with a static parameter"))

test/functions_ir.jl

@@ -147,36 +147,6 @@ end
     2   (return %₁)
 14  (return %₄)
 
-########################################
-# Error: Duplicate function argument names
-function f(x, x)
-end
-#---------------------
-LoweringError:
-function f(x, x)
-#             ╙ ── function argument name not unique
-end
-
-########################################
-# Error: Static parameter name not unique
-function f() where T where T
-end
-#---------------------
-LoweringError:
-function f() where T where T
-#                  ╙ ── function static parameter name not unique
-end
-
-########################################
-# Error: static parameter colliding with argument names
-function f(x::x) where x
-end
-#---------------------
-LoweringError:
-function f(x::x) where x
-#                      ╙ ── static parameter name not distinct from function argument
-end
-
 ########################################
 # Return types
 function f(x)::Int
@@ -682,9 +652,8 @@ end
 14  (return %₁)
 
 ########################################
-# Broken: the following repeated destructured args should probably be an error
-# but they're just normal locals so it's a bit hard to trigger.
-function f((x,), (x,))
+# Duplicate destructured placeholders ok
+function f((_,), (_,))
 end
 #---------------------
 1   (method :f)
@@ -694,24 +663,12 @@ end
 5   (call core.svec %₃ %₄ :($(QuoteNode(:(#= line 1 =#)))))
 6   --- method core.nothing %₅
     1   (call top.indexed_iterate slot₂/destructured_arg_1 1)
-    2   (= slot₄/x (call core.getfield %₁ 1))
+    2   (call core.getfield %₁ 1)
     3   (call top.indexed_iterate slot₃/destructured_arg_2 1)
-    4   (= slot₄/x (call core.getfield %₃ 1))
+    4   (call core.getfield %₃ 1)
     5   (return core.nothing)
 7   (return %₁)
 
-########################################
-# Error: Function argument destructuring conflicting with a global decl
-function f((x,))
-    global x
-end
-#---------------------
-LoweringError:
-function f((x,))
-#           ╙ ── Variable `x` declared both local and global
-    global x
-end
-
 ########################################
 # Binding docs to functions
 """