Merge pull request #1910 from xiemaisi/js/unused-index-variable

Approved by esben-semmle, shati-semmle

Author: semmle-qlci

change-notes/1.23/analysis-javascript.md

@@ -13,6 +13,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

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

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) != "_"
   }

javascript/ql/src/LanguageFeatures/UnusedIndexVariable.qhelp

@@ -0,0 +1,45 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+If the loop variable of a <code>for</code> 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.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Examine the loop carefully to ensure it is behaving as expected. You may want to consider using
+a <code>for</code>-<code>of</code> loop to iterate over all elements of an array without the need
+for error-prone index manipulations.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows a function that is intended to sum up the elements of an array
+<code>xs</code>. The loop variable <code>i</code> is counted up from zero to
+<code>xs.length-1</code>, but instead of adding <code>xs[i]</code> to the running sum
+<code>res</code>, the code adds <code>xs[0]</code>, the first element of <code>xs</code>,
+to it, which is likely a mistake:
+</p>
+<sample src="examples/UnusedIndexVariable.js"/>
+<p>
+The problem can be fixed by adding <code>xs[i]</code> instead:
+</p>
+<sample src="examples/UnusedIndexVariableGood.js"/>
+<p>
+Alternatively, the function can be written more succinctly using a <code>for</code>-<code>of</code>
+loop:
+</p>
+<sample src="examples/UnusedIndexVariableGood2.js"/>
+</example>
+
+<references>
+<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for">for</a></li>
+<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of">for...of</a></li>
+</references>
+</qhelp>

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 + "."

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()))
+  )
+}

javascript/ql/src/LanguageFeatures/examples/UnusedIndexVariable.js

@@ -0,0 +1,6 @@
+function sum(xs) {
+  var res = 0;
+  for(var i=0; i<xs.length; ++i)
+    res += xs[0]; // BAD: should be xs[i]
+  return res;
+}

javascript/ql/src/LanguageFeatures/examples/UnusedIndexVariableGood.js

@@ -0,0 +1,6 @@
+function sum(xs) {
+  var res = 0;
+  for(var i=0; i<xs.length; ++i)
+    res += xs[i];
+  return res;
+}

javascript/ql/src/LanguageFeatures/examples/UnusedIndexVariableGood2.js

@@ -0,0 +1,6 @@
+function sum(xs) {
+  var res = 0;
+  for(var x of xs)
+    res += x;
+  return res;
+}

javascript/ql/test/query-tests/Declarations/UnusedVariable/UnusedIndexVariable.js

@@ -0,0 +1,6 @@
+function sum(xs, i) {
+  var res = 0;
+  for(;i++<xs.length;) // NOT OK, but flagged by js/unused-index-variable
+    res += xs[0];
+  return res;
+}