diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index ba235a7bc63d..c0523a6da401 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -151,6 +151,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e | [`@typescript-eslint/prefer-namespace-keyword`](./docs/rules/prefer-namespace-keyword.md) | Require the use of the `namespace` keyword instead of the `module` keyword to declare custom TypeScript modules. (`no-internal-module` from TSLint) | :heavy_check_mark: | :wrench: | | [`@typescript-eslint/promise-function-async`](./docs/rules/promise-function-async.md) | Requires any function or method that returns a Promise to be marked async. (`promise-function-async` from TSLint) | :heavy_check_mark: | | | [`@typescript-eslint/restrict-plus-operands`](./docs/rules/restrict-plus-operands.md) | When adding two variables, operands must both be of type number or of type string. (`restrict-plus-operands` from TSLint) | | | +| [`@typescript-eslint/strict-comparisons`](./docs/rules/strict-comparisons.md) | Only allow comparisons between primitive types. | | | | [`@typescript-eslint/type-annotation-spacing`](./docs/rules/type-annotation-spacing.md) | Require consistent spacing around type annotations (`typedef-whitespace` from TSLint) | :heavy_check_mark: | :wrench: | | [`@typescript-eslint/unified-signatures`](./docs/rules/unified-signatures.md) | Warns for any two overloads that could be unified into one. (`unified-signatures` from TSLint) | | | diff --git a/packages/eslint-plugin/docs/rules/strict-comparisons.md b/packages/eslint-plugin/docs/rules/strict-comparisons.md new file mode 100644 index 000000000000..20ecfadfa139 --- /dev/null +++ b/packages/eslint-plugin/docs/rules/strict-comparisons.md @@ -0,0 +1,89 @@ +# Only allow comparisons between primitive types. (strict-comparison) + +This rule disallows comparisons between any non-primitive types. It also disables ordering comparisons between strings. + +Using `>` `<` `>=` `<=` to compare strings is often a mistake, but can be allowed using the `allowStringOrderComparison` option. + +By default, any comparisons between objects are not allowed. Objects are compared by reference, and in many cases this is not what +the developer wanted. This can be allowed by using the `allowObjectEqualComparison` option. + +A couple of errors that would be caught by this rule: + + +```ts +if (true > false) {} // Not allowed +if (2 > undefined) {} // Not allowed +if ({} > {}) {} // Not allowed + +if ('' > '') {} // This can be allowed by using the "allowStringOrderComparison" option. + +// The following expressions are invalid by default, but can be allowed by using the "allowObjectEqualComparison" option +if ({} === {}) {} // Not allowed +if ([] === []) {} // Not allowed +if ({} === undefined) {} // Not allowed + +const date1 = new Date(1550971894640) +const date2 = new Date(1550971894640) +if (date1 === date2) {} // Not allowed + +function sameObject(a: T, b: T): boolean { + return a === b; // Not allowed +} +``` + +## Rule Details + +This rule aims to enforce typesafe comparisons. + +## Options + +This rule has an object option: + +- `"allowObjectEqualComparison": false`, allows `!=` `==` `!==` `===` comparison between objects. +- `"allowStringOrderComparison": false`, allows `>` `<` `>=` `<=` comparison between strings. + +### defaults + +Examples of **incorrect** code for this rule with no options: + + +```ts +const object1 = {}; +const object2 = {}; + +if (object1 === object2) {} // Not allowed to compare objects by reference without "allowObjectEqualComparison" option. +if (object1 > object2) {} // Not allowed to compare objects using an ordering operator. +``` + +Examples of **correct** code for this rule with no options at all: + + +```ts +const object1 = {}; +const object2 = {}; +if (isEqual(object1, object2)) {} +``` + +### allowObjectEqualComparison + +Examples of **correct** code for this rule with `{ "allowObjectEqualComparison": true }`: + + +```ts +const object1 = {}; +const object2 = {}; +if (object1 === object2) {} +``` + +### allowStringOrderComparison + +Examples of **correct** code for this rule with `{ "allowStringOrderComparison": true }` options: + + +```ts +if ('a' < 'b') {} +``` + +## When Not To Use It + +If you know you want to compare objects by reference, you can safely turn this rule off. diff --git a/packages/eslint-plugin/src/rules/strict-comparisons.ts b/packages/eslint-plugin/src/rules/strict-comparisons.ts new file mode 100644 index 000000000000..e1f9caffcb12 --- /dev/null +++ b/packages/eslint-plugin/src/rules/strict-comparisons.ts @@ -0,0 +1,314 @@ +import { TSESTree } from '@typescript-eslint/typescript-estree'; +import { isUnionType, isTypeFlagSet } from 'tsutils'; +import ts from 'typescript'; + +import * as util from '../util'; + +interface Config { + allowObjectEqualComparison: boolean; + allowStringOrderComparison: boolean; +} + +type Options = [Config]; + +type MessageIds = 'nonComparableTypes' | 'invalidTypeForOperator'; + +export default util.createRule({ + name: 'strict-comparisons', + meta: { + type: 'problem', + docs: { + category: 'Best Practices', + description: 'Only allow comparisons between primitive types.', + tslintRuleName: 'strict-comparisons', + recommended: 'error', + }, + messages: { + nonComparableTypes: + "Cannot compare type '{{ typesLeft }}' to type '{{ typesRight }}'.", + invalidTypeForOperator: + "Cannot use '{{ comparator }}' comparator for type '{{ type }}'.", + }, + schema: [ + { + type: 'object', + properties: { + allowObjectEqualComparison: { + type: 'boolean', + }, + allowStringOrderComparison: { + type: 'boolean', + }, + }, + additionalProperties: false, + }, + ], + }, + defaultOptions: [ + { + allowObjectEqualComparison: false, + allowStringOrderComparison: false, + }, + ], + create( + context, + [{ allowObjectEqualComparison, allowStringOrderComparison }], + ) { + const service = util.getParserServices(context); + + const typeChecker = service.program.getTypeChecker(); + + const enum TypeKind { + Any = 0, + Number = 1, + Enum = 2, + String = 3, + Boolean = 4, + Null = 5, + Undefined = 6, + Object = 7, + } + + const typeNames = { + [TypeKind.Any]: 'any', + [TypeKind.Number]: 'number', + [TypeKind.Enum]: 'enum', + [TypeKind.String]: 'string', + [TypeKind.Boolean]: 'boolean', + [TypeKind.Null]: 'null', + [TypeKind.Undefined]: 'undefined', + [TypeKind.Object]: 'object', + }; + + /** + * Get TypeKinds of a typescript type + * @param type + */ + function getKinds(type: ts.Type): TypeKind[] { + return isUnionType(type) + ? Array.from(new Set(type.types.map(getKind))) + : [getKind(type)]; + } + + /** + * Get TypeKind of a typescript type + * @param type + */ + + function getKind(type: ts.Type): TypeKind { + if (is(ts.TypeFlags.String | ts.TypeFlags.StringLiteral)) { + return TypeKind.String; + } else if (is(ts.TypeFlags.Number | ts.TypeFlags.NumberLiteral)) { + return TypeKind.Number; + } else if (is(ts.TypeFlags.BooleanLike)) { + return TypeKind.Boolean; + } else if (is(ts.TypeFlags.Null)) { + return TypeKind.Null; + } else if (is(ts.TypeFlags.Undefined)) { + return TypeKind.Undefined; + } else if (is(ts.TypeFlags.Any)) { + return TypeKind.Any; + } else { + return TypeKind.Object; + } + + function is(flags: ts.TypeFlags) { + return isTypeFlagSet(type, flags); + } + } + + /** + * Check if a specific TypeKind is present in an array + * @param typesLeft array of TypeKinds + * @param typesRight TypeKind to check + */ + function getStrictestComparableType( + typesLeft: TypeKind[], + typesRight: TypeKind[], + ): TypeKind | undefined { + const overlappingTypes = typesLeft.filter( + type => typesRight.indexOf(type) >= 0, + ); + + if (overlappingTypes.length > 0) { + return getStrictestKind(overlappingTypes); + } + + // In case one of the types is "any", get the strictest type of the other array + if (arrayContainsKind(typesLeft, TypeKind.Any)) { + return getStrictestKind(typesRight); + } + if (arrayContainsKind(typesRight, TypeKind.Any)) { + return getStrictestKind(typesLeft); + } + + // In case one array contains Null or Undefined and the other an Object, return Object + if ( + (arrayContainsKind(typesLeft, TypeKind.Null, TypeKind.Undefined) && + arrayContainsKind(typesRight, TypeKind.Object)) || + (arrayContainsKind(typesRight, TypeKind.Null, TypeKind.Undefined) && + arrayContainsKind(typesLeft, TypeKind.Object)) + ) { + return TypeKind.Object; + } + return undefined; + } + + /** + * Check if a specific TypeKind is present in an array + * @param types array of TypeKinds + * @param typeToCheck TypeKind to check + */ + function arrayContainsKind( + types: TypeKind[], + ...typeToCheck: TypeKind[] + ): boolean { + return types.some(type => typeToCheck.includes(type)); + } + + /** + * Return the strictest kind of an array + * @param types array of TypeKinds + */ + function getStrictestKind(types: TypeKind[]): TypeKind { + return Math.max.apply(Math, types); + } + + /** + * Check if the operator is a comparison operator + * @param operator the operator to check + */ + function isComparisonOperator(operator: string): boolean { + if (isEqualityOperator(operator)) { + return true; + } + switch (operator) { + case '<': + case '>': + case '<=': + case '>=': + return true; + default: + return false; + } + } + + /** + * Check if the operator is an equality operator + * @param operator the operator to check + */ + function isEqualityOperator(operator: string): boolean { + switch (operator) { + case '==': + case '!=': + case '===': + case '!==': + return true; + default: + return false; + } + } + + /** + * Helper function to get base type of node + * @param node the node to be evaluated. + */ + function getNodeType(node: TSESTree.Node): ts.Type { + const tsNode = service.esTreeNodeToTSNodeMap.get(node); + return typeChecker.getTypeAtLocation(tsNode); + } + + return { + BinaryExpression(node) { + if (isComparisonOperator(node.operator)) { + const leftType = getNodeType(node.left); + const rightType = getNodeType(node.right); + + const leftKinds = getKinds(leftType); + const rightKinds = getKinds(rightType); + + const operandKind = getStrictestComparableType(leftKinds, rightKinds); + + if (operandKind === undefined) { + context.report({ + node, + messageId: 'nonComparableTypes', + data: { + typesLeft: leftKinds.map(type => typeNames[type]).join(' | '), + typesRight: rightKinds.map(type => typeNames[type]).join(' | '), + }, + }); + } else { + const isEquality = isEqualityOperator(node.operator); + if (isEquality) { + // Check !=, ==, !==, === + switch (operandKind) { + case TypeKind.Any: + case TypeKind.Number: + case TypeKind.Enum: + case TypeKind.String: + case TypeKind.Boolean: + break; + case TypeKind.Null: + case TypeKind.Undefined: + case TypeKind.Object: + if (allowObjectEqualComparison) { + break; + } + context.report({ + node, + messageId: 'invalidTypeForOperator', + data: { + comparator: node.operator, + type: typeNames[operandKind], + }, + }); + + break; + default: + context.report({ + node, + messageId: 'invalidTypeForOperator', + data: { + comparator: node.operator, + type: typeNames[operandKind], + }, + }); + } + } else { + // Check >, <, >=, <= + switch (operandKind) { + case TypeKind.Any: + case TypeKind.Number: + break; + case TypeKind.String: + if (allowStringOrderComparison) { + break; + } + context.report({ + node, + messageId: 'invalidTypeForOperator', + data: { + comparator: node.operator, + type: typeNames[operandKind], + }, + }); + + break; + default: + context.report({ + node, + messageId: 'invalidTypeForOperator', + data: { + comparator: node.operator, + type: typeNames[operandKind], + }, + }); + } + } + } + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts b/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts new file mode 100644 index 000000000000..ca57d5493bb9 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts @@ -0,0 +1,597 @@ +import path from 'path'; +import rule from '../../src/rules/strict-comparisons'; +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('strict-comparisons', rule, { + valid: [ + `if (2) {}`, + `if (2 > 1) {}`, + `if (2 < 1) {}`, + `if (2 >= 1) {}`, + `if (2 <= 1) {}`, + `if (2 == 1) {}`, + `if (2 === 1) {}`, + `if (2 != 1) {}`, + `if (2 !== 1) {}`, + `if (true) {}`, + `if (true == false) {}`, + `if (true === false) {}`, + `if (true != false) {}`, + `if (true !== false) {}`, + `if ('') {}`, + `if ('' == '') {}`, + `if ('' === '') {}`, + `if ('' != '') {}`, + `if ('' !== '') {}`, + `if ({}) {}`, + `if (3 > 2 || 2 > 1 && true === true) {}`, + ` +function sameObject(a: any, b: any): boolean { + return a === b; +}`, + ` +function sameObject(a: any, b: number): boolean { + return a === b; +}`, + ` +function sameObject(a: number, b: any): boolean { + return a === b; +}`, + ` +type myNumber = number; +const a1: myNumber = 1 +const a2: myNumber = 2 + +if (a1 < a2) {} +if (a2 < a1) {} + `, + ` +type myString = string; +const b1: myString = '' +const b2: myString = '' + +if (b1 === b2) {} +if (b2 === b1) {} + `, + ` +const d1: any = 'string' +const d2: any = 2 +if (d1 === d2) {} +if (d2 === d1) {} + `, + ` +enum TestNumericEnum { + One = 1, + Two = 2, +} + +const e1: TestNumericEnum = TestNumericEnum.One + +if (e1 === TestNumericEnum.Two) {} +if (TestNumericEnum.Two === e1) {} +if (e1 > TestNumericEnum.Two) {} +if (TestNumericEnum.Two > e1) {} + `, + ` +const f1: TestNumericEnum | undefined +const f2: TestNumericEnum | undefined + +if (f1 === f2) {} +if (f2 === f1) {} + `, + ` +enum TestStringEnum { + One = 'one', + Two = 'two', +} + +const g1: TestStringEnum = TestStringEnum.One + +if (g1 === TestStringEnum.Two) {} +if (TestStringEnum.Two === g1) {} + `, + ` +const h1: string | number = Math.random() > 0.5 ? 'text' : 5; +const h2: string | number = Math.random() > 0.5 ? 'test' : 2; +if (h1 === h2) {} +if (h2 === h1) {} + `, + ` +const a: any = 5 as any; +const b: number = 2; +if (a < a) {} +if (b > b) {} +if (a <= a) {} +if (b >= b) {} +if (a == a) {} +if (b != b) {} +if (a === a) {} +if (b !== b) {} + `, + ` +const b: any = 5 as any; +const a: number = 2; +if (a < a) {} +if (b > b) {} +if (a <= a) {} +if (b >= b) {} +if (a == a) {} +if (b != b) {} +if (a === a) {} +if (b !== b) {} + `, + { + code: `if ('' > '') {}`, + options: [ + { allowObjectEqualComparison: false, allowStringOrderComparison: true }, + ], + }, + { + code: `if ('' < '') {}`, + options: [ + { allowObjectEqualComparison: false, allowStringOrderComparison: true }, + ], + }, + { + code: `if ('' >= '') {}`, + options: [ + { allowObjectEqualComparison: false, allowStringOrderComparison: true }, + ], + }, + { + code: `if ('' <= '') {}`, + options: [ + { allowObjectEqualComparison: false, allowStringOrderComparison: true }, + ], + }, + { + code: ` +type myString = string; +const b1: myString = '' +const b2: myString = '' + +if (b1 === b2) {} +if (b2 === b1) {} + `, + options: [ + { allowObjectEqualComparison: false, allowStringOrderComparison: true }, + ], + }, + { + code: ` +enum TestStringEnum { + One = 'one', + Two = 'two', +} + +const g1: TestStringEnum = TestStringEnum.One + +if (g1 === TestStringEnum.Two) {} +if (TestStringEnum.Two === g1) {} +if (g1 > TestStringEnum.Two) {} +if (TestStringEnum.Two > g1) {} + `, + options: [ + { allowObjectEqualComparison: false, allowStringOrderComparison: true }, + ], + }, + { + code: ` +const h1: string | number = Math.random() > 0.5 ? 'text' : 5; +const h2: string | number = Math.random() > 0.5 ? 'test' : 2; +if (h1 > h2) {} +if (h2 > h1) {} +if (h1 === h2) {} +if (h2 === h1) {} + `, + options: [ + { allowObjectEqualComparison: false, allowStringOrderComparison: true }, + ], + }, + { + code: ` +const a = {}; +const b = {}; +if (a == a) {} +if (b != b) {} +if (a === a) {} +if (b !== b) {} + `, + options: [ + { allowObjectEqualComparison: true, allowStringOrderComparison: false }, + ], + }, + ], + invalid: [ + { + code: `if (2 > undefined) {}`, + errors: [ + { + messageId: 'nonComparableTypes', + data: { + typesLeft: 'number', + typesRight: 'undefined', + }, + }, + ], + }, + { + code: `if (undefined > 2) {}`, + errors: [ + { + messageId: 'nonComparableTypes', + data: { + typesLeft: 'undefined', + typesRight: 'number', + }, + }, + ], + }, + { + code: `if (2 === undefined) {}`, + errors: [ + { + messageId: 'nonComparableTypes', + data: { + typesLeft: 'number', + typesRight: 'undefined', + }, + }, + ], + }, + { + code: `if (undefined === 2) {}`, + errors: [ + { + messageId: 'nonComparableTypes', + data: { + typesLeft: 'undefined', + typesRight: 'number', + }, + }, + ], + }, + { + code: `if (true > false) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '>', + type: 'boolean', + }, + }, + ], + }, + { + code: `if (true < false) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '<', + type: 'boolean', + }, + }, + ], + }, + { + code: `if (true >= false) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '>=', + type: 'boolean', + }, + }, + ], + }, + { + code: `if (true <= false) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '<=', + type: 'boolean', + }, + }, + ], + }, + { + code: `if ('' > '') {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '>', + type: 'string', + }, + }, + ], + }, + { + code: `if ('' < '') {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '<', + type: 'string', + }, + }, + ], + }, + { + code: `if ('' >= '') {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '>=', + type: 'string', + }, + }, + ], + }, + { + code: `if ('' <= '') {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '<=', + type: 'string', + }, + }, + ], + }, + { + code: `if ({} > {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '>', + type: 'object', + }, + }, + ], + }, + { + code: `if ({} > {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '>', + type: 'object', + }, + }, + ], + }, + { + code: `if ({} >= {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '>=', + type: 'object', + }, + }, + ], + }, + { + code: `if ({} <= {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '<=', + type: 'object', + }, + }, + ], + }, + { + code: `if ({} == {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '==', + type: 'object', + }, + }, + ], + }, + { + code: `if ({} === {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '===', + type: 'object', + }, + }, + ], + }, + { + code: `if ({} != {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '!=', + type: 'object', + }, + }, + ], + }, + { + code: `if ({} !== {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '!==', + type: 'object', + }, + }, + ], + }, + { + code: `if ([] === []) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '===', + type: 'object', + }, + }, + ], + }, + { + code: `if ('' > '' || 2 > 1 || {} > {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '>', + type: 'string', + }, + }, + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '>', + type: 'object', + }, + }, + ], + }, + { + code: `if ('' > '' && 2 > 1 && {} > {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '>', + type: 'string', + }, + }, + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '>', + type: 'object', + }, + }, + ], + }, + { + code: `if ({} === null) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '===', + type: 'object', + }, + }, + ], + }, + { + code: `if (null === {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '===', + type: 'object', + }, + }, + ], + }, + { + code: `if ({} === undefined) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '===', + type: 'object', + }, + }, + ], + }, + { + code: `if (undefined === {}) {}`, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '===', + type: 'object', + }, + }, + ], + }, + { + code: ` +function sameObject(a: T, b: T): boolean { + return a === b; +} + `, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '===', + type: 'object', + }, + }, + ], + }, + { + code: ` +type myObject = Object; +const c1: myObject = {} +const c2: myObject = {} + +if (c1 === c2) {} +if (c2 === c1) {} + `, + errors: [ + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '===', + type: 'object', + }, + }, + { + messageId: 'invalidTypeForOperator', + data: { + comparator: '===', + type: 'object', + }, + }, + ], + }, + ], +}); diff --git a/packages/eslint-plugin/typings/ts-eslint.d.ts b/packages/eslint-plugin/typings/ts-eslint.d.ts index 33b667dab46a..f201d931a09c 100644 --- a/packages/eslint-plugin/typings/ts-eslint.d.ts +++ b/packages/eslint-plugin/typings/ts-eslint.d.ts @@ -406,6 +406,7 @@ declare module 'ts-eslint' { ArrowFunctionExpression?: RuleFunction; AssignmentPattern?: RuleFunction; AwaitExpression?: RuleFunction; + BinaryExpression?: RuleFunction; BlockStatement?: RuleFunction; BreakStatement?: RuleFunction; CallExpression?: RuleFunction;