JS-1466 Fix S1121 false positives for idiomatic assignment patterns#6643
Conversation
Tests cover scenarios where assignments in conditional expressions are intentional JavaScript idioms: assignments as the direct test of an if statement (e.g. regex matching), assignments as the test condition of a for loop (e.g. array traversal), and assignments in return statements (e.g. memoization). Seven previously-invalid test cases that become valid under the fix are moved to the valid array, and nine new test cases are added to document the idiomatic patterns. Relates to JS-1466
S1121 was raising false positives for three common JavaScript idioms where assignment in an expression is intentional: assignments used directly as the test of an `if` statement (e.g., `if (m = regex.exec(src))`), assignments used as the test condition of a `for` loop (e.g., `for (; node = arr[i]; i++)`), and assignments in `return` statements (e.g., `return (cache[key] = fn())`). Three new guard functions are added to `rule.ts` — `isIfCondition`, `isForCondition`, and `isReturnStatement` — mirroring the existing pattern of `isWhileCondition` and `isForInitOrUpdate`. The rspec exceptions list and code example are updated accordingly. Relates to JS-1466
Ruling analysis confirmed the implementation is correct with zero mismatches across 997 entries: 560 previously-raised issues are now correctly suppressed (idiomatic return, if-test, and for-test assignment patterns), and 437 issues continue to be raised as true positives (assignments in function arguments, ternary branches, nested logical expressions, and unary operators). Enhanced tests with four new cases drawn from ruling patterns: - Valid: compound assignment in return (e.g., `return (carry += value)`) - Valid: node-traversal for-loop test (e.g., `for (var i = 0; node = nodes[i++];)`) - Invalid: assignment wrapped in unary NOT in if condition (`if (!(a = getValue()))`) - Invalid: assignment inside a function call in if condition (`if (isDefined(a = getValue()))`)
SummaryRule S1121 now exempts three idiomatic assignment patterns where assignment in an expression is intentional and readable:
The change eliminates ~560 false positives from the TypeScript/JavaScript ecosystem (confirmed via ruling analysis), while maintaining 437 legitimate violations in patterns like function arguments, ternary branches, and nested logical expressions. What reviewers should knowCode review focus:
Test changes:
Ruling cleanup: The large JSON diffs are expected — ~560 line numbers removed from expected issues across multiple test projects. These represent idiomatic patterns now correctly recognized as non-violations. Spot-check one or two projects if concerned about legitimacy.
|
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
| return parent.type === 'ReturnStatement'; | ||
| } | ||
|
|
||
| function isIfCondition(parent: estree.Node, expr: estree.AssignmentExpression) { |
There was a problem hiding this comment.
Logic duplication: isIfCondition and isForCondition (line 124) both implement parent.type === 'XStatement' && parent.test === expr, which is structurally identical to the existing isWhileCondition. All three functions are now parallel — any future behavioral change to the "direct test condition" exemption (e.g. adding a type guard, or extending to DoWhileStatement) would require updating all three in non-obvious ways.
Merge them into a single helper:
function isDirectTestCondition(parent: estree.Node, expr: estree.AssignmentExpression) {
return (
(parent.type === 'IfStatement' ||
parent.type === 'WhileStatement' ||
parent.type === 'ForStatement') &&
parent.test === expr
);
}Then replace the three call sites with !isDirectTestCondition(parent, assignment) and drop isIfCondition, isWhileCondition, and isForCondition.
- Mark as noise
There was a problem hiding this comment.
Good catch. Merged isIfCondition, isWhileCondition, and isForCondition into a single isDirectTestCondition helper covering IfStatement, WhileStatement, DoWhileStatement, and ForStatement. The three separate call sites are replaced by a single !isDirectTestCondition(parent, assignment).
|
Acknowledged — this is an automated PR summary. No action needed. |
…ules/S1121/rule.ts Comment: **Logic duplication:** `isIfCondition` and `isForCondition` (line 124) both implement `parent.type === 'XStatement' && parent.test === expr`, which is structurally identical to the existing `isWhileCondition`. All three functions are now parallel — any future behavioral change to the "direct test condition" exemption (e.g. adding a type guard, or extending to `DoWhileStatement`) would require updating all three in non-obvious ways. Merge them into a single helper: ```typescript function isDirectTestCondition(parent: estree.Node, expr: estree.AssignmentExpression) { return ( (parent.type === 'IfStatement' || parent.type === 'WhileStatement' || parent.type === 'ForStatement') && parent.test === expr ); } ``` Then replace the three call sites with `!isDirectTestCondition(parent, assignment)` and drop `isIfCondition`, `isWhileCondition`, and `isForCondition`. - [ ] Mark as noise
|
Thank you for the review! |
s1121-assignment-in-conditional-expressions-for-idiomatic-patterns-sonnet
|
Minor observation: two test cases were removed from Before this PR, |
Comment: Minor observation: two test cases were removed from `invalid` but not added to `valid`, leaving a small documentation gap.
Before this PR, `if (a = b = 0) {}` and `for (; i = j = 0;);` both produced 1 error each. After the fix they produce 0 errors (outer assignment suppressed by `isIfCondition`/`isForCondition`, inner chained assignment by the existing `isEnclosingChain`). Since these are genuinely valid patterns under the new rule, consider adding them to the `valid` array so the test suite explicitly documents the expected behavior.
|
Thanks for the review and approval! |
|
Good catch. Added both |
|
|
Thank you for the review! |
1 participant
Fixes false positives in rule S1121 (assignment in conditional expressions) for three common JavaScript idioms where assignment in an expression is intentional.
Changes
isIfCondition,isForCondition, andisReturnStatementtorule.tsto exempt idiomatic assignment patterns from the rule, mirroring the existingisWhileConditionandisForInitOrUpdateguards.Idiomatic patterns now allowed
if (m = regex.exec(src))— assignment as the direct test of anifstatementfor (; node = arr[i]; i++)— assignment as the test condition of aforloopreturn (cache[key] = fn())— assignment in areturnstatement (e.g. memoization)Ruling validation
Ruling analysis confirmed zero mismatches across 997 entries: 560 previously-raised issues are now correctly suppressed (idiomatic return, if-test, and for-test patterns), and 437 issues continue to be raised as true positives (assignments in function arguments, ternary branches, nested logical expressions, and unary operators).
Fixes JS-1466
Proposed rspec changes