diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 3bd5ddd313f..503c06e6894 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -194,6 +194,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e | [`@typescript-eslint/prefer-function-type`](./docs/rules/prefer-function-type.md) | Use function types instead of interfaces with call signatures | | :wrench: | | | [`@typescript-eslint/prefer-includes`](./docs/rules/prefer-includes.md) | Enforce `includes` method over `indexOf` method | :heavy_check_mark: | :wrench: | :thought_balloon: | | [`@typescript-eslint/prefer-namespace-keyword`](./docs/rules/prefer-namespace-keyword.md) | Require the use of the `namespace` keyword instead of the `module` keyword to declare custom TypeScript modules | :heavy_check_mark: | :wrench: | | +| [`@typescript-eslint/prefer-nullish-coalescing`](./docs/rules/prefer-nullish-coalescing.md) | Enforce the usage of the nullish coalescing operator instead of logical chaining | | :wrench: | :thought_balloon: | | [`@typescript-eslint/prefer-optional-chain`](./docs/rules/prefer-optional-chain.md) | Prefer using concise optional chain expressions instead of chained logical ands | | :wrench: | | | [`@typescript-eslint/prefer-readonly`](./docs/rules/prefer-readonly.md) | Requires that private members are marked as `readonly` if they're never modified outside of the constructor | | :wrench: | :thought_balloon: | | [`@typescript-eslint/prefer-regexp-exec`](./docs/rules/prefer-regexp-exec.md) | Prefer RegExp#exec() over String#match() if no global flag is provided | :heavy_check_mark: | | :thought_balloon: | diff --git a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md new file mode 100644 index 00000000000..140ade2dbe7 --- /dev/null +++ b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md @@ -0,0 +1,141 @@ +# Enforce the usage of the nullish coalescing operator instead of logical chaining (prefer-nullish-coalescing) + +TypeScript 3.7 added support for the nullish coalescing operator. +This operator allows you to safely cascade a value when dealing with `null` or `undefined`. + +```ts +function myFunc(foo: string | null) { + return foo ?? 'a string'; +} + +// is equivalent to + +function myFunc(foo: string | null) { + return foo !== null && foo !== undefined ? foo : 'a string'; +} +``` + +Because the nullish coalescing operator _only_ coalesces when the original value is `null` or `undefined`, it is much safer than relying upon logical OR operator chaining `||`; which coalesces on any _falsey_ value: + +```ts +const emptyString = ''; + +const nullish1 = emptyString ?? 'unsafe'; +const logical1 = emptyString || 'unsafe'; + +// nullish1 === '' +// logical1 === 'unsafe' + +declare const nullString: string | null; + +const nullish2 = nullString ?? 'safe'; +const logical2 = nullString || 'safe'; + +// nullish2 === 'safe' +// logical2 === 'safe' +``` + +## Rule Details + +This rule aims enforce the usage of the safer operator. + +## Options + +```ts +type Options = [ + { + ignoreConditionalTests?: boolean; + ignoreMixedLogicalExpressions?: boolean; + }, +]; + +const defaultOptions = [ + { + ignoreConditionalTests: true, + ignoreMixedLogicalExpressions: true; + }, +]; +``` + +### ignoreConditionalTests + +Setting this option to `true` (the default) will cause the rule to ignore any cases that are located within a conditional test. + +Generally expressions within conditional tests intentionally use the falsey fallthrough behaviour of the logical or operator, meaning that fixing the operator to the nullish coalesce operator could cause bugs. + +If you're looking to enforce stricter conditional tests, you should consider using the `strict-boolean-expressions` rule. + +Incorrect code for `ignoreConditionalTests: false`, and correct code for `ignoreConditionalTests: true`: + +```ts +declare const a: string | null; +declare const b: string | null; + +if (a || b) { +} +while (a || b) {} +do {} while (a || b); +for (let i = 0; a || b; i += 1) {} +a || b ? true : false; +``` + +Correct code for `ignoreConditionalTests: false`: + +```ts +declare const a: string | null; +declare const b: string | null; + +if (a ?? b) { +} +while (a ?? b) {} +do {} while (a ?? b); +for (let i = 0; a ?? b; i += 1) {} +a ?? b ? true : false; +``` + +### ignoreMixedLogicalExpressions + +Setting this option to `true` (the default) will cause the rule to ignore any logical or expressions thare are part of a mixed logical expression (with `&&`). + +Generally expressions within mixed logical expressions intentionally use the falsey fallthrough behaviour of the logical or operator, meaning that fixing the operator to the nullish coalesce operator could cause bugs. + +If you're looking to enforce stricter conditional tests, you should consider using the `strict-boolean-expressions` rule. + +Incorrect code for `ignoreMixedLogicalExpressions: false`, and correct code for `ignoreMixedLogicalExpressions: true`: + +```ts +declare const a: string | null; +declare const b: string | null; +declare const c: string | null; +declare const d: string | null; + +a || (b && c); +(a && b) || c || d; +a || (b && c) || d; +a || (b && c && d); +``` + +Correct code for `ignoreMixedLogicalExpressions: false`: + +```ts +declare const a: string | null; +declare const b: string | null; +declare const c: string | null; +declare const d: string | null; + +a ?? (b && c); +(a && b) ?? c ?? d; +a ?? (b && c) ?? d; +a ?? (b && c && d); +``` + +**_NOTE:_** Errors for this specific case will be presented as suggestions, instead of fixes. This is because it is not always safe to automatically convert `||` to `??` within a mixed logical expression, as we cannot tell the intended precedence of the operator. Note that by design, `??` requires parentheses when used with `&&` or `||` in the same expression. + +## When Not To Use It + +If you are not using TypeScript 3.7 (or greater), then you will not be able to use this rule, as the operator is not supported. + +## Further Reading + +- [TypeScript 3.7 Release Notes](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html) +- [Nullish Coalescing Operator Proposal](https://github.com/tc39/proposal-nullish-coalescing/) diff --git a/packages/eslint-plugin/src/configs/all.json b/packages/eslint-plugin/src/configs/all.json index e2deb5fb1be..c3ee46bc95a 100644 --- a/packages/eslint-plugin/src/configs/all.json +++ b/packages/eslint-plugin/src/configs/all.json @@ -67,6 +67,7 @@ "@typescript-eslint/prefer-function-type": "error", "@typescript-eslint/prefer-includes": "error", "@typescript-eslint/prefer-namespace-keyword": "error", + "@typescript-eslint/prefer-nullish-coalescing": "error", "@typescript-eslint/prefer-optional-chain": "error", "@typescript-eslint/prefer-readonly": "error", "@typescript-eslint/prefer-regexp-exec": "error", diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index e71a1adb5bb..a4dc193e939 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -52,6 +52,7 @@ import preferForOf from './prefer-for-of'; import preferFunctionType from './prefer-function-type'; import preferIncludes from './prefer-includes'; import preferNamespaceKeyword from './prefer-namespace-keyword'; +import preferNullishCoalescing from './prefer-nullish-coalescing'; import preferOptionalChain from './prefer-optional-chain'; import preferReadonly from './prefer-readonly'; import preferRegexpExec from './prefer-regexp-exec'; @@ -127,6 +128,7 @@ export default { 'prefer-function-type': preferFunctionType, 'prefer-includes': preferIncludes, 'prefer-namespace-keyword': preferNamespaceKeyword, + 'prefer-nullish-coalescing': preferNullishCoalescing, 'prefer-optional-chain': preferOptionalChain, 'prefer-readonly': preferReadonly, 'prefer-regexp-exec': preferRegexpExec, diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts new file mode 100644 index 00000000000..afb564025fd --- /dev/null +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -0,0 +1,174 @@ +import { + AST_NODE_TYPES, + AST_TOKEN_TYPES, + TSESLint, + TSESTree, +} from '@typescript-eslint/experimental-utils'; +import ts from 'typescript'; +import * as util from '../util'; + +export type Options = [ + { + ignoreConditionalTests?: boolean; + ignoreMixedLogicalExpressions?: boolean; + }, +]; +export type MessageIds = 'preferNullish'; + +export default util.createRule({ + name: 'prefer-nullish-coalescing', + meta: { + type: 'suggestion', + docs: { + description: + 'Enforce the usage of the nullish coalescing operator instead of logical chaining', + category: 'Best Practices', + recommended: false, + requiresTypeChecking: true, + }, + fixable: 'code', + messages: { + preferNullish: + 'Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.', + }, + schema: [ + { + type: 'object', + properties: { + ignoreConditionalTests: { + type: 'boolean', + }, + ignoreMixedLogicalExpressions: { + type: 'boolean', + }, + }, + additionalProperties: false, + }, + ], + }, + defaultOptions: [ + { + ignoreConditionalTests: true, + ignoreMixedLogicalExpressions: true, + }, + ], + create(context, [{ ignoreConditionalTests, ignoreMixedLogicalExpressions }]) { + const parserServices = util.getParserServices(context); + const sourceCode = context.getSourceCode(); + const checker = parserServices.program.getTypeChecker(); + + return { + 'LogicalExpression[operator = "||"]'( + node: TSESTree.LogicalExpression, + ): void { + const tsNode = parserServices.esTreeNodeToTSNodeMap.get< + ts.BinaryExpression + >(node); + const type = checker.getTypeAtLocation(tsNode.left); + const isNullish = util.isNullableType(type, { allowUndefined: true }); + if (!isNullish) { + return; + } + + if (ignoreConditionalTests === true && isConditionalTest(node)) { + return; + } + + const isMixedLogical = isMixedLogicalExpression(node); + if (ignoreMixedLogicalExpressions === true && isMixedLogical) { + return; + } + + const barBarOperator = sourceCode.getTokenAfter( + node.left, + token => + token.type === AST_TOKEN_TYPES.Punctuator && + token.value === node.operator, + )!; // there _must_ be an operator + + const fixer = isMixedLogical + ? // suggestion instead for cases where we aren't sure if the fixer is completely safe + ({ + suggest: [ + { + messageId: 'preferNullish', + fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix { + return fixer.replaceText(barBarOperator, '??'); + }, + }, + ], + } as const) + : { + fix(fixer: TSESLint.RuleFixer): TSESLint.RuleFix { + return fixer.replaceText(barBarOperator, '??'); + }, + }; + + context.report({ + node: barBarOperator, + messageId: 'preferNullish', + ...fixer, + }); + }, + }; + }, +}); + +function isConditionalTest(node: TSESTree.Node): boolean { + const parents = new Set([node]); + let current = node.parent; + while (current) { + parents.add(current); + + if ( + (current.type === AST_NODE_TYPES.ConditionalExpression || + current.type === AST_NODE_TYPES.DoWhileStatement || + current.type === AST_NODE_TYPES.IfStatement || + current.type === AST_NODE_TYPES.ForStatement || + current.type === AST_NODE_TYPES.WhileStatement) && + parents.has(current.test) + ) { + return true; + } + + if ( + [ + AST_NODE_TYPES.ArrowFunctionExpression, + AST_NODE_TYPES.FunctionExpression, + ].includes(current.type) + ) { + /** + * This is a weird situation like: + * `if (() => a || b) {}` + * `if (function () { return a || b }) {}` + */ + return false; + } + + current = current.parent; + } + + return false; +} + +function isMixedLogicalExpression(node: TSESTree.LogicalExpression): boolean { + const seen = new Set(); + const queue = [node.parent, node.left, node.right]; + for (const current of queue) { + if (seen.has(current)) { + continue; + } + seen.add(current); + + if (current && current.type === AST_NODE_TYPES.LogicalExpression) { + if (current.operator === '&&') { + return true; + } else if (current.operator === '||') { + // check the pieces of the node to catch cases like `a || b || c && d` + queue.push(current.parent, current.left, current.right); + } + } + } + + return false; +} diff --git a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts new file mode 100644 index 00000000000..1f05d6b9a3f --- /dev/null +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -0,0 +1,439 @@ +import { TSESLint } from '@typescript-eslint/experimental-utils'; +import path from 'path'; +import rule, { + MessageIds, + Options, +} from '../../src/rules/prefer-nullish-coalescing'; +import { RuleTester } from '../RuleTester'; + +const rootPath = path.join(process.cwd(), 'tests/fixtures/'); + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + tsconfigRootDir: rootPath, + project: './tsconfig.json', + }, +}); + +const types = ['string', 'number', 'boolean', 'object']; +const nullishTypes = ['null', 'undefined', 'null | undefined']; + +function typeValidTest( + cb: (type: string) => TSESLint.ValidTestCase | string, +): (TSESLint.ValidTestCase | string)[] { + return types.map(type => cb(type)); +} +function nullishTypeValidTest( + cb: ( + nullish: string, + type: string, + ) => TSESLint.ValidTestCase | string, +): (TSESLint.ValidTestCase | string)[] { + return nullishTypes.reduce<(TSESLint.ValidTestCase | string)[]>( + (acc, nullish) => { + types.forEach(type => { + acc.push(cb(nullish, type)); + }); + return acc; + }, + [], + ); +} +function nullishTypeInvalidTest( + cb: ( + nullish: string, + type: string, + ) => TSESLint.InvalidTestCase, +): TSESLint.InvalidTestCase[] { + return nullishTypes.reduce[]>( + (acc, nullish) => { + types.forEach(type => { + acc.push(cb(nullish, type)); + }); + return acc; + }, + [], + ); +} + +ruleTester.run('prefer-nullish-coalescing', rule, { + valid: [ + ...typeValidTest( + type => ` +declare const x: ${type}; +x || 'foo'; + `, + ), + ...nullishTypeValidTest( + (nullish, type) => ` +declare const x: ${type} | ${nullish}; +x ?? 'foo'; + `, + ), + + // ignoreConditionalTests + ...nullishTypeValidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +x || 'foo' ? null : null; + `, + options: [{ ignoreConditionalTests: true }], + })), + ...nullishTypeValidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +if (x || 'foo') {} + `, + options: [{ ignoreConditionalTests: true }], + })), + ...nullishTypeValidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +do {} while (x || 'foo') + `, + options: [{ ignoreConditionalTests: true }], + })), + ...nullishTypeValidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +for (;x || 'foo';) {} + `, + options: [{ ignoreConditionalTests: true }], + })), + ...nullishTypeValidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +while (x || 'foo') {} + `, + options: [{ ignoreConditionalTests: true }], + })), + + // ignoreMixedLogicalExpressions + ...nullishTypeValidTest((nullish, type) => ({ + code: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +a || b && c; + `, + options: [{ ignoreMixedLogicalExpressions: true }], + })), + ...nullishTypeValidTest((nullish, type) => ({ + code: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +declare const d: ${type} | ${nullish}; +a || b || c && d; + `, + options: [{ ignoreMixedLogicalExpressions: true }], + })), + ...nullishTypeValidTest((nullish, type) => ({ + code: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +declare const d: ${type} | ${nullish}; +a && b || c || d; + `, + options: [{ ignoreMixedLogicalExpressions: true }], + })), + ], + invalid: [ + ...nullishTypeInvalidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +x || 'foo'; + `, + output: ` +declare const x: ${type} | ${nullish}; +x ?? 'foo'; + `, + errors: [ + { + messageId: 'preferNullish', + line: 3, + column: 3, + endLine: 3, + endColumn: 5, + }, + ], + })), + + // ignoreConditionalTests + ...nullishTypeInvalidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +x || 'foo' ? null : null; + `, + output: ` +declare const x: ${type} | ${nullish}; +x ?? 'foo' ? null : null; + `, + options: [{ ignoreConditionalTests: false }], + errors: [ + { + messageId: 'preferNullish', + line: 3, + column: 3, + endLine: 3, + endColumn: 5, + }, + ], + })), + ...nullishTypeInvalidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +if (x || 'foo') {} + `, + output: ` +declare const x: ${type} | ${nullish}; +if (x ?? 'foo') {} + `, + options: [{ ignoreConditionalTests: false }], + errors: [ + { + messageId: 'preferNullish', + line: 3, + column: 7, + endLine: 3, + endColumn: 9, + }, + ], + })), + ...nullishTypeInvalidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +do {} while (x || 'foo') + `, + output: ` +declare const x: ${type} | ${nullish}; +do {} while (x ?? 'foo') + `, + options: [{ ignoreConditionalTests: false }], + errors: [ + { + messageId: 'preferNullish', + line: 3, + column: 16, + endLine: 3, + endColumn: 18, + }, + ], + })), + ...nullishTypeInvalidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +for (;x || 'foo';) {} + `, + output: ` +declare const x: ${type} | ${nullish}; +for (;x ?? 'foo';) {} + `, + options: [{ ignoreConditionalTests: false }], + errors: [ + { + messageId: 'preferNullish', + line: 3, + column: 9, + endLine: 3, + endColumn: 11, + }, + ], + })), + ...nullishTypeInvalidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +while (x || 'foo') {} + `, + output: ` +declare const x: ${type} | ${nullish}; +while (x ?? 'foo') {} + `, + options: [{ ignoreConditionalTests: false }], + errors: [ + { + messageId: 'preferNullish', + line: 3, + column: 10, + endLine: 3, + endColumn: 12, + }, + ], + })), + + // ignoreMixedLogicalExpressions + ...nullishTypeInvalidTest((nullish, type) => ({ + code: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +a || b && c; + `.trimRight(), + options: [{ ignoreMixedLogicalExpressions: false }], + errors: [ + { + messageId: 'preferNullish', + line: 5, + column: 3, + endLine: 5, + endColumn: 5, + suggestions: [ + { + messageId: 'preferNullish', + output: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +a ?? b && c; + `.trimRight(), + }, + ], + }, + ], + })), + ...nullishTypeInvalidTest((nullish, type) => ({ + code: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +declare const d: ${type} | ${nullish}; +a || b || c && d; + `.trimRight(), + options: [{ ignoreMixedLogicalExpressions: false }], + errors: [ + { + messageId: 'preferNullish', + line: 6, + column: 3, + endLine: 6, + endColumn: 5, + suggestions: [ + { + messageId: 'preferNullish', + output: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +declare const d: ${type} | ${nullish}; +a ?? b || c && d; + `.trimRight(), + }, + ], + }, + { + messageId: 'preferNullish', + line: 6, + column: 8, + endLine: 6, + endColumn: 10, + suggestions: [ + { + messageId: 'preferNullish', + output: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +declare const d: ${type} | ${nullish}; +a || b ?? c && d; + `.trimRight(), + }, + ], + }, + ], + })), + ...nullishTypeInvalidTest((nullish, type) => ({ + code: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +declare const d: ${type} | ${nullish}; +a && b || c || d; + `.trimRight(), + options: [{ ignoreMixedLogicalExpressions: false }], + errors: [ + { + messageId: 'preferNullish', + line: 6, + column: 8, + endLine: 6, + endColumn: 10, + suggestions: [ + { + messageId: 'preferNullish', + output: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +declare const d: ${type} | ${nullish}; +a && b ?? c || d; + `.trimRight(), + }, + ], + }, + { + messageId: 'preferNullish', + line: 6, + column: 13, + endLine: 6, + endColumn: 15, + suggestions: [ + { + messageId: 'preferNullish', + output: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +declare const d: ${type} | ${nullish}; +a && b || c ?? d; + `.trimRight(), + }, + ], + }, + ], + })), + + // should not false postivie for functions inside conditional tests + ...nullishTypeInvalidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +if (() => x || 'foo') {} + `, + output: ` +declare const x: ${type} | ${nullish}; +if (() => x ?? 'foo') {} + `, + options: [{ ignoreConditionalTests: true }], + errors: [ + { + messageId: 'preferNullish', + line: 3, + column: 13, + endLine: 3, + endColumn: 15, + }, + ], + })), + ...nullishTypeInvalidTest((nullish, type) => ({ + code: ` +declare const x: ${type} | ${nullish}; +if (function werid() { return x || 'foo' }) {} + `, + output: ` +declare const x: ${type} | ${nullish}; +if (function werid() { return x ?? 'foo' }) {} + `, + options: [{ ignoreConditionalTests: true }], + errors: [ + { + messageId: 'preferNullish', + line: 3, + column: 33, + endLine: 3, + endColumn: 35, + }, + ], + })), + ], +}); diff --git a/packages/experimental-utils/src/ts-eslint/Rule.ts b/packages/experimental-utils/src/ts-eslint/Rule.ts index 2b365528bf9..c74358da558 100644 --- a/packages/experimental-utils/src/ts-eslint/Rule.ts +++ b/packages/experimental-utils/src/ts-eslint/Rule.ts @@ -105,9 +105,9 @@ interface RuleFixer { type ReportFixFunction = ( fixer: RuleFixer, ) => null | RuleFix | RuleFix[] | IterableIterator; -type ReportSuggestionArray = ReportDescriptorBase< - TMessageIds ->[]; +type ReportSuggestionArray = Readonly< + ReportDescriptorBase[] +>; interface ReportDescriptorBase { /**