Content-Length: 16942 | pFad | http://github.com/typescript-eslint/typescript-eslint/pull/9994.patch

thub.com From f5cf2339051858492de52b1d3847e6615def0c12 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 16 Sep 2024 02:52:44 -0600 Subject: [PATCH 1/4] ban !in and !instanceof --- .../rules/no-confusing-non-null-assertion.mdx | 19 +- .../rules/no-confusing-non-null-assertion.ts | 164 ++++++++++++++---- .../no-confusing-non-null-assertion.test.ts | 73 +++++++- 3 files changed, 218 insertions(+), 38 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-confusing-non-null-assertion.mdx b/packages/eslint-plugin/docs/rules/no-confusing-non-null-assertion.mdx index 0ff48e14c926..01a47a9dde78 100644 --- a/packages/eslint-plugin/docs/rules/no-confusing-non-null-assertion.mdx +++ b/packages/eslint-plugin/docs/rules/no-confusing-non-null-assertion.mdx @@ -9,14 +9,27 @@ import TabItem from '@theme/TabItem'; > > See **https://typescript-eslint.io/rules/no-confusing-non-null-assertion** for documentation. -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 (`!=` `!==`). +Using a non-null assertion (`!`) next to an assignment or equality check (`=` or `==` or `===`) creates code that is confusing as it looks similar to an inequality check (`!=` `!==`). ```typescript -a! == b; // a non-null assertions(`!`) and an equals test(`==`) +a! == b; // a non-null assertion(`!`) and an equals test(`==`) a !== b; // not equals test(`!==`) -a! === b; // a non-null assertions(`!`) and an triple equals test(`===`) +a! === b; // a non-null assertion(`!`) and a triple equals test(`===`) ``` +Using a non-null assertion (`!`) next to an in test (`in`) or an instanceof test (`instanceof`) creates code that is confusing since it may look like the operator is negated, but it is actually not. + +{/* prettier-ignore */} +```typescript +a! in b; // a non-null assertion(`!`) and an in test(`in`) +a !in b; // also a non-null assertion(`!`) and an in test(`in`) +!(a in b); // a negated in test + +a! instanceof b; // a non-null assertion(`!`) and an instanceof test(`instanceof`) +a !instanceof b; // also a non-null assertion(`!`) and an instanceof test(`instanceof`) +!(a instanceof b); // a negated instanceof test +```` + This rule flags confusing `!` assertions and suggests either removing them or wrapping the asserted expression in `()` parenthesis. ## Examples 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 index 9caf777aabad..83a6a03fea06 100644 --- a/packages/eslint-plugin/src/rules/no-confusing-non-null-assertion.ts +++ b/packages/eslint-plugin/src/rules/no-confusing-non-null-assertion.ts @@ -1,9 +1,39 @@ +import { MessagingOptions } from 'node:child_process'; + import type { TSESLint, TSESTree } from '@typescript-eslint/utils'; import { AST_NODE_TYPES, AST_TOKEN_TYPES } from '@typescript-eslint/utils'; +import type { + ReportDescriptor, + RuleFix, + SuggestionReportDescriptor, +} from '@typescript-eslint/utils/ts-eslint'; import { createRule } from '../util'; -export default createRule({ +type MessageId = + | 'confusingEqual' + | 'confusingAssign' + | 'confusingOperator' + | 'notNeedInEqualTest' + | 'notNeedInAssign' + | 'notNeedInOperator' + | 'wrapUpLeft'; + +const confusingOperators = new Set([ + '=', + '==', + '===', + 'in', + 'instanceof', +] as const); +type ConfusingOperator = + typeof confusingOperators extends Set ? T : never; + +function isConfusingOperator(operator: string): operator is ConfusingOperator { + return confusingOperators.has(operator as ConfusingOperator); +} + +export default createRule<[], MessageId>({ name: 'no-confusing-non-null-assertion', meta: { type: 'problem', @@ -15,35 +45,62 @@ export default createRule({ hasSuggestions: true, messages: { confusingEqual: - 'Confusing combinations of non-null assertion and equal test like "a! == b", which looks very similar to not equal "a !== b".', + 'Confusing combination of non-null assertion and equality test like `a! == b`, which looks very similar to `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.', + 'Confusing combination of non-null assertion and assignment like `a! = b`, which looks very similar to `a != b`.', + confusingOperator: + 'Confusing combination of non-null assertion and `{{operator}}` operator like `a! {{operator}} b`, which might be misinterpreted as `!(a {{operator}} b)`.', + + notNeedInEqualTest: + 'Remove unnecessary non-null assertion (!) in equality test.', notNeedInAssign: - 'Unnecessary non-null assertion (!) in assignment left hand.', + 'Remove unnecessary non-null assertion (!) in assignment left-hand side.', + + notNeedInOperator: + 'Remove possibly unnecessary non-null assertion (!) in the left operand of the `{{operator}}` operator.', + wrapUpLeft: - 'Wrap up left hand to avoid putting non-null assertion "!" and "=" together.', + 'Wrap the left-hand side in parentheses to avoid confusion with "{{operator}}" operator.', }, schema: [], }, defaultOptions: [], create(context) { + function confusingOperatorToMessageData( + operator: ConfusingOperator, + ): Pick, 'messageId' | 'data'> { + switch (operator) { + case '=': + return { + messageId: 'confusingAssign', + }; + case '==': + case '===': + return { + messageId: 'confusingEqual', + }; + case 'in': + case 'instanceof': + return { + messageId: 'confusingOperator', + data: { operator }, + }; + default: + operator satisfies never; + throw new Error(`Unexpected operator ${operator as string}`); + } + } + return { 'BinaryExpression, AssignmentExpression'( node: TSESTree.AssignmentExpression | TSESTree.BinaryExpression, ): void { - function isLeftHandPrimaryExpression( - node: TSESTree.Expression | TSESTree.PrivateIdentifier, - ): boolean { - return node.type === AST_NODE_TYPES.TSNonNullExpression; - } + const operator = node.operator; - if ( - node.operator === '==' || - node.operator === '===' || - node.operator === '=' - ) { - const isAssign = node.operator === '='; + if (isConfusingOperator(operator)) { + // Look for a non-null assertion as the last token on the left hand side. + // That way, we catch things like `1 + two! === 3`, even though the left + // hand side isn't a non-null assertion AST node. const leftHandFinalToken = context.sourceCode.getLastToken(node.left); const tokenAfterLeft = context.sourceCode.getTokenAfter(node.left); if ( @@ -51,32 +108,62 @@ export default createRule({ leftHandFinalToken.value === '!' && tokenAfterLeft?.value !== ')' ) { - if (isLeftHandPrimaryExpression(node.left)) { + if (node.left.type === AST_NODE_TYPES.TSNonNullExpression) { + let suggestions: TSESLint.SuggestionReportDescriptor[]; + switch (operator) { + case '=': + suggestions = [ + { + messageId: 'notNeedInAssign', + fix: (fixer): RuleFix => fixer.remove(leftHandFinalToken), + }, + ]; + break; + + case '==': + case '===': + suggestions = [ + { + messageId: 'notNeedInEqualTest', + fix: (fixer): RuleFix => fixer.remove(leftHandFinalToken), + }, + ]; + break; + + case 'in': + case 'instanceof': + suggestions = [ + { + messageId: 'notNeedInOperator', + data: { operator }, + fix: (fixer): RuleFix => fixer.remove(leftHandFinalToken), + }, + { + messageId: 'wrapUpLeft', + data: { operator }, + fix: wrapUpLeftFixer(node), + }, + ]; + break; + + default: + operator satisfies never; + return; + } context.report({ node, - messageId: isAssign ? 'confusingAssign' : 'confusingEqual', - suggest: [ - { - messageId: isAssign - ? 'notNeedInAssign' - : 'notNeedInEqualTest', - fix: (fixer): TSESLint.RuleFix[] => [ - fixer.remove(leftHandFinalToken), - ], - }, - ], + ...confusingOperatorToMessageData(operator), + suggest: suggestions, }); } else { context.report({ node, - messageId: isAssign ? 'confusingAssign' : 'confusingEqual', + ...confusingOperatorToMessageData(operator), suggest: [ { messageId: 'wrapUpLeft', - fix: (fixer): TSESLint.RuleFix[] => [ - fixer.insertTextBefore(node.left, '('), - fixer.insertTextAfter(node.left, ')'), - ], + data: { operator }, + fix: wrapUpLeftFixer(node), }, ], }); @@ -87,3 +174,12 @@ export default createRule({ }; }, }); + +function wrapUpLeftFixer( + node: TSESTree.AssignmentExpression | TSESTree.BinaryExpression, +): TSESLint.ReportFixFunction { + return (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 index 19f3cfec601d..6cdefc9b6c59 100644 --- 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 @@ -3,7 +3,7 @@ /* eslint "@typescript-eslint/internal/plugin-test-formatting": ["error", { formatWithPrettier: false }] */ /* eslint-enable eslint-comments/no-use */ -import { RuleTester } from '@typescript-eslint/rule-tester'; +import { noFormat, RuleTester } from '@typescript-eslint/rule-tester'; import rule from '../../src/rules/no-confusing-non-null-assertion'; @@ -18,6 +18,8 @@ ruleTester.run('no-confusing-non-null-assertion', rule, { 'a != b;', '(a + b!) == c;', '(a + b!) = c;', + '(a + b!) in c;', + '(a || b!) instanceof c;', ], invalid: [ { @@ -148,5 +150,74 @@ ruleTester.run('no-confusing-non-null-assertion', rule, { }, ], }, + { + code: 'a! in b;', + errors: [ + { + messageId: 'confusingOperator', + data: { operator: 'in' }, + line: 1, + column: 1, + suggestions: [ + { + messageId: 'notNeedInOperator', + output: 'a in b;', + }, + { + messageId: 'wrapUpLeft', + output: '(a!) in b;', + }, + ], + }, + ], + }, + { + code: noFormat` +a !in b; + `, + errors: [ + { + messageId: 'confusingOperator', + data: { operator: 'in' }, + line: 2, + column: 1, + suggestions: [ + { + messageId: 'notNeedInOperator', + output: ` +a in b; + `, + }, + { + messageId: 'wrapUpLeft', + output: ` +(a !)in b; + `, + }, + ], + }, + ], + }, + { + code: 'a! instanceof b;', + errors: [ + { + messageId: 'confusingOperator', + data: { operator: 'instanceof' }, + line: 1, + column: 1, + suggestions: [ + { + messageId: 'notNeedInOperator', + output: 'a instanceof b;', + }, + { + messageId: 'wrapUpLeft', + output: '(a!) instanceof b;', + }, + ], + }, + ], + }, ], }); From 06527974bec3a3c33b7c0ca7ae3d91a046ea4bdb Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Mon, 16 Sep 2024 03:03:09 -0600 Subject: [PATCH 2/4] lintfix --- .../eslint-plugin/src/rules/no-confusing-non-null-assertion.ts | 3 --- 1 file changed, 3 deletions(-) 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 index 83a6a03fea06..3185e1e54be6 100644 --- a/packages/eslint-plugin/src/rules/no-confusing-non-null-assertion.ts +++ b/packages/eslint-plugin/src/rules/no-confusing-non-null-assertion.ts @@ -1,11 +1,8 @@ -import { MessagingOptions } from 'node:child_process'; - import type { TSESLint, TSESTree } from '@typescript-eslint/utils'; import { AST_NODE_TYPES, AST_TOKEN_TYPES } from '@typescript-eslint/utils'; import type { ReportDescriptor, RuleFix, - SuggestionReportDescriptor, } from '@typescript-eslint/utils/ts-eslint'; import { createRule } from '../util'; From 76298e152e75923714b43e4821746cdc9c872c3e Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sun, 22 Sep 2024 19:47:15 -0600 Subject: [PATCH 3/4] snapshots --- .../no-confusing-non-null-assertion.shot | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-confusing-non-null-assertion.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-confusing-non-null-assertion.shot index 074f8f721db5..fc3fe7bcc1be 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-confusing-non-null-assertion.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-confusing-non-null-assertion.shot @@ -10,9 +10,9 @@ interface Foo { const foo: Foo = getFoo(); const isEqualsBar = foo.bar! == 'hello'; - ~~~~~~~~~~~~~~~~~~~ Confusing combinations of non-null assertion and equal test like "a! == b", which looks very similar to not equal "a !== b". + ~~~~~~~~~~~~~~~~~~~ Confusing combination of non-null assertion and equality test like \`a! == b\`, which looks very similar to \`a !== b\`. const isEqualsNum = 1 + foo.num! == 2; - ~~~~~~~~~~~~~~~~~ Confusing combinations of non-null assertion and equal test like "a! == b", which looks very similar to not equal "a !== b". + ~~~~~~~~~~~~~~~~~ Confusing combination of non-null assertion and equality test like \`a! == b\`, which looks very similar to \`a !== b\`. " `; From e3172b7b82d3e7cb2b434518a579a9767d16e52f Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sun, 22 Sep 2024 20:17:49 -0600 Subject: [PATCH 4/4] cov --- .../eslint-plugin/src/rules/no-confusing-non-null-assertion.ts | 2 ++ 1 file changed, 2 insertions(+) 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 index 3185e1e54be6..160827fc5a32 100644 --- a/packages/eslint-plugin/src/rules/no-confusing-non-null-assertion.ts +++ b/packages/eslint-plugin/src/rules/no-confusing-non-null-assertion.ts @@ -82,6 +82,7 @@ export default createRule<[], MessageId>({ messageId: 'confusingOperator', data: { operator }, }; + // istanbul ignore next default: operator satisfies never; throw new Error(`Unexpected operator ${operator as string}`); @@ -143,6 +144,7 @@ export default createRule<[], MessageId>({ ]; break; + // istanbul ignore next default: operator satisfies never; return;

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!