Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jun 20, 2025
+56 −2
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7292a76
JS: add test cases for false positives in `loop-iteration-skipped-due…
Napalys Jun 12, 2025
66d66fe
JS: fix false positives for splice with conditional index decrement
Napalys Jun 12, 2025
885e836
JS: add `quality` and `reliability` tags to `loop-iteration-skipped-d…
Napalys Jun 12, 2025
10d1028
JS: add change notes
Napalys Jun 12, 2025
53cae4f
Merge remote-tracking branch 'origin/main' into js/quality/loop_shift
Napalys Jun 19, 2025
88f6687
Updated extended expected file after merge
Napalys Jun 19, 2025
5448071
Update javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md
Napalys Jun 19, 2025
8679151
Update javascript/ql/src/change-notes/2025-06-12-loop-iteration.md
Napalys Jun 19, 2025
bca536c
Merge remote-tracking branch 'origin/main' into js/quality/loop_shift
Napalys Jun 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions javascript/ql/integration-tests/query-suite/javascript-code-quality-extended.qls.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 9 additions & 2 deletions javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/

Expand Down Expand Up @@ -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
)
}
}

Expand Down
4 changes: 4 additions & 0 deletions javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
4 changes: 4 additions & 0 deletions javascript/ql/src/change-notes/2025-06-12-loop-iteration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The `js/loop-iteration-skipped-due-to-shifting` query now has the `reliability` tag.
2 changes: 2 additions & 0 deletions ...s/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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. |
35 changes: 35 additions & 0 deletions javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this code is safe (i.e. why the alert is spurious). As far as I can tell, splice doesn't raise an exception, so it seems to me that in some cases i can get decremented without actually removing any items.

Copy link
Contributor Author

@Napalys Napalys Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that it could be a true positive!
This is a situation where the autofix tool correctly refuses to provide a fix because the alert is a false positive. In this specific code, there is nothing that could throw an exception between the call to splice and the decrement of i. As a result, there is no risk of accidentally skipping iterations. However, if any code that could potentially throw an exception were added between the splice operation and the decrement of i, then the alert would be justified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. So is this a false positive or not? Or put differently: what is the value in marking it as "spurious"?

Copy link
Contributor Author

@Napalys Napalys Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, in this case it really is a false positive, the loop won’t skip anything, since there’s no way an exception could happen between splice and i -= 1.

The reason I marked it as spurious is because it is a false positive and I wanted to expand our test coverage a bit. Plus, if we ever change something elsewhere and this test suddenly failing, showing something like “FIXED SPURIOUS ALERT!”, which then proves that their changes had a positive effect?

Maybe it would of been better if I have added one more test case?

function withTryCatch2(pendingCSS) {
  for (let i = 0; i < pendingCSS.length; ++i) {
      try {
          pendingCSS.splice(i, 1); // $Alert
          throw new Error("Something went wrong!");
          i -= 1;
      } catch (ex) {}
  }
}

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--;
}
}
}
}