JS-1421 Fix FP in S2234: suppress intentional comparator argument reversal#6590
Conversation
Tests cover the comparator-reversal pattern where an enclosing 2-parameter function's sole body is a call with its parameters in reversed order (e.g. `(a, b) => compare(b, a)`). This is a legitimate descending sort comparator, not a bug. Three valid cases are added: arrow function expression body, function expression with sole return statement, and inline sort callback. Two invalid boundary cases verify the exception is narrow: a 3-parameter wrapper and a block body with extra statements. Relates to JS-1421
S2234 was incorrectly flagging the common pattern of a 2-parameter wrapper function that intentionally reverses its arguments to produce a descending comparator, e.g. `(a, b) => compare(b, a)`. Added `isIntentionalComparatorReversal()` inside the `create` closure in rule.ts. It is called before `raiseIssue` in `checkArguments`. The check finds the immediately enclosing ArrowFunctionExpression or FunctionExpression, confirms it has exactly 2 identifier parameters whose names match the two swapped arguments, and confirms the call is the direct expression body or the sole return statement of that function. If all conditions hold, the issue is suppressed. Also updated rspec to document the new exception as a compliant code block under diff-id=1, following the same style as the existing if-statement exception. Relates to JS-1421
Ticket: JS-1421 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Remove three unnecessary type assertions (typescript:S4325) from the isIntentionalComparatorReversal() function in rule.ts. TypeScript 5.5+ infers these types via inferred type predicates and discriminated union narrowing, making the explicit `as` casts redundant: - Removed `as ArrowFunctionExpression | FunctionExpression | undefined` from the .find() result (inferred type predicates in Array.prototype.find) - Removed `as Expression | BlockStatement` from enclosingFunc.body (inferred from the narrowed ArrowFunctionExpression | FunctionExpression union) - Removed `as ReturnStatement` inside the if-condition (TypeScript narrows body.body[0] after the .type === 'ReturnStatement' discriminant check)
Ruling Report✅ No changes to ruling expected issues in this PR |
|
The two early-exit guards in if (!enclosingFunc || enclosingFunc.params.length !== 2) {
return false;
} |
Comment: The two early-exit guards in `isIntentionalComparatorReversal()` can be merged into one:
```ts
if (!enclosingFunc || enclosingFunc.params.length !== 2) {
return false;
}
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Done, merged the two guards into one: if (!enclosingFunc || enclosingFunc.params.length !== 2) {
return false;
} |
README Freshness Check❌ The rules README is out of date. A fix PR has been created: #6622 Please review and merge it into your branch. |
|
Thank you for the review! |
Address external SonarQube issue typescript:S6582 in rule.ts line 156. Replace the `!enclosingFunc || enclosingFunc.params.length !== 2` pattern with the more concise optional chain expression `enclosingFunc?.params.length !== 2` in `isIntentionalComparatorReversal()`. The semantics are identical: the condition is truthy (triggering early return) when the enclosing function is absent or does not have exactly 2 parameters.
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
…ules/S2234/rule.ts Comment: The function is named `isIntentionalComparatorReversal`, but it suppresses **any** 2-param wrapper that solely calls another function with reversed args — it has no way to know whether the wrapped function is actually a comparator. This means a genuine bug like: ```js function formatDate(year, month) { return `${year}-${month}`; } const wrongWrapper = (year, month) => formatDate(month, year); // ← real bug, now silenced ``` …will be silently suppressed. S2234 would correctly flag this today; after this change it won't. Is this tradeoff intentional? If the accepted position is "a sole-reversal wrapper is *always* intentional regardless of the callee", that should be documented in the function comment. If the intent really is to limit suppression to comparator-style functions, a narrower heuristic is needed — for example, only suppress when the enclosing function is passed directly as an argument to a known sort-accepting method (`.sort()`, `.toSorted()`, etc.). - [ ] Mark as noise
|
Thanks for the summary. The design concern you raise was already flagged in the previous round of review, and the accepted tradeoff is now explicitly documented in the JSDoc on
The ruling data (8 FPs across kibana, JupyterLab, ESLint, Blueprint, semver) shows that the pattern arises broadly beyond |
|
Thanks for the LGTM! ✅ |
|
|
Thank you for the review! |
1 participant
S2234 was incorrectly flagging the common pattern of a 2-parameter wrapper function that intentionally reverses its arguments to produce a descending comparator, e.g.
(a, b) => compare(b, a).Changes
isIntentionalComparatorReversal()inrule.tsthat detects when a call site is the direct body or sole return of a 2-parameter enclosing function with its parameters swapped. If matched, the issue is suppressed.astype assertions fromisIntentionalComparatorReversal()that TypeScript 5.5+ infers automatically via type predicates and discriminated union narrowing.Relates to JS-1421
Proposed rspec changes