Merge pull request #255 from julia-vscode/unused-binding

check for unused bindings

Author: ZacLN

src/StaticLint.jl

@@ -117,6 +117,7 @@ function (state::Delayed)(x::EXPR)
     if state.scope != s0
         for (k, b) in state.scope.names
             infer_type_by_use(b, state.server)
+            check_unused_binding(b, state.scope)    
         end
         state.scope = s0
     end
@@ -160,6 +161,7 @@ function semantic_pass(file, modified_expr=nothing)
             traverse(x, Delayed(scopeof(x), server)) 
             for (k, b) in scopeof(x).names
                 infer_type_by_use(b, state.server)
+                check_unused_binding(b, scopeof(x))
             end
         else
             traverse(x, Delayed(retrieve_delayed_scope(x), server))

src/imports.jl

@@ -78,7 +78,7 @@ function add_to_imported_modules(scope::Scope, name::Symbol, val)
     if scope.modules isa Dict
         scope.modules[name] = val
     else
-        modules = Dict(name => val)
+        Dict(name => val)
     end
 end
 no_modules_above(s::Scope) = !CSTParser.defines_module(s.expr) || s.parent === nothing || no_modules_above(s.parent)

src/linting/checks.jl

@@ -39,7 +39,7 @@ const LintCodeDescriptions = Dict{LintCodes,String}(IncorrectCallArgs => "Possib
     EqInIfConditional => "Unbracketed assignment in if conditional statements is not allowed, did you mean to use ==?",
     PointlessOR => "The first argument of a `||` call is a boolean literal.",
     PointlessAND => "The first argument of a `&&` call is `false`.",
-    UnusedBinding => "The variable name has been bound but not used.",
+    UnusedBinding => "Variable has been assigned but not used.",
     InvalidTypeDeclaration => "A non-DataType has been used in a type declaration statement.",
     UnusedTypeParameter => "A DataType parameter has been specified but not used.",
     IncludeLoop => "Loop detected, this file has already been included.",
@@ -571,6 +571,9 @@ function should_mark_missing_getfield_ref(x, server)
             # a module, we should know this.
             return true
         elseif lhsref isa Binding
+            # by-use type inference runs after we've resolved references so we may not have known lhsref's type first time round, lets try and find `x` again
+            resolve_getfield(x, lhsref, ResolveOnly(retrieve_scope(x), server))
+            hasref(x) && return false # We've resolved
             if lhsref.val isa Binding
                 lhsref = lhsref.val
             end
@@ -862,3 +865,132 @@ function check_const(x::EXPR)
         end
     end
 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)    
+        end
+    end
+end
+
+all_underscore(s) = false
+all_underscore(s::String) = all(==(0x5f), codeunits(s))
+
+function is_sig_arg(x)
+    is_in_fexpr(x, CSTParser.iscall)
+end
+
+function is_overwritten_in_loop(x)
+    # Cuts out false positives for check_unused_binding - the linear nature of our 
+    # semantic passes mean a variable declared at the end of a loop's block but used at 
+    # the start won't appear to be referenced.
+
+    # Cheap version:
+    # is_in_fexpr(x, x -> x.head === :while || x.head === :for)
+    
+    # We really want to check whether the enclosing scope(s) of the loop has a binding 
+    # with matching name.
+    # Is this too expensive?
+    loop = maybe_get_parent_fexpr(x, x -> x.head === :while || x.head === :for)
+    if loop !== nothing
+        s = scopeof(loop)
+        if s isa Scope && parentof(s) isa Scope
+            s2 = check_parent_scopes_for(s, valof(x))
+            if s2 isa Scope
+                prev_binding = parentof(s2).names[valof(x)]
+                if prev_binding isa Binding
+                    s = ComesBefore(prev_binding.name, s2.expr, 0)
+                    traverse(parentof(s2).expr, s)
+                    return s.result == 1
+                    # for r in prev_binding.refs
+                    #     if r isa EXPR && is_in_fexpr(r, x -> x === loop)
+                    #         return true
+                    #     end
+                    # end
+                else
+                    return false
+                end
+            end
+        else
+            return false
+        end
+    else
+        false
+    end
+    false
+end
+
+"""
+    ComesBefore
+
+Check whether x1 comes before x2
+"""
+mutable struct ComesBefore
+    x1::EXPR
+    x2::EXPR
+    result::Int
+end
+
+function (state::ComesBefore)(x::EXPR)
+    state.result > 0 && return
+    if x == state.x1
+        state.result = 1
+        return 
+    elseif x == state.x2
+        state.result = 2
+        return
+    end
+    if !hasscope(x)
+        traverse(x, state)
+        state.result > 0 && return
+    end
+end
+
+"""
+    check_parent_scopes_for(s::Scope, name)
+
+Checks whether the parent scope of `s` has the name `name`.
+"""
+function check_parent_scopes_for(s::Scope, name)
+    # This returns `s` rather than the parent so that s.expr can be used in the linear
+    # search (e.g. `bound_before`)
+    if s.expr.head !== :module && parentof(s) isa Scope && haskey(parentof(s).names, name)
+        s
+    elseif s.parent isa Scope 
+        check_parent_scopes_for(parentof(s), name)
+    end
+end
+
+
+
+function is_overwritten_subsequently(b::Binding, scope::Scope)
+    valof(b.name) === nothing && return false
+    s = BoundAfter(b.name, valof(b.name), 0)
+    traverse(scope.expr, s)
+    return s.result == 2
+end
+
+"""
+    ComesBefore
+
+Check whether x1 comes before x2
+"""
+mutable struct BoundAfter
+    x1::EXPR
+    name::String
+    result::Int
+end
+
+function (state::BoundAfter)(x::EXPR)
+    state.result > 1 && return
+    if x == state.x1
+        state.result = 1
+        return 
+    end
+    if scopeof(x) isa Scope && haskey(scopeof(x).names, state.name)
+        state.result = 2
+        return
+    end
+    traverse(x, state)
+end

src/references.jl

@@ -20,7 +20,7 @@ end
 # run over the entire top-level scope.
 function resolve_ref(x, state)
     if !(parentof(x) isa EXPR && headof(parentof(x)) === :quotenode)
-        resolved = resolve_ref(x, state.scope, state)
+        resolve_ref(x, state.scope, state)
     end
 end
 

src/type_inf.jl

@@ -54,7 +54,6 @@ function infer_type_assignment_rhs(binding, state, scope)
     elseif isidentifier(rhs) || is_getfield_w_quotenode(rhs)
         refof_rhs = isidentifier(rhs) ? refof(rhs) : refof_maybe_getfield(rhs)
         if refof_rhs isa Binding
-            rhs_bind = refof(rhs)
             if refof_rhs.val isa SymbolServer.GenericStore && refof_rhs.val.typ isa SymbolServer.FakeTypeName
                 settype!(binding, maybe_lookup(refof_rhs.val.typ.name, state.server))
             elseif refof_rhs.val isa SymbolServer.FunctionStore

src/utils.jl

@@ -217,7 +217,6 @@ function iterate_over_ss_methods(b::SymbolServer.FunctionStore, tls::Scope, serv
         # above should be modified,
         rootmod = SymbolServer._lookup(b.extends.parent, getsymbolserver(server)) # points to the module containing the initial function declaration
         if rootmod !== nothing && haskey(rootmod, b.extends.name) # check rootmod exists, and that it has the variable
-            rootfunc = rootmod[b.extends.name]
             # find extensoions
             if haskey(getsymbolextendeds(server), b.extends) # method extensions listed
                 for vr in getsymbolextendeds(server)[b.extends] # iterate over packages with extensions
@@ -251,7 +250,6 @@ function iterate_over_ss_methods(b::SymbolServer.DataTypeStore, tls::Scope, serv
         # above should be modified,
         rootmod = SymbolServer._lookup(bname.parent, getsymbolserver(server), true) # points to the module containing the initial function declaration
         if rootmod !== nothing && haskey(rootmod, bname.name) # check rootmod exists, and that it has the variable
-            rootfunc = rootmod[bname.name]
             # find extensoions
             if haskey(getsymbolextendeds(server), bname) # method extensions listed
                 for vr in getsymbolextendeds(server)[bname] # iterate over packages with extensions

test/runtests.jl

@@ -876,7 +876,7 @@ f(arg) = arg
             f1
             f2
         end
-        Base.getproperty(x::T, s) = 1
+        Base.getproperty(x::T, s) = (x,s)
         f(x::T) = x.f3
         """)
             @test !StaticLint.hasref(cst.args[3].args[2].args[1].args[2].args[1])
@@ -887,7 +887,7 @@ f(arg) = arg
             f1
             f2
         end
-        Base.getproperty(x::T{Int}, s) = 1
+        Base.getproperty(x::T{Int}, s) = (x,s)
         f(x::T) = x.f3
         """)
             @test !StaticLint.hasref(cst.args[3].args[2].args[1].args[2].args[1])
@@ -1368,13 +1368,13 @@ f(arg) = arg
                 ret = "hello"
             end
         end""")
-        @test !StaticLint.haserror(cst.args[2].args[2].args[1].args[3].args[1].args[1])
-        @test !StaticLint.haserror(cst.args[3].args[2].args[1].args[3].args[1].args[1])
+        @test errorof(cst.args[2].args[2].args[1].args[3].args[1].args[1]) !== StaticLint.InvalidRedefofConst
+        @test errorof(cst.args[3].args[2].args[1].args[3].args[1].args[1]) !== StaticLint.InvalidRedefofConst
     end
 
     if VERSION > v"1.5-"
         @testset "issue #210" begin
-            cst = parse_and_pass("""h()::@NamedTuple{a::Int,b::String} = (a=1, b = "s")""")
+            cst = parse_and_pass("""h()::@NamedTuple{a::Int,b::String} = ()""")
             @test isempty(StaticLint.collect_hints(cst, server))
         end
     end
@@ -1515,6 +1515,69 @@ end
     @test isempty(StaticLint.collect_hints(cst, server))
 end
 
+@testset "unused bindings" begin
+    cst = parse_and_pass("""
+    function f(arg, arg2)
+        arg*arg2
+        arg3 = 1
+    end
+    """)
+    @test errorof(cst[1][3][2][1]) !== nothing
+
+    cst = parse_and_pass("""
+    function f()
+        arg = false
+        while arg
+            if arg
+            end
+            arg = true
+        end
+    end
+    """)
+    @test isempty(StaticLint.collect_hints(cst, server))
+
+    cst = parse_and_pass("""
+    function f(arg)
+        arg
+        while true
+            arg = 1
+        end
+    end 
+    """)
+    @test isempty(StaticLint.collect_hints(cst, server))
+
+    cst = parse_and_pass("""
+    function f(arg)
+        arg
+        while true
+            while true
+                arg = 1
+            end
+        end
+    end 
+    """)
+    @test isempty(StaticLint.collect_hints(cst, server))
+
+    cst = parse_and_pass("""
+    function f()
+        (a = 1, b = 2)
+    end 
+    """)
+    @test isempty(StaticLint.collect_hints(cst, server))
+
+    cst = parse_and_pass("""
+    function f()
+        arg = 0
+        if 1
+            while true
+                arg = 1
+            end
+        end
+    end 
+    """)
+    @test isempty(StaticLint.collect_hints(cst, server))
+end
+                                                
 @testset "unwrap sig" begin
     cst = parse_and_pass("""
     function multiply!(x::T, y::Integer) where {T} end
@@ -1530,7 +1593,6 @@ end
 
     @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
 
 @testset "clear .type refs" begin