Merge pull request #19743 from Napalys/js/quality/loop_shift

JS: Promote `js/loop-iteration-skipped-due-to-shifting` to the Code Quality suite

Author: Napalys

javascript/ql/integration-tests/query-suite/javascript-code-quality-extended.qls.expected

@@ -88,6 +88,7 @@ ql/javascript/ql/src/Statements/DanglingElse.ql
 ql/javascript/ql/src/Statements/IgnoreArrayResult.ql
 ql/javascript/ql/src/Statements/InconsistentLoopOrientation.ql
 ql/javascript/ql/src/Statements/LabelInCase.ql
+ql/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql
 ql/javascript/ql/src/Statements/MisleadingIndentationAfterControlStmt.ql
 ql/javascript/ql/src/Statements/ReturnAssignsLocal.ql
 ql/javascript/ql/src/Statements/SuspiciousUnusedLoopIterationVariable.ql

javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected

@@ -88,6 +88,7 @@ ql/javascript/ql/src/Statements/DanglingElse.ql
 ql/javascript/ql/src/Statements/IgnoreArrayResult.ql
 ql/javascript/ql/src/Statements/InconsistentLoopOrientation.ql
 ql/javascript/ql/src/Statements/LabelInCase.ql
+ql/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql
 ql/javascript/ql/src/Statements/MisleadingIndentationAfterControlStmt.ql
 ql/javascript/ql/src/Statements/ReturnAssignsLocal.ql
 ql/javascript/ql/src/Statements/SuspiciousUnusedLoopIterationVariable.ql

javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql

@@ -5,7 +5,9 @@
  * @kind problem
  * @problem.severity warning
  * @id js/loop-iteration-skipped-due-to-shifting
- * @tags correctness
+ * @tags quality
+ *       reliability
+ *       correctness
  * @precision high
  */
 
@@ -146,7 +148,12 @@ class ArrayIterationLoop extends ForStmt {
     or
     this.hasPathThrough(splice, cfg.getAPredecessor()) and
     this.getLoopEntry().dominates(cfg.getBasicBlock()) and
-    not this.hasIndexingManipulation(cfg)
+    not this.hasIndexingManipulation(cfg) and
+    // Don't continue through a branch that tests the splice call's return value
+    not exists(ConditionGuardNode guard | cfg = guard |
+      guard.getTest() = splice.asExpr() and
+      guard.getOutcome() = false
+    )
   }
 }
 

javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Fixed false positives in the `js/loop-iteration-skipped-due-to-shifting` query when the return value of `splice` is used to decide whether to adjust the loop counter.

javascript/ql/src/change-notes/2025-06-12-loop-iteration.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `js/loop-iteration-skipped-due-to-shifting` query now has the `reliability` tag.

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

@@ -1,3 +1,5 @@
 | tst.js:4:27:4:44 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. |
 | tst.js:13:29:13:46 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. |
 | tst.js:24:9:24:26 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. |
+| tst.js:128:11:128:33 | pending ... e(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. |
+| tst.js:153:11:153:26 | toc.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. |

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

@@ -121,3 +121,38 @@ function inspectNextElement(string) {
   }
   return parts.join('/');
 }
+
+function withTryCatch(pendingCSS) {
+  for (let i = 0; i < pendingCSS.length; ++i) {
+      try {
+          pendingCSS.splice(i, 1); // $ SPURIOUS:Alert
+          i -= 1;
+      } catch (ex) {}
+  }
+}
+
+function andOperand(toc) {
+  for (let i = 0; i < toc.length; i++) {
+    toc[i].ignoreSubHeading && toc.splice(i, 1) && i--;
+  }
+}
+
+function ifStatement(toc) {
+  for (let i = 0; i < toc.length; i++) {
+    if(toc[i].ignoreSubHeading){
+      if(toc.splice(i, 1)){
+        i--; 
+      }
+    }
+  }
+}
+
+function ifStatement2(toc) {
+  for (let i = 0; i < toc.length; i++) {
+    if(toc[i].ignoreSubHeading){
+      if(!toc.splice(i, 1)){ // $Alert
+        i--;
+      }
+    }
+  }
+}