Merge pull request #4185 from erik-krogh/unusedArrDestruct

Approved by esbena

Author: codeql-ci

change-notes/1.26/analysis-javascript.md

@@ -28,6 +28,7 @@
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
 | Incomplete URL substring sanitization (`js/incomplete-url-substring-sanitization`) | More results | This query now recognizes additional URLs when the substring check is an inclusion check. |
 | Ambiguous HTML id attribute (`js/duplicate-html-id`) | Results no longer shown | Precision tag reduced to "low". The query is no longer run by default. |
+| Unused loop iteration variable (`js/unused-loop-variable`) | Fewer results | This query no longer flags variables in a destructuring array assignment that are not the last variable in the destructed array. |
 
 
 ## Changes to libraries

javascript/ql/src/Statements/SuspiciousUnusedLoopIterationVariable.ql

@@ -45,11 +45,28 @@ predicate countingLoop(EnhancedForLoop efl) {
   )
 }
 
+/**
+ * Holds if `iter` is a non-last variable of array-destructuring in a for-loop.
+ * For example `foo` or `iter` in `for(const [foo, iter, bar] of array) {..}`.
+ */
+predicate isNonLastDestructedArrayElement(PurelyLocalVariable iter) {
+  exists(ArrayPattern pattern | pattern = any(EnhancedForLoop loop).getLValue() |
+    iter = pattern.getAVariable() and
+    iter =
+      pattern
+          .getElement([0 .. pattern.getSize() - 2])
+          .(BindingPattern)
+          .getABindingVarRef()
+          .getVariable()
+  )
+}
+
 from EnhancedForLoop efl, PurelyLocalVariable iter
 where
   iter = efl.getAnIterationVariable() and
   not exists(SsaExplicitDefinition ssa | ssa.defines(efl.getIteratorExpr(), iter)) and
   exists(ReachableBasicBlock body | body.getANode() = efl.getBody() | body.getASuccessor+() = body) and
   not countingLoop(efl) and
+  not isNonLastDestructedArrayElement(iter) and
   not iter.getName().toLowerCase().regexpMatch("(_|dummy|unused).*")
 select efl.getIterator(), "For loop variable " + iter.getName() + " is not used in the loop body."

javascript/ql/test/query-tests/Statements/SuspiciousUnusedLoopIterationVariable/SuspiciousUnusedLoopIterationVariable.expected

@@ -1 +1,4 @@
 | tst.js:4:7:4:11 | let x | For loop variable x is not used in the loop body. |
+| tst.js:138:6:138:23 | const [key, value] | For loop variable value is not used in the loop body. |
+| tst.js:151:6:151:35 | const [ ...  value] | For loop variable value is not used in the loop body. |
+| tst.js:152:6:152:10 | let i | For loop variable i is not used in the loop body. |

javascript/ql/test/query-tests/Statements/SuspiciousUnusedLoopIterationVariable/tst.js

@@ -133,3 +133,20 @@ function countOccurrencesDead(xs, p) {
 		a;
 	}
 });
+
+// NOT OK
+for (const [key, value] of array) {}
+
+// OK: for array-destructurings we only flag the last element
+for (const [key, value] of array) {
+	console.log(value)
+}
+
+// OK: for array-destructurings we only flag the last element
+for (const [key, key2, key3, value] of array) {
+	console.log(value)
+}
+
+// NOT OK
+for (const [key, key2, key3, value] of array) {}
+for (let i of [1, 2]) {}