From d295aa41c5dac9ebdc38cf6d75b66cb5277c9cae Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Fri, 15 Nov 2019 20:16:21 -0800 Subject: [PATCH 1/2] feat(eslint-plugin): add prefer-nullish-coalescing --- packages/eslint-plugin/README.md | 1 + .../docs/rules/prefer-nullish-coalescing.md | 141 ++++++ packages/eslint-plugin/src/configs/all.json | 1 + packages/eslint-plugin/src/rules/index.ts | 2 + .../src/rules/prefer-nullish-coalescing.ts | 158 +++++++ .../rules/prefer-nullish-coalescing.test.ts | 400 ++++++++++++++++++ 6 files changed, 703 insertions(+) create mode 100644 packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md create mode 100644 packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts create mode 100644 packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 9f82b851d29b..70384ba919e4 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -192,6 +192,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-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: | | [`@typescript-eslint/prefer-string-starts-ends-with`](./docs/rules/prefer-string-starts-ends-with.md) | Enforce the use of `String#startsWith` and `String#endsWith` instead of other equivalent methods of checking substrings | :heavy_check_mark: | :wrench: | :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 000000000000..beb6709af485 --- /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. + +**_Note that option will make the rule's fixer unsafe to run on an existing codebase. You should manually fix up existing violations before running the autofixer._** + +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); +``` + +## 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 23b38b458a3b..7cea4acdf3c0 100644 --- a/packages/eslint-plugin/src/configs/all.json +++ b/packages/eslint-plugin/src/configs/all.json @@ -65,6 +65,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-readonly": "error", "@typescript-eslint/prefer-regexp-exec": "error", "@typescript-eslint/prefer-string-starts-ends-with": "error", diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index 35f2d7582912..4255cb187fdb 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -49,6 +49,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 preferReadonly from './prefer-readonly'; import preferRegexpExec from './prefer-regexp-exec'; import preferStringStartsEndsWith from './prefer-string-starts-ends-with'; @@ -120,6 +121,7 @@ export default { 'prefer-function-type': preferFunctionType, 'prefer-includes': preferIncludes, 'prefer-namespace-keyword': preferNamespaceKeyword, + 'prefer-nullish-coalescing': preferNullishCoalescing, 'prefer-readonly': preferReadonly, 'prefer-regexp-exec': preferRegexpExec, 'prefer-string-starts-ends-with': preferStringStartsEndsWith, 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 000000000000..852953cd0a4f --- /dev/null +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -0,0 +1,158 @@ +import { + AST_TOKEN_TYPES, + TSESTree, + AST_NODE_TYPES, +} 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; + } + + if ( + ignoreMixedLogicalExpressions === true && + isMixedLogicalExpression(node) + ) { + return; + } + + const barBarOperator = sourceCode.getTokenAfter( + node.left, + token => + token.type === AST_TOKEN_TYPES.Punctuator && + token.value === node.operator, + )!; // there _must_ be an operator + context.report({ + node: barBarOperator, + messageId: 'preferNullish', + fix(fixer) { + return fixer.replaceText(barBarOperator, '??'); + }, + }); + }, + }; + }, +}); + +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 000000000000..639941f64cc8 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -0,0 +1,400 @@ +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; + `, + output: ` +declare const a: ${type} | ${nullish}; +declare const b: ${type} | ${nullish}; +declare const c: ${type} | ${nullish}; +a ?? b && c; + `, + options: [{ ignoreMixedLogicalExpressions: false }], + errors: [ + { + messageId: 'preferNullish', + line: 5, + column: 3, + endLine: 5, + endColumn: 5, + }, + ], + })), + ...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; + `, + 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; + `, + options: [{ ignoreMixedLogicalExpressions: false }], + errors: [ + { + messageId: 'preferNullish', + line: 6, + column: 3, + endLine: 6, + endColumn: 5, + }, + { + messageId: 'preferNullish', + line: 6, + column: 8, + endLine: 6, + endColumn: 10, + }, + ], + })), + ...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; + `, + 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; + `, + options: [{ ignoreMixedLogicalExpressions: false }], + errors: [ + { + messageId: 'preferNullish', + line: 6, + column: 8, + endLine: 6, + endColumn: 10, + }, + { + messageId: 'preferNullish', + line: 6, + column: 13, + endLine: 6, + endColumn: 15, + }, + ], + })), + + // 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, + }, + ], + })), + ], +}); From 9e4157d1a84de674f990fadd0ad4eee00c727edc Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Sun, 24 Nov 2019 21:57:25 -0800 Subject: [PATCH 2/2] feat: switch to suggestions for unsafe conversions --- .../docs/rules/prefer-nullish-coalescing.md | 4 +- .../src/rules/prefer-nullish-coalescing.ts | 32 +++++-- .../rules/prefer-nullish-coalescing.test.ts | 85 ++++++++++++++----- .../experimental-utils/src/ts-eslint/Rule.ts | 6 +- 4 files changed, 91 insertions(+), 36 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md index beb6709af485..140ade2dbe7c 100644 --- a/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md +++ b/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md @@ -99,8 +99,6 @@ Setting this option to `true` (the default) will cause the rule to ignore any lo 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. -**_Note that option will make the rule's fixer unsafe to run on an existing codebase. You should manually fix up existing violations before running the autofixer._** - 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`: @@ -131,6 +129,8 @@ 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. diff --git a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts index 852953cd0a4f..afb564025fd7 100644 --- a/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts +++ b/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts @@ -1,7 +1,8 @@ import { + AST_NODE_TYPES, AST_TOKEN_TYPES, + TSESLint, TSESTree, - AST_NODE_TYPES, } from '@typescript-eslint/experimental-utils'; import ts from 'typescript'; import * as util from '../util'; @@ -73,10 +74,8 @@ export default util.createRule({ return; } - if ( - ignoreMixedLogicalExpressions === true && - isMixedLogicalExpression(node) - ) { + const isMixedLogical = isMixedLogicalExpression(node); + if (ignoreMixedLogicalExpressions === true && isMixedLogical) { return; } @@ -86,12 +85,29 @@ export default util.createRule({ 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', - fix(fixer) { - return fixer.replaceText(barBarOperator, '??'); - }, + ...fixer, }); }, }; 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 639941f64cc8..1f05d6b9a3fc 100644 --- a/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-nullish-coalescing.test.ts @@ -270,13 +270,7 @@ declare const a: ${type} | ${nullish}; declare const b: ${type} | ${nullish}; declare const c: ${type} | ${nullish}; a || b && c; - `, - output: ` -declare const a: ${type} | ${nullish}; -declare const b: ${type} | ${nullish}; -declare const c: ${type} | ${nullish}; -a ?? b && c; - `, + `.trimRight(), options: [{ ignoreMixedLogicalExpressions: false }], errors: [ { @@ -285,6 +279,17 @@ a ?? b && c; 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(), + }, + ], }, ], })), @@ -295,14 +300,7 @@ declare const b: ${type} | ${nullish}; declare const c: ${type} | ${nullish}; declare const d: ${type} | ${nullish}; a || b || c && d; - `, - 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(), options: [{ ignoreMixedLogicalExpressions: false }], errors: [ { @@ -311,6 +309,18 @@ a ?? b ?? c && d; 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', @@ -318,6 +328,18 @@ a ?? b ?? c && d; 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(), + }, + ], }, ], })), @@ -328,14 +350,7 @@ declare const b: ${type} | ${nullish}; declare const c: ${type} | ${nullish}; declare const d: ${type} | ${nullish}; a && b || c || d; - `, - 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(), options: [{ ignoreMixedLogicalExpressions: false }], errors: [ { @@ -344,6 +359,18 @@ a && b ?? c ?? d; 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', @@ -351,6 +378,18 @@ a && b ?? c ?? d; 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(), + }, + ], }, ], })), diff --git a/packages/experimental-utils/src/ts-eslint/Rule.ts b/packages/experimental-utils/src/ts-eslint/Rule.ts index 2b365528bf96..c74358da5587 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 { /**