JS: fix false positives for splice with conditional index decrement

Author: Napalys

javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql

@@ -146,7 +146,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/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected

@@ -2,5 +2,4 @@
 | 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:136:32:136:47 | toc.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. |
-| tst.js:143:10:143:25 | toc.splice(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

@@ -133,16 +133,26 @@ function withTryCatch(pendingCSS) {
 
 function andOperand(toc) {
   for (let i = 0; i < toc.length; i++) {
-    toc[i].ignoreSubHeading && toc.splice(i, 1) && i--; // $ SPURIOUS:Alert
+    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)){ // $ SPURIOUS:Alert
+      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--;
+      }
+    }
+  }
+}