Merge pull request #269 from julia-vscode/sp/noclobber

respect soft scope and stop clobbering bindings

Author: pfitzseb

src/bindings.jl

@@ -344,7 +344,10 @@ function add_binding(x, state, scope=state.scope)
         elseif scopehasbinding(scope, name)
             # TODO: some checks about rebinding of consts
             check_const_decl(name, b, scope)
+
             scope.names[name] = b
+        elseif is_soft_scope(scope) && parentof(scope) isa Scope && scopehasbinding(parentof(scope), valofid(b.name))
+            add_binding(x, state, scope.parent)
         else
             scope.names[name] = b
         end

src/linting/checks.jl

@@ -492,9 +492,16 @@ function check_farg_unused(x::EXPR)
                     return
                 end
                 b = bindingof(arg)
-                if b === nothing || (isempty(b.refs) || (length(b.refs) == 1 && first(b.refs) == b.name))
+                if b === nothing ||
+                    # no refs:
+                   isempty(b.refs) ||
+                    # only self ref:
+                   (length(b.refs) == 1 && first(b.refs) == b.name) ||
+                    # first usage is assignment:
+                   (length(b.refs) > 1 && CSTParser.hasparent(b.refs[2]) && isassignment(parentof(b.refs[2])) && parentof(b.refs[2]).args[1] == b.refs[2])
                     seterror!(arg, UnusedFunctionArgument)
                 end
+
                 if valof(b.name) === nothing
                 elseif valof(b.name) in arg_names
                     seterror!(arg, DuplicateFuncArgName)
@@ -868,8 +875,9 @@ end
 
 function check_unused_binding(b::Binding, scope::Scope)
     if headof(scope.expr) !== :struct && headof(scope.expr) !== :tuple && !all_underscore(valof(b.name))
-        if (isempty(b.refs) || length(b.refs) == 1 && b.refs[1] == b.name) && !is_sig_arg(b.name) && !is_overwritten_in_loop(b.name) && !is_overwritten_subsequently(b, scope)
-            seterror!(b.name, UnusedBinding)    
+        refs = loose_refs(b)
+        if (isempty(refs) || length(refs) == 1 && refs[1] == b.name) && !is_sig_arg(b.name) && !is_overwritten_in_loop(b.name) && !is_overwritten_subsequently(b, scope)
+            seterror!(b.name, UnusedBinding)
         end
     end
 end

src/references.jl

@@ -38,6 +38,12 @@ end
 # can't be resolved.
 
 function resolve_ref(x::EXPR, scope::Scope, state::State)::Bool
+    # if the current scope is a soft scope we should check the parent scope first
+    # before trying to resolve the ref locally
+    if is_soft_scope(scope) && parentof(scope) isa Scope
+        resolve_ref(x, parentof(scope), state) && return true
+    end
+
     hasref(x) && return true
     resolved = false
 

src/scope.jl

@@ -56,6 +56,8 @@ Checks whether s has a binding for variable named `n`.
 """
 scopehasbinding(s::Scope, n::String) = haskey(s.names, n)
 
+is_soft_scope(scope::Scope) = scope.expr.head == :for || scope.expr.head == :while || scope.expr.head == :try
+
 """
     introduces_scope(x::EXPR, state)
 

src/utils.jl

@@ -301,3 +301,35 @@ function is_nameof_func(name)
     f = get_parent_fexpr(name, CSTParser.defines_function)
     f !== nothing && CSTParser.get_name(f) == name
 end
+
+function loose_refs(b::Binding)
+    scope = retrieve_scope(b.val)
+    name_str = valofid(b.name)
+    name_str isa String || return b.refs
+
+    if is_soft_scope(scope) && parentof(scope) isa Scope && scopehasbinding(parentof(scope), name_str) && !scopehasbinding(scope, name_str)
+        scope = parentof(scope)
+    end
+    state = LooseRefs(scope.expr, name_str, scope, [])
+    state(scope.expr)
+    vcat([r.refs for r in state.result]...)
+end
+
+mutable struct LooseRefs
+    x::EXPR
+    name::String
+    scope::Scope
+    result::Vector{Binding}
+end
+
+function (state::LooseRefs)(x::EXPR)
+    if hasbinding(x)
+        ex = bindingof(x).name
+        if isidentifier(ex) && valofid(ex) == state.name
+            push!(state.result, bindingof(x))
+        end
+    end
+    if !hasscope(x) || (hasscope(x) && ((is_soft_scope(scopeof(x)) && !scopehasbinding(scopeof(x), state.name)) || scopeof(x) == state.scope))
+        traverse(x, state)
+    end
+end

test/runtests.jl

@@ -791,6 +791,14 @@ f(arg) = arg
             StaticLint.check_farg_unused(cst[1])
             @test StaticLint.errorof(CSTParser.get_sig(cst[1])[3]) === StaticLint.UnusedFunctionArgument
         end
+        let cst = parse_and_pass(
+             """function f(arg)
+                    x = arg
+                    arg = x
+                end""")
+            StaticLint.check_farg_unused(cst[1])
+            @test StaticLint.errorof(CSTParser.get_sig(cst[1])[3]) === nothing
+        end
         let cst = parse_and_pass("function f(arg) 1 end")
             StaticLint.check_farg_unused(cst[1])
             @test StaticLint.errorof(CSTParser.get_sig(cst[1])[3]) === nothing
@@ -836,7 +844,7 @@ f(arg) = arg
         ASDF(1)
         """)
             # Check inner constructor is hoisted
-            @test isempty(StaticLint.collect_hints(cst, server)) 
+            @test isempty(StaticLint.collect_hints(cst, server))
         end
     end
 
@@ -1411,7 +1419,7 @@ f(arg) = arg
             f1(x)
         end""")
         @test bindingof(cst.args[3].args[1].args[2]).type !== nothing
-        
+
         cst = parse_and_pass("""
         f(x::String) = true
         f1(x::Char) = true
@@ -1465,7 +1473,7 @@ end
 @testset "duplicate function argument" begin
     cst = parse_and_pass("""
     f(a,a) = a
-    """) 
+    """)
     @test errorof(cst[1][1][5]) == StaticLint.DuplicateFuncArgName
 end
 
@@ -1590,7 +1598,7 @@ end
     multiply!(1, 3)
     """)
     @test errorof(cst[2]) === nothing
-    
+
     @test StaticLint.haserror(parse_and_pass("function f(z::T)::Nothing where T end")[1].args[1].args[1].args[1].args[2])
     @test StaticLint.haserror(parse_and_pass("function f(z::T) where T end")[1].args[1].args[1].args[2])
 end
@@ -1644,3 +1652,55 @@ end
     @test cst[2].meta.scope.names["V"].type isa SymbolServer.DataTypeStore
     @test isempty(StaticLint.collect_hints(cst, server))
 end
+
+@testset "softscope" begin
+    cst = parse_and_pass("""
+    function foo()
+        x = 1
+        x
+        if rand(Bool)
+            x = 2
+        end
+        x
+        while rand(Bool)
+            x = 3
+        end
+        x
+        for _ in 1:2
+            x = 4
+            y = 1
+        end
+        x
+    end
+    """)
+    
+    # check soft-scope bindings are lifted to parent scope
+    @test refof(cst[1][3][2]) == bindingof(cst[1][3][1][1])
+    @test refof(cst[1][3][4]) == bindingof(cst[1][3][3][3][1][1])
+    @test refof(cst[1][3][6]) == bindingof(cst[1][3][5][3][1][1])
+    @test refof(cst[1][3][8]) == bindingof(cst[1][3][7][3][1][1])
+
+    # check binding made in soft-scope with no matching binidng in parent scope isn't lifted
+    @test !haskey(scopeof(cst[1]).names, "y")
+    @test haskey(scopeof(cst[1][3][7]).names, "y")
+    
+
+    @test length(StaticLint.loose_refs(bindingof(cst[1][3][1][1]))) == 8
+    @test length(StaticLint.loose_refs(bindingof(cst[1][3][3][3][1][1]))) == 8
+    @test length(StaticLint.loose_refs(bindingof(cst[1][3][5][3][1][1]))) == 8
+    @test length(StaticLint.loose_refs(bindingof(cst[1][3][7][3][1][1]))) == 8
+
+    cst = parse_and_pass("""
+    function foo()
+        for _ in 1:2
+            x = 1
+            x
+        end
+        x
+        x = 1
+        x
+    end
+    """)
+    @test length(StaticLint.loose_refs(bindingof(cst[1][3][1][3][1][1]))) == 2
+    @test length(StaticLint.loose_refs(bindingof(cst[1][3][3][1]))) == 2
+end