diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 975bb88f08b..01de689fd78 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -105,6 +105,7 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int | [`@typescript-eslint/method-signature-style`](./docs/rules/method-signature-style.md) | Enforces using a particular method signature syntax. | | :wrench: | | | [`@typescript-eslint/naming-convention`](./docs/rules/naming-convention.md) | Enforces naming conventions for everything across a codebase | | | :thought_balloon: | | [`@typescript-eslint/no-base-to-string`](./docs/rules/no-base-to-string.md) | Requires that `.toString()` is only called on objects which provide useful information when stringified | | | :thought_balloon: | +| [`@typescript-eslint/no-confusing-non-null-assertion`](./docs/rules/no-confusing-non-null-assertion.md) | Disallow non-null assertion in locations that may be confusing | | :wrench: | | | [`@typescript-eslint/no-dynamic-delete`](./docs/rules/no-dynamic-delete.md) | Disallow the delete operator with computed key expressions | | :wrench: | | | [`@typescript-eslint/no-empty-interface`](./docs/rules/no-empty-interface.md) | Disallow the declaration of empty interfaces | :heavy_check_mark: | :wrench: | | | [`@typescript-eslint/no-explicit-any`](./docs/rules/no-explicit-any.md) | Disallow usage of the `any` type | :heavy_check_mark: | :wrench: | | diff --git a/packages/eslint-plugin/docs/rules/no-confusing-non-null-assertion.md b/packages/eslint-plugin/docs/rules/no-confusing-non-null-assertion.md new file mode 100644 index 00000000000..a61f6a8a9f4 --- /dev/null +++ b/packages/eslint-plugin/docs/rules/no-confusing-non-null-assertion.md @@ -0,0 +1,46 @@ +# Disallow non-null assertion in locations that may be confusing (`no-confusing-non-null-assertion`) + +## Rule Details + +Using a non-null assertion (`!`) next to an assign or equals check (`=` or `==` or `===`) creates code that is confusing as it looks similar to a not equals check (`!=` `!==`). + +```typescript +a! == b; // a non-null assertions(`!`) and an equals test(`==`) +a !== b; // not equals test(`!==`) +a! === b; // a non-null assertions(`!`) and an triple equals test(`===`) +``` + +Examples of **incorrect** code for this rule: + +```ts +interface Foo { + bar?: string; + num?: number; +} + +const foo: Foo = getFoo(); +const isEqualsBar = foo.bar! == 'hello'; +const isEqualsNum = 1 + foo.num! == 2; +``` + +Examples of **correct** code for this rule: + + +```ts +interface Foo { + bar?: string; + num?: number; +} + +const foo: Foo = getFoo(); +const isEqualsBar = foo.bar == 'hello'; +const isEqualsNum = (1 + foo.num!) == 2; +``` + +## When Not To Use It + +If you don't care about this confusion, then you will not need this rule. + +## Further Reading + +- [`Issue: Easy misunderstanding: "! ==="`](https://github.com/microsoft/TypeScript/issues/37837) in [TypeScript repo](https://github.com/microsoft/TypeScript) diff --git a/packages/eslint-plugin/src/configs/all.ts b/packages/eslint-plugin/src/configs/all.ts index a02d6db9d69..f70cd1c9a14 100644 --- a/packages/eslint-plugin/src/configs/all.ts +++ b/packages/eslint-plugin/src/configs/all.ts @@ -37,6 +37,7 @@ export = { 'no-array-constructor': 'off', '@typescript-eslint/no-array-constructor': 'error', '@typescript-eslint/no-base-to-string': 'error', + '@typescript-eslint/no-confusing-non-null-assertion': 'error', 'no-dupe-class-members': 'off', '@typescript-eslint/no-dupe-class-members': 'error', '@typescript-eslint/no-dynamic-delete': 'error', diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index 03ebb633b00..918f4856c59 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -6,6 +6,7 @@ import banTypes from './ban-types'; import braceStyle from './brace-style'; import classLiteralPropertyStyle from './class-literal-property-style'; import commaSpacing from './comma-spacing'; +import confusingNonNullAssertionLikeNotEqual from './no-confusing-non-null-assertion'; import consistentTypeAssertions from './consistent-type-assertions'; import consistentTypeDefinitions from './consistent-type-definitions'; import defaultParamLast from './default-param-last'; @@ -104,6 +105,7 @@ export default { 'brace-style': braceStyle, 'class-literal-property-style': classLiteralPropertyStyle, 'comma-spacing': commaSpacing, + 'no-confusing-non-null-assertion': confusingNonNullAssertionLikeNotEqual, 'consistent-type-assertions': consistentTypeAssertions, 'consistent-type-definitions': consistentTypeDefinitions, 'default-param-last': defaultParamLast, diff --git a/packages/eslint-plugin/src/rules/no-confusing-non-null-assertion.ts b/packages/eslint-plugin/src/rules/no-confusing-non-null-assertion.ts new file mode 100644 index 00000000000..bcf561b5856 --- /dev/null +++ b/packages/eslint-plugin/src/rules/no-confusing-non-null-assertion.ts @@ -0,0 +1,94 @@ +import { + AST_NODE_TYPES, + AST_TOKEN_TYPES, + TSESLint, + TSESTree, +} from '@typescript-eslint/experimental-utils'; +import * as util from '../util'; + +export default util.createRule({ + name: 'no-confusing-non-null-assertion', + meta: { + type: 'problem', + docs: { + description: + 'Disallow non-null assertion in locations that may be confusing', + category: 'Stylistic Issues', + recommended: false, + }, + fixable: 'code', + messages: { + confusingEqual: + 'Confusing combinations of non-null assertion and equal test like "a! == b", which looks very similar to not equal "a !== b"', + confusingAssign: + 'Confusing combinations of non-null assertion and equal test like "a! = b", which looks very similar to not equal "a != b"', + notNeedInEqualTest: 'Unnecessary non-null assertion (!) in equal test', + notNeedInAssign: + 'Unnecessary non-null assertion (!) in assignment left hand', + wrapUpLeft: + 'Wrap up left hand to avoid putting non-null assertion "!" and "=" together', + }, + schema: [], + }, + defaultOptions: [], + create(context) { + const sourceCode = context.getSourceCode(); + return { + 'BinaryExpression, AssignmentExpression'( + node: TSESTree.BinaryExpression | TSESTree.AssignmentExpression, + ): void { + function isLeftHandPrimaryExpression( + node: TSESTree.Expression, + ): boolean { + return node.type === AST_NODE_TYPES.TSNonNullExpression; + } + + if ( + node.operator === '==' || + node.operator === '===' || + node.operator === '=' + ) { + const isAssign = node.operator === '='; + const leftHandFinalToken = sourceCode.getLastToken(node.left); + const tokenAfterLeft = sourceCode.getTokenAfter(node.left); + if ( + leftHandFinalToken?.type === AST_TOKEN_TYPES.Punctuator && + leftHandFinalToken?.value === '!' && + tokenAfterLeft?.value !== ')' + ) { + if (isLeftHandPrimaryExpression(node.left)) { + context.report({ + node, + messageId: isAssign ? 'confusingAssign' : 'confusingEqual', + suggest: [ + { + messageId: isAssign + ? 'notNeedInAssign' + : 'notNeedInEqualTest', + fix: (fixer): TSESLint.RuleFix[] => [ + fixer.remove(leftHandFinalToken), + ], + }, + ], + }); + } else { + context.report({ + node, + messageId: isAssign ? 'confusingAssign' : 'confusingEqual', + suggest: [ + { + messageId: 'wrapUpLeft', + fix: (fixer): TSESLint.RuleFix[] => [ + fixer.insertTextBefore(node.left, '('), + fixer.insertTextAfter(node.left, ')'), + ], + }, + ], + }); + } + } + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin/tests/rules/no-confusing-non-null-assertion.test.ts b/packages/eslint-plugin/tests/rules/no-confusing-non-null-assertion.test.ts new file mode 100644 index 00000000000..333b0182559 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/no-confusing-non-null-assertion.test.ts @@ -0,0 +1,153 @@ +/* eslint-disable eslint-comments/no-use */ +// this rule enforces adding parens, which prettier will want to fix and break the tests +/* eslint "@typescript-eslint/internal/plugin-test-formatting": ["error", { formatWithPrettier: false }] */ +/* eslint-enable eslint-comments/no-use */ + +import rule from '../../src/rules/no-confusing-non-null-assertion'; +import { RuleTester } from '../RuleTester'; + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', +}); + +ruleTester.run('no-confusing-non-null-assertion', rule, { + valid: [ + // + 'a == b!;', + 'a = b!;', + 'a !== b;', + 'a != b;', + '(a + b!) == c;', + '(a + b!) = c;', + ], + invalid: [ + { + code: 'a! == b;', + errors: [ + { + messageId: 'confusingEqual', + line: 1, + column: 1, + suggestions: [ + { + messageId: 'notNeedInEqualTest', + output: 'a == b;', + }, + ], + }, + ], + }, + { + code: 'a! === b;', + errors: [ + { + messageId: 'confusingEqual', + line: 1, + column: 1, + suggestions: [ + { + messageId: 'notNeedInEqualTest', + output: 'a === b;', + }, + ], + }, + ], + }, + { + code: 'a + b! == c;', + errors: [ + { + messageId: 'confusingEqual', + line: 1, + column: 1, + suggestions: [ + { + messageId: 'wrapUpLeft', + output: '(a + b!) == c;', + }, + ], + }, + ], + }, + { + code: '(obj = new new OuterObj().InnerObj).Name! == c;', + errors: [ + { + messageId: 'confusingEqual', + line: 1, + column: 1, + suggestions: [ + { + messageId: 'notNeedInEqualTest', + output: '(obj = new new OuterObj().InnerObj).Name == c;', + }, + ], + }, + ], + }, + { + code: '(a==b)! ==c;', + errors: [ + { + messageId: 'confusingEqual', + line: 1, + column: 1, + suggestions: [ + { + messageId: 'notNeedInEqualTest', + output: '(a==b) ==c;', + }, + ], + }, + ], + }, + { + code: 'a! = b;', + errors: [ + { + messageId: 'confusingAssign', + line: 1, + column: 1, + suggestions: [ + { + messageId: 'notNeedInAssign', + output: 'a = b;', + }, + ], + }, + ], + }, + { + code: '(obj = new new OuterObj().InnerObj).Name! = c;', + errors: [ + { + messageId: 'confusingAssign', + line: 1, + column: 1, + suggestions: [ + { + messageId: 'notNeedInAssign', + output: '(obj = new new OuterObj().InnerObj).Name = c;', + }, + ], + }, + ], + }, + { + code: '(a=b)! =c;', + errors: [ + { + messageId: 'confusingAssign', + line: 1, + column: 1, + suggestions: [ + { + messageId: 'notNeedInAssign', + output: '(a=b) =c;', + }, + ], + }, + ], + }, + ], +});