From a07ac2da91d44bab2d64854f4f7566e12ed4db98 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Thu, 12 May 2022 16:55:01 +0200 Subject: [PATCH 01/24] feat(eslint-plugin): added ignoreTernaryTests option to prefer-nullish-coalescing rule --- .../docs/rules/prefer-nullish-coalescing.md | 38 +++ .../src/rules/prefer-nullish-coalescing.ts | 226 +++++++++++++++++- .../rules/prefer-nullish-coalescing.test.ts | 124 +++++++++- 3 files changed, 385 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md index 9b1dfe98820..0d1f96d8b70 100644 --- a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md +++ b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md @@ -46,6 +46,7 @@ This rule aims enforce the usage of the safer operator. ```ts type Options = [ { + ignoreTernaryTests?: boolean; ignoreConditionalTests?: boolean; ignoreMixedLogicalExpressions?: boolean; }, @@ -53,12 +54,49 @@ type Options = [ const defaultOptions = [ { + ignoreTernaryTests: true; ignoreConditionalTests: true, ignoreMixedLogicalExpressions: true, }, ]; ``` +### `ignoreTernaryTests` + +Setting this option to `true` (the default) will cause the rule to ignore any ternary expressions that could be simplified by using the nullish coalescing operator. + +Incorrect code for `ignoreTernaryTests: false`, and correct code for `ignoreTernaryTests: true`: + +```ts +const foo: any = 'bar'; +foo !== undefined && foo !== null ? foo : 'a string'; +foo === undefined || foo === null ? 'a string' : foo; + +const foo: ?string = 'bar'; +foo !== undefined ? foo : 'a string'; +foo === undefined ? 'a string' : foo; + +const foo: string | null = 'bar'; +foo !== null ? foo : 'a string'; +foo === null ? 'a string' : foo; +``` + +Correct code for `ignoreTernaryTests: false`: + +```ts +const foo: any = 'bar'; +foo ?? 'a string'; +foo ?? 'a string'; + +const foo: ?string = 'bar'; +foo ?? 'a string'; +foo ?? 'a string'; + +const foo: string | null = 'bar'; +foo ?? 'a string'; +foo ?? 'a string'; +``` + ### `ignoreConditionalTests` Setting this option to `true` (the default) will cause the rule to ignore any cases that are located within a conditional test. diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index c1676d3fd32..61b353038d4 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -5,14 +5,20 @@ import { TSESTree, } from '@typescript-eslint/utils'; import * as util from '../util'; +import * as ts from 'typescript'; export type Options = [ { ignoreConditionalTests?: boolean; + ignoreTernaryTests?: boolean; ignoreMixedLogicalExpressions?: boolean; }, ]; -export type MessageIds = 'preferNullish' | 'suggestNullish'; + +export type MessageIds = + | 'preferNullish' + | 'preferNullishOverTernary' + | 'suggestNullish'; export default util.createRule({ name: 'prefer-nullish-coalescing', @@ -29,6 +35,8 @@ export default util.createRule({ messages: { preferNullish: 'Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.', + preferNullishOverTernary: + 'Prefer using nullish coalescing operator (`??`) instead of ternary expression, as it is simpler to read.', suggestNullish: 'Fix to nullish coalescing operator (`??`).', }, schema: [ @@ -38,6 +46,9 @@ export default util.createRule({ ignoreConditionalTests: { type: 'boolean', }, + ignoreTernaryTests: { + type: 'boolean', + }, ignoreMixedLogicalExpressions: { type: 'boolean', }, @@ -52,15 +63,87 @@ export default util.createRule({ defaultOptions: [ { ignoreConditionalTests: true, + ignoreTernaryTests: true, ignoreMixedLogicalExpressions: true, }, ], - create(context, [{ ignoreConditionalTests, ignoreMixedLogicalExpressions }]) { + create( + context, + [ + { + ignoreConditionalTests, + ignoreTernaryTests, + ignoreMixedLogicalExpressions, + }, + ], + ) { const parserServices = util.getParserServices(context); const sourceCode = context.getSourceCode(); const checker = parserServices.program.getTypeChecker(); return { + ConditionalExpression(node: TSESTree.ConditionalExpression): void { + if (ignoreTernaryTests) { + return; + } + + let identifier: TSESTree.Identifier; + let alternate: TSESTree.Expression; + let requiredOperator: '!==' | '==='; + if ( + node.consequent.type === AST_NODE_TYPES.Identifier && + ((node.test.type === AST_NODE_TYPES.BinaryExpression && + node.test.operator === '!==') || + (node.test.type === AST_NODE_TYPES.LogicalExpression && + node.test.left.type === AST_NODE_TYPES.BinaryExpression && + node.test.left.operator === '!==')) + ) { + identifier = node.consequent; + alternate = node.alternate; + requiredOperator = '!=='; + } else if (node.alternate.type === AST_NODE_TYPES.Identifier) { + identifier = node.alternate; + alternate = node.consequent; + requiredOperator = '==='; + } else { + return; + } + + if ( + isFixableExplicitTernary({ + requiredOperator, + identifier, + node, + }) || + isFixableImplicitTernary({ + parserServices, + checker, + requiredOperator, + identifier, + node, + }) + ) { + context.report({ + node, + messageId: 'preferNullishOverTernary', + suggest: [ + { + messageId: 'suggestNullish', + fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix { + return fixer.replaceText( + node, + `${identifier.name} ?? ${sourceCode.text.slice( + alternate.range[0], + alternate.range[1], + )}`, + ); + }, + }, + ], + }); + } + }, + 'LogicalExpression[operator = "||"]'( node: TSESTree.LogicalExpression, ): void { @@ -181,3 +264,142 @@ function isMixedLogicalExpression(node: TSESTree.LogicalExpression): boolean { return false; } + +/** + * This is for cases where we check both undefined and null. + * example: foo === undefined && foo === null ? 'a string' : foo + * output: foo ?? 'a string' + */ +function isFixableExplicitTernary({ + requiredOperator, + identifier, + node, +}: { + requiredOperator: '!==' | '==='; + identifier: TSESTree.Identifier; + node: TSESTree.ConditionalExpression; +}): boolean { + if (node.test.type !== AST_NODE_TYPES.LogicalExpression) { + return false; + } + if (requiredOperator === '===' && node.test.operator === '&&') { + return false; + } + const { left, right } = node.test; + if ( + left.type !== AST_NODE_TYPES.BinaryExpression || + right.type !== AST_NODE_TYPES.BinaryExpression + ) { + return false; + } + + if ( + left.operator !== requiredOperator || + right.operator !== requiredOperator + ) { + return false; + } + + const isIdentifier = ( + i: TSESTree.Expression | TSESTree.PrivateIdentifier, + ): boolean => + i.type === AST_NODE_TYPES.Identifier && i.name === identifier.name; + + const hasUndefinedCheck = + (isIdentifier(left.left) && isUndefined(left.right)) || + (isIdentifier(left.right) && isUndefined(left.left)) || + (isIdentifier(right.left) && isUndefined(right.right)) || + (isIdentifier(right.right) && isUndefined(right.left)); + + if (!hasUndefinedCheck) { + return false; + } + + const hasNullCheck = + (isIdentifier(left.left) && isNull(left.right)) || + (isIdentifier(left.right) && isNull(left.left)) || + (isIdentifier(right.left) && isNull(right.right)) || + (isIdentifier(right.right) && isNull(right.left)); + + if (!hasNullCheck) { + return false; + } + + return true; +} + +/** + * This is for cases where we check either undefined or null and fall back to + * using type information to ensure that our checks are correct. + * example: const foo:? string = 'bar'; + * foo !== undefined ? foo : 'a string'; + * output: foo ?? 'a string' + */ +function isFixableImplicitTernary({ + parserServices, + checker, + requiredOperator, + identifier, + node, +}: { + parserServices: ReturnType; + checker: ts.TypeChecker; + requiredOperator: '!==' | '==='; + identifier: TSESTree.Identifier; + node: TSESTree.ConditionalExpression; +}): boolean { + if (node.test.type !== AST_NODE_TYPES.BinaryExpression) { + return false; + } + const { left, right, operator } = node.test; + if (operator !== requiredOperator) { + return false; + } + const isIdentifier = ( + i: TSESTree.Expression | TSESTree.PrivateIdentifier, + ): boolean => + i.type === AST_NODE_TYPES.Identifier && i.name === identifier.name; + + const i = isIdentifier(left) ? left : isIdentifier(right) ? right : null; + if (!i) { + return false; + } + + const tsNode = parserServices.esTreeNodeToTSNodeMap.get(i); + const type = checker.getTypeAtLocation(tsNode); + const flags = util.getTypeFlags(type); + + if (flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)) { + return false; + } + + const hasNullType = (flags & ts.TypeFlags.Null) !== 0; + const hasUndefinedType = (flags & ts.TypeFlags.Undefined) !== 0; + + if ( + (hasUndefinedType && hasNullType) || + (!hasUndefinedType && !hasNullType) + ) { + return false; + } + + if (hasNullType && (isNull(right) || isNull(left))) { + return true; + } + + if (hasUndefinedType && (isUndefined(right) || isUndefined(left))) { + return true; + } + + return false; +} + +function isUndefined( + i: TSESTree.Expression | TSESTree.PrivateIdentifier, +): boolean { + return i.type === AST_NODE_TYPES.Identifier && i.name === 'undefined'; +} + +function isNull(i: TSESTree.Expression | TSESTree.PrivateIdentifier): boolean { + return i.type === AST_NODE_TYPES.Literal && i.value === null; +} diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index 8096df94c32..94752b2a1bb 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -71,6 +71,34 @@ x ?? 'foo'; `, ), + { + code: 'x !== undefined && x !== null ? x : y;', + options: [{ ignoreTernaryTests: true }], + }, + + ...[ + 'x !== null && x !== undefined && x !== 5 ? x : y', + 'x === null || x === undefined || x === 5 ? x : y', + 'x === undefined && x !== null ? x : y;', + 'x === undefined && x === null ? x : y;', + 'x !== undefined && x === null ? x : y;', + 'x === undefined || x !== null ? x : y;', + 'x === undefined || x === null ? x : y;', + 'x !== undefined || x === null ? x : y;', + 'x !== undefined || x === null ? y : x;', + ` +declare const x: string | undefined | null; +x !== undefined ? x : y; + `, + ` +declare const x: string | undefined | null; +x !== null ? x : y; + `, + ].map(code => ({ + code: code.trim(), + options: [{ ignoreTernaryTests: false }] as const, + })), + // ignoreConditionalTests ...nullishTypeValidTest((nullish, type) => ({ code: ` @@ -166,6 +194,101 @@ x ?? 'foo'; ], })), + ...[ + 'x !== undefined && x !== null ? x : y;', + 'x !== null && x !== undefined ? x : y;', + 'x === undefined || x === null ? y : x;', + 'x === null || x === undefined ? y : x;', + 'undefined !== x && x !== null ? x : y;', + 'null !== x && x !== undefined ? x : y;', + 'undefined === x || x === null ? y : x;', + 'null === x || x === undefined ? y : x;', + 'x !== undefined && null !== x ? x : y;', + 'x !== null && undefined !== x ? x : y;', + 'x === undefined || null === x ? y : x;', + 'x === null || undefined === x ? y : x;', + 'undefined !== x && null !== x ? x : y;', + 'null !== x && undefined !== x ? x : y;', + 'undefined === x || null === x ? y : x;', + 'null === x || undefined === x ? y : x;', + ].map(code => ({ + code, + output: null, + options: [{ ignoreTernaryTests: false }] as const, + errors: [ + { + messageId: 'preferNullishOverTernary' as const, + line: 1, + column: 1, + endLine: 1, + endColumn: 38, + suggestions: [ + { + messageId: 'suggestNullish' as const, + output: 'x ?? y;', + }, + ], + }, + ], + })), + + ...[ + ` +declare const x: string | undefined; +x !== undefined ? x : y; + `.trim(), + ` +declare const x: string | undefined; +undefined !== x ? x : y; + `.trim(), + ` +declare const x: string | undefined; +undefined === x ? y : x; + `.trim(), + ` +declare const x: string | undefined; +undefined === x ? y : x; + `.trim(), + ` +declare const x: string | null; +x !== null ? x : y; + `.trim(), + ` +declare const x: string | null; +null !== x ? x : y; + `.trim(), + ` +declare const x: string | null; +null === x ? y : x; + `.trim(), + ` +declare const x: string | null; +null === x ? y : x; + `.trim(), + ].map(code => ({ + code, + output: null, + options: [{ ignoreTernaryTests: false }] as const, + errors: [ + { + messageId: 'preferNullishOverTernary' as const, + line: 2, + column: 1, + endLine: 2, + endColumn: code.split('\n')[1].length, + suggestions: [ + { + messageId: 'suggestNullish' as const, + output: ` +${code.split('\n')[0]} +x ?? y; + `.trim(), + }, + ], + }, + ], + })), + // ignoreConditionalTests ...nullishTypeInvalidTest((nullish, type) => ({ code: ` @@ -482,7 +605,6 @@ if (function werid() { return x ?? 'foo' }) {} }, ], })), - // https://github.com/typescript-eslint/typescript-eslint/issues/1290 ...nullishTypeInvalidTest((nullish, type) => ({ code: ` From 0487bb7c3e3db5f0b1ee4c2dc84eec517338e2cf Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Mon, 16 May 2022 09:52:22 +0200 Subject: [PATCH 02/24] feat(eslint-plugin): added checking of loose equal ternary cases for prefer-nullish-coalescing rule --- .../docs/rules/prefer-nullish-coalescing.md | 4 ++ .../src/rules/prefer-nullish-coalescing.ts | 46 ++++++++++++++++++- .../rules/prefer-nullish-coalescing.test.ts | 18 +++++++- 3 files changed, 65 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md index 0d1f96d8b70..10010d1b93b 100644 --- a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md +++ b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md @@ -71,6 +71,8 @@ Incorrect code for `ignoreTernaryTests: false`, and correct code for `ignoreTern const foo: any = 'bar'; foo !== undefined && foo !== null ? foo : 'a string'; foo === undefined || foo === null ? 'a string' : foo; +foo == undefined ? 'a string' : foo; +foo == null ? 'a string' : foo; const foo: ?string = 'bar'; foo !== undefined ? foo : 'a string'; @@ -87,6 +89,8 @@ Correct code for `ignoreTernaryTests: false`: const foo: any = 'bar'; foo ?? 'a string'; foo ?? 'a string'; +foo ?? 'a string'; +foo ?? 'a string'; const foo: ?string = 'bar'; foo ?? 'a string'; diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index 61b353038d4..a6686738e8b 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -93,10 +93,11 @@ export default util.createRule({ if ( node.consequent.type === AST_NODE_TYPES.Identifier && ((node.test.type === AST_NODE_TYPES.BinaryExpression && - node.test.operator === '!==') || + (node.test.operator === '!==' || node.test.operator === '!=')) || (node.test.type === AST_NODE_TYPES.LogicalExpression && node.test.left.type === AST_NODE_TYPES.BinaryExpression && - node.test.left.operator === '!==')) + (node.test.left.operator === '!==' || + node.test.left.operator === '!='))) ) { identifier = node.consequent; alternate = node.alternate; @@ -110,6 +111,11 @@ export default util.createRule({ } if ( + isFixableLooseTernary({ + requiredOperator, + identifier, + node, + }) || isFixableExplicitTernary({ requiredOperator, identifier, @@ -328,6 +334,42 @@ function isFixableExplicitTernary({ return true; } +function isFixableLooseTernary({ + node, + identifier, + requiredOperator, +}: { + requiredOperator: '!==' | '==='; + identifier: TSESTree.Identifier; + node: TSESTree.ConditionalExpression; +}): boolean { + if (node.test.type !== AST_NODE_TYPES.BinaryExpression) { + return false; + } + + const { left, right, operator } = node.test; + if (requiredOperator === '===' && operator !== '==') { + return false; + } + if (requiredOperator === '!==' && operator !== '!=') { + return false; + } + + const isIdentifier = ( + i: TSESTree.Expression | TSESTree.PrivateIdentifier, + ): boolean => + i.type === AST_NODE_TYPES.Identifier && i.name === identifier.name; + + if (isIdentifier(right) && (isNull(left) || isUndefined(left))) { + return true; + } + + if (isIdentifier(left) && (isNull(right) || isUndefined(right))) { + return true; + } + return false; +} + /** * This is for cases where we check either undefined or null and fall back to * using type information to ensure that our checks are correct. diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index 94752b2a1bb..b5094608dab 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -86,6 +86,14 @@ x ?? 'foo'; 'x === undefined || x === null ? x : y;', 'x !== undefined || x === null ? x : y;', 'x !== undefined || x === null ? y : x;', + 'x == null ? x : y;', + 'x == undefined ? x : y;', + 'x != null ? y : x;', + 'x != undefined ? y : x;', + 'null == x ? x : y;', + 'undefined == x ? x : y;', + 'null != x ? y : x;', + 'undefined != x ? y : x;', ` declare const x: string | undefined | null; x !== undefined ? x : y; @@ -211,6 +219,14 @@ x ?? 'foo'; 'null !== x && undefined !== x ? x : y;', 'undefined === x || null === x ? y : x;', 'null === x || undefined === x ? y : x;', + 'undefined != x ? x : y;', + 'null != x ? x : y;', + 'undefined == x ? y : x;', + 'null == x ? y : x;', + 'x != undefined ? x : y;', + 'x != null ? x : y;', + 'x == undefined ? y : x;', + 'x == null ? y : x;', ].map(code => ({ code, output: null, @@ -221,7 +237,7 @@ x ?? 'foo'; line: 1, column: 1, endLine: 1, - endColumn: 38, + endColumn: code.length, suggestions: [ { messageId: 'suggestNullish' as const, From 33a6053f382ac2350fb1d1a352b273fed460bb24 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Mon, 16 May 2022 09:58:35 +0200 Subject: [PATCH 03/24] feat(eslint-plugin): fixed typo in docs for prefer-nullish-coalescing rule --- packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md index 10010d1b93b..10d6ebaf8a1 100644 --- a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md +++ b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md @@ -74,7 +74,7 @@ foo === undefined || foo === null ? 'a string' : foo; foo == undefined ? 'a string' : foo; foo == null ? 'a string' : foo; -const foo: ?string = 'bar'; +const foo: string | undefined = 'bar'; foo !== undefined ? foo : 'a string'; foo === undefined ? 'a string' : foo; From b7b07587a1463b34e635fac7ae66113494b8a31c Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Mon, 16 May 2022 10:20:27 +0200 Subject: [PATCH 04/24] feat(eslint-plugin): added more test cases for prefer-nullish-coalescing rule --- .../rules/prefer-nullish-coalescing.test.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index b5094608dab..3f061ffd857 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -77,6 +77,7 @@ x ?? 'foo'; }, ...[ + 'x !== undefined && x !== null ? "foo" : "bar";', 'x !== null && x !== undefined && x !== 5 ? x : y', 'x === null || x === undefined || x === 5 ? x : y', 'x === undefined && x !== null ? x : y;', @@ -95,6 +96,10 @@ x ?? 'foo'; 'null != x ? y : x;', 'undefined != x ? y : x;', ` +declare const x: string | null; +x === undefined ? x : y; + `, + ` declare const x: string | undefined | null; x !== undefined ? x : y; `, @@ -102,6 +107,18 @@ x !== undefined ? x : y; declare const x: string | undefined | null; x !== null ? x : y; `, + ` +declare const x: string | null | any; +x === null ? x : y; + `, + ` +declare const x: string | null | unknown; +x === null ? x : y; + `, + ` +declare const x: string | undefined; +x === null ? x : y; + `, ].map(code => ({ code: code.trim(), options: [{ ignoreTernaryTests: false }] as const, From 42e973749b8bea9fb6cc63829f0ef369fa47b873 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Thu, 19 May 2022 16:22:34 +0200 Subject: [PATCH 05/24] feat(eslint-plugin): added support for MemberExpressions for ignoreTernaryTests option --- .../src/rules/prefer-nullish-coalescing.ts | 107 ++++++++++++++---- .../rules/prefer-nullish-coalescing.test.ts | 63 +++++++---- 2 files changed, 131 insertions(+), 39 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index a6686738e8b..940ba018fdf 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -87,11 +87,12 @@ export default util.createRule({ return; } - let identifier: TSESTree.Identifier; + let identifier: TSESTree.Identifier | TSESTree.MemberExpression; let alternate: TSESTree.Expression; let requiredOperator: '!==' | '==='; if ( - node.consequent.type === AST_NODE_TYPES.Identifier && + (node.consequent.type === AST_NODE_TYPES.Identifier || + node.consequent.type === AST_NODE_TYPES.MemberExpression) && ((node.test.type === AST_NODE_TYPES.BinaryExpression && (node.test.operator === '!==' || node.test.operator === '!=')) || (node.test.type === AST_NODE_TYPES.LogicalExpression && @@ -102,7 +103,10 @@ export default util.createRule({ identifier = node.consequent; alternate = node.alternate; requiredOperator = '!=='; - } else if (node.alternate.type === AST_NODE_TYPES.Identifier) { + } else if ( + node.alternate.type === AST_NODE_TYPES.Identifier || + node.alternate.type === AST_NODE_TYPES.MemberExpression + ) { identifier = node.alternate; alternate = node.consequent; requiredOperator = '==='; @@ -138,7 +142,10 @@ export default util.createRule({ fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix { return fixer.replaceText( node, - `${identifier.name} ?? ${sourceCode.text.slice( + `${sourceCode.text.slice( + identifier.range[0], + identifier.range[1], + )} ?? ${sourceCode.text.slice( alternate.range[0], alternate.range[1], )}`, @@ -282,7 +289,7 @@ function isFixableExplicitTernary({ node, }: { requiredOperator: '!==' | '==='; - identifier: TSESTree.Identifier; + identifier: TSESTree.Identifier | TSESTree.MemberExpression; node: TSESTree.ConditionalExpression; }): boolean { if (node.test.type !== AST_NODE_TYPES.LogicalExpression) { @@ -306,10 +313,7 @@ function isFixableExplicitTernary({ return false; } - const isIdentifier = ( - i: TSESTree.Expression | TSESTree.PrivateIdentifier, - ): boolean => - i.type === AST_NODE_TYPES.Identifier && i.name === identifier.name; + const isIdentifier = isEqualIndentierCurry(identifier); const hasUndefinedCheck = (isIdentifier(left.left) && isUndefined(left.right)) || @@ -340,7 +344,7 @@ function isFixableLooseTernary({ requiredOperator, }: { requiredOperator: '!==' | '==='; - identifier: TSESTree.Identifier; + identifier: TSESTree.Identifier | TSESTree.MemberExpression; node: TSESTree.ConditionalExpression; }): boolean { if (node.test.type !== AST_NODE_TYPES.BinaryExpression) { @@ -355,10 +359,7 @@ function isFixableLooseTernary({ return false; } - const isIdentifier = ( - i: TSESTree.Expression | TSESTree.PrivateIdentifier, - ): boolean => - i.type === AST_NODE_TYPES.Identifier && i.name === identifier.name; + const isIdentifier = isEqualIndentierCurry(identifier); if (isIdentifier(right) && (isNull(left) || isUndefined(left))) { return true; @@ -387,7 +388,7 @@ function isFixableImplicitTernary({ parserServices: ReturnType; checker: ts.TypeChecker; requiredOperator: '!==' | '==='; - identifier: TSESTree.Identifier; + identifier: TSESTree.Identifier | TSESTree.MemberExpression; node: TSESTree.ConditionalExpression; }): boolean { if (node.test.type !== AST_NODE_TYPES.BinaryExpression) { @@ -397,10 +398,7 @@ function isFixableImplicitTernary({ if (operator !== requiredOperator) { return false; } - const isIdentifier = ( - i: TSESTree.Expression | TSESTree.PrivateIdentifier, - ): boolean => - i.type === AST_NODE_TYPES.Identifier && i.name === identifier.name; + const isIdentifier = isEqualIndentierCurry(identifier); const i = isIdentifier(left) ? left : isIdentifier(right) ? right : null; if (!i) { @@ -436,6 +434,77 @@ function isFixableImplicitTernary({ return false; } +function isEqualIndentierCurry( + a: TSESTree.Identifier | TSESTree.MemberExpression, +) { + if (a.type === AST_NODE_TYPES.Identifier) { + return function (b: any): boolean { + if (a.type !== b.type) { + return false; + } + return !!a.name && !!b.name && a.name === b.name; + }; + } + return function (b: any): boolean { + if (a.type !== b.type) { + return false; + } + return isEqualMemberExpression(a, b); + }; +} +function isEqualMemberExpression( + a: TSESTree.MemberExpression, + b: TSESTree.MemberExpression, +): boolean { + return ( + isEqualMemberExpressionProperty(a.property, b.property) && + isEqualMemberExpressionObject(a.object, b.object) + ); +} + +function isEqualMemberExpressionProperty( + a: TSESTree.MemberExpression['property'], + b: TSESTree.MemberExpression['property'], +): boolean { + if (a.type !== b.type) { + return false; + } + if (a.type === AST_NODE_TYPES.ThisExpression) { + return true; + } + if ( + a.type === AST_NODE_TYPES.Literal || + a.type === AST_NODE_TYPES.Identifier + ) { + return ( + // @ts-ignore + (!!a.name && !!b.name && a.name === b.name) || + // @ts-ignore + (!!a.value && !!b.value && a.value === b.value) + ); + } + if (a.type === AST_NODE_TYPES.MemberExpression) { + return isEqualMemberExpression(a, b as typeof a); + } + return false; +} + +function isEqualMemberExpressionObject(a: any, b: any): boolean { + if (a.type !== b.type) { + return false; + } + if (a.type === AST_NODE_TYPES.ThisExpression) { + return true; + } + if (a.type === AST_NODE_TYPES.Identifier) { + return a.name === b.name; + } + if (a.type === AST_NODE_TYPES.MemberExpression) { + return isEqualMemberExpression(a, b); + } + return false; +} + function isUndefined( i: TSESTree.Expression | TSESTree.PrivateIdentifier, ): boolean { diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index 3f061ffd857..9ad3be9a3ce 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -244,26 +244,49 @@ x ?? 'foo'; 'x != null ? x : y;', 'x == undefined ? y : x;', 'x == null ? y : x;', - ].map(code => ({ - code, - output: null, - options: [{ ignoreTernaryTests: false }] as const, - errors: [ - { - messageId: 'preferNullishOverTernary' as const, - line: 1, - column: 1, - endLine: 1, - endColumn: code.length, - suggestions: [ - { - messageId: 'suggestNullish' as const, - output: 'x ?? y;', - }, - ], - }, - ], - })), + ].flatMap(code => [ + { + code, + output: null, + options: [{ ignoreTernaryTests: false }] as const, + errors: [ + { + messageId: 'preferNullishOverTernary' as const, + line: 1, + column: 1, + endLine: 1, + endColumn: code.length, + suggestions: [ + { + messageId: 'suggestNullish' as const, + output: 'x ?? y;', + }, + ], + }, + ], + }, + { + code: code.replace(/x/g, 'x.z[1][this[this.o]]["3"][a.b.c]'), + output: null, + options: [{ ignoreTernaryTests: false }] as const, + errors: [ + { + messageId: 'preferNullishOverTernary' as const, + line: 1, + column: 1, + endLine: 1, + endColumn: code.replace(/x/g, 'x.z[1][this[this.o]]["3"][a.b.c]') + .length, + suggestions: [ + { + messageId: 'suggestNullish' as const, + output: 'x.z[1][this[this.o]]["3"][a.b.c] ?? y;', + }, + ], + }, + ], + }, + ]), ...[ ` From 61b9680f21d0275564baa9badede9d92f3193e14 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Wed, 25 May 2022 13:17:44 +0200 Subject: [PATCH 06/24] refactor(eslint-plugin): cleanup prefer-nullish-coalescing rule --- .../docs/rules/prefer-nullish-coalescing.md | 2 +- .../src/rules/prefer-nullish-coalescing.ts | 146 +++++------------- packages/eslint-plugin/src/util/index.ts | 3 + .../eslint-plugin/src/util/isNodeEqual.ts | 77 +++++++++ packages/eslint-plugin/src/util/isNull.ts | 5 + .../eslint-plugin/src/util/isUndefined.ts | 5 + .../rules/prefer-nullish-coalescing.test.ts | 8 + .../tests/util/isNodeEqual.test.ts | 106 +++++++++++++ 8 files changed, 244 insertions(+), 108 deletions(-) create mode 100644 packages/eslint-plugin/src/util/isNodeEqual.ts create mode 100644 packages/eslint-plugin/src/util/isNull.ts create mode 100644 packages/eslint-plugin/src/util/isUndefined.ts create mode 100644 packages/eslint-plugin/tests/util/isNodeEqual.test.ts diff --git a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md index 10d6ebaf8a1..21e2231f356 100644 --- a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md +++ b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md @@ -92,7 +92,7 @@ foo ?? 'a string'; foo ?? 'a string'; foo ?? 'a string'; -const foo: ?string = 'bar'; +const foo: string | undefined = 'bar'; foo ?? 'a string'; foo ?? 'a string'; diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index 940ba018fdf..e90aa3b6312 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -36,7 +36,7 @@ export default util.createRule({ preferNullish: 'Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.', preferNullishOverTernary: - 'Prefer using nullish coalescing operator (`??`) instead of ternary expression, as it is simpler to read.', + 'Prefer using nullish coalescing operator (`??`) instead of a ternary expression, as it is simpler to read.', suggestNullish: 'Fix to nullish coalescing operator (`??`).', }, schema: [ @@ -279,7 +279,7 @@ function isMixedLogicalExpression(node: TSESTree.LogicalExpression): boolean { } /** - * This is for cases where we check both undefined and null. + * This is for ternary expressions where we check both undefined and null. * example: foo === undefined && foo === null ? 'a string' : foo * output: foo ?? 'a string' */ @@ -313,23 +313,22 @@ function isFixableExplicitTernary({ return false; } - const isIdentifier = isEqualIndentierCurry(identifier); - const hasUndefinedCheck = - (isIdentifier(left.left) && isUndefined(left.right)) || - (isIdentifier(left.right) && isUndefined(left.left)) || - (isIdentifier(right.left) && isUndefined(right.right)) || - (isIdentifier(right.right) && isUndefined(right.left)); + (util.isNodeEqual(identifier, left.left) && util.isUndefined(left.right)) || + (util.isNodeEqual(identifier, left.right) && util.isUndefined(left.left)) || + (util.isNodeEqual(identifier, right.left) && + util.isUndefined(right.right)) || + (util.isNodeEqual(identifier, right.right) && util.isUndefined(right.left)); if (!hasUndefinedCheck) { return false; } const hasNullCheck = - (isIdentifier(left.left) && isNull(left.right)) || - (isIdentifier(left.right) && isNull(left.left)) || - (isIdentifier(right.left) && isNull(right.right)) || - (isIdentifier(right.right) && isNull(right.left)); + (util.isNodeEqual(identifier, left.left) && util.isNull(left.right)) || + (util.isNodeEqual(identifier, left.right) && util.isNull(left.left)) || + (util.isNodeEqual(identifier, right.left) && util.isNull(right.right)) || + (util.isNodeEqual(identifier, right.right) && util.isNull(right.left)); if (!hasNullCheck) { return false; @@ -338,6 +337,11 @@ function isFixableExplicitTernary({ return true; } +/* + * this is for ternary expressions where == or != is used. + * example: foo == null ? 'a string' : a + * example: foo != undefined ? a : 'a string' + */ function isFixableLooseTernary({ node, identifier, @@ -359,21 +363,26 @@ function isFixableLooseTernary({ return false; } - const isIdentifier = isEqualIndentierCurry(identifier); - - if (isIdentifier(right) && (isNull(left) || isUndefined(left))) { + if ( + util.isNodeEqual(identifier, right) && + (util.isNull(left) || util.isUndefined(left)) + ) { return true; } - if (isIdentifier(left) && (isNull(right) || isUndefined(right))) { + if ( + util.isNodeEqual(identifier, left) && + (util.isNull(right) || util.isUndefined(right)) + ) { return true; } + return false; } /** - * This is for cases where we check either undefined or null and fall back to - * using type information to ensure that our checks are correct. + * This is for ternary expressions where we check only undefined or null and + * need to type information to ensure that our checks are still correct. * example: const foo:? string = 'bar'; * foo !== undefined ? foo : 'a string'; * output: foo ?? 'a string' @@ -398,9 +407,12 @@ function isFixableImplicitTernary({ if (operator !== requiredOperator) { return false; } - const isIdentifier = isEqualIndentierCurry(identifier); - const i = isIdentifier(left) ? left : isIdentifier(right) ? right : null; + const i = util.isNodeEqual(identifier, left) + ? left + : util.isNodeEqual(identifier, right) + ? right + : null; if (!i) { return false; } @@ -417,100 +429,20 @@ function isFixableImplicitTernary({ const hasUndefinedType = (flags & ts.TypeFlags.Undefined) !== 0; if ( - (hasUndefinedType && hasNullType) || - (!hasUndefinedType && !hasNullType) + hasNullType && + !hasUndefinedType && + (util.isNull(right) || util.isNull(left)) ) { - return false; - } - - if (hasNullType && (isNull(right) || isNull(left))) { return true; } - if (hasUndefinedType && (isUndefined(right) || isUndefined(left))) { - return true; - } - - return false; -} - -function isEqualIndentierCurry( - a: TSESTree.Identifier | TSESTree.MemberExpression, -) { - if (a.type === AST_NODE_TYPES.Identifier) { - return function (b: any): boolean { - if (a.type !== b.type) { - return false; - } - return !!a.name && !!b.name && a.name === b.name; - }; - } - return function (b: any): boolean { - if (a.type !== b.type) { - return false; - } - return isEqualMemberExpression(a, b); - }; -} -function isEqualMemberExpression( - a: TSESTree.MemberExpression, - b: TSESTree.MemberExpression, -): boolean { - return ( - isEqualMemberExpressionProperty(a.property, b.property) && - isEqualMemberExpressionObject(a.object, b.object) - ); -} - -function isEqualMemberExpressionProperty( - a: TSESTree.MemberExpression['property'], - b: TSESTree.MemberExpression['property'], -): boolean { - if (a.type !== b.type) { - return false; - } - if (a.type === AST_NODE_TYPES.ThisExpression) { - return true; - } if ( - a.type === AST_NODE_TYPES.Literal || - a.type === AST_NODE_TYPES.Identifier + hasUndefinedType && + !hasNullType && + (util.isUndefined(right) || util.isUndefined(left)) ) { - return ( - // @ts-ignore - (!!a.name && !!b.name && a.name === b.name) || - // @ts-ignore - (!!a.value && !!b.value && a.value === b.value) - ); - } - if (a.type === AST_NODE_TYPES.MemberExpression) { - return isEqualMemberExpression(a, b as typeof a); - } - return false; -} - -function isEqualMemberExpressionObject(a: any, b: any): boolean { - if (a.type !== b.type) { - return false; - } - if (a.type === AST_NODE_TYPES.ThisExpression) { return true; } - if (a.type === AST_NODE_TYPES.Identifier) { - return a.name === b.name; - } - if (a.type === AST_NODE_TYPES.MemberExpression) { - return isEqualMemberExpression(a, b); - } - return false; -} -function isUndefined( - i: TSESTree.Expression | TSESTree.PrivateIdentifier, -): boolean { - return i.type === AST_NODE_TYPES.Identifier && i.name === 'undefined'; -} - -function isNull(i: TSESTree.Expression | TSESTree.PrivateIdentifier): boolean { - return i.type === AST_NODE_TYPES.Literal && i.value === null; + return false; } diff --git a/packages/eslint-plugin/src/util/index.ts b/packages/eslint-plugin/src/util/index.ts index b2932466388..8dbff44a671 100644 --- a/packages/eslint-plugin/src/util/index.ts +++ b/packages/eslint-plugin/src/util/index.ts @@ -9,6 +9,9 @@ export * from './getThisExpression'; export * from './getWrappingFixer'; export * from './misc'; export * from './objectIterators'; +export * from './isNull'; +export * from './isUndefined'; +export * from './isNodeEqual'; // this is done for convenience - saves migrating all of the old rules export * from '@typescript-eslint/type-utils'; diff --git a/packages/eslint-plugin/src/util/isNodeEqual.ts b/packages/eslint-plugin/src/util/isNodeEqual.ts new file mode 100644 index 00000000000..2bb4b876158 --- /dev/null +++ b/packages/eslint-plugin/src/util/isNodeEqual.ts @@ -0,0 +1,77 @@ +import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/utils'; +/* + * Supported: + * Identifier Literal MemberExpression ThisExpression + * Currently not supported: + * ArrayExpression ArrayPattern ArrowFunctionExpression AssignmentExpression + * AssignmentPattern AwaitExpression BinaryExpression BlockStatement + * BreakStatement CallExpression CatchClause ChainExpression ClassBody + * ClassDeclaration ClassExpression ConditionalExpression ContinueStatement + * DebuggerStatement Decorator DoWhileStatement EmptyStatement + * ExportAllDeclaration ExportDefaultDeclaration ExportNamedDeclaration + * ExportSpecifier ExpressionStatement ForInStatement ForOfStatement + * ForStatement FunctionDeclaration FunctionExpression IfStatement + * ImportAttribute ImportDeclaration ImportDefaultSpecifier ImportExpression + * ImportNamespaceSpecifier ImportSpecifier JSXAttribute JSXClosingElement + * JSXClosingFragment JSXElement JSXEmptyExpression JSXExpressionContainer + * JSXFragment JSXIdentifier JSXMemberExpression JSXNamespacedName + * JSXOpeningElement JSXOpeningFragment JSXSpreadAttribute JSXSpreadChild + * JSXText LabeledStatement LogicalExpression MetaProperty MethodDefinition + * NewExpression ObjectExpression ObjectPattern PrivateIdentifier Program + * Property PropertyDefinition RestElement ReturnStatement SequenceExpression + * SpreadElement StaticBlock Super SwitchCase SwitchStatement + * TaggedTemplateExpression TemplateElement TemplateLiteral ThrowStatement + * TryStatement TSAbstractKeyword TSAbstractMethodDefinition + * TSAbstractPropertyDefinition TSAnyKeyword TSArrayType TSAsExpression + * TSAsyncKeyword TSBigIntKeyword TSBooleanKeyword TSCallSignatureDeclaration + * TSClassImplements TSConditionalType TSConstructorType + * TSConstructSignatureDeclaration TSDeclareFunction TSDeclareKeyword + * TSEmptyBodyFunctionExpression TSEnumDeclaration TSEnumMember + * TSExportAssignment TSExportKeyword TSExternalModuleReference + * TSFunctionType TSImportEqualsDeclaration TSImportType TSIndexedAccessType + * TSIndexSignature TSInferType TSInterfaceBody TSInterfaceDeclaration + * TSInterfaceHeritage TSIntersectionType TSIntrinsicKeyword TSLiteralType + * TSMappedType TSMethodSignature TSModuleBlock TSModuleDeclaration + * TSNamedTupleMember TSNamespaceExportDeclaration TSNeverKeyword + * TSNonNullExpression TSNullKeyword TSNumberKeyword TSObjectKeyword + * TSOptionalType TSParameterProperty TSPrivateKeyword TSPropertySignature + * TSProtectedKeyword TSPublicKeyword TSQualifiedName TSReadonlyKeyword + * TSRestType TSStaticKeyword TSStringKeyword TSSymbolKeyword + * TSTemplateLiteralType TSThisType TSTupleType TSTypeAliasDeclaration + * TSTypeAnnotation TSTypeAssertion TSTypeLiteral TSTypeOperator + * TSTypeParameter TSTypeParameterDeclaration TSTypeParameterInstantiation + * TSTypePredicate TSTypeQuery TSTypeReference TSUndefinedKeyword TSUnionType + * TSUnknownKeyword TSVoidKeyword UnaryExpression UpdateExpression + * VariableDeclaration VariableDeclarator WhileStatement WithStatement + * YieldExpression + */ + +export function isNodeEqual(a: TSESTree.Node, b: TSESTree.Node): boolean { + if (a.type !== b.type) { + return false; + } + if ( + a.type === AST_NODE_TYPES.ThisExpression && + b.type === AST_NODE_TYPES.ThisExpression + ) { + return true; + } + if (a.type === AST_NODE_TYPES.Literal && b.type === AST_NODE_TYPES.Literal) { + return a.value === b.value; + } + if ( + a.type === AST_NODE_TYPES.Identifier && + b.type === AST_NODE_TYPES.Identifier + ) { + return a.name === b.name; + } + if ( + a.type === AST_NODE_TYPES.MemberExpression && + b.type === AST_NODE_TYPES.MemberExpression + ) { + return ( + isNodeEqual(a.property, b.property) && isNodeEqual(a.object, b.object) + ); + } + return false; +} diff --git a/packages/eslint-plugin/src/util/isNull.ts b/packages/eslint-plugin/src/util/isNull.ts new file mode 100644 index 00000000000..a42757a025a --- /dev/null +++ b/packages/eslint-plugin/src/util/isNull.ts @@ -0,0 +1,5 @@ +import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/utils'; + +export function isNull(i: TSESTree.Node): boolean { + return i.type === AST_NODE_TYPES.Literal && i.value === null; +} diff --git a/packages/eslint-plugin/src/util/isUndefined.ts b/packages/eslint-plugin/src/util/isUndefined.ts new file mode 100644 index 00000000000..220dedd0dd6 --- /dev/null +++ b/packages/eslint-plugin/src/util/isUndefined.ts @@ -0,0 +1,5 @@ +import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/utils'; + +export function isUndefined(i: TSESTree.Node): boolean { + return i.type === AST_NODE_TYPES.Identifier && i.name === 'undefined'; +} diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index 9ad3be9a3ce..e13103b895b 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -96,6 +96,14 @@ x ?? 'foo'; 'null != x ? y : x;', 'undefined != x ? y : x;', ` +declare const x: string; +x === null ? x : y; + `, + ` +declare const x: string | undefined; +x === null ? x : y; + `, + ` declare const x: string | null; x === undefined ? x : y; `, diff --git a/packages/eslint-plugin/tests/util/isNodeEqual.test.ts b/packages/eslint-plugin/tests/util/isNodeEqual.test.ts new file mode 100644 index 00000000000..76da9eabdb4 --- /dev/null +++ b/packages/eslint-plugin/tests/util/isNodeEqual.test.ts @@ -0,0 +1,106 @@ +import { TSESTree, TSESLint } from '@typescript-eslint/utils'; +import { getFixturesRootDir, RuleTester } from '../RuleTester'; +import { createRule, isNodeEqual } from '../../src/util'; + +const rule = createRule({ + name: 'no-useless-expression', + defaultOptions: [], + meta: { + type: 'suggestion', + fixable: 'code', + docs: { + description: 'Remove useless expressions.', + recommended: false, + }, + messages: { + removeExpression: 'Remove useless expression', + }, + schema: [], + }, + + create(context) { + const sourceCode = context.getSourceCode(); + + return { + LogicalExpression: (node: TSESTree.LogicalExpression): void => { + if ( + (node.operator === '??' || + node.operator === '||' || + node.operator === '&&') && + isNodeEqual(node.left, node.right) + ) { + context.report({ + node, + messageId: 'removeExpression', + fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix { + return fixer.replaceText( + node, + sourceCode.text.slice(node.left.range[0], node.left.range[1]), + ); + }, + }); + } + }, + }; + }, +}); + +const rootPath = getFixturesRootDir(); +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + tsconfigRootDir: rootPath, + project: './tsconfig.json', + }, +}); + +ruleTester.run('isNodeEqual', rule, { + valid: [ + { code: 'a || b' }, + { code: '!true || true' }, + { code: 'a() || a()' }, + { code: 'foo.bar || foo.bar.foo' }, + ], + invalid: [ + { + code: 'undefined || undefined', + errors: [{ messageId: 'removeExpression' }], + output: 'undefined', + }, + { + code: 'true && true', + errors: [{ messageId: 'removeExpression' }], + output: 'true', + }, + { + code: 'a || a', + errors: [{ messageId: 'removeExpression' }], + output: 'a', + }, + { + code: 'a && a', + errors: [{ messageId: 'removeExpression' }], + output: 'a', + }, + { + code: 'a ?? a', + errors: [{ messageId: 'removeExpression' }], + output: 'a', + }, + { + code: 'foo.bar || foo.bar', + errors: [{ messageId: 'removeExpression' }], + output: 'foo.bar', + }, + { + code: 'this.foo.bar || this.foo.bar', + errors: [{ messageId: 'removeExpression' }], + output: 'this.foo.bar', + }, + { + code: 'x.z[1][this[this.o]]["3"][a.b.c] || x.z[1][this[this.o]]["3"][a.b.c]', + errors: [{ messageId: 'removeExpression' }], + output: 'x.z[1][this[this.o]]["3"][a.b.c]', + }, + ], +}); From 40609b1cf427e82478441518935b548986b01eeb Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Wed, 25 May 2022 14:07:34 +0200 Subject: [PATCH 07/24] test(eslint-plugin): added missing test case for prefer-nullish-coalescing rule --- .../eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index e13103b895b..a0fd763e33f 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -87,6 +87,8 @@ x ?? 'foo'; 'x === undefined || x === null ? x : y;', 'x !== undefined || x === null ? x : y;', 'x !== undefined || x === null ? y : x;', + 'x === null || x === null ? y : x;', + 'x === undefined || x === undefined ? y : x;', 'x == null ? x : y;', 'x == undefined ? x : y;', 'x != null ? y : x;', From e9626e9392419426e007d5fdacd8d6bd74a3bfdc Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 11:35:25 +0200 Subject: [PATCH 08/24] chore: removed currently not supported comment Co-authored-by: Josh Goldberg --- .../eslint-plugin/src/util/isNodeEqual.ts | 30 ------------------- 1 file changed, 30 deletions(-) diff --git a/packages/eslint-plugin/src/util/isNodeEqual.ts b/packages/eslint-plugin/src/util/isNodeEqual.ts index 2bb4b876158..03acc88eb25 100644 --- a/packages/eslint-plugin/src/util/isNodeEqual.ts +++ b/packages/eslint-plugin/src/util/isNodeEqual.ts @@ -2,36 +2,6 @@ import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/utils'; /* * Supported: * Identifier Literal MemberExpression ThisExpression - * Currently not supported: - * ArrayExpression ArrayPattern ArrowFunctionExpression AssignmentExpression - * AssignmentPattern AwaitExpression BinaryExpression BlockStatement - * BreakStatement CallExpression CatchClause ChainExpression ClassBody - * ClassDeclaration ClassExpression ConditionalExpression ContinueStatement - * DebuggerStatement Decorator DoWhileStatement EmptyStatement - * ExportAllDeclaration ExportDefaultDeclaration ExportNamedDeclaration - * ExportSpecifier ExpressionStatement ForInStatement ForOfStatement - * ForStatement FunctionDeclaration FunctionExpression IfStatement - * ImportAttribute ImportDeclaration ImportDefaultSpecifier ImportExpression - * ImportNamespaceSpecifier ImportSpecifier JSXAttribute JSXClosingElement - * JSXClosingFragment JSXElement JSXEmptyExpression JSXExpressionContainer - * JSXFragment JSXIdentifier JSXMemberExpression JSXNamespacedName - * JSXOpeningElement JSXOpeningFragment JSXSpreadAttribute JSXSpreadChild - * JSXText LabeledStatement LogicalExpression MetaProperty MethodDefinition - * NewExpression ObjectExpression ObjectPattern PrivateIdentifier Program - * Property PropertyDefinition RestElement ReturnStatement SequenceExpression - * SpreadElement StaticBlock Super SwitchCase SwitchStatement - * TaggedTemplateExpression TemplateElement TemplateLiteral ThrowStatement - * TryStatement TSAbstractKeyword TSAbstractMethodDefinition - * TSAbstractPropertyDefinition TSAnyKeyword TSArrayType TSAsExpression - * TSAsyncKeyword TSBigIntKeyword TSBooleanKeyword TSCallSignatureDeclaration - * TSClassImplements TSConditionalType TSConstructorType - * TSConstructSignatureDeclaration TSDeclareFunction TSDeclareKeyword - * TSEmptyBodyFunctionExpression TSEnumDeclaration TSEnumMember - * TSExportAssignment TSExportKeyword TSExternalModuleReference - * TSFunctionType TSImportEqualsDeclaration TSImportType TSIndexedAccessType - * TSIndexSignature TSInferType TSInterfaceBody TSInterfaceDeclaration - * TSInterfaceHeritage TSIntersectionType TSIntrinsicKeyword TSLiteralType - * TSMappedType TSMethodSignature TSModuleBlock TSModuleDeclaration * TSNamedTupleMember TSNamespaceExportDeclaration TSNeverKeyword * TSNonNullExpression TSNullKeyword TSNumberKeyword TSObjectKeyword * TSOptionalType TSParameterProperty TSPrivateKeyword TSPropertySignature From 81f1a1d609ba189ecbac81d0b97732436419d8c9 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 11:37:22 +0200 Subject: [PATCH 09/24] refactor: simplified return statement Co-authored-by: Josh Goldberg --- .../eslint-plugin/src/rules/prefer-nullish-coalescing.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index e90aa3b6312..9589a0333f8 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -330,11 +330,7 @@ function isFixableExplicitTernary({ (util.isNodeEqual(identifier, right.left) && util.isNull(right.right)) || (util.isNodeEqual(identifier, right.right) && util.isNull(right.left)); - if (!hasNullCheck) { - return false; - } - - return true; + return hasNullCheck; } /* From 060002a51607c3cc872d951e5205dfc1393b2ec9 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 12:57:54 +0200 Subject: [PATCH 10/24] refactor: renamed utilities --- .../src/rules/prefer-nullish-coalescing.ts | 37 ++++++++++++------- packages/eslint-plugin/src/util/index.ts | 4 +- .../src/util/{isNull.ts => isNullLiteral.ts} | 2 +- ...sUndefined.ts => isUndefinedIdentifier.ts} | 2 +- 4 files changed, 27 insertions(+), 18 deletions(-) rename packages/eslint-plugin/src/util/{isNull.ts => isNullLiteral.ts} (69%) rename packages/eslint-plugin/src/util/{isUndefined.ts => isUndefinedIdentifier.ts} (68%) diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index 9589a0333f8..9d15126a471 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -162,8 +162,10 @@ export default util.createRule({ ): void { const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); const type = checker.getTypeAtLocation(tsNode.left); - const isNullish = util.isNullableType(type, { allowUndefined: true }); - if (!isNullish) { + const isNullLiteralish = util.isNullableType(type, { + allowUndefined: true, + }); + if (!isNullLiteralish) { return; } @@ -314,21 +316,28 @@ function isFixableExplicitTernary({ } const hasUndefinedCheck = - (util.isNodeEqual(identifier, left.left) && util.isUndefined(left.right)) || - (util.isNodeEqual(identifier, left.right) && util.isUndefined(left.left)) || + (util.isNodeEqual(identifier, left.left) && + util.isUndefinedIdentifier(left.right)) || + (util.isNodeEqual(identifier, left.right) && + util.isUndefinedIdentifier(left.left)) || (util.isNodeEqual(identifier, right.left) && - util.isUndefined(right.right)) || - (util.isNodeEqual(identifier, right.right) && util.isUndefined(right.left)); + util.isUndefinedIdentifier(right.right)) || + (util.isNodeEqual(identifier, right.right) && + util.isUndefinedIdentifier(right.left)); if (!hasUndefinedCheck) { return false; } const hasNullCheck = - (util.isNodeEqual(identifier, left.left) && util.isNull(left.right)) || - (util.isNodeEqual(identifier, left.right) && util.isNull(left.left)) || - (util.isNodeEqual(identifier, right.left) && util.isNull(right.right)) || - (util.isNodeEqual(identifier, right.right) && util.isNull(right.left)); + (util.isNodeEqual(identifier, left.left) && + util.isNullLiteral(left.right)) || + (util.isNodeEqual(identifier, left.right) && + util.isNullLiteral(left.left)) || + (util.isNodeEqual(identifier, right.left) && + util.isNullLiteral(right.right)) || + (util.isNodeEqual(identifier, right.right) && + util.isNullLiteral(right.left)); return hasNullCheck; } @@ -361,14 +370,14 @@ function isFixableLooseTernary({ if ( util.isNodeEqual(identifier, right) && - (util.isNull(left) || util.isUndefined(left)) + (util.isNullLiteral(left) || util.isUndefinedIdentifier(left)) ) { return true; } if ( util.isNodeEqual(identifier, left) && - (util.isNull(right) || util.isUndefined(right)) + (util.isNullLiteral(right) || util.isUndefinedIdentifier(right)) ) { return true; } @@ -427,7 +436,7 @@ function isFixableImplicitTernary({ if ( hasNullType && !hasUndefinedType && - (util.isNull(right) || util.isNull(left)) + (util.isNullLiteral(right) || util.isNullLiteral(left)) ) { return true; } @@ -435,7 +444,7 @@ function isFixableImplicitTernary({ if ( hasUndefinedType && !hasNullType && - (util.isUndefined(right) || util.isUndefined(left)) + (util.isUndefinedIdentifier(right) || util.isUndefinedIdentifier(left)) ) { return true; } diff --git a/packages/eslint-plugin/src/util/index.ts b/packages/eslint-plugin/src/util/index.ts index 8dbff44a671..98ef1cf8707 100644 --- a/packages/eslint-plugin/src/util/index.ts +++ b/packages/eslint-plugin/src/util/index.ts @@ -9,8 +9,8 @@ export * from './getThisExpression'; export * from './getWrappingFixer'; export * from './misc'; export * from './objectIterators'; -export * from './isNull'; -export * from './isUndefined'; +export * from './isNullLiteral'; +export * from './isUndefinedIdentifier'; export * from './isNodeEqual'; // this is done for convenience - saves migrating all of the old rules diff --git a/packages/eslint-plugin/src/util/isNull.ts b/packages/eslint-plugin/src/util/isNullLiteral.ts similarity index 69% rename from packages/eslint-plugin/src/util/isNull.ts rename to packages/eslint-plugin/src/util/isNullLiteral.ts index a42757a025a..e700e415f63 100644 --- a/packages/eslint-plugin/src/util/isNull.ts +++ b/packages/eslint-plugin/src/util/isNullLiteral.ts @@ -1,5 +1,5 @@ import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/utils'; -export function isNull(i: TSESTree.Node): boolean { +export function isNullLiteral(i: TSESTree.Node): boolean { return i.type === AST_NODE_TYPES.Literal && i.value === null; } diff --git a/packages/eslint-plugin/src/util/isUndefined.ts b/packages/eslint-plugin/src/util/isUndefinedIdentifier.ts similarity index 68% rename from packages/eslint-plugin/src/util/isUndefined.ts rename to packages/eslint-plugin/src/util/isUndefinedIdentifier.ts index 220dedd0dd6..91cae07aa81 100644 --- a/packages/eslint-plugin/src/util/isUndefined.ts +++ b/packages/eslint-plugin/src/util/isUndefinedIdentifier.ts @@ -1,5 +1,5 @@ import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/utils'; -export function isUndefined(i: TSESTree.Node): boolean { +export function isUndefinedIdentifier(i: TSESTree.Node): boolean { return i.type === AST_NODE_TYPES.Identifier && i.name === 'undefined'; } From a20e74163182976b7e0ac3de48da48df91b8423b Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 12:58:25 +0200 Subject: [PATCH 11/24] refactor: used new utilities in prefer-optional-chain.ts --- .../src/rules/prefer-optional-chain.ts | 22 ++++--------------- 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 04cd7e64993..662439ac743 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -437,24 +437,10 @@ function isValidChainTarget( - foo !== undefined - foo != undefined */ - if ( + return ( node.type === AST_NODE_TYPES.BinaryExpression && ['!==', '!='].includes(node.operator) && - isValidChainTarget(node.left, allowIdentifier) - ) { - if ( - node.right.type === AST_NODE_TYPES.Identifier && - node.right.name === 'undefined' - ) { - return true; - } - if ( - node.right.type === AST_NODE_TYPES.Literal && - node.right.value === null - ) { - return true; - } - } - - return false; + isValidChainTarget(node.left, allowIdentifier) && + (util.isUndefinedIdentifier(node.right) || util.isNullLiteral(node.right)) + ); } From ee6bbf6a7fc6bdef50074020f392a43804ce90de Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 15:36:56 +0200 Subject: [PATCH 12/24] refactor: simplified prefer-nullish-coalescing.ts --- .../src/rules/prefer-nullish-coalescing.ts | 330 +++++++----------- 1 file changed, 118 insertions(+), 212 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index 9d15126a471..32270b73144 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -87,52 +87,77 @@ export default util.createRule({ return; } - let identifier: TSESTree.Identifier | TSESTree.MemberExpression; - let alternate: TSESTree.Expression; - let requiredOperator: '!==' | '==='; - if ( - (node.consequent.type === AST_NODE_TYPES.Identifier || - node.consequent.type === AST_NODE_TYPES.MemberExpression) && - ((node.test.type === AST_NODE_TYPES.BinaryExpression && - (node.test.operator === '!==' || node.test.operator === '!=')) || - (node.test.type === AST_NODE_TYPES.LogicalExpression && - node.test.left.type === AST_NODE_TYPES.BinaryExpression && - (node.test.left.operator === '!==' || - node.test.left.operator === '!='))) - ) { - identifier = node.consequent; - alternate = node.alternate; - requiredOperator = '!=='; - } else if ( - node.alternate.type === AST_NODE_TYPES.Identifier || - node.alternate.type === AST_NODE_TYPES.MemberExpression - ) { - identifier = node.alternate; - alternate = node.consequent; - requiredOperator = '==='; - } else { + const operator = getOperator(node.test); + + if (!operator) { return; } - if ( - isFixableLooseTernary({ - requiredOperator, - identifier, - node, - }) || - isFixableExplicitTernary({ - requiredOperator, - identifier, - node, - }) || - isFixableImplicitTernary({ - parserServices, - checker, - requiredOperator, - identifier, - node, - }) - ) { + let identifier: TSESTree.Node | undefined; + let hasUndefinedCheck = false; + let hasNullCheck = false; + + // we check that the test only contains null, undefined and the identifier + for (const n of getNodes(node.test)) { + if (util.isNullLiteral(n)) { + hasNullCheck = true; + } else if (util.isUndefinedIdentifier(n)) { + hasUndefinedCheck = true; + } else if ( + (operator === '!==' || operator === '!=') && + util.isNodeEqual(n, node.consequent) + ) { + identifier = n; + } else if ( + (operator === '===' || operator === '==') && + util.isNodeEqual(n, node.alternate) + ) { + identifier = n; + } else { + return; + } + } + + if (!identifier) { + return; + } + + const isFixable = ((): boolean => { + // it is fixable if we check for both null and undefined + if (hasUndefinedCheck && hasNullCheck) { + return true; + } + + // it is fixable if we loosly check for eihter null or undefined + if ( + (operator === '==' || operator === '!=') && + (hasUndefinedCheck || hasNullCheck) + ) { + return true; + } + + const tsNode = parserServices.esTreeNodeToTSNodeMap.get(identifier); + const type = checker.getTypeAtLocation(tsNode); + const flags = util.getTypeFlags(type); + + if (flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)) { + return false; + } + + const hasNullType = (flags & ts.TypeFlags.Null) !== 0; + + // it is fixable if we check for undefined and the type is not nullable + if (hasUndefinedCheck && !hasNullType) { + return true; + } + + const hasUndefinedType = (flags & ts.TypeFlags.Undefined) !== 0; + + // it is fixable if we check for null and the type can't be undefined + return hasNullCheck && !hasUndefinedType; + })(); + + if (isFixable) { context.report({ node, messageId: 'preferNullishOverTernary', @@ -140,14 +165,18 @@ export default util.createRule({ { messageId: 'suggestNullish', fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix { + const [left, right] = + operator === '===' || operator === '==' + ? [node.alternate, node.consequent] + : [node.consequent, node.alternate]; return fixer.replaceText( node, `${sourceCode.text.slice( - identifier.range[0], - identifier.range[1], + left.range[0], + left.range[1], )} ?? ${sourceCode.text.slice( - alternate.range[0], - alternate.range[1], + right.range[0], + right.range[1], )}`, ); }, @@ -280,174 +309,51 @@ function isMixedLogicalExpression(node: TSESTree.LogicalExpression): boolean { return false; } -/** - * This is for ternary expressions where we check both undefined and null. - * example: foo === undefined && foo === null ? 'a string' : foo - * output: foo ?? 'a string' - */ -function isFixableExplicitTernary({ - requiredOperator, - identifier, - node, -}: { - requiredOperator: '!==' | '==='; - identifier: TSESTree.Identifier | TSESTree.MemberExpression; - node: TSESTree.ConditionalExpression; -}): boolean { - if (node.test.type !== AST_NODE_TYPES.LogicalExpression) { - return false; - } - if (requiredOperator === '===' && node.test.operator === '&&') { - return false; - } - const { left, right } = node.test; - if ( - left.type !== AST_NODE_TYPES.BinaryExpression || - right.type !== AST_NODE_TYPES.BinaryExpression - ) { - return false; - } - - if ( - left.operator !== requiredOperator || - right.operator !== requiredOperator - ) { - return false; - } - - const hasUndefinedCheck = - (util.isNodeEqual(identifier, left.left) && - util.isUndefinedIdentifier(left.right)) || - (util.isNodeEqual(identifier, left.right) && - util.isUndefinedIdentifier(left.left)) || - (util.isNodeEqual(identifier, right.left) && - util.isUndefinedIdentifier(right.right)) || - (util.isNodeEqual(identifier, right.right) && - util.isUndefinedIdentifier(right.left)); - - if (!hasUndefinedCheck) { - return false; - } - - const hasNullCheck = - (util.isNodeEqual(identifier, left.left) && - util.isNullLiteral(left.right)) || - (util.isNodeEqual(identifier, left.right) && - util.isNullLiteral(left.left)) || - (util.isNodeEqual(identifier, right.left) && - util.isNullLiteral(right.right)) || - (util.isNodeEqual(identifier, right.right) && - util.isNullLiteral(right.left)); - - return hasNullCheck; -} - -/* - * this is for ternary expressions where == or != is used. - * example: foo == null ? 'a string' : a - * example: foo != undefined ? a : 'a string' - */ -function isFixableLooseTernary({ - node, - identifier, - requiredOperator, -}: { - requiredOperator: '!==' | '==='; - identifier: TSESTree.Identifier | TSESTree.MemberExpression; - node: TSESTree.ConditionalExpression; -}): boolean { - if (node.test.type !== AST_NODE_TYPES.BinaryExpression) { - return false; - } - - const { left, right, operator } = node.test; - if (requiredOperator === '===' && operator !== '==') { - return false; - } - if (requiredOperator === '!==' && operator !== '!=') { - return false; - } - - if ( - util.isNodeEqual(identifier, right) && - (util.isNullLiteral(left) || util.isUndefinedIdentifier(left)) - ) { - return true; - } - - if ( - util.isNodeEqual(identifier, left) && - (util.isNullLiteral(right) || util.isUndefinedIdentifier(right)) +function getOperator( + node: TSESTree.Expression, +): '==' | '!=' | '===' | '!==' | undefined { + if (node.type === AST_NODE_TYPES.BinaryExpression) { + if ( + node.operator === '==' || + node.operator === '!=' || + node.operator === '===' || + node.operator === '!==' + ) { + return node.operator; + } + } else if ( + node.type === AST_NODE_TYPES.LogicalExpression && + node.left.type === AST_NODE_TYPES.BinaryExpression && + node.right.type === AST_NODE_TYPES.BinaryExpression ) { - return true; + if (node.operator === '||') { + if (node.left.operator === '===' && node.right.operator === '===') { + return '==='; + } else if ( + (node.left.operator === '===' || node.right.operator === '===') && + (node.left.operator === '==' || node.right.operator === '==') + ) { + return '=='; + } + } else if (node.operator === '&&') { + if (node.left.operator === '!==' && node.right.operator === '!==') { + return '!=='; + } else if ( + (node.left.operator === '!==' || node.right.operator === '!==') && + (node.left.operator === '!=' || node.right.operator === '!=') + ) { + return '!='; + } + } } - - return false; + return; } -/** - * This is for ternary expressions where we check only undefined or null and - * need to type information to ensure that our checks are still correct. - * example: const foo:? string = 'bar'; - * foo !== undefined ? foo : 'a string'; - * output: foo ?? 'a string' - */ -function isFixableImplicitTernary({ - parserServices, - checker, - requiredOperator, - identifier, - node, -}: { - parserServices: ReturnType; - checker: ts.TypeChecker; - requiredOperator: '!==' | '==='; - identifier: TSESTree.Identifier | TSESTree.MemberExpression; - node: TSESTree.ConditionalExpression; -}): boolean { - if (node.test.type !== AST_NODE_TYPES.BinaryExpression) { - return false; - } - const { left, right, operator } = node.test; - if (operator !== requiredOperator) { - return false; - } - - const i = util.isNodeEqual(identifier, left) - ? left - : util.isNodeEqual(identifier, right) - ? right - : null; - if (!i) { - return false; - } - - const tsNode = parserServices.esTreeNodeToTSNodeMap.get(i); - const type = checker.getTypeAtLocation(tsNode); - const flags = util.getTypeFlags(type); - - if (flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)) { - return false; - } - - const hasNullType = (flags & ts.TypeFlags.Null) !== 0; - const hasUndefinedType = (flags & ts.TypeFlags.Undefined) !== 0; - - if ( - hasNullType && - !hasUndefinedType && - (util.isNullLiteral(right) || util.isNullLiteral(left)) - ) { - return true; - } - - if ( - hasUndefinedType && - !hasNullType && - (util.isUndefinedIdentifier(right) || util.isUndefinedIdentifier(left)) - ) { - return true; +function getNodes(node: TSESTree.Node): TSESTree.Node[] { + if (node.type === AST_NODE_TYPES.BinaryExpression) { + return [node.left, node.right]; + } else if (node.type === AST_NODE_TYPES.LogicalExpression) { + return [...getNodes(node.left), ...getNodes(node.right)]; } - - return false; + return [node]; } From 0726c9dfdb3ce7ec9b748b1fec6a6d7b4b11c7c8 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 15:37:14 +0200 Subject: [PATCH 13/24] test: added test case for prefer-nullish-coalescing --- .../rules/prefer-nullish-coalescing.test.ts | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index a0fd763e33f..ec2db5cc047 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -298,6 +298,27 @@ x ?? 'foo'; }, ]), + { + code: 'this != undefined ? this : y;', + output: null, + options: [{ ignoreTernaryTests: false }] as const, + errors: [ + { + messageId: 'preferNullishOverTernary' as const, + line: 1, + column: 1, + endLine: 1, + endColumn: 29, + suggestions: [ + { + messageId: 'suggestNullish' as const, + output: 'this ?? y;', + }, + ], + }, + ], + }, + ...[ ` declare const x: string | undefined; From c4146c0185346fbd9ae169ec751e67af1ec9bd51 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 15:38:44 +0200 Subject: [PATCH 14/24] refactor: renamed message id to preferNullishOverOr --- .../src/rules/prefer-nullish-coalescing.ts | 6 ++-- .../rules/prefer-nullish-coalescing.test.ts | 28 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index 32270b73144..cfc60daa0bb 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -16,7 +16,7 @@ export type Options = [ ]; export type MessageIds = - | 'preferNullish' + | 'preferNullishOverOr' | 'preferNullishOverTernary' | 'suggestNullish'; @@ -33,7 +33,7 @@ export default util.createRule({ }, hasSuggestions: true, messages: { - preferNullish: + preferNullishOverOr: 'Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.', preferNullishOverTernary: 'Prefer using nullish coalescing operator (`??`) instead of a ternary expression, as it is simpler to read.', @@ -237,7 +237,7 @@ export default util.createRule({ context.report({ node: barBarOperator, - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', suggest: [ { messageId: 'suggestNullish', diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index ec2db5cc047..6bbf2521053 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -211,7 +211,7 @@ x || 'foo'; output: null, errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 3, column: 3, endLine: 3, @@ -386,7 +386,7 @@ x || 'foo' ? null : null; options: [{ ignoreConditionalTests: false }], errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 3, column: 3, endLine: 3, @@ -412,7 +412,7 @@ if (x || 'foo') {} options: [{ ignoreConditionalTests: false }], errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 3, column: 7, endLine: 3, @@ -438,7 +438,7 @@ do {} while (x || 'foo') options: [{ ignoreConditionalTests: false }], errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 3, column: 16, endLine: 3, @@ -464,7 +464,7 @@ for (;x || 'foo';) {} options: [{ ignoreConditionalTests: false }], errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 3, column: 9, endLine: 3, @@ -490,7 +490,7 @@ while (x || 'foo') {} options: [{ ignoreConditionalTests: false }], errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 3, column: 10, endLine: 3, @@ -519,7 +519,7 @@ a || b && c; options: [{ ignoreMixedLogicalExpressions: false }], errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 5, column: 3, endLine: 5, @@ -549,7 +549,7 @@ a || b || c && d; options: [{ ignoreMixedLogicalExpressions: false }], errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 6, column: 3, endLine: 6, @@ -568,7 +568,7 @@ declare const d: ${type} | ${nullish}; ], }, { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 6, column: 8, endLine: 6, @@ -599,7 +599,7 @@ a && b || c || d; options: [{ ignoreMixedLogicalExpressions: false }], errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 6, column: 8, endLine: 6, @@ -618,7 +618,7 @@ a && (b ?? c) || d; ], }, { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 6, column: 13, endLine: 6, @@ -649,7 +649,7 @@ if (() => x || 'foo') {} options: [{ ignoreConditionalTests: true }], errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 3, column: 13, endLine: 3, @@ -675,7 +675,7 @@ if (function werid() { return x || 'foo' }) {} options: [{ ignoreConditionalTests: true }], errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 3, column: 33, endLine: 3, @@ -703,7 +703,7 @@ a || b || c; output: null, errors: [ { - messageId: 'preferNullish', + messageId: 'preferNullishOverOr', line: 5, column: 3, endLine: 5, From 65415e6563dfdafa387df12a5f8457f44b7f5027 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 15:48:02 +0200 Subject: [PATCH 15/24] fix: covered edge case where we have two loose comparisons --- .../src/rules/prefer-nullish-coalescing.ts | 10 ++++++---- .../tests/rules/prefer-nullish-coalescing.test.ts | 5 ++++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index cfc60daa0bb..0eaa39b1f0b 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -330,8 +330,9 @@ function getOperator( if (node.left.operator === '===' && node.right.operator === '===') { return '==='; } else if ( - (node.left.operator === '===' || node.right.operator === '===') && - (node.left.operator === '==' || node.right.operator === '==') + ((node.left.operator === '===' || node.right.operator === '===') && + (node.left.operator === '==' || node.right.operator === '==')) || + (node.left.operator === '==' && node.right.operator === '==') ) { return '=='; } @@ -339,8 +340,9 @@ function getOperator( if (node.left.operator === '!==' && node.right.operator === '!==') { return '!=='; } else if ( - (node.left.operator === '!==' || node.right.operator === '!==') && - (node.left.operator === '!=' || node.right.operator === '!=') + ((node.left.operator === '!==' || node.right.operator === '!==') && + (node.left.operator === '!=' || node.right.operator === '!=')) || + (node.left.operator === '!=' && node.right.operator === '!=') ) { return '!='; } diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index 6bbf2521053..6c5513c724d 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -246,9 +246,12 @@ x ?? 'foo'; 'null !== x && undefined !== x ? x : y;', 'undefined === x || null === x ? y : x;', 'null === x || undefined === x ? y : x;', + 'x != undefined && x != null ? x : y;', + 'x != undefined && x !== null ? x : y;', + 'x !== undefined && x != null ? x : y;', 'undefined != x ? x : y;', 'null != x ? x : y;', - 'undefined == x ? y : x;', + 'undefined == x ? y : x;', 'null == x ? y : x;', 'x != undefined ? x : y;', 'x != null ? x : y;', From dda02a37fef8c413149e788d767a6f58dce0c8ef Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 15:51:48 +0200 Subject: [PATCH 16/24] refactor: removed useless .trim --- .../eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index 6c5513c724d..43656abd009 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -130,7 +130,7 @@ declare const x: string | undefined; x === null ? x : y; `, ].map(code => ({ - code: code.trim(), + code, options: [{ ignoreTernaryTests: false }] as const, })), From cafa047355f448e38d69f054c24fb60962865450 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 15:55:59 +0200 Subject: [PATCH 17/24] chore: fixed typo --- .../src/rules/prefer-nullish-coalescing.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index 0eaa39b1f0b..90e23c8a350 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -97,7 +97,7 @@ export default util.createRule({ let hasUndefinedCheck = false; let hasNullCheck = false; - // we check that the test only contains null, undefined and the identifier + // we check that the test only contain null, undefined and the identifier for (const n of getNodes(node.test)) { if (util.isNullLiteral(n)) { hasNullCheck = true; @@ -128,7 +128,7 @@ export default util.createRule({ return true; } - // it is fixable if we loosly check for eihter null or undefined + // it is fixable if we loosely check for either null or undefined if ( (operator === '==' || operator === '!=') && (hasUndefinedCheck || hasNullCheck) @@ -191,10 +191,8 @@ export default util.createRule({ ): void { const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); const type = checker.getTypeAtLocation(tsNode.left); - const isNullLiteralish = util.isNullableType(type, { - allowUndefined: true, - }); - if (!isNullLiteralish) { + const isNullish = util.isNullableType(type, { allowUndefined: true }); + if (!isNullish) { return; } From 8ee505cdf4d2a99285c8b90ba877cf410ec234bb Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 16:28:30 +0200 Subject: [PATCH 18/24] test: added more test cases --- .../tests/rules/prefer-nullish-coalescing.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index 43656abd009..b81ccea8394 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -90,6 +90,7 @@ x ?? 'foo'; 'x === null || x === null ? y : x;', 'x === undefined || x === undefined ? y : x;', 'x == null ? x : y;', + 'undefined != z ? x : y;', 'x == undefined ? x : y;', 'x != null ? y : x;', 'x != undefined ? y : x;', @@ -247,7 +248,9 @@ x ?? 'foo'; 'undefined === x || null === x ? y : x;', 'null === x || undefined === x ? y : x;', 'x != undefined && x != null ? x : y;', + 'x == undefined && x == null ? y : x;', 'x != undefined && x !== null ? x : y;', + 'x == undefined && x === null ? y : x;', 'x !== undefined && x != null ? x : y;', 'undefined != x ? x : y;', 'null != x ? x : y;', From 119f14d1860eca3e592eff63ce09e72eed5da9a2 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 16:31:45 +0200 Subject: [PATCH 19/24] fix: fixed tests --- .../tests/rules/prefer-nullish-coalescing.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index b81ccea8394..72a36938c36 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -248,9 +248,9 @@ x ?? 'foo'; 'undefined === x || null === x ? y : x;', 'null === x || undefined === x ? y : x;', 'x != undefined && x != null ? x : y;', - 'x == undefined && x == null ? y : x;', + 'x == undefined || x == null ? y : x;', 'x != undefined && x !== null ? x : y;', - 'x == undefined && x === null ? y : x;', + 'x == undefined || x === null ? y : x;', 'x !== undefined && x != null ? x : y;', 'undefined != x ? x : y;', 'null != x ? x : y;', From 31966ff2b186bece0542c3253e2729f7c6b13df0 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 16:39:47 +0200 Subject: [PATCH 20/24] chore: fixed typo --- packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index 90e23c8a350..dbb32994f16 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -97,7 +97,7 @@ export default util.createRule({ let hasUndefinedCheck = false; let hasNullCheck = false; - // we check that the test only contain null, undefined and the identifier + // we check that the test only contains null, undefined and the identifier for (const n of getNodes(node.test)) { if (util.isNullLiteral(n)) { hasNullCheck = true; From 3d0eab1d863b733009b66cbe443d9730cd5927fe Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Sat, 11 Jun 2022 16:55:43 +0200 Subject: [PATCH 21/24] test: added more test cases --- .../eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts index 72a36938c36..55fb4452973 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -90,6 +90,7 @@ x ?? 'foo'; 'x === null || x === null ? y : x;', 'x === undefined || x === undefined ? y : x;', 'x == null ? x : y;', + 'undefined == null ? x : y;', 'undefined != z ? x : y;', 'x == undefined ? x : y;', 'x != null ? y : x;', From 73034b08fcd27f376a1b822a668414bbdbfebc3b Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Mon, 13 Jun 2022 11:11:22 +0200 Subject: [PATCH 22/24] refactor: inlined getOperator and getNodes --- .../src/rules/prefer-nullish-coalescing.ts | 123 +++++++++--------- 1 file changed, 64 insertions(+), 59 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index dbb32994f16..c651137df1b 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -87,7 +87,63 @@ export default util.createRule({ return; } - const operator = getOperator(node.test); + let operator: '==' | '!=' | '===' | '!==' | undefined; + let nodesInsideTestExpression: TSESTree.Node[] = []; + if (node.test.type === AST_NODE_TYPES.BinaryExpression) { + nodesInsideTestExpression = [node.test.left, node.test.right]; + if ( + node.test.operator === '==' || + node.test.operator === '!=' || + node.test.operator === '===' || + node.test.operator === '!==' + ) { + operator = node.test.operator; + } + } else if ( + node.test.type === AST_NODE_TYPES.LogicalExpression && + node.test.left.type === AST_NODE_TYPES.BinaryExpression && + node.test.right.type === AST_NODE_TYPES.BinaryExpression + ) { + nodesInsideTestExpression = [ + node.test.left.left, + node.test.left.right, + node.test.right.left, + node.test.right.right, + ]; + if (node.test.operator === '||') { + if ( + node.test.left.operator === '===' && + node.test.right.operator === '===' + ) { + operator = '==='; + } else if ( + ((node.test.left.operator === '===' || + node.test.right.operator === '===') && + (node.test.left.operator === '==' || + node.test.right.operator === '==')) || + (node.test.left.operator === '==' && + node.test.right.operator === '==') + ) { + operator = '=='; + } + } else if (node.test.operator === '&&') { + if ( + node.test.left.operator === '!==' && + node.test.right.operator === '!==' + ) { + operator = '!=='; + } else if ( + ((node.test.left.operator === '!==' || + node.test.right.operator === '!==') && + (node.test.left.operator === '!=' || + node.test.right.operator === '!=')) || + (node.test.left.operator === '!=' && + node.test.right.operator === '!=') + ) { + operator = '!='; + } + } + } if (!operator) { return; @@ -98,21 +154,21 @@ export default util.createRule({ let hasNullCheck = false; // we check that the test only contains null, undefined and the identifier - for (const n of getNodes(node.test)) { - if (util.isNullLiteral(n)) { + for (const testNode of nodesInsideTestExpression) { + if (util.isNullLiteral(testNode)) { hasNullCheck = true; - } else if (util.isUndefinedIdentifier(n)) { + } else if (util.isUndefinedIdentifier(testNode)) { hasUndefinedCheck = true; } else if ( (operator === '!==' || operator === '!=') && - util.isNodeEqual(n, node.consequent) + util.isNodeEqual(testNode, node.consequent) ) { - identifier = n; + identifier = testNode; } else if ( (operator === '===' || operator === '==') && - util.isNodeEqual(n, node.alternate) + util.isNodeEqual(testNode, node.alternate) ) { - identifier = n; + identifier = testNode; } else { return; } @@ -306,54 +362,3 @@ function isMixedLogicalExpression(node: TSESTree.LogicalExpression): boolean { return false; } - -function getOperator( - node: TSESTree.Expression, -): '==' | '!=' | '===' | '!==' | undefined { - if (node.type === AST_NODE_TYPES.BinaryExpression) { - if ( - node.operator === '==' || - node.operator === '!=' || - node.operator === '===' || - node.operator === '!==' - ) { - return node.operator; - } - } else if ( - node.type === AST_NODE_TYPES.LogicalExpression && - node.left.type === AST_NODE_TYPES.BinaryExpression && - node.right.type === AST_NODE_TYPES.BinaryExpression - ) { - if (node.operator === '||') { - if (node.left.operator === '===' && node.right.operator === '===') { - return '==='; - } else if ( - ((node.left.operator === '===' || node.right.operator === '===') && - (node.left.operator === '==' || node.right.operator === '==')) || - (node.left.operator === '==' && node.right.operator === '==') - ) { - return '=='; - } - } else if (node.operator === '&&') { - if (node.left.operator === '!==' && node.right.operator === '!==') { - return '!=='; - } else if ( - ((node.left.operator === '!==' || node.right.operator === '!==') && - (node.left.operator === '!=' || node.right.operator === '!=')) || - (node.left.operator === '!=' && node.right.operator === '!=') - ) { - return '!='; - } - } - } - return; -} - -function getNodes(node: TSESTree.Node): TSESTree.Node[] { - if (node.type === AST_NODE_TYPES.BinaryExpression) { - return [node.left, node.right]; - } else if (node.type === AST_NODE_TYPES.LogicalExpression) { - return [...getNodes(node.left), ...getNodes(node.right)]; - } - return [node]; -} From 4ef1ef7c4b910105d6a603567e814790923ae412 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Mon, 13 Jun 2022 11:15:00 +0200 Subject: [PATCH 23/24] perf: skip type check if both null and undefined are not checked --- .../src/rules/prefer-nullish-coalescing.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index c651137df1b..b460eba03bb 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -184,11 +184,13 @@ export default util.createRule({ return true; } + // it is not fixable if it has neither a null nor undefined check + if (!hasUndefinedCheck && !hasNullCheck) { + return false; + } + // it is fixable if we loosely check for either null or undefined - if ( - (operator === '==' || operator === '!=') && - (hasUndefinedCheck || hasNullCheck) - ) { + if (operator === '==' || operator === '!=') { return true; } From 67e7ab58bcafde43f21f90f61239305f86cbb8e5 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 23 Jul 2022 12:08:23 -0400 Subject: [PATCH 24/24] Apply suggestions from code review --- .../src/rules/prefer-nullish-coalescing.ts | 11 +++-------- packages/eslint-plugin/src/util/isNodeEqual.ts | 16 ---------------- 2 files changed, 3 insertions(+), 24 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index b460eba03bb..b2321fd0757 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -179,14 +179,9 @@ export default util.createRule({ } const isFixable = ((): boolean => { - // it is fixable if we check for both null and undefined - if (hasUndefinedCheck && hasNullCheck) { - return true; - } - - // it is not fixable if it has neither a null nor undefined check - if (!hasUndefinedCheck && !hasNullCheck) { - return false; + // it is fixable if we check for both null and undefined, or not if neither + if (hasUndefinedCheck === hasNullCheck) { + return hasUndefinedCheck; } // it is fixable if we loosely check for either null or undefined diff --git a/packages/eslint-plugin/src/util/isNodeEqual.ts b/packages/eslint-plugin/src/util/isNodeEqual.ts index 03acc88eb25..ef879163ee4 100644 --- a/packages/eslint-plugin/src/util/isNodeEqual.ts +++ b/packages/eslint-plugin/src/util/isNodeEqual.ts @@ -1,20 +1,4 @@ import { AST_NODE_TYPES, TSESTree } from '@typescript-eslint/utils'; -/* - * Supported: - * Identifier Literal MemberExpression ThisExpression - * TSNamedTupleMember TSNamespaceExportDeclaration TSNeverKeyword - * TSNonNullExpression TSNullKeyword TSNumberKeyword TSObjectKeyword - * TSOptionalType TSParameterProperty TSPrivateKeyword TSPropertySignature - * TSProtectedKeyword TSPublicKeyword TSQualifiedName TSReadonlyKeyword - * TSRestType TSStaticKeyword TSStringKeyword TSSymbolKeyword - * TSTemplateLiteralType TSThisType TSTupleType TSTypeAliasDeclaration - * TSTypeAnnotation TSTypeAssertion TSTypeLiteral TSTypeOperator - * TSTypeParameter TSTypeParameterDeclaration TSTypeParameterInstantiation - * TSTypePredicate TSTypeQuery TSTypeReference TSUndefinedKeyword TSUnionType - * TSUnknownKeyword TSVoidKeyword UnaryExpression UpdateExpression - * VariableDeclaration VariableDeclarator WhileStatement WithStatement - * YieldExpression - */ export function isNodeEqual(a: TSESTree.Node, b: TSESTree.Node): boolean { if (a.type !== b.type) {