From 500cde68c3122d7e5005b5502862ca3831971d64 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 9 Sep 2019 17:01:21 +0100 Subject: [PATCH] JavaScript: Add new query `UnusedIndexVariable`. --- change-notes/1.23/analysis-javascript.md | 1 + .../config/suites/javascript/correctness-core | 1 + .../ql/src/Declarations/UnusedVariable.qll | 3 ++ .../UnusedIndexVariable.qhelp | 45 +++++++++++++++++++ .../LanguageFeatures/UnusedIndexVariable.ql | 17 +++++++ .../LanguageFeatures/UnusedIndexVariable.qll | 40 +++++++++++++++++ .../examples/UnusedIndexVariable.js | 6 +++ .../examples/UnusedIndexVariableGood.js | 6 +++ .../examples/UnusedIndexVariableGood2.js | 6 +++ .../UnusedVariable/UnusedIndexVariable.js | 6 +++ .../UnusedIndexVariable.expected | 2 + .../UnusedIndexVariable.js | 6 +++ .../UnusedIndexVariable.qlref | 1 + .../UnusedIndexVariable2.js | 6 +++ .../UnusedIndexVariableGood.js | 6 +++ .../UnusedIndexVariableGood2.js | 6 +++ .../UnusedIndexVariable/tst.js | 12 +++++ 17 files changed, 170 insertions(+) create mode 100644 javascript/ql/src/LanguageFeatures/UnusedIndexVariable.qhelp create mode 100644 javascript/ql/src/LanguageFeatures/UnusedIndexVariable.ql create mode 100644 javascript/ql/src/LanguageFeatures/UnusedIndexVariable.qll create mode 100644 javascript/ql/src/LanguageFeatures/examples/UnusedIndexVariable.js create mode 100644 javascript/ql/src/LanguageFeatures/examples/UnusedIndexVariableGood.js create mode 100644 javascript/ql/src/LanguageFeatures/examples/UnusedIndexVariableGood2.js create mode 100644 javascript/ql/test/query-tests/Declarations/UnusedVariable/UnusedIndexVariable.js create mode 100644 javascript/ql/test/query-tests/LanguageFeatures/UnusedIndexVariable/UnusedIndexVariable.expected create mode 100644 javascript/ql/test/query-tests/LanguageFeatures/UnusedIndexVariable/UnusedIndexVariable.js create mode 100644 javascript/ql/test/query-tests/LanguageFeatures/UnusedIndexVariable/UnusedIndexVariable.qlref create mode 100644 javascript/ql/test/query-tests/LanguageFeatures/UnusedIndexVariable/UnusedIndexVariable2.js create mode 100644 javascript/ql/test/query-tests/LanguageFeatures/UnusedIndexVariable/UnusedIndexVariableGood.js create mode 100644 javascript/ql/test/query-tests/LanguageFeatures/UnusedIndexVariable/UnusedIndexVariableGood2.js create mode 100644 javascript/ql/test/query-tests/LanguageFeatures/UnusedIndexVariable/tst.js diff --git a/change-notes/1.23/analysis-javascript.md b/change-notes/1.23/analysis-javascript.md index c4666d79f3b..bc9a9cab922 100644 --- a/change-notes/1.23/analysis-javascript.md +++ b/change-notes/1.23/analysis-javascript.md @@ -11,6 +11,7 @@ | **Query** | **Tags** | **Purpose** | |---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| Unused index variable (`js/unused-index-variable`) | correctness | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. | ## Changes to existing queries diff --git a/javascript/config/suites/javascript/correctness-core b/javascript/config/suites/javascript/correctness-core index 344e8936a38..6a53405e1c3 100644 --- a/javascript/config/suites/javascript/correctness-core +++ b/javascript/config/suites/javascript/correctness-core @@ -28,6 +28,7 @@ + semmlecode-javascript-queries/LanguageFeatures/SyntaxError.ql: /Correctness/Language Features + semmlecode-javascript-queries/LanguageFeatures/TemplateSyntaxInStringLiteral.ql: /Correctness/Language Features + semmlecode-javascript-queries/LanguageFeatures/ThisBeforeSuper.ql: /Correctness/Language Features ++ semmlecode-javascript-queries/LanguageFeatures/UnusedIndexVariable.ql: /Correctness/Language Features + semmlecode-javascript-queries/RegExp/BackrefBeforeGroup.ql: /Correctness/Regular Expressions + semmlecode-javascript-queries/RegExp/BackrefIntoNegativeLookahead.ql: /Correctness/Regular Expressions + semmlecode-javascript-queries/RegExp/DuplicateCharacterInCharacterClass.ql: /Correctness/Regular Expressions diff --git a/javascript/ql/src/Declarations/UnusedVariable.qll b/javascript/ql/src/Declarations/UnusedVariable.qll index 143a8c5e074..bd6d56c6686 100644 --- a/javascript/ql/src/Declarations/UnusedVariable.qll +++ b/javascript/ql/src/Declarations/UnusedVariable.qll @@ -3,6 +3,7 @@ */ import javascript +import LanguageFeatures.UnusedIndexVariable /** * A local variable that is neither used nor exported, and is not a parameter @@ -16,6 +17,8 @@ class UnusedLocal extends LocalVariable { not exists(ClassExpr ce | this = ce.getVariable()) and not exists(ExportDeclaration ed | ed.exportsAs(this, _)) and not exists(LocalVarTypeAccess type | type.getVariable() = this) and + // avoid double reporting + not unusedIndexVariable(_, this, _) and // common convention: variables with leading underscore are intentionally unused getName().charAt(0) != "_" } diff --git a/javascript/ql/src/LanguageFeatures/UnusedIndexVariable.qhelp b/javascript/ql/src/LanguageFeatures/UnusedIndexVariable.qhelp new file mode 100644 index 00000000000..f16d04ae1c1 --- /dev/null +++ b/javascript/ql/src/LanguageFeatures/UnusedIndexVariable.qhelp @@ -0,0 +1,45 @@ + + + + +

+If the loop variable of a for loop ranges over the indices of an array, that variable +would normally be used as an array index in the body of the loop. If, instead, the loop body only +refers to array elements at constant indices, this may indicate a logic error or leftover testing +code. +

+ + + +

+Examine the loop carefully to ensure it is behaving as expected. You may want to consider using +a for-of loop to iterate over all elements of an array without the need +for error-prone index manipulations. +

+ + + +

+The following example shows a function that is intended to sum up the elements of an array +xs. The loop variable i is counted up from zero to +xs.length-1, but instead of adding xs[i] to the running sum +res, the code adds xs[0], the first element of xs, +to it, which is likely a mistake: +

+ +

+The problem can be fixed by adding xs[i] instead: +

+ +

+Alternatively, the function can be written more succinctly using a for-of +loop: +

+ + + + +
  • Mozilla Developer Network: for
    • +
  • Mozilla Developer Network: for...of
    • +     +     diff --git a/javascript/ql/src/LanguageFeatures/UnusedIndexVariable.ql b/javascript/ql/src/LanguageFeatures/UnusedIndexVariable.ql new file mode 100644 index 00000000000..da3c1d4b4cf --- /dev/null +++ b/javascript/ql/src/LanguageFeatures/UnusedIndexVariable.ql @@ -0,0 +1,17 @@ +/** + * @name Unused index variable + * @description Iterating over an array but not using the index variable to access array elements + * may indicate a typo or logic error. + * @kind problem + * @problem.severity warning + * @id js/unused-index-variable + * @precision high + * @tags correctness + */ + +import javascript +import UnusedIndexVariable + +from RelationalComparison rel, Variable idx, Variable v +where unusedIndexVariable(rel, idx, v) +select rel, "Index variable " + idx + " is never used to access elements of " + v + "." diff --git a/javascript/ql/src/LanguageFeatures/UnusedIndexVariable.qll b/javascript/ql/src/LanguageFeatures/UnusedIndexVariable.qll new file mode 100644 index 00000000000..ed53158d51c --- /dev/null +++ b/javascript/ql/src/LanguageFeatures/UnusedIndexVariable.qll @@ -0,0 +1,40 @@ +/** + * Provides a predicate for identifying unused index variables in loops. + */ + +import javascript + +/** + * Holds if `arr` is of the form `base[idx]` and is nested inside loop `fs`. + */ +private predicate arrayIndexInLoop(IndexExpr arr, Variable base, Expr idx, ForStmt fs) { + arr.getEnclosingStmt().getParentStmt*() = fs.getBody() and + arr.getBase() = base.getAnAccess() and + arr.getIndex() = idx +} + +/** + * Gets `e` or a sub-expression `s` resulting from `e` by peeling off unary and binary + * operators, increments, decrements, type assertions, parentheses, sequence expressions, + * and assignments. + */ +private Expr unwrap(Expr e) { + result = e or + result = unwrap(e.(UpdateExpr).getOperand()) or + result = unwrap(e.(UnaryExpr).getOperand()) or + result = unwrap(e.(BinaryExpr).getAnOperand()) or + result = unwrap(e.getUnderlyingValue()) +} + +/** + * Holds if `rel` is a for-loop condition of the form `idx <= v.length`, but all array + * indices `v[c]` inside the loop body (of which there must be at least one) use a constant + * index `c` instead of an index based on `idx`. + */ +predicate unusedIndexVariable(RelationalComparison rel, Variable idx, Variable v) { + exists(ForStmt fs | fs.getTest() = rel | + unwrap(rel.getLesserOperand()) = idx.getAnAccess() and + rel.getGreaterOperand().(PropAccess).accesses(v.getAnAccess(), "length") and + forex(IndexExpr arr, Expr e | arrayIndexInLoop(arr, v, e, fs) | exists(e.getIntValue())) + ) +} diff --git a/javascript/ql/src/LanguageFeatures/examples/UnusedIndexVariable.js b/javascript/ql/src/LanguageFeatures/examples/UnusedIndexVariable.js new file mode 100644 index 00000000000..7f6071b8696 --- /dev/null +++ b/javascript/ql/src/LanguageFeatures/examples/UnusedIndexVariable.js @@ -0,0 +1,6 @@ +function sum(xs) { + var res = 0; + for(var i=0; i