Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat(eslint-plugin): add rule
no-confusing-non-null-assertion (#1941)
- Loading branch information
Showing
6 changed files
with
297 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
46 changes: 46 additions & 0 deletions
46
packages/eslint-plugin/docs/rules/no-confusing-non-null-assertion.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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: | ||
|
|
||
| <!-- prettier-ignore --> | ||
| ```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) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
94 changes: 94 additions & 0 deletions
94
packages/eslint-plugin/src/rules/no-confusing-non-null-assertion.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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, ')'), | ||
| ], | ||
| }, | ||
| ], | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }); |
153 changes: 153 additions & 0 deletions
153
packages/eslint-plugin/tests/rules/no-confusing-non-null-assertion.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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;', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); |