Skip to content

fix(eslint-plugin): [no-confusing-non-null-assertion] check !in and !instanceof #9994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 23, 2024
Merged

fix(eslint-plugin): [no-confusing-non-null-assertion] check !in and !instanceof #9994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f5cf233
ban !in and !instanceof
kirkwaiblinger Sep 16, 2024
0652797
lintfix
kirkwaiblinger Sep 16, 2024
d3434c7
Merge branch 'main' into !instanceof
kirkwaiblinger Sep 23, 2024
76298e1
snapshots
kirkwaiblinger Sep 23, 2024
e3172b7
cov
kirkwaiblinger Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions packages/eslint-plugin/docs/rules/no-confusing-non-null-assertion.mdx
View file
Open in desktop
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
View file
Open in desktop
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, ')'),
];
}
4 changes: 2 additions & 2 deletions ...ges/eslint-plugin/tests/docs-eslint-output-snapshots/no-confusing-non-null-assertion.shot
View file
Open in desktop

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

73 changes: 72 additions & 1 deletion packages/eslint-plugin/tests/rules/no-confusing-non-null-assertion.test.ts
View file
Open in desktop
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;',
},
],
},
],
},
],
});
Loading