Rework the rule to only emit a dedicated message for arrays of strings

SonarSource · Jan 31, 2024 · 18accf6 · 18accf6
1 parent ecb5770
commit 18accf6
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 139 deletions.
diff --git a/packages/jsts/src/rules/S2871/4531.test.ts b/packages/jsts/src/rules/S2871/4531.test.ts
@@ -0,0 +1,3 @@
+const a = Object.keys({});
+
+a.sort();
diff --git a/packages/jsts/src/rules/S2871/rule.ts b/packages/jsts/src/rules/S2871/rule.ts
@@ -31,8 +31,6 @@ import {
   isStringArray,
   sortLike,
   copyingSortLike,
-  getTypeArguments,
-  isAUnionType,
 } from '../helpers';
 
 const compareNumberFunctionPlaceholder = '(a, b) => (a - b)';
@@ -55,6 +53,8 @@ export const rule: Rule.RuleModule = {
     messages: {
       provideCompareFunction:
         'Provide a compare function to avoid sorting elements alphabetically.',
+      provideCompareFunctionForArrayOfStrings:
+        'Provide a compare function that depends on String.localeCompare, to reliably sort elements alphabetically.',
       suggestNumericOrder: 'Add a comparator function to sort in ascending order',
       suggestLanguageSensitiveOrder:
         'Add a comparator function to sort in ascending language-sensitive order',
@@ -76,22 +76,9 @@ export const rule: Rule.RuleModule = {
         const type = getTypeFromTreeNode(object, services);
 
         if ([...sortLike, ...copyingSortLike].includes(text) && isArrayLikeType(type, services)) {
-          let [typeArgument] = getTypeArguments(type, services);
-
-          if (typeArgument === undefined) {
-            typeArgument = type;
-          }
-
-          const isTypeAString = (candidate: ts.Type): boolean => {
-            return candidate.flags === 4;
-          };
-
-          const types = isAUnionType(typeArgument) ? typeArgument.types : [typeArgument];
-
-          if (!types.every(isTypeAString)) {
-            const suggest = getSuggestions(call, type);
-            context.report({ node, suggest, messageId: 'provideCompareFunction' });
-          }
+          const suggest = getSuggestions(call, type);
+          const messageId = getMessageId(type);
+          context.report({ node, suggest, messageId });
         }
       },
     };
@@ -117,6 +104,14 @@ export const rule: Rule.RuleModule = {
       return suggestions;
     }
 
+    function getMessageId(type: ts.Type) {
+      if (isStringArray(type, services)) {
+        return 'provideCompareFunctionForArrayOfStrings';
+      }
+
+      return 'provideCompareFunction';
+    }
+
     function fixer(call: estree.CallExpression, ...placeholder: string[]): Rule.ReportFixer {
       const closingParenthesis = sourceCode.getLastToken(call, token => token.value === ')')!;
       const indent = ' '.repeat(call.loc?.start.column!);

diff --git a/packages/jsts/src/rules/S2871/unit.test.ts b/packages/jsts/src/rules/S2871/unit.test.ts
@@ -145,50 +145,6 @@ ruleTester.run(`A compare function should be provided when using "Array.prototyp
     {
       code: `Array.prototype.sort.apply([1, 2, 10])`,
     },
-    {
-      code: 'const array = ["foo", "bar"]; array.sort();',
-    },
-    // optional chain
-    {
-      code: `
-        function f(a: string[]) {
-          a?.sort();
-        }
-      `,
-    },
-    {
-      code: `
-        ['foo', 'bar', 'baz'].sort();
-      `,
-    },
-    {
-      code: `
-        function getString() {
-          return 'foo';
-        }
-        [getString(), getString()].sort();
-      `,
-    },
-    {
-      code: `
-        const foo = 'foo';
-        const bar = 'bar';
-        const baz = 'baz';
-        [foo, bar, baz].sort();
-      `,
-    },
-    {
-      code: `
-const names: Array<string> = [];
-names.sort();
-`,
-    },
-    {
-      code: `
-const names: Array<string | 'foo'> = [];
-names.sort();
-`,
-    },
   ],
   invalid: [
     {
@@ -331,18 +287,50 @@ names.sort();
       `,
       errors: [{ suggestions: [] }],
     },
+    {
+      code: 'const array = ["foo", "bar"]; array.sort();',
+      errors: [
+        {
+          suggestions: [
+            {
+              desc: 'Add a comparator function to sort in ascending language-sensitive order',
+              output: 'const array = ["foo", "bar"]; array.sort((a, b) => a.localeCompare(b));',
+            },
+          ],
+        },
+      ],
+    },
+    // optional chain
     {
       code: `
-const names: Array<number> = [];
-names.sort();
-`,
+        function f(a: string[]) {
+          a?.sort();
+        }
+      `,
       errors: 1,
     },
     {
       code: `
-const names: Array<string | number> = [];
-names.sort();
-`,
+        ['foo', 'bar', 'baz'].sort();
+      `,
+      errors: 1,
+    },
+    {
+      code: `
+        function getString() {
+          return 'foo';
+        }
+        [getString(), getString()].sort();
+      `,
+      errors: 1,
+    },
+    {
+      code: `
+        const foo = 'foo';
+        const bar = 'bar';
+        const baz = 'baz';
+        [foo, bar, baz].sort();
+      `,
       errors: 1,
     },
   ],
@@ -469,50 +457,6 @@ ruleTester.run(
       {
         code: `const sorted = Array.prototype.toSorted.apply([1, 2, 10])`,
       },
-      {
-        code: 'const array = ["foo", "bar"]; const sortedArray = array.toSorted();',
-      },
-      // optional chain
-      {
-        code: `
-        function f(a: string[]) {
-          return a?.toSorted();
-        }
-      `,
-      },
-      {
-        code: `
-        const sorted = ['foo', 'bar', 'baz'].toSorted();
-      `,
-      },
-      {
-        code: `
-        function getString() {
-          return 'foo';
-        }
-        const sorted = [getString(), getString()].toSorted();
-      `,
-      },
-      {
-        code: `
-        const foo = 'foo';
-        const bar = 'bar';
-        const baz = 'baz';
-        const sorted = [foo, bar, baz].toSorted();
-      `,
-      },
-      {
-        code: `
-const names: Array<string> = [];
-names.sort();
-`,
-      },
-      {
-        code: `
-const names: Array<string | 'foo'> = [];
-names.sort();
-`,
-      },
     ],
     invalid: [
       {
@@ -658,18 +602,53 @@ names.sort();
       `,
         errors: [{ suggestions: [] }],
       },
+      {
+        code: 'const array = ["foo", "bar"]; const sortedArray = array.toSorted();',
+        errors: [
+          {
+            message:
+              'Provide a compare function that depends on String.localeCompare, to reliably sort elements alphabetically.',
+            suggestions: [
+              {
+                desc: 'Add a comparator function to sort in ascending language-sensitive order',
+                output:
+                  'const array = ["foo", "bar"]; const sortedArray = array.toSorted((a, b) => a.localeCompare(b));',
+              },
+            ],
+          },
+        ],
+      },
+      // optional chain
       {
         code: `
-const names: Array<number> = [];
-names.sort();
-`,
+        function f(a: string[]) {
+          return a?.toSorted();
+        }
+      `,
         errors: 1,
       },
       {
         code: `
-const names: Array<string | number> = [];
-names.sort();
-`,
+        const sorted = ['foo', 'bar', 'baz'].toSorted();
+      `,
+        errors: 1,
+      },
+      {
+        code: `
+        function getString() {
+          return 'foo';
+        }
+        const sorted = [getString(), getString()].toSorted();
+      `,
+        errors: 1,
+      },
+      {
+        code: `
+        const foo = 'foo';
+        const bar = 'bar';
+        const baz = 'baz';
+        const sorted = [foo, bar, baz].toSorted();
+      `,
         errors: 1,
       },
     ],

diff --git a/packages/jsts/src/rules/helpers/type.ts b/packages/jsts/src/rules/helpers/type.ts
@@ -281,24 +281,3 @@ export function isTypeAlias(node: TSESTree.TypeNode, context: Rule.RuleContext)
   const variable = getVariableFromScope(scope, node.typeName.name);
   return variable?.defs.some(def => def.node.type === 'TSTypeAliasDeclaration');
 }
-
-export function getTypeArguments(
-  type: ts.Type,
-  services: RequiredParserServices,
-): readonly ts.Type[] {
-  const checker = services.program.getTypeChecker();
-  return checker.getTypeArguments(type as ts.TypeReference);
-}
-
-type UnionType = ts.Type & {
-  types: ts.Type[];
-};
-
-/**
- * Type guard that guarantees that the passed candidate is a union and narrows its type to {@link UnionType}.
- *
- * @param candidate
- */
-export function isAUnionType(candidate: ts.Type): candidate is UnionType {
-  return candidate.flags === 1048576;
-}