From 0f929ec836e6a1a55e4b32f6ad7ebddc905dbbcb Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Tue, 12 Feb 2019 23:00:15 +0900 Subject: [PATCH 1/2] feat(eslint-plugin): add require-array-sort-compare rule (fixes #247) --- .vscode/settings.json | 5 +- .../docs/rules/require-array-sort-compare.md | 50 +++++++ .../src/rules/require-array-sort-compare.ts | 66 +++++++++ .../rules/require-array-sort-compare.test.ts | 127 ++++++++++++++++++ 4 files changed, 247 insertions(+), 1 deletion(-) create mode 100644 packages/eslint-plugin/docs/rules/require-array-sort-compare.md create mode 100644 packages/eslint-plugin/src/rules/require-array-sort-compare.ts create mode 100644 packages/eslint-plugin/tests/rules/require-array-sort-compare.test.ts diff --git a/.vscode/settings.json b/.vscode/settings.json index adcf2422e8f3..f726cb624e11 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -22,5 +22,8 @@ "javascript.preferences.importModuleSpecifier": "auto", "typescript.preferences.importModuleSpecifier": "auto", "javascript.preferences.quoteStyle": "single", - "typescript.preferences.quoteStyle": "single" + "typescript.preferences.quoteStyle": "single", + + "editor.formatOnSave": true, + "editor.formatOnPaste": false } diff --git a/packages/eslint-plugin/docs/rules/require-array-sort-compare.md b/packages/eslint-plugin/docs/rules/require-array-sort-compare.md new file mode 100644 index 000000000000..ead78b3c3d2e --- /dev/null +++ b/packages/eslint-plugin/docs/rules/require-array-sort-compare.md @@ -0,0 +1,50 @@ +# Enforce giving `compare` argument to `Array#sort` (require-array-sort-compare) + +This rule prevents to invoke `Array#sort()` method without `compare` argument. + +`Array#sort()` method sorts that element by the alphabet order. + +```ts +[1, 2, 3, 10, 20, 30].sort(); //→ [1, 10, 2, 20, 3, 30] +``` + +The language specification also noted this trap. + +> NOTE 2: Method calls performed by the ToString abstract operations in steps 5 and 7 have the potential to cause SortCompare to not behave as a consistent comparison function.
> https://www.ecma-international.org/ecma-262/9.0/#sec-sortcompare + +## Rule Details + +This rule is aimed at preventing the calls of `Array#sort` method. +This rule ignores the `sort` methods of user-defined types. + +Examples of **incorrect** code for this rule: + +```ts +const array: any[]; +const stringArray: string[]; + +array.sort(); + +// Even if a string array, warns it in favor of `String#localeCompare` method. +stringArray.sort(); +``` + +Examples of **correct** code for this rule: + +```ts +const array: any[]; +const userDefinedType: { sort(): void }; + +array.sort((a, b) => a - b); +array.sort((a, b) => a.localeCompare(b)); + +userDefinedType.sort(); +``` + +### Options + +There is no option. + +## When Not To Use It + +If you understand the language specification enough, you can turn this rule off safely. diff --git a/packages/eslint-plugin/src/rules/require-array-sort-compare.ts b/packages/eslint-plugin/src/rules/require-array-sort-compare.ts new file mode 100644 index 000000000000..4942df9bc8a7 --- /dev/null +++ b/packages/eslint-plugin/src/rules/require-array-sort-compare.ts @@ -0,0 +1,66 @@ +/** + * @fileoverview Enforce giving `compare` argument to `Array#sort` + * @author Toru Nagashima + */ + +import * as ts from 'typescript'; +import { TSESTree } from '@typescript-eslint/typescript-estree'; +import * as util from '../util'; + +export default util.createRule({ + name: 'require-array-sort-compare', + defaultOptions: [], + + meta: { + type: 'problem', + docs: { + description: 'Enforce giving `compare` argument to `Array#sort`', + category: 'Best Practices', + recommended: false + }, + messages: { + requireCompare: "Require 'compare' argument." + }, + schema: [] + }, + + create(context) { + const service = util.getParserServices(context); + const checker = service.program.getTypeChecker(); + + return { + CallExpression(node: TSESTree.CallExpression) { + // Skip if this is not a call of `sort` method or giving arguments. + if ( + node.callee.type !== 'MemberExpression' || + node.callee.computed || + node.callee.property.type !== 'Identifier' || + node.callee.property.name !== 'sort' || + node.arguments.length >= 1 + ) { + return; + } + + // Get the symbol of the `sort` method. + const tsNode = service.esTreeNodeToTSNodeMap.get(node.callee); + const sortSymbol = checker.getSymbolAtLocation(tsNode); + if (sortSymbol == null) { + return; + } + + // Check the owner type of the `sort` method. + for (const methodDecl of sortSymbol.declarations) { + const typeDecl = methodDecl.parent; + if ( + ts.isInterfaceDeclaration(typeDecl) && + ts.isSourceFile(typeDecl.parent) && + typeDecl.name.escapedText === 'Array' + ) { + context.report({ node, messageId: 'requireCompare' }); + return; + } + } + } + }; + } +}); diff --git a/packages/eslint-plugin/tests/rules/require-array-sort-compare.test.ts b/packages/eslint-plugin/tests/rules/require-array-sort-compare.test.ts new file mode 100644 index 000000000000..7614ab6c06a6 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/require-array-sort-compare.test.ts @@ -0,0 +1,127 @@ +import path from 'path'; +import rule from '../../src/rules/require-array-sort-compare'; +import { RuleTester } from '../RuleTester'; + +const rootPath = path.join(process.cwd(), 'tests/fixtures/'); + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + tsconfigRootDir: rootPath, + project: './tsconfig.json' + } +}); + +ruleTester.run('require-array-sort-compare', rule, { + valid: [ + ` + function f(a: any[]) { + a.sort(undefined) + } + `, + ` + function f(a: any[]) { + a.sort((a, b) => a - b) + } + `, + ` + function f(a: Array) { + a.sort(undefined) + } + `, + ` + function f(a: Array) { + a.sort((a, b) => a - b) + } + `, + ` + function f(a: { sort(): void }) { + a.sort() + } + `, + ` + class A { sort(): void {} } + function f(a: A) { + a.sort() + } + `, + ` + interface A { sort(): void } + function f(a: A) { + a.sort() + } + `, + ` + interface A { sort(): void } + function f(a: T) { + a.sort() + } + `, + ` + function f(a: any) { + a.sort() + } + `, + ` + namespace UserDefined { + interface Array { + sort(): void + } + function f(a: Array) { + a.sort() + } + } + ` + ], + invalid: [ + { + code: ` + function f(a: Array) { + a.sort() + } + `, + errors: [{ messageId: 'requireCompare' }] + }, + { + code: ` + function f(a: string[]) { + a.sort() + } + `, + errors: [{ messageId: 'requireCompare' }] + }, + { + code: ` + function f(a: string | string[]) { + if (Array.isArray(a)) + a.sort() + } + `, + errors: [{ messageId: 'requireCompare' }] + }, + { + code: ` + function f(a: number[] | string[]) { + a.sort() + } + `, + errors: [{ messageId: 'requireCompare' }] + }, + { + code: ` + function f(a: T) { + a.sort() + } + `, + errors: [{ messageId: 'requireCompare' }] + }, + { + code: ` + function f(a: U) { + a.sort() + } + `, + errors: [{ messageId: 'requireCompare' }] + } + ] +}); From 05f9910ed3c307c344b32ece3cef612011a56df2 Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Wed, 13 Feb 2019 16:10:19 +0900 Subject: [PATCH 2/2] fix for suggested changes --- .vscode/settings.json | 5 +---- .../src/rules/require-array-sort-compare.ts | 19 +++++-------------- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index f726cb624e11..adcf2422e8f3 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -22,8 +22,5 @@ "javascript.preferences.importModuleSpecifier": "auto", "typescript.preferences.importModuleSpecifier": "auto", "javascript.preferences.quoteStyle": "single", - "typescript.preferences.quoteStyle": "single", - - "editor.formatOnSave": true, - "editor.formatOnPaste": false + "typescript.preferences.quoteStyle": "single" } diff --git a/packages/eslint-plugin/src/rules/require-array-sort-compare.ts b/packages/eslint-plugin/src/rules/require-array-sort-compare.ts index 4942df9bc8a7..c1aeeb50d130 100644 --- a/packages/eslint-plugin/src/rules/require-array-sort-compare.ts +++ b/packages/eslint-plugin/src/rules/require-array-sort-compare.ts @@ -29,20 +29,11 @@ export default util.createRule({ const checker = service.program.getTypeChecker(); return { - CallExpression(node: TSESTree.CallExpression) { - // Skip if this is not a call of `sort` method or giving arguments. - if ( - node.callee.type !== 'MemberExpression' || - node.callee.computed || - node.callee.property.type !== 'Identifier' || - node.callee.property.name !== 'sort' || - node.arguments.length >= 1 - ) { - return; - } - + "CallExpression[arguments.length=0] > MemberExpression[property.name='sort'][computed=false]"( + node: TSESTree.MemberExpression + ) { // Get the symbol of the `sort` method. - const tsNode = service.esTreeNodeToTSNodeMap.get(node.callee); + const tsNode = service.esTreeNodeToTSNodeMap.get(node); const sortSymbol = checker.getSymbolAtLocation(tsNode); if (sortSymbol == null) { return; @@ -56,7 +47,7 @@ export default util.createRule({ ts.isSourceFile(typeDecl.parent) && typeDecl.name.escapedText === 'Array' ) { - context.report({ node, messageId: 'requireCompare' }); + context.report({ node: node.parent!, messageId: 'requireCompare' }); return; } }