From 1a36d4ba3f37d54949d7c3d9f669a7a788b4a7f1 Mon Sep 17 00:00:00 2001 From: Jakob Guddas Date: Thu, 12 May 2022 16:55:01 +0200 Subject: [PATCH] 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 dd9c02009f9c..5ebef2a4e05e 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 a nullish coalesce 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 b492287d4bbe..a30e27f20697 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: string; + 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: string; + 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 8096df94c329..94752b2a1bbb 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: `