Skip to content

feat(eslint-plugin): add require-array-sort-compare rule (fixes #247) #261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions packages/eslint-plugin/docs/rules/require-array-sort-compare.md
Original file line number Diff line number Diff line change
@@ -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.<br> > 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.
57 changes: 57 additions & 0 deletions packages/eslint-plugin/src/rules/require-array-sort-compare.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/**
* @fileoverview Enforce giving `compare` argument to `Array#sort`
* @author Toru Nagashima <https://github.com/mysticatea>
*/

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[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);
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: node.parent!, messageId: 'requireCompare' });
return;
}
}
}
};
}
});
127 changes: 127 additions & 0 deletions packages/eslint-plugin/tests/rules/require-array-sort-compare.test.ts
Original file line number Diff line number Diff line change
@@ -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<string>) {
a.sort(undefined)
}
`,
`
function f(a: Array<number>) {
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<T extends A>(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<any>) {
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<T extends number[]>(a: T) {
a.sort()
}
`,
errors: [{ messageId: 'requireCompare' }]
},
{
code: `
function f<T, U extends T[]>(a: U) {
a.sort()
}
`,
errors: [{ messageId: 'requireCompare' }]
}
]
});