From 18accf63033d33379e057eba2bb2e4306bab4cef Mon Sep 17 00:00:00 2001 From: Eric MORAND Date: Wed, 31 Jan 2024 14:05:26 +0100 Subject: [PATCH] Rework the rule to only emit a dedicated message for arrays of strings --- packages/jsts/src/rules/S2871/4531.test.ts | 3 + packages/jsts/src/rules/S2871/rule.ts | 31 ++-- packages/jsts/src/rules/S2871/unit.test.ts | 179 +++++++++------------ packages/jsts/src/rules/helpers/type.ts | 21 --- 4 files changed, 95 insertions(+), 139 deletions(-) create mode 100644 packages/jsts/src/rules/S2871/4531.test.ts diff --git a/packages/jsts/src/rules/S2871/4531.test.ts b/packages/jsts/src/rules/S2871/4531.test.ts new file mode 100644 index 00000000000..c0461276ef9 --- /dev/null +++ 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 index 7820cb18551..18800c2625a 100644 --- 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 index 697f5af3876..34285cdb5f6 100644 --- 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 = []; -names.sort(); -`, - }, - { - code: ` -const names: Array = []; -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 = []; -names.sort(); -`, + function f(a: string[]) { + a?.sort(); + } + `, errors: 1, }, { code: ` -const names: Array = []; -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 = []; -names.sort(); -`, - }, - { - code: ` -const names: Array = []; -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 = []; -names.sort(); -`, + function f(a: string[]) { + return a?.toSorted(); + } + `, errors: 1, }, { code: ` -const names: Array = []; -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 index 795cd283347..e00a450553c 100644 --- 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; -}