JS-1491 Fix S2392 false positive for loop counter variables reused across loops#6646
Draft
sonar-nigel[bot] wants to merge 5 commits intomasterfrom
Conversation
added 4 commits
March 19, 2026 15:16
Tests cover the scenario where the same `var` loop counter variable is reused across multiple sequential for-loops (e.g., for-in then for, or three consecutive for-loops sharing `i`). Because `var` is hoisted to function scope, each loop independently re-declares the variable; the rule incorrectly flags the first declaration as having outside-scope references that are actually covered by sibling loops. The tests verify that sequential for-loop counter reuse is treated as compliant, while references to a loop counter genuinely used after all loops are still flagged. Relates to JS-1491
S2392 incorrectly flagged the first `for (var i ...)` declaration when the same counter variable was reused across multiple sequential for-loops. Because `var` is hoisted to function scope, ESLint treats all declarations as the same variable, making the second loop's references appear "outside" the first loop's binding range. The fix collects AST ranges of other for-loops that also declare the same variable. If every outside reference falls within one of those other-loop ranges, the reuse is intentional and the issue is suppressed. If any reference falls outside all other loops (e.g., `console.log(i)` after the last loop), the issue is still reported. The rspec was also updated: the noncompliant example now shows a genuine violation (single loop with reference after it), and a new compliant example demonstrates the sequential for-loop counter reuse pattern that is now accepted. Relates to JS-1491
Only apply the sequential-loop FP suppression when the current `var` declaration is directly in a for-loop header (init/left position). This prevents over-suppression when a variable is declared as a regular statement inside a for-loop body or other block. Ruling analysis confirmed that nested redeclarations (e.g., `var rule` re-declared inside a nested for-loop body when `rule` is already the outer for-in iterator) are legitimate issues and should continue to be reported. No implementation changes were required in this pass; the ruling entry for `ace:src/mode/text.js:323` was correctly identified as a true positive (shouldRaise=true), resolving the only mismatch from the previous attempt.
Contributor
Refactor rule.ts to address code quality issues reported by SonarQube: - S3776 (Cognitive Complexity): Extracted isForLoopNode, getOtherLoopRanges, and isCoveredByOtherLoops as module-level helper functions, reducing the VariableDeclaration handler's cognitive complexity from 26 to within the allowed limit of 15. - S134 (Deep nesting): The deeply nested for/if loops inside the FP suppression logic (previously at nesting depth 4+) are now in getOtherLoopRanges at depth 1-2, satisfying the max-3-level nesting requirement. - S2966 (Forbidden non-null assertion): Replaced loopNode.range! with a loopRange const using optional chaining plus a truthy guard, and replaced ref.range![0]/[1] with a range const plus an explicit undefined check.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix a false positive in S2392 where
varloop counter variables reused across multiple sequentialfor-loops were incorrectly flagged.Problem
S2392 incorrectly reported the first
for (var i ...)declaration when the same counter variable was reused across multiple sequential for-loops. Becausevaris hoisted to function scope, ESLint treats all declarations as the same variable, making references in subsequent loops appear to be "outside" the first loop's binding range.Changes
console.log(i)after the last loop) are still reported.i), and confirming that genuine violations are still caught.Fixes JS-1491
Proposed rspec changes