JS-1300 Fix FP for S2871: default sort for string arrays in specific contexts#6463
Conversation
Ruling ReportCode no longer flagged (45 issues)S2871Ghost/core/test/unit/permissions_spec.js:41 39 | should.exist(actionsMap);
40 |
> 41 | actionsMap.edit.sort().should.eql(['post', 'tag', 'user', 'page'].sort());
42 |
43 | actionsMap.should.equal(permissions.actionsMap);TypeScript/src/compiler/tsc.ts:381 379 |
380 | // We check if the project file list has changed. If so, we just throw away the old program and start fresh.
> 381 | if (!arrayIsEqualTo(newFileNames && newFileNames.sort(), canonicalRootFileNames && canonicalRootFileNames.sort())) {
382 | setCachedProgram(undefined);
383 | startTimerForRecompilation();TypeScript/src/compiler/tsc.ts:381 379 |
380 | // We check if the project file list has changed. If so, we just throw away the old program and start fresh.
> 381 | if (!arrayIsEqualTo(newFileNames && newFileNames.sort(), canonicalRootFileNames && canonicalRootFileNames.sort())) {
382 | setCachedProgram(undefined);
383 | startTimerForRecompilation();TypeScript/src/harness/fourslash.ts:2388 2386 | scriptInfo.updateContent(originalContent);
2387 | }
> 2388 | const sortedExpectedArray = ts.map(expectedTextArray, str => this.normalizeNewlines(str)).sort();
2389 | const sortedActualArray = actualTextArray.sort();
2390 | if (!ts.arrayIsEqualTo(sortedExpectedArray, sortedActualArray)) {TypeScript/src/harness/fourslash.ts:2389 2387 | }
2388 | const sortedExpectedArray = ts.map(expectedTextArray, str => this.normalizeNewlines(str)).sort();
> 2389 | const sortedActualArray = actualTextArray.sort();
2390 | if (!ts.arrayIsEqualTo(sortedExpectedArray, sortedActualArray)) {
2391 | this.raiseError(TypeScript/src/server/editorServices.ts:594 592 |
593 | // We check if the project file list has changed. If so, we update the project.
> 594 | if (!arrayIsEqualTo(currentRootFiles.sort(), newRootFiles.sort())) {
595 | // For configured projects, the change is made outside the tsconfig file, and
596 | // it is not likely to affect the project for other files opened by the client. We canTypeScript/src/server/editorServices.ts:594 592 |
593 | // We check if the project file list has changed. If so, we update the project.
> 594 | if (!arrayIsEqualTo(currentRootFiles.sort(), newRootFiles.sort())) {
595 | // For configured projects, the change is made outside the tsconfig file, and
596 | // it is not likely to affect the project for other files opened by the client. We canTypeScript/src/server/utilities.ts:191 189 |
190 | export function toSortedReadonlyArray(arr: string[]): SortedReadonlyArray<string> {
> 191 | arr.sort();
192 | return <any>arr;
193 | }ace/lib/ace/mode/html/saxparser.js:9325 9323 | return false;
9324 | //the same set of keys (although not necessarily the same order),
> 9325 | ka.sort();
9326 | kb.sort();
9327 | //~~~cheap key testace/lib/ace/mode/html/saxparser.js:9326 9324 | //the same set of keys (although not necessarily the same order),
9325 | ka.sort();
> 9326 | kb.sort();
9327 | //~~~cheap key test
9328 | for (i = ka.length - 1; i >= 0; i--) {...and 35 more New issues flagged (1 issue)S2871vuetify/packages/docs/src/examples/v-treeview/misc-selectable-icons.vue:133 131 |
132 | return acc
> 133 | }, []).sort()
134 | },
135 | },📋 View full reportCode no longer flagged (45)
New issues flagged (1) |
|
The ruling report confirms the fix is working as expected. The 10 issues that are no longer flagged are false positives that have been correctly suppressed by the new |
|
The Suggested fix — a one-word change in - isCallingMethod(node, 1, 'keys')
+ isCallingMethod(node, 1, 'keys', 'getOwnPropertyNames')Don't forget to add the corresponding test cases ( |
|
Good catch! Applied the suggested one-word change in - isCallingMethod(node, 1, 'keys')
+ isCallingMethod(node, 1, 'keys', 'getOwnPropertyNames')Also added test cases for both |
|
|
Thanks for confirming the ruling results look good. |
|
Done — all three parts have been addressed:
|
|
Confirmed — the changes described are already in place: |
|
Thanks for the ruling report. The 10 S2871 false positives no longer flagged (Object.keys().sort(), Array.from(map.keys()).sort(), etc.) are the intended outcome of this fix. The ruling results have already been updated in the branch. |
|
Thanks for confirming the ruling results look correct! |
|
Done — addressed in the 'Address PR comment from francois-mora-sonarsource' commit. The one-word change to |
|
The changes described in this comment have already been applied in the previous commit. No further action needed. |
|
Acknowledged — no action needed. |
|
Thank you! |
|
Acknowledged — all three parts are confirmed in place. |
|
Acknowledged. |
|
Thanks for the ruling report. The 10 S2871 issues no longer flagged are the expected false positives fixed by this PR. The ruling results have already been updated in the latest commit. |
|
Thanks for confirming — glad the ruling results validate the fix. |
|
Done — all three items are already in place from the previous commit:
|
|
All changes described in the comment have already been applied in a prior commit. No further action needed. |
|
Acknowledged — no action needed. |
|
Acknowledged. |
|
Confirmed — all three parts were addressed in the previous commit. |
|
Acknowledged — no action needed. |
|
Acknowledged — no further action needed. |
|
You're welcome! |
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
…s/src/rules/S2871/rule.ts Comment: We might want the AST-based version also in case the type checker is not available. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ules/S2871/rule.ts Comment: `Object.getOwnPropertySymbols` does not belong here. It returns `symbol[]`, not `string[]`. TypeScript correctly infers the return type as `symbol[]`, so `isStringArray` returns false and the call falls through to this check — which then incorrectly suppresses the warning for `Object.getOwnPropertySymbols(obj).sort()`. Sorting symbols without a comparator is just as problematic as sorting numbers. Remove `'getOwnPropertySymbols'` from this array. - [ ] Mark as noise 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ules/S2871/rule.ts Comment: `isArrayFromIterableMethod` is too broad — it matches `.values()` and `.entries()` on any object, including plain arrays. `Array.from(numArr.values()).sort()` (where `numArr: number[]`) is suppressed because the argument is `numArr.values()`, yet TypeScript types the result as `number[]`, so `isStringArray` returns false and execution reaches here. This is exactly the bug S2871 exists to catch — `[0,1,2,10].sort()` gives `[0,1,10,2]`. Restrict the method list to `['keys']` only, or add a check that the receiver is a `Map`/`Set` (not an array), or simply remove `.values()` and `.entries()` from the list. The test comment "always intentional and order-independent" does not justify suppressing a genuine numeric-sort bug. - [ ] Mark as noise 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ules/S2871/rule.ts Comment: `isSpreadIterableMethod` is dead code. The `object` variable is `call.callee.object` — i.e., the receiver of `.sort()`. In ESTree, a `SpreadElement` is not a valid `Expression` and can only appear inside an `ArrayExpression` element list or function argument list, never as the object of a `MemberExpression`. `node.type !== 'SpreadElement'` will always be true, so this function can never return `true` and `isArrayFromKeyOrEntryCall` can never be satisfied via this branch. Remove it. - [ ] Mark as noise 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
s2871-default-sort-for-string-arrays-in-logging-and-data-structures-sonnet
Ticket: JS-1300 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add AST-based suppression for the for-in key array pattern where an empty array is populated exclusively by pushing loop variables from for-in iterations and then sorted. This pattern is semantically equivalent to Object.keys() and represents intentional alphabetical ordering of object property names. The check uses ESLint's scope analysis to verify that: - The sorted identifier is initialized as an empty array - All array mutation references are push() calls inside for-in loops - Plain reads (e.g., return statements) are allowed alongside for-in pushes Test cases added to cover: basic for-in key sort (valid), for-in with return statement (valid), push outside for-in loop (invalid), and non-empty initial array (invalid).
Refactored isForInKeyArray to reduce its cognitive complexity (S3776) from 20 to below 15 by extracting two focused helper functions: findVariableInScope (handles scope chain traversal) and isInsideForIn (handles ForInStatement ancestry check without crossing function boundaries). Each helper now handles a single concern. Applied additional clean-ups flagged by the external scanner: - S121: Added braces to all single-line if statements - S6582: Used optional chain expressions (variable?.defs.length, decl.init?.type) instead of null-guard || chains - S4325: Removed unnecessary type assertions throughout the function (def.node as estree.VariableDeclarator, memberParent as estree.MemberExpression, prop as estree.Identifier, and the call-site cast object as estree.Identifier); TypeScript narrows these correctly after discriminant checks.
…ules/S2871/rule.ts Comment: **Bug:** Write references (reassignments like `arr = someOtherArray`) are silently treated as plain reads here. An `AssignmentExpression` wrapping `arr` has `memberParent.type === 'AssignmentExpression'`, so `memberParent?.type !== 'MemberExpression'` is true and the loop hits `continue`. This means: ```js var arr = []; for (var key in obj) arr.push(key); arr = [1, 2, 3]; // write ref → skipped arr.sort(); // incorrectly suppressed — no issue reported ``` Fix: reject the reference when it is a write (i.e. `ref.isWrite()`) that isn't the initializer: ```ts if (memberParent?.type !== 'MemberExpression' || memberParent.object !== refId) { if (ref.isWrite()) return false; // reassignment invalidates the pattern continue; } ``` - [ ] Mark as noise 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Please tighten Right now the helper suppresses on the shape It would also be good to add a regression test so |
Comment: Please tighten `isArrayFromIterableMethod()` so it only matches actual `Array.from(...)` calls, not any arbitrary `.from(...)` invocation. Right now the helper suppresses on the shape `*.from(x.keys())`, which can create false negatives for unrelated APIs that also expose a `from()` method. I’d suggest keeping the existing `.from(...)` and `x.keys()` checks, but additionally verifying that the receiver is really `Array`, ideally with `getFullyQualifiedName(context, node.callee.object) === 'Array'`, and falling back to `isIdentifier(node.callee.object, 'Array')` if needed. It would also be good to add a regression test so `Array.from(map.keys()).sort()` stays compliant, while something like `Foo.from(map.keys()).sort()` is still reported.
When TypeScript is available, rely on isStringArray() for all type-based suppression instead of running AST-based string-pattern checks upfront. AST-based checks (isArrayFromKeyOrEntryCall, isForInKeyArray) are now confined to the no-type-checker fallback branch. This allows TypeScript to correctly identify Array.from(arr.keys()) on a number[] as number[] (array indices) and report it, rather than suppressing it via the broad AST-based keys() pattern match. When TypeScript is available it covers all the originally targeted patterns (Object.keys, Array.from(map.keys()), for-in key collections) through the unified isStringArray() check. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Yes — restructured the rule to use TypeScript as the primary mechanism when the type checker is available. Previously, the AST-based string-pattern checks (
The concrete fix this enables: Updated the test suite to reflect this: moved |
|
All three bugs have been addressed in subsequent commits:
Additionally, the architecture was refactored so TypeScript type-checker is now the primary suppression mechanism, with AST-based checks ( |
|
Thanks for the review! The for-in heuristic bug (write references/reassignments being silently skipped instead of invalidating the pattern) was already fixed in a prior commit. The current implementation at lines 170–174 of |
|
Thank you for the thorough review! |
|
Good catch. I've tightened I also added two regression tests in the |
|
|
Thanks for the review! |
2 participants
Fixes false positives in rule S2871 where
sort()without a comparator was incorrectly flagged on string arrays in contexts where default alphabetical ordering is clearly intentional.Changes
isSuppressedSort()to check if a sort call should be exempt from the ruleObject.keys()Array.from(map.keys()),Array.from(map.entries()), orArray.from(map.values())sort()solely for order-independent comparison (e.g.,a.sort() === b.sort())isCallingMethodandisIdentifierhelpers for a clean implementationRelates to JS-1300.