Skip to content

Commit

Permalink
fix(eslint-plugin): [no-confusing-non-null-assertion] check !in and !…
Browse files Browse the repository at this point in the history
…instanceof (#9994)

* ban !in and !instanceof

* lintfix

* snapshots

* cov
  • Loading branch information
kirkwaiblinger authored Sep 23, 2024
1 parent b75d42b commit 8293546
Show file tree
Hide file tree
Showing 4 changed files with 219 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
163 changes: 129 additions & 34 deletions packages/eslint-plugin/src/rules/no-confusing-non-null-assertion.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,36 @@
import type { TSESLint, TSESTree } from '@typescript-eslint/utils';
import { AST_NODE_TYPES, AST_TOKEN_TYPES } from '@typescript-eslint/utils';
import type {
ReportDescriptor,
RuleFix,
} 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<infer T> ? 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',
Expand All @@ -15,68 +42,127 @@ 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<ReportDescriptor<MessageId>, 'messageId' | 'data'> {
switch (operator) {
case '=':
return {
messageId: 'confusingAssign',
};
case '==':
case '===':
return {
messageId: 'confusingEqual',
};
case 'in':
case 'instanceof':
return {
messageId: 'confusingOperator',
data: { operator },
};
// istanbul ignore next
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 (
leftHandFinalToken?.type === AST_TOKEN_TYPES.Punctuator &&
leftHandFinalToken.value === '!' &&
tokenAfterLeft?.value !== ')'
) {
if (isLeftHandPrimaryExpression(node.left)) {
if (node.left.type === AST_NODE_TYPES.TSNonNullExpression) {
let suggestions: TSESLint.SuggestionReportDescriptor<MessageId>[];
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;

// istanbul ignore next
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),
},
],
});
Expand All @@ -87,3 +173,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, ')'),
];
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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: [
{
Expand Down Expand Up @@ -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;',
},
],
},
],
},
],
});

0 comments on commit 8293546

Please sign in to comment.