Merge pull request #338 from julia-vscode/sp/loop-over-length-check

LoopOverLength check

Author: pfitzseb

src/linting/checks.jl

@@ -1,37 +1,40 @@
-@enum(LintCodes,
-MissingRef,
-IncorrectCallArgs,
-IncorrectIterSpec,
-NothingEquality,
-NothingNotEq,
-ConstIfCondition,
-EqInIfConditional,
-PointlessOR,
-PointlessAND,
-UnusedBinding,
-InvalidTypeDeclaration,
-UnusedTypeParameter,
-IncludeLoop,
-MissingFile,
-InvalidModuleName,
-TypePiracy,
-UnusedFunctionArgument,
-CannotDeclareConst,
-InvalidRedefofConst,
-NotEqDef,
-KwDefaultMismatch,
-InappropriateUseOfLiteral,
-ShouldBeInALoop,
-TypeDeclOnGlobalVariable,
-UnsupportedConstLocalVariable,
-UnassignedKeywordArgument,
-CannotDefineFuncAlreadyHasValue,
-DuplicateFuncArgName,
-IncludePathContainsNULL)
-
-
-
-const LintCodeDescriptions = Dict{LintCodes,String}(IncorrectCallArgs => "Possible method call error.",
+@enum(
+    LintCodes,
+
+    MissingRef,
+    IncorrectCallArgs,
+    IncorrectIterSpec,
+    NothingEquality,
+    NothingNotEq,
+    ConstIfCondition,
+    EqInIfConditional,
+    PointlessOR,
+    PointlessAND,
+    UnusedBinding,
+    InvalidTypeDeclaration,
+    UnusedTypeParameter,
+    IncludeLoop,
+    MissingFile,
+    InvalidModuleName,
+    TypePiracy,
+    UnusedFunctionArgument,
+    CannotDeclareConst,
+    InvalidRedefofConst,
+    NotEqDef,
+    KwDefaultMismatch,
+    InappropriateUseOfLiteral,
+    ShouldBeInALoop,
+    TypeDeclOnGlobalVariable,
+    UnsupportedConstLocalVariable,
+    UnassignedKeywordArgument,
+    CannotDefineFuncAlreadyHasValue,
+    DuplicateFuncArgName,
+    IncludePathContainsNULL,
+    IndexFromLength
+)
+
+const LintCodeDescriptions = Dict{LintCodes,String}(
+    IncorrectCallArgs => "Possible method call error.",
     IncorrectIterSpec => "A loop iterator has been used that will likely error.",
     NothingEquality => "Compare against `nothing` using `===`",
     NothingNotEq => "Compare against `nothing` using `!==`",
@@ -58,8 +61,9 @@ const LintCodeDescriptions = Dict{LintCodes,String}(IncorrectCallArgs => "Possib
     UnassignedKeywordArgument => "Keyword argument not assigned.",
     CannotDefineFuncAlreadyHasValue => "Cannot define function ; it already has a value.",
     DuplicateFuncArgName => "Function argument name not unique.",
-    IncludePathContainsNULL => "Cannot include file, path cotains NULL characters."
-    )
+    IncludePathContainsNULL => "Cannot include file, path contains NULL characters.",
+    IndexFromLength => "Indexing with indices obtained from `length`, `size` etc is discouraged. Use `eachindex` or `axes` instead."
+)
 
 haserror(m::Meta) = m.error !== nothing
 haserror(x::EXPR) = hasmeta(x) && haserror(x.meta)
@@ -343,22 +347,75 @@ isdocumented(x::EXPR) = parentof(x) isa EXPR && CSTParser.ismacrocall(parentof(x
 
 function check_loop_iter(x::EXPR, env::ExternalEnv)
     if headof(x) === :for
-        if length(x.args) > 0 && CSTParser.is_range(x.args[1])
-            rng = rhs_of_iterator(x.args[1])
-            if headof(rng) === :FLOAT || headof(rng) === :INTEGER || (iscall(rng) && refof(rng.args[1]) === getsymbols(env)[:Base][:length])
-                seterror!(x.args[1], IncorrectIterSpec)
+        if length(x.args) > 1
+            body = x.args[2]
+            if headof(x.args[1]) === :block && x.args[1].args !== nothing
+                for arg in x.args[1].args
+                    check_incorrect_iter_spec(arg, body, env)
+                end
+            else
+                check_incorrect_iter_spec(x.args[1], body, env)
             end
         end
     elseif headof(x) === :generator
+        body = x.args[1]
         for i = 2:length(x.args)
-            if CSTParser.is_range(x.args[i])
-                rng = rhs_of_iterator(x.args[i])
-                if headof(rng) === :FLOAT || headof(rng) === :INTEGER || (iscall(rng) && refof(rng.args[1]) === getsymbols(env)[:Base][:length])
-                    seterror!(x.args[i], IncorrectIterSpec)
+            check_incorrect_iter_spec(x.args[i], body, env)
+        end
+    end
+end
+
+function check_incorrect_iter_spec(x, body, env)
+    if x.args !== nothing && CSTParser.is_range(x)
+        rng = rhs_of_iterator(x)
+
+        if headof(rng) === :FLOAT || headof(rng) === :INTEGER || (iscall(rng) && refof(rng.args[1]) === getsymbols(env)[:Base][:length])
+            seterror!(x, IncorrectIterSpec)
+        elseif iscall(rng) && valof(rng.args[1]) == ":" &&
+            length(rng.args) === 3 &&
+            headof(rng.args[2]) === :INTEGER &&
+            iscall(rng.args[3]) &&
+            length(rng.args[3].args) > 1 && (
+                refof(rng.args[3].args[1]) === getsymbols(env)[:Base][:length] ||
+                refof(rng.args[3].args[1]) === getsymbols(env)[:Base][:size]
+            )
+            if length(x.args) >= 1
+                lhs = x.args[1]
+                arr = rng.args[3].args[2]
+                b = refof(arr)
+
+                # 1:length(arr) indexing is ok for Vector and Array specifically
+                if b isa Binding && (CoreTypes.isarray(b.type) || CoreTypes.isvector(b.type))
+                    return
+                end
+                if !all_underscore(valof(lhs))
+                    if check_is_used_in_getindex(body, lhs, arr)
+                        seterror!(x, IndexFromLength)
+                    end
+                end
+            end
+        end
+    end
+end
+
+function check_is_used_in_getindex(expr, lhs, arr)
+    if headof(expr) === :ref && expr.args !== nothing && length(expr.args) > 1
+        this_arr = expr.args[1]
+        if hasref(this_arr) && hasref(arr) && refof(this_arr) == refof(arr)
+            for index_arg in expr.args[2:end]
+                if hasref(index_arg) && hasref(lhs) && refof(index_arg) == refof(lhs)
+                    seterror!(expr, IndexFromLength)
+                    return true
                 end
             end
         end
     end
+    if expr.args !== nothing
+        for arg in expr.args
+            check_is_used_in_getindex(arg, lhs, arr) && return true
+        end
+    end
+    return false
 end
 
 function check_nothing_equality(x::EXPR, env::ExternalEnv)

test/runtests.jl

@@ -1840,3 +1840,108 @@ end
         @test length(b.refs) == 2
     end
 end
+
+@testset "iteration over 1:length(...)" begin
+    cst = parse_and_pass("arr = []; [1 for _ in 1:length(arr)]")
+    @test isempty(StaticLint.collect_hints(cst, server))
+    cst = parse_and_pass("arr = []; [arr[i] for i in 1:length(arr)]")
+    @test length(StaticLint.collect_hints(cst, server)) == 2
+    cst = parse_and_pass("arr = []; [i for i in 1:length(arr)]")
+    @test length(StaticLint.collect_hints(cst, server)) == 0
+
+    cst = parse_and_pass("""
+    arr = []
+    for _ in 1:length(arr)
+    end
+    """)
+    @test isempty(StaticLint.collect_hints(cst, server))
+    cst = parse_and_pass("""
+    arr = []
+    for i in 1:length(arr)
+        arr[i]
+    end
+    """)
+    @test length(StaticLint.collect_hints(cst, server)) == 2
+    cst = parse_and_pass("""
+    arr = []
+    for i in 1:length(arr)
+        println(i)
+    end
+    """)
+    @test length(StaticLint.collect_hints(cst, server)) == 0
+
+    cst = parse_and_pass("""
+    arr = []
+    for _ in 1:length(arr), _ in 1:length(arr)
+    end
+    """)
+    @test isempty(StaticLint.collect_hints(cst, server))
+    cst = parse_and_pass("""
+    arr = []
+    for i in 1:length(arr), j in 1:length(arr)
+        arr[i] + arr[j]
+    end
+    """)
+    @test length(StaticLint.collect_hints(cst, server)) == 4
+    cst = parse_and_pass("""
+    arr = []
+    for i in 1:length(arr), j in 1:length(arr)
+        println(i + j)
+    end
+    """)
+    @test length(StaticLint.collect_hints(cst, server)) == 0
+
+    cst = parse_and_pass("""
+    function f(arr::Vector)
+        for i in 1:length(arr), j in 1:length(arr)
+            arr[i] + arr[j]
+        end
+    end
+    """)
+    @test length(StaticLint.collect_hints(cst, server)) == 0
+
+    cst = parse_and_pass("""
+    function f(arr::Array)
+        for i in 1:length(arr), j in 1:length(arr)
+            arr[i] + arr[j]
+        end
+    end
+    """)
+    @test length(StaticLint.collect_hints(cst, server)) == 0
+
+    cst = parse_and_pass("""
+    function f(arr::Matrix)
+        for i in 1:length(arr), j in 1:length(arr)
+            arr[i] + arr[j]
+        end
+    end
+    """)
+    @test length(StaticLint.collect_hints(cst, server)) == 0
+
+    cst = parse_and_pass("""
+    function f(arr::Array{T,N}) where T where N
+        for i in 1:length(arr), j in 1:length(arr)
+            arr[i] + arr[j]
+        end
+    end
+    """)
+    @test length(StaticLint.collect_hints(cst, server)) == 0
+
+    cst = parse_and_pass("""
+    function f(arr::AbstractArray)
+        for i in 1:length(arr), j in 1:length(arr)
+            arr[i] + arr[j]
+        end
+    end
+    """)
+    @test length(StaticLint.collect_hints(cst, server)) == 4
+
+    cst = parse_and_pass("""
+    function f(arr)
+        for i in 1:length(arr), j in 1:length(arr)
+            arr[i] + arr[j]
+        end
+    end
+    """)
+    @test length(StaticLint.collect_hints(cst, server)) == 4
+end