From ba9b86a04630bdf2fcfe4be225920aa3be8b591a Mon Sep 17 00:00:00 2001 From: AndreasGassmann Date: Sun, 24 Feb 2019 02:35:43 +0100 Subject: [PATCH 1/8] feat(eslint-plugin): add strict-comparisons rule --- packages/eslint-plugin/README.md | 1 + .../docs/rules/strict-comparison.md | 89 +++++ .../src/rules/strict-comparisons.ts | 291 ++++++++++++++++ .../tests/rules/strict-comparisons.test.ts | 316 ++++++++++++++++++ 4 files changed, 697 insertions(+) create mode 100644 packages/eslint-plugin/docs/rules/strict-comparison.md create mode 100644 packages/eslint-plugin/src/rules/strict-comparisons.ts create mode 100644 packages/eslint-plugin/tests/rules/strict-comparisons.test.ts diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 10268b006351..9186a6571d85 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: | diff --git a/packages/eslint-plugin/docs/rules/strict-comparison.md b/packages/eslint-plugin/docs/rules/strict-comparison.md new file mode 100644 index 000000000000..31ccaacf62d9 --- /dev/null +++ b/packages/eslint-plugin/docs/rules/strict-comparison.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..4910d9175c5e --- /dev/null +++ b/packages/eslint-plugin/src/rules/strict-comparisons.ts @@ -0,0 +1,291 @@ +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: 'object-comparison', + 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, + NullOrUndefined = 5, + Object = 6, + } + + const typeNames = { + [TypeKind.Any]: 'any', + [TypeKind.Number]: 'number', + [TypeKind.Enum]: 'enum', + [TypeKind.String]: 'string', + [TypeKind.Boolean]: 'boolean', + [TypeKind.NullOrUndefined]: 'null | 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 { + // tslint:disable:no-bitwise + return is(ts.TypeFlags.String | ts.TypeFlags.StringLiteral) + ? TypeKind.String + : is(ts.TypeFlags.Number | ts.TypeFlags.NumberLiteral) + ? TypeKind.Number + : is(ts.TypeFlags.BooleanLike) + ? TypeKind.Boolean + : is(ts.TypeFlags.Null | ts.TypeFlags.Undefined | ts.TypeFlags.Void) + ? TypeKind.NullOrUndefined + : is(ts.TypeFlags.Any) + ? TypeKind.Any + : TypeKind.Object; + // tslint:enable:no-bitwise + + 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); + } else { + // 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 NullOrUndefined and the other an Object, return Object + if ( + (arrayContainsKind(typesLeft, TypeKind.NullOrUndefined) && + arrayContainsKind(typesRight, TypeKind.Object)) || + (arrayContainsKind(typesRight, TypeKind.NullOrUndefined) && + 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 => type === typeToCheck); + } + + /** + * Return the strictest kind of an array + * @param types array of TypeKinds + */ + function getStrictestKind(types: TypeKind[]): TypeKind { + // tslint:disable-next-line:no-unsafe-any + 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: TSESTree.BinaryExpression) { + if (isComparisonOperator(node.operator)) { + const leftType = getNodeType(node.left); + const rightType = getNodeType(node.right); + + const leftKinds: TypeKind[] = getKinds(leftType); + const rightKinds: TypeKind[] = 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 { + function reportInvalidTypeForOperator() { + context.report({ + node, + messageId: 'invalidTypeForOperator', + data: { + comparator: node.operator, + type: typeNames[operandKind as TypeKind], + }, + }); + } + + 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.NullOrUndefined: + case TypeKind.Object: + if (allowObjectEqualComparison) { + break; + } + reportInvalidTypeForOperator(); + break; + default: + reportInvalidTypeForOperator(); + } + } else { + // Check >, <, >=, <= + switch (operandKind) { + case TypeKind.Any: + case TypeKind.Number: + break; + case TypeKind.String: + if (allowStringOrderComparison) { + break; + } + reportInvalidTypeForOperator(); + break; + default: + reportInvalidTypeForOperator(); + } + } + } + } + }, + }; + }, +}); 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..3cfbd0c4343a --- /dev/null +++ b/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts @@ -0,0 +1,316 @@ +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; +}`, + ` +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) {} + `, + { + 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 }, + ], + }, + ], + invalid: [ + { + code: `if (2 > undefined) {}`, + errors: [ + { + messageId: 'nonComparableTypes', + data: { + typesLeft: 'number', + typesRight: 'null | undefined', + }, + }, + ], + }, + { + code: `if (undefined > 2) {}`, + errors: [ + { + messageId: 'nonComparableTypes', + data: { + typesLeft: 'null | undefined', + typesRight: 'number', + }, + }, + ], + }, + { + code: `if (2 === undefined) {}`, + errors: [ + { + messageId: 'nonComparableTypes', + data: { + typesLeft: 'number', + typesRight: 'null | undefined', + }, + }, + ], + }, + { + code: `if (undefined === 2) {}`, + errors: [ + { + messageId: 'nonComparableTypes', + data: { + typesLeft: 'null | 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', + }, + }, + ], + }, + ], +}); From 8dcdb33be847a7d37d3a1862182e4916b361b324 Mon Sep 17 00:00:00 2001 From: AndreasGassmann Date: Sun, 24 Feb 2019 02:51:46 +0100 Subject: [PATCH 2/8] fix(strict-comparisons): fix linting rule --- .../src/rules/strict-comparisons.ts | 49 +++++++++++++------ 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-comparisons.ts b/packages/eslint-plugin/src/rules/strict-comparisons.ts index 4910d9175c5e..5da7ff00d0f5 100644 --- a/packages/eslint-plugin/src/rules/strict-comparisons.ts +++ b/packages/eslint-plugin/src/rules/strict-comparisons.ts @@ -236,17 +236,6 @@ export default util.createRule({ }, }); } else { - function reportInvalidTypeForOperator() { - context.report({ - node, - messageId: 'invalidTypeForOperator', - data: { - comparator: node.operator, - type: typeNames[operandKind as TypeKind], - }, - }); - } - const isEquality = isEqualityOperator(node.operator); if (isEquality) { // Check !=, ==, !==, === @@ -262,10 +251,25 @@ export default util.createRule({ if (allowObjectEqualComparison) { break; } - reportInvalidTypeForOperator(); + context.report({ + node, + messageId: 'invalidTypeForOperator', + data: { + comparator: node.operator, + type: typeNames[operandKind], + }, + }); + break; default: - reportInvalidTypeForOperator(); + context.report({ + node, + messageId: 'invalidTypeForOperator', + data: { + comparator: node.operator, + type: typeNames[operandKind], + }, + }); } } else { // Check >, <, >=, <= @@ -277,10 +281,25 @@ export default util.createRule({ if (allowStringOrderComparison) { break; } - reportInvalidTypeForOperator(); + context.report({ + node, + messageId: 'invalidTypeForOperator', + data: { + comparator: node.operator, + type: typeNames[operandKind], + }, + }); + break; default: - reportInvalidTypeForOperator(); + context.report({ + node, + messageId: 'invalidTypeForOperator', + data: { + comparator: node.operator, + type: typeNames[operandKind], + }, + }); } } } From 0cc70ebfba1ebfa0ef6788949c48fdc8061e2e78 Mon Sep 17 00:00:00 2001 From: AndreasGassmann Date: Sun, 24 Feb 2019 05:52:33 +0100 Subject: [PATCH 3/8] feat(strict-comparisons): add more tests --- .../tests/rules/strict-comparisons.test.ts | 248 ++++++++++++++++++ 1 file changed, 248 insertions(+) diff --git a/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts b/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts index 3cfbd0c4343a..aec39f3f311d 100644 --- a/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts @@ -98,6 +98,18 @@ const h2: string | number = Math.random() > 0.5 ? 'test' : 2; if (h1 === h2) {} if (h2 === h1) {} `, + ` +const a: any = 5; +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) {} + `, { code: `if ('' > '') {}`, options: [ @@ -312,5 +324,241 @@ if (h2 === h1) {} }, ], }, + { + 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', + }, + }, + ], + }, ], }); From da3eb38e4bd33c2cfb8f99c2591082e1fbf171e6 Mon Sep 17 00:00:00 2001 From: AndreasGassmann Date: Sun, 24 Feb 2019 06:10:19 +0100 Subject: [PATCH 4/8] feat(strict-comparisons): add more tests --- .../tests/rules/strict-comparisons.test.ts | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts b/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts index aec39f3f311d..7b2b909a7a30 100644 --- a/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts @@ -99,7 +99,7 @@ if (h1 === h2) {} if (h2 === h1) {} `, ` -const a: any = 5; +const a: any = 5 as any; const b: number = 2; if (a < a) {} if (b > b) {} @@ -108,6 +108,18 @@ 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) {} `, { @@ -178,6 +190,19 @@ if (h2 === h1) {} { 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: [ { From 8990b7cb6d1683273e72203c8cfe005838b36031 Mon Sep 17 00:00:00 2001 From: AndreasGassmann Date: Sun, 24 Feb 2019 06:32:54 +0100 Subject: [PATCH 5/8] feat(strict-comparisons): add more tests for test coverage --- .../eslint-plugin/tests/rules/strict-comparisons.test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts b/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts index 7b2b909a7a30..9b156eba97ec 100644 --- a/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts @@ -40,6 +40,14 @@ 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 From f1f30a16c49fe338cc3d828187d50277ca08c2f6 Mon Sep 17 00:00:00 2001 From: AndreasGassmann Date: Thu, 14 Mar 2019 05:28:12 +0100 Subject: [PATCH 6/8] fix(strict-comparisons): remove tslint comments --- packages/eslint-plugin/src/rules/strict-comparisons.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-comparisons.ts b/packages/eslint-plugin/src/rules/strict-comparisons.ts index 5da7ff00d0f5..6b8534795d2c 100644 --- a/packages/eslint-plugin/src/rules/strict-comparisons.ts +++ b/packages/eslint-plugin/src/rules/strict-comparisons.ts @@ -20,7 +20,7 @@ export default util.createRule({ docs: { category: 'Best Practices', description: 'Only allow comparisons between primitive types.', - tslintRuleName: 'object-comparison', + tslintRuleName: 'strict-comparisons', recommended: 'error', }, messages: { @@ -94,7 +94,6 @@ export default util.createRule({ */ function getKind(type: ts.Type): TypeKind { - // tslint:disable:no-bitwise return is(ts.TypeFlags.String | ts.TypeFlags.StringLiteral) ? TypeKind.String : is(ts.TypeFlags.Number | ts.TypeFlags.NumberLiteral) @@ -106,7 +105,6 @@ export default util.createRule({ : is(ts.TypeFlags.Any) ? TypeKind.Any : TypeKind.Object; - // tslint:enable:no-bitwise function is(flags: ts.TypeFlags) { return isTypeFlagSet(type, flags); @@ -167,7 +165,6 @@ export default util.createRule({ * @param types array of TypeKinds */ function getStrictestKind(types: TypeKind[]): TypeKind { - // tslint:disable-next-line:no-unsafe-any return Math.max.apply(Math, types); } From 404806472c0b56b317ec4575165b0fae2fddf6ed Mon Sep 17 00:00:00 2001 From: AndreasGassmann Date: Thu, 14 Mar 2019 05:44:49 +0100 Subject: [PATCH 7/8] refactor(strict-comparisons): split up null and undefined kinds --- .../src/rules/strict-comparisons.ts | 59 +++++++++++-------- .../tests/rules/strict-comparisons.test.ts | 8 +-- 2 files changed, 37 insertions(+), 30 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-comparisons.ts b/packages/eslint-plugin/src/rules/strict-comparisons.ts index 6b8534795d2c..909de0be78ae 100644 --- a/packages/eslint-plugin/src/rules/strict-comparisons.ts +++ b/packages/eslint-plugin/src/rules/strict-comparisons.ts @@ -25,9 +25,9 @@ export default util.createRule({ }, messages: { nonComparableTypes: - "cannot compare type '{{ typesLeft }}' to type '{{ typesRight }}'", + "Cannot compare type '{{ typesLeft }}' to type '{{ typesRight }}'.", invalidTypeForOperator: - "cannot use '{{ comparator }}' comparator for type '{{ type }}'", + "Cannot use '{{ comparator }}' comparator for type '{{ type }}'.", }, schema: [ { @@ -64,8 +64,9 @@ export default util.createRule({ Enum = 2, String = 3, Boolean = 4, - NullOrUndefined = 5, - Object = 6, + Null = 5, + Undefined = 6, + Object = 7, } const typeNames = { @@ -74,7 +75,8 @@ export default util.createRule({ [TypeKind.Enum]: 'enum', [TypeKind.String]: 'string', [TypeKind.Boolean]: 'boolean', - [TypeKind.NullOrUndefined]: 'null | undefined', + [TypeKind.Null]: 'null', + [TypeKind.Undefined]: 'undefined', [TypeKind.Object]: 'object', }; @@ -94,17 +96,21 @@ export default util.createRule({ */ function getKind(type: ts.Type): TypeKind { - return is(ts.TypeFlags.String | ts.TypeFlags.StringLiteral) - ? TypeKind.String - : is(ts.TypeFlags.Number | ts.TypeFlags.NumberLiteral) - ? TypeKind.Number - : is(ts.TypeFlags.BooleanLike) - ? TypeKind.Boolean - : is(ts.TypeFlags.Null | ts.TypeFlags.Undefined | ts.TypeFlags.Void) - ? TypeKind.NullOrUndefined - : is(ts.TypeFlags.Any) - ? TypeKind.Any - : TypeKind.Object; + 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); @@ -128,19 +134,19 @@ export default util.createRule({ return getStrictestKind(overlappingTypes); } else { // In case one of the types is "any", get the strictest type of the other array - if (arrayContainsKind(typesLeft, TypeKind.Any)) { + if (arrayContainsKind(typesLeft, [TypeKind.Any])) { return getStrictestKind(typesRight); } - if (arrayContainsKind(typesRight, TypeKind.Any)) { + if (arrayContainsKind(typesRight, [TypeKind.Any])) { return getStrictestKind(typesLeft); } - // In case one array contains NullOrUndefined and the other an Object, return Object + // In case one array contains Null or Undefined and the other an Object, return Object if ( - (arrayContainsKind(typesLeft, TypeKind.NullOrUndefined) && - arrayContainsKind(typesRight, TypeKind.Object)) || - (arrayContainsKind(typesRight, TypeKind.NullOrUndefined) && - arrayContainsKind(typesLeft, TypeKind.Object)) + (arrayContainsKind(typesLeft, [TypeKind.Null, TypeKind.Undefined]) && + arrayContainsKind(typesRight, [TypeKind.Object])) || + (arrayContainsKind(typesRight, [TypeKind.Null, TypeKind.Undefined]) && + arrayContainsKind(typesLeft, [TypeKind.Object])) ) { return TypeKind.Object; } @@ -155,9 +161,9 @@ export default util.createRule({ */ function arrayContainsKind( types: TypeKind[], - typeToCheck: TypeKind, + typeToCheck: TypeKind[], ): boolean { - return types.some(type => type === typeToCheck); + return types.some(type => typeToCheck.includes(type)); } /** @@ -243,7 +249,8 @@ export default util.createRule({ case TypeKind.String: case TypeKind.Boolean: break; - case TypeKind.NullOrUndefined: + case TypeKind.Null: + case TypeKind.Undefined: case TypeKind.Object: if (allowObjectEqualComparison) { break; diff --git a/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts b/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts index 9b156eba97ec..ca57d5493bb9 100644 --- a/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-comparisons.test.ts @@ -220,7 +220,7 @@ if (b !== b) {} messageId: 'nonComparableTypes', data: { typesLeft: 'number', - typesRight: 'null | undefined', + typesRight: 'undefined', }, }, ], @@ -231,7 +231,7 @@ if (b !== b) {} { messageId: 'nonComparableTypes', data: { - typesLeft: 'null | undefined', + typesLeft: 'undefined', typesRight: 'number', }, }, @@ -244,7 +244,7 @@ if (b !== b) {} messageId: 'nonComparableTypes', data: { typesLeft: 'number', - typesRight: 'null | undefined', + typesRight: 'undefined', }, }, ], @@ -255,7 +255,7 @@ if (b !== b) {} { messageId: 'nonComparableTypes', data: { - typesLeft: 'null | undefined', + typesLeft: 'undefined', typesRight: 'number', }, }, From 2e043ff9c1844807285df3d2745bd287f093a0bb Mon Sep 17 00:00:00 2001 From: Andreas Gassmann Date: Thu, 21 Mar 2019 19:12:00 +0100 Subject: [PATCH 8/8] fix(strict-comparisons): small refactoring --- ...ct-comparison.md => strict-comparisons.md} | 6 +-- .../src/rules/strict-comparisons.ts | 44 +++++++++---------- packages/eslint-plugin/typings/ts-eslint.d.ts | 1 + 3 files changed, 26 insertions(+), 25 deletions(-) rename packages/eslint-plugin/docs/rules/{strict-comparison.md => strict-comparisons.md} (86%) diff --git a/packages/eslint-plugin/docs/rules/strict-comparison.md b/packages/eslint-plugin/docs/rules/strict-comparisons.md similarity index 86% rename from packages/eslint-plugin/docs/rules/strict-comparison.md rename to packages/eslint-plugin/docs/rules/strict-comparisons.md index 31ccaacf62d9..20ecfadfa139 100644 --- a/packages/eslint-plugin/docs/rules/strict-comparison.md +++ b/packages/eslint-plugin/docs/rules/strict-comparisons.md @@ -2,7 +2,7 @@ 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. +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. @@ -39,8 +39,8 @@ This rule aims to enforce typesafe comparisons. This rule has an object option: -- `"allowObjectEqualComparison": false`, allows \`!=\` \`==\` \`!==\` \`===\` comparison between objects. -- `"allowStringOrderComparison": false`, allows \`>\` \`<\` \`>=\` \`<=\` comparison between strings. +- `"allowObjectEqualComparison": false`, allows `!=` `==` `!==` `===` comparison between objects. +- `"allowStringOrderComparison": false`, allows `>` `<` `>=` `<=` comparison between strings. ### defaults diff --git a/packages/eslint-plugin/src/rules/strict-comparisons.ts b/packages/eslint-plugin/src/rules/strict-comparisons.ts index 909de0be78ae..e1f9caffcb12 100644 --- a/packages/eslint-plugin/src/rules/strict-comparisons.ts +++ b/packages/eslint-plugin/src/rules/strict-comparisons.ts @@ -132,26 +132,26 @@ export default util.createRule({ if (overlappingTypes.length > 0) { return getStrictestKind(overlappingTypes); - } else { - // 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; + // 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; } /** @@ -161,7 +161,7 @@ export default util.createRule({ */ function arrayContainsKind( types: TypeKind[], - typeToCheck: TypeKind[], + ...typeToCheck: TypeKind[] ): boolean { return types.some(type => typeToCheck.includes(type)); } @@ -219,13 +219,13 @@ export default util.createRule({ } return { - BinaryExpression(node: TSESTree.BinaryExpression) { + BinaryExpression(node) { if (isComparisonOperator(node.operator)) { const leftType = getNodeType(node.left); const rightType = getNodeType(node.right); - const leftKinds: TypeKind[] = getKinds(leftType); - const rightKinds: TypeKind[] = getKinds(rightType); + const leftKinds = getKinds(leftType); + const rightKinds = getKinds(rightType); const operandKind = getStrictestComparableType(leftKinds, rightKinds); diff --git a/packages/eslint-plugin/typings/ts-eslint.d.ts b/packages/eslint-plugin/typings/ts-eslint.d.ts index 998c69583428..09ae791d072e 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;