diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 9a4949b734b..f5ed3ccb589 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -490,10 +490,20 @@ function reportIfMoreThanOne({ shouldHandleChainedAnds && previous.right.type === AST_NODE_TYPES.BinaryExpression ) { + let operator = previous.right.operator; + if ( + previous.right.operator === '!==' && + // TODO(#4820): Use the type checker to know whether this is `null` + previous.right.right.type === AST_NODE_TYPES.Literal && + previous.right.right.raw === 'null' + ) { + // case like foo !== null && foo.bar !== null + operator = '!='; + } // case like foo && foo.bar !== someValue - optionallyChainedCode += ` ${ - previous.right.operator - } ${sourceCode.getText(previous.right.right)}`; + optionallyChainedCode += ` ${operator} ${sourceCode.getText( + previous.right.right, + )}`; } context.report({ diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/base-cases.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/base-cases.ts new file mode 100644 index 00000000000..99cfe6b0ff9 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/base-cases.ts @@ -0,0 +1,228 @@ +import type { TSESLint } from '@typescript-eslint/utils'; + +import type rule from '../../../src/rules/prefer-optional-chain'; +import type { + InferMessageIdsTypeFromRule, + InferOptionsTypeFromRule, +} from '../../../src/util'; + +type InvalidTestCase = TSESLint.InvalidTestCase< + InferMessageIdsTypeFromRule, + InferOptionsTypeFromRule +>; + +interface BaseCase { + canReplaceAndWithOr: boolean; + output: string; + code: string; +} + +const mapper = (c: BaseCase): InvalidTestCase => ({ + code: c.code.trim(), + output: null, + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: c.output.trim(), + }, + ], + }, + ], +}); + +const baseCases: Array = [ + // chained members + { + code: 'foo && foo.bar', + output: 'foo?.bar', + canReplaceAndWithOr: true, + }, + { + code: 'foo.bar && foo.bar.baz', + output: 'foo.bar?.baz', + canReplaceAndWithOr: true, + }, + { + code: 'foo && foo()', + output: 'foo?.()', + canReplaceAndWithOr: true, + }, + { + code: 'foo.bar && foo.bar()', + output: 'foo.bar?.()', + canReplaceAndWithOr: true, + }, + { + code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz.buzz', + output: 'foo?.bar?.baz?.buzz', + canReplaceAndWithOr: true, + }, + { + code: 'foo.bar && foo.bar.baz && foo.bar.baz.buzz', + output: 'foo.bar?.baz?.buzz', + canReplaceAndWithOr: true, + }, + // case with a jump (i.e. a non-nullish prop) + { + code: 'foo && foo.bar && foo.bar.baz.buzz', + output: 'foo?.bar?.baz.buzz', + canReplaceAndWithOr: true, + }, + { + code: 'foo.bar && foo.bar.baz.buzz', + output: 'foo.bar?.baz.buzz', + canReplaceAndWithOr: true, + }, + // case where for some reason there is a doubled up expression + { + code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz && foo.bar.baz.buzz', + output: 'foo?.bar?.baz?.buzz', + canReplaceAndWithOr: true, + }, + { + code: 'foo.bar && foo.bar.baz && foo.bar.baz && foo.bar.baz.buzz', + output: 'foo.bar?.baz?.buzz', + canReplaceAndWithOr: true, + }, + // chained members with element access + { + code: 'foo && foo[bar] && foo[bar].baz && foo[bar].baz.buzz', + output: 'foo?.[bar]?.baz?.buzz', + canReplaceAndWithOr: true, + }, + { + // case with a jump (i.e. a non-nullish prop) + code: 'foo && foo[bar].baz && foo[bar].baz.buzz', + output: 'foo?.[bar].baz?.buzz', + canReplaceAndWithOr: true, + }, + // case with a property access in computed property + { + code: 'foo && foo[bar.baz] && foo[bar.baz].buzz', + output: 'foo?.[bar.baz]?.buzz', + canReplaceAndWithOr: true, + }, + // case with this keyword + { + code: 'foo[this.bar] && foo[this.bar].baz', + output: 'foo[this.bar]?.baz', + canReplaceAndWithOr: true, + }, + // chained calls + { + code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz.buzz()', + output: 'foo?.bar?.baz?.buzz()', + canReplaceAndWithOr: true, + }, + { + code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz.buzz && foo.bar.baz.buzz()', + output: 'foo?.bar?.baz?.buzz?.()', + canReplaceAndWithOr: true, + }, + { + code: 'foo.bar && foo.bar.baz && foo.bar.baz.buzz && foo.bar.baz.buzz()', + output: 'foo.bar?.baz?.buzz?.()', + canReplaceAndWithOr: true, + }, + // case with a jump (i.e. a non-nullish prop) + { + code: 'foo && foo.bar && foo.bar.baz.buzz()', + output: 'foo?.bar?.baz.buzz()', + canReplaceAndWithOr: true, + }, + { + code: 'foo.bar && foo.bar.baz.buzz()', + output: 'foo.bar?.baz.buzz()', + canReplaceAndWithOr: true, + }, + { + // case with a jump (i.e. a non-nullish prop) + code: 'foo && foo.bar && foo.bar.baz.buzz && foo.bar.baz.buzz()', + output: 'foo?.bar?.baz.buzz?.()', + canReplaceAndWithOr: true, + }, + { + // case with a call expr inside the chain for some inefficient reason + code: 'foo && foo.bar() && foo.bar().baz && foo.bar().baz.buzz && foo.bar().baz.buzz()', + output: 'foo?.bar()?.baz?.buzz?.()', + canReplaceAndWithOr: true, + }, + // chained calls with element access + { + code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz[buzz]()', + output: 'foo?.bar?.baz?.[buzz]()', + canReplaceAndWithOr: true, + }, + { + code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz[buzz] && foo.bar.baz[buzz]()', + output: 'foo?.bar?.baz?.[buzz]?.()', + canReplaceAndWithOr: true, + }, + // (partially) pre-optional chained + { + code: 'foo && foo?.bar && foo?.bar.baz && foo?.bar.baz[buzz] && foo?.bar.baz[buzz]()', + output: 'foo?.bar?.baz?.[buzz]?.()', + canReplaceAndWithOr: true, + }, + { + code: 'foo && foo?.bar.baz && foo?.bar.baz[buzz]', + output: 'foo?.bar.baz?.[buzz]', + canReplaceAndWithOr: true, + }, + { + code: 'foo && foo?.() && foo?.().bar', + output: 'foo?.()?.bar', + canReplaceAndWithOr: true, + }, + { + code: 'foo.bar && foo.bar?.() && foo.bar?.().baz', + output: 'foo.bar?.()?.baz', + canReplaceAndWithOr: true, + }, + { + code: 'foo !== null && foo.bar !== null', + output: 'foo?.bar != null', + canReplaceAndWithOr: false, + }, + { + code: 'foo != null && foo.bar != null', + output: 'foo?.bar != null', + canReplaceAndWithOr: false, + }, + { + code: 'foo != null && foo.bar !== null', + output: 'foo?.bar != null', + canReplaceAndWithOr: false, + }, + { + code: 'foo !== null && foo.bar != null', + output: 'foo?.bar != null', + canReplaceAndWithOr: false, + }, +]; + +interface Selector { + all(): Array; + select>( + key: K, + value: BaseCase[K], + ): Selector; +} + +const selector = (cases: Array): Selector => ({ + all: () => cases.map(mapper), + select: >( + key: K, + value: BaseCase[K], + ): Selector => { + const selectedCases = baseCases.filter(c => c[key] === value); + return selector(selectedCases); + }, +}); + +const { all, select } = selector(baseCases); + +export { all, select }; diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts similarity index 85% rename from packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts rename to packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 120ad20aaea..ca783df13b3 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -1,160 +1,11 @@ -import type { TSESLint } from '@typescript-eslint/utils'; - -import rule from '../../src/rules/prefer-optional-chain'; -import type { - InferMessageIdsTypeFromRule, - InferOptionsTypeFromRule, -} from '../../src/util'; -import { noFormat, RuleTester } from '../RuleTester'; +import rule from '../../../src/rules/prefer-optional-chain'; +import { noFormat, RuleTester } from '../../RuleTester'; +import * as BaseCases from './base-cases'; const ruleTester = new RuleTester({ parser: '@typescript-eslint/parser', }); -const baseCases = [ - // chained members - { - code: 'foo && foo.bar', - output: 'foo?.bar', - }, - { - code: 'foo.bar && foo.bar.baz', - output: 'foo.bar?.baz', - }, - { - code: 'foo && foo()', - output: 'foo?.()', - }, - { - code: 'foo.bar && foo.bar()', - output: 'foo.bar?.()', - }, - { - code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz.buzz', - output: 'foo?.bar?.baz?.buzz', - }, - { - code: 'foo.bar && foo.bar.baz && foo.bar.baz.buzz', - output: 'foo.bar?.baz?.buzz', - }, - // case with a jump (i.e. a non-nullish prop) - { - code: 'foo && foo.bar && foo.bar.baz.buzz', - output: 'foo?.bar?.baz.buzz', - }, - { - code: 'foo.bar && foo.bar.baz.buzz', - output: 'foo.bar?.baz.buzz', - }, - // case where for some reason there is a doubled up expression - { - code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz && foo.bar.baz.buzz', - output: 'foo?.bar?.baz?.buzz', - }, - { - code: 'foo.bar && foo.bar.baz && foo.bar.baz && foo.bar.baz.buzz', - output: 'foo.bar?.baz?.buzz', - }, - // chained members with element access - { - code: 'foo && foo[bar] && foo[bar].baz && foo[bar].baz.buzz', - output: 'foo?.[bar]?.baz?.buzz', - }, - { - // case with a jump (i.e. a non-nullish prop) - code: 'foo && foo[bar].baz && foo[bar].baz.buzz', - output: 'foo?.[bar].baz?.buzz', - }, - // case with a property access in computed property - { - code: 'foo && foo[bar.baz] && foo[bar.baz].buzz', - output: 'foo?.[bar.baz]?.buzz', - }, - // case with this keyword - { - code: 'foo[this.bar] && foo[this.bar].baz', - output: 'foo[this.bar]?.baz', - }, - // chained calls - { - code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz.buzz()', - output: 'foo?.bar?.baz?.buzz()', - }, - { - code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz.buzz && foo.bar.baz.buzz()', - output: 'foo?.bar?.baz?.buzz?.()', - }, - { - code: 'foo.bar && foo.bar.baz && foo.bar.baz.buzz && foo.bar.baz.buzz()', - output: 'foo.bar?.baz?.buzz?.()', - }, - // case with a jump (i.e. a non-nullish prop) - { - code: 'foo && foo.bar && foo.bar.baz.buzz()', - output: 'foo?.bar?.baz.buzz()', - }, - { - code: 'foo.bar && foo.bar.baz.buzz()', - output: 'foo.bar?.baz.buzz()', - }, - { - // case with a jump (i.e. a non-nullish prop) - code: 'foo && foo.bar && foo.bar.baz.buzz && foo.bar.baz.buzz()', - output: 'foo?.bar?.baz.buzz?.()', - }, - { - // case with a call expr inside the chain for some inefficient reason - code: 'foo && foo.bar() && foo.bar().baz && foo.bar().baz.buzz && foo.bar().baz.buzz()', - output: 'foo?.bar()?.baz?.buzz?.()', - }, - // chained calls with element access - { - code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz[buzz]()', - output: 'foo?.bar?.baz?.[buzz]()', - }, - { - code: 'foo && foo.bar && foo.bar.baz && foo.bar.baz[buzz] && foo.bar.baz[buzz]()', - output: 'foo?.bar?.baz?.[buzz]?.()', - }, - // (partially) pre-optional chained - { - code: 'foo && foo?.bar && foo?.bar.baz && foo?.bar.baz[buzz] && foo?.bar.baz[buzz]()', - output: 'foo?.bar?.baz?.[buzz]?.()', - }, - { - code: 'foo && foo?.bar.baz && foo?.bar.baz[buzz]', - output: 'foo?.bar.baz?.[buzz]', - }, - { - code: 'foo && foo?.() && foo?.().bar', - output: 'foo?.()?.bar', - }, - { - code: 'foo.bar && foo.bar?.() && foo.bar?.().baz', - output: 'foo.bar?.()?.baz', - }, -].map( - c => - ({ - code: c.code.trim(), - output: null, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: c.output.trim(), - }, - ], - }, - ], - } as TSESLint.InvalidTestCase< - InferMessageIdsTypeFromRule, - InferOptionsTypeFromRule - >), -); - ruleTester.run('prefer-optional-chain', rule, { valid: [ '!a || !b;', @@ -219,18 +70,18 @@ ruleTester.run('prefer-optional-chain', rule, { '!new.target || true;', ], invalid: [ - ...baseCases, + ...BaseCases.all(), // it should ignore whitespace in the expressions - ...baseCases.map(c => ({ + ...BaseCases.all().map(c => ({ ...c, code: c.code.replace(/\./g, '. '), })), - ...baseCases.map(c => ({ + ...BaseCases.all().map(c => ({ ...c, code: c.code.replace(/\./g, '.\n'), })), // it should ignore parts of the expression that aren't part of the expression chain - ...baseCases.map(c => ({ + ...BaseCases.all().map(c => ({ ...c, code: `${c.code} && bing`, errors: [ @@ -245,7 +96,7 @@ ruleTester.run('prefer-optional-chain', rule, { }, ], })), - ...baseCases.map(c => ({ + ...BaseCases.all().map(c => ({ ...c, code: `${c.code} && bing.bong`, errors: [ @@ -261,22 +112,42 @@ ruleTester.run('prefer-optional-chain', rule, { ], })), // strict nullish equality checks x !== null && x.y !== null - ...baseCases.map(c => ({ + ...BaseCases.all().map(c => ({ ...c, code: c.code.replace(/&&/g, '!== null &&'), })), - ...baseCases.map(c => ({ + ...BaseCases.all().map(c => ({ ...c, code: c.code.replace(/&&/g, '!= null &&'), })), - ...baseCases.map(c => ({ + ...BaseCases.all().map(c => ({ ...c, code: c.code.replace(/&&/g, '!== undefined &&'), })), - ...baseCases.map(c => ({ + ...BaseCases.all().map(c => ({ ...c, code: c.code.replace(/&&/g, '!= undefined &&'), })), + + // replace && with ||: foo && foo.bar -> !foo || !foo.bar + ...BaseCases.select('canReplaceAndWithOr', true) + .all() + .map(c => ({ + ...c, + code: c.code.replace(/(^|\s)foo/g, '$1!foo').replace(/&&/g, '||'), + errors: [ + { + ...c.errors[0], + suggestions: [ + { + ...c.errors[0].suggestions![0], + output: `!${c.errors[0].suggestions![0].output}`, + }, + ], + }, + ], + })), + // two errors { code: noFormat`foo && foo.bar && foo.bar.baz || baz && baz.bar && baz.bar.foo`, @@ -1211,21 +1082,6 @@ foo?.bar(/* comment */a, }, ], }, - ...baseCases.map(c => ({ - ...c, - code: c.code.replace(/foo/g, '!foo').replace(/&&/g, '||'), - errors: [ - { - ...c.errors[0], - suggestions: [ - { - ...c.errors[0].suggestions![0], - output: `!${c.errors[0].suggestions![0].output}`, - }, - ], - }, - ], - })), // case with this keyword at the start of expression { code: '!this.bar || !this.bar.baz;',