Skip to content

JS-1121 Fix FP on S2310: Allow intentional loop counter skip-ahead patterns#6336

Merged
ss-vibe-bot[bot] merged 6 commits intomasterfrom
fix/JS-1121-fix-fp-on-s2310-intentional-loop-counter-skip-ahead-in-parsing-code-opus
Feb 6, 2026
Merged

JS-1121 Fix FP on S2310: Allow intentional loop counter skip-ahead patterns#6336
ss-vibe-bot[bot] merged 6 commits intomasterfrom
fix/JS-1121-fix-fp-on-s2310-intentional-loop-counter-skip-ahead-in-parsing-code-opus

Conversation

@ss-vibe-bot
Copy link
Contributor

@ss-vibe-bot ss-vibe-bot bot commented Feb 5, 2026

Summary

Fixes false positives in S2310 where intentional loop counter modifications in parsing code were incorrectly flagged. The rule now distinguishes between legitimate skip-ahead patterns and assignments that should use break.

Changes

  • Allow loop counter modifications via UpdateExpression (i++, i--, ++i, --i) and compound assignment operators (i += n, i -= n) which are common skip-ahead patterns in parsing code
  • Continue flagging simple assignments (i = value) which typically indicate early-exit patterns that should use break
  • Add special case handling to still report modifications in nested for-loop update clauses

Test Coverage

Added test cases covering:

  • Pre/post increment/decrement patterns for various use cases (tuple consumption, escape character skipping, surrogate pair processing)
  • Compound assignment for batch skip-ahead processing
  • Reverse iteration patterns with decrement
  • Array manipulation with splice and index modification
  • 6 additional test cases from ruling analysis covering real-world patterns

All 68 ruling entries analyzed with no mismatches between expected and actual behavior.

Fixes JS-1121

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2026

Ruling Report

Code no longer flagged (53 issues)

S2310

Joust/ts/components/EventLog.tsx:72

    70 | 						entityId: entity.entityId, targetId: target.entityId
    71 | 					});
>   72 | 					i++;
    73 | 				}
    74 | 				else if (curr.entityId == next.entityId && (curr.type == LineType.AttackBuff && next.type == LineType.HealthBuff

Joust/ts/components/EventLog.tsx:82

    80 | 						data2: curr.type == LineType.AttackBuff ? next.data : curr.data
    81 | 					});
>   82 | 					i++;
    83 | 				}
    84 | 				else {

TypeScript/lib/tsc.js:18167

  18165 |                 while (!clauses[i].statements.length && i + 1 < clauses.length) {
  18166 |                     bind(clauses[i]);
> 18167 |                     i++;
  18168 |                 }
  18169 |                 var preCaseLabel = createBranchLabel();

TypeScript/lib/tsc.js:22894

  22892 |                         if (i + count <= types.length && types[i + count - 1] === baseType.types[count - 1]) {
  22893 |                             result.push(baseType);
> 22894 |                             i += count - 1;
  22895 |                             continue;
  22896 |                         }

TypeScript/src/compiler/binder.ts:1145

  1143 |                 while (!clauses[i].statements.length && i + 1 < clauses.length) {
  1144 |                     bind(clauses[i]);
> 1145 |                     i++;
  1146 |                 }
  1147 |                 const preCaseLabel = createBranchLabel();

TypeScript/src/compiler/checker.ts:2890

  2888 |                         if (i + count <= types.length && types[i + count - 1] === baseType.types[count - 1]) {
  2889 |                             result.push(baseType);
> 2890 |                             i += count - 1;
  2891 |                             continue;
  2892 |                         }

TypeScript/src/services/documentHighlights.ts:583

   581 |                         kind: HighlightSpanKind.reference
   582 |                     });
>  583 |                     i++; // skip the next keyword
   584 |                     continue;
   585 |                 }

ace/lib/ace/mode/html/saxparser.js:820

   818 | 		for (var i = 1; i < token.data.length; i++) {
   819 | 			if (!token.data[i].nodeName)
>  820 | 				token.data.splice(i--, 1);
   821 | 		}
   822 | 	} else if (token.type === 'EndTag') {

ace/lib/ace/mode/yaml/yaml-lint.js:623

   621 |         result += encodeHex((char - 0xD800) * 0x400 + nextChar - 0xDC00 + 0x10000);
   622 |         // Advance index one extra since we already used that char here.
>  623 |         i++; continue;
   624 |       }
   625 |     }

ace/src/editor.js:1880

  1878 |                 var first = rows.first;
  1879 |                 var last = rows.last;
> 1880 |                 while (++i < l) {
  1881 |                     if (totalDiff) ranges[i].moveBy(totalDiff, 0);
  1882 |                     var subRows = this.$getSelectedRows(ranges[i]);

...and 43 more (see ruling JSON files for full list)

@ss-vibe-bot
Copy link
Contributor Author

ss-vibe-bot bot commented Feb 5, 2026

github-actions[bot] 2026-02-05T15:41:33Z addressed
These 53 issues are no longer flagged because they all use UpdateExpression (i++, i--, ++i, --i) or compound assignment (i += count - 1) patterns for intentional skip-ahead in parsing/iteration code. This is the expected behavior per the approved approach.

Looking at the examples:

  • i++ for skipping already-processed elements (EventLog.tsx, tsc.js, binder.ts, documentHighlights.ts, yaml-lint.js)
  • i += count - 1 for batch skip-ahead (checker.ts)
  • i-- in splice operations for array removal (saxparser.js)
  • ++i in while conditions controlling loop advancement (editor.js)

The implementation correctly distinguishes these legitimate patterns from simple assignments (i = value) which are still flagged as potential early-exit patterns that should use break.

@ss-vibe-bot ss-vibe-bot bot marked this pull request as ready for review February 5, 2026 16:08
@ss-vibe-bot ss-vibe-bot bot force-pushed the fix/JS-1121-fix-fp-on-s2310-intentional-loop-counter-skip-ahead-in-parsing-code-opus branch 2 times, most recently from e38c0cc to 4e28cc0 Compare February 6, 2026 15:33
@francois-mora-sonarsource francois-mora-sonarsource force-pushed the fix/JS-1121-fix-fp-on-s2310-intentional-loop-counter-skip-ahead-in-parsing-code-opus branch from 4e28cc0 to 0597d43 Compare February 6, 2026 15:49
@ss-vibe-bot ss-vibe-bot bot force-pushed the fix/JS-1121-fix-fp-on-s2310-intentional-loop-counter-skip-ahead-in-parsing-code-opus branch 2 times, most recently from 927f16f to 2f31159 Compare February 6, 2026 16:33
@francois-mora-sonarsource
Copy link
Contributor

The PR is missing unit tests for the nested loop case.

@ss-vibe-bot
Copy link
Contributor Author

ss-vibe-bot bot commented Feb 6, 2026

francois-mora-sonarsource 2026-02-06T16:42:29Z addressed
Good catch. I've added 4 dedicated nested loop test cases:

Valid (compliant):

  • i++ on outer counter in nested for-loop body (intentional skip-ahead)
  • i += batchSize - 1 on outer counter after nested for-loop (compound assignment skip-ahead)

Invalid (noncompliant):

  • i += 2 in nested for-loop's update clause (flagged even though compound assignments are normally allowed — the nested update clause exception takes precedence)
  • i = j simple assignment to outer counter in nested loop body (flagged as expected)

These cover the isInNestedForLoopUpdate logic path and verify the algorithm's step 4: "For writes in nested for-loop update clauses that modify the outer counter: Continue to report." All tests pass.

@ss-vibe-bot ss-vibe-bot bot force-pushed the fix/JS-1121-fix-fp-on-s2310-intentional-loop-counter-skip-ahead-in-parsing-code-opus branch from a4bf617 to f769c09 Compare February 6, 2026 17:03
Vibe Bot and others added 6 commits February 6, 2026 18:03
Tests cover the scenario where loop counter modifications via
UpdateExpression (i++, i--, ++i, --i) and compound assignments
(i += n, i -= n) are intentional skip-ahead patterns in parsing code.

Test cases added:
- Pre-increment for tuple consumption from flat arrays
- Post-increment for escape character skipping in string parsing
- Compound assignment for batch skip-ahead processing
- Decrement patterns (i--, --i, i -= n) for reverse iteration
- Updated invalid test to expect only simple assignments (i = value)
  to be flagged

Relates to JS-1121
Allow intentional loop counter modifications via UpdateExpression (i++, i--,
++i, --i) and compound assignment operators (i+=n, i-=n) which are legitimate
skip-ahead patterns in parsing code. Continue flagging simple assignments
(i = value) which typically indicate early-exit patterns that should use break.

The fix adds a check for the parent node type of write references and skips
reporting when the counter is modified via increment/decrement or compound
assignment. A special case ensures that modifications in nested for-loop
update clauses are still reported.

Implementation follows the approved proposal algorithm and fixes ~80% of
identified false positives while preserving true positive detection.

Test cases use pure JavaScript syntax for compatibility with the default
parser (espree) used by DefaultParserRuleTester.

Relates to JS-1121
Added 6 new test cases based on real-world patterns found during ruling
analysis. These tests verify that the implementation correctly allows
intentional loop counter modifications via UpdateExpression and compound
assignment operators:

- Splice with post-decrement for array removal (i--)
- Inline decrement in splice arguments (splice(i--, 1))
- Pre-increment in while condition controlling outer for-loop
- Post-increment in array index assignment (arr[index++])
- Surrogate pair processing with post-increment skip
- Coordinate pair processing with pre-increment (coords[++i])

All 68 ruling entries analyzed; no mismatches found between expected
and actual behavior. The implementation correctly distinguishes between
simple assignments (which are flagged) and UpdateExpression/compound
assignments (which are allowed).
Ticket: JS-1121

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Reduce cognitive complexity (S3776) and nesting depth (S134) in the
checkCounter function by extracting the skip-ahead detection logic into
a standalone isIntentionalSkipAhead helper function. This moves the
UpdateExpression and compound assignment checks out of deeply nested
control flow, lowering checkCounter's cognitive complexity from 17 to ~8
(allowed: 15) and eliminating nesting beyond 3 levels. No behavioral
changes; all existing tests pass.
Comment: The PR is missing unit tests for the nested loop case.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@ss-vibe-bot ss-vibe-bot bot force-pushed the fix/JS-1121-fix-fp-on-s2310-intentional-loop-counter-skip-ahead-in-parsing-code-opus branch from f769c09 to ebf8416 Compare February 6, 2026 18:03
Copy link
Contributor

@francois-mora-sonarsource francois-mora-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqube-next
Copy link

sonarqube-next bot commented Feb 6, 2026

@ss-vibe-bot ss-vibe-bot bot merged commit d19b92e into master Feb 6, 2026
41 checks passed
@ss-vibe-bot ss-vibe-bot bot deleted the fix/JS-1121-fix-fp-on-s2310-intentional-loop-counter-skip-ahead-in-parsing-code-opus branch February 6, 2026 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant