From 7a8afe29039c6c80fe584acaf5d933424a4452a9 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Mon, 29 Aug 2022 22:01:53 +0930 Subject: [PATCH] fix(eslint-plugin): revert #5266 (#5564) --- .../docs/rules/prefer-optional-chain.md | 12 +- .../src/rules/prefer-optional-chain.ts | 346 ++++-------------- .../tests/rules/prefer-optional-chain.test.ts | 189 ---------- 3 files changed, 82 insertions(+), 465 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.md b/packages/eslint-plugin/docs/rules/prefer-optional-chain.md index 16b6d31883f..a0ff0a0ab22 100644 --- a/packages/eslint-plugin/docs/rules/prefer-optional-chain.md +++ b/packages/eslint-plugin/docs/rules/prefer-optional-chain.md @@ -1,5 +1,5 @@ --- -description: 'Enforce using concise optional chain expressions instead of chained logical ands, negated logical ors, or empty objects.' +description: 'Enforce using concise optional chain expressions instead of chained logical ands.' --- > 🛑 This file is source code, not the primary documentation location! 🛑 @@ -65,15 +65,9 @@ foo && foo.a && foo.a.b && foo.a.b.c; foo && foo['a'] && foo['a'].b && foo['a'].b.c; foo && foo.a && foo.a.b && foo.a.b.method && foo.a.b.method(); -// With empty objects (((foo || {}).a || {}).b || {}).c; (((foo || {})['a'] || {}).b || {}).c; -// With negated `or`s -!foo || !foo.bar; -!foo || !foo[bar]; -!foo || !foo.bar || !foo.bar.baz || !foo.bar.baz(); - // this rule also supports converting chained strict nullish checks: foo && foo.a != null && @@ -91,10 +85,6 @@ foo?.['a']?.b?.c; foo?.a?.b?.method?.(); foo?.a?.b?.c?.d?.e; - -!foo?.bar; -!foo?.[bar]; -!foo?.bar?.baz?.(); ``` **Note:** there are a few edge cases where this rule will false positive. Use your best judgement when evaluating reported errors. diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index d33729d5eba..2e93a8d6b92 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -13,7 +13,6 @@ type ValidChainTarget = /* The AST is always constructed such the first element is always the deepest element. - I.e. for this code: `foo && foo.bar && foo.bar.baz && foo.bar.baz.buzz` The AST will look like this: { @@ -34,7 +33,7 @@ export default util.createRule({ type: 'suggestion', docs: { description: - 'Enforce using concise optional chain expressions instead of chained logical ands, negated logical ors, or empty objects', + 'Enforce using concise optional chain expressions instead of chained logical ands', recommended: 'strict', }, hasSuggestions: true, @@ -110,81 +109,6 @@ export default util.createRule({ ], }); }, - [[ - 'LogicalExpression[operator="||"] > UnaryExpression[operator="!"] > Identifier', - 'LogicalExpression[operator="||"] > UnaryExpression[operator="!"] > MemberExpression', - 'LogicalExpression[operator="||"] > UnaryExpression[operator="!"] > ChainExpression > MemberExpression', - ].join(',')]( - initialIdentifierOrNotEqualsExpr: - | TSESTree.Identifier - | TSESTree.MemberExpression, - ): void { - // selector guarantees this cast - const initialExpression = ( - initialIdentifierOrNotEqualsExpr.parent!.type === - AST_NODE_TYPES.ChainExpression - ? initialIdentifierOrNotEqualsExpr.parent.parent - : initialIdentifierOrNotEqualsExpr.parent - )!.parent as TSESTree.LogicalExpression; - - if ( - initialExpression.left.type !== AST_NODE_TYPES.UnaryExpression || - initialExpression.left.argument !== initialIdentifierOrNotEqualsExpr - ) { - // the node(identifier or member expression) is not the deepest left node - return; - } - - // walk up the tree to figure out how many logical expressions we can include - let previous: TSESTree.LogicalExpression = initialExpression; - let current: TSESTree.Node = initialExpression; - let previousLeftText = getText(initialIdentifierOrNotEqualsExpr); - let optionallyChainedCode = previousLeftText; - let expressionCount = 1; - while (current.type === AST_NODE_TYPES.LogicalExpression) { - if ( - current.right.type !== AST_NODE_TYPES.UnaryExpression || - !isValidChainTarget( - current.right.argument, - // only allow unary '!' with identifiers for the first chain - !foo || !foo() - expressionCount === 1, - ) - ) { - break; - } - const { rightText, shouldBreak } = breakIfInvalid({ - rightNode: current.right.argument, - previousLeftText, - }); - if (shouldBreak) { - break; - } - - ({ - expressionCount, - previousLeftText, - optionallyChainedCode, - previous, - current, - } = normalizeRepeatingPatterns( - rightText, - expressionCount, - previousLeftText, - optionallyChainedCode, - previous, - current, - )); - } - - reportIfMoreThanOne({ - expressionCount, - previous, - optionallyChainedCode, - sourceCode, - context, - shouldHandleChainedAnds: false, - }); - }, [[ 'LogicalExpression[operator="&&"] > Identifier', 'LogicalExpression[operator="&&"] > MemberExpression', @@ -229,76 +153,94 @@ export default util.createRule({ ) { break; } - const { rightText, shouldBreak } = breakIfInvalid({ - rightNode: current.right, - previousLeftText, - }); - if (shouldBreak) { + + const leftText = previousLeftText; + const rightText = getText(current.right); + // can't just use startsWith because of cases like foo && fooBar.baz; + const matchRegex = new RegExp( + `^${ + // escape regex characters + leftText.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + }[^a-zA-Z0-9_$]`, + ); + if ( + !matchRegex.test(rightText) && + // handle redundant cases like foo.bar && foo.bar + leftText !== rightText + ) { break; } - ({ - expressionCount, - previousLeftText, - optionallyChainedCode, - previous, - current, - } = normalizeRepeatingPatterns( - rightText, - expressionCount, - previousLeftText, - optionallyChainedCode, - previous, - current, - )); + // omit weird doubled up expression that make no sense like foo.bar && foo.bar + if (rightText !== leftText) { + expressionCount += 1; + previousLeftText = rightText; + + /* + Diff the left and right text to construct the fix string + There are the following cases: + 1) + rightText === 'foo.bar.baz.buzz' + leftText === 'foo.bar.baz' + diff === '.buzz' + 2) + rightText === 'foo.bar.baz.buzz()' + leftText === 'foo.bar.baz' + diff === '.buzz()' + 3) + rightText === 'foo.bar.baz.buzz()' + leftText === 'foo.bar.baz.buzz' + diff === '()' + 4) + rightText === 'foo.bar.baz[buzz]' + leftText === 'foo.bar.baz' + diff === '[buzz]' + 5) + rightText === 'foo.bar.baz?.buzz' + leftText === 'foo.bar.baz' + diff === '?.buzz' + */ + const diff = rightText.replace(leftText, ''); + if (diff.startsWith('?')) { + // item was "pre optional chained" + optionallyChainedCode += diff; + } else { + const needsDot = diff.startsWith('(') || diff.startsWith('['); + optionallyChainedCode += `?${needsDot ? '.' : ''}${diff}`; + } + } + + previous = current; + current = util.nullThrows( + current.parent, + util.NullThrowsReasons.MissingParent, + ); } - reportIfMoreThanOne({ - expressionCount, - previous, - optionallyChainedCode, - sourceCode, - context, - shouldHandleChainedAnds: true, - }); + if (expressionCount > 1) { + if (previous.right.type === AST_NODE_TYPES.BinaryExpression) { + // case like foo && foo.bar !== someValue + optionallyChainedCode += ` ${ + previous.right.operator + } ${sourceCode.getText(previous.right.right)}`; + } + + context.report({ + node: previous, + messageId: 'preferOptionalChain', + suggest: [ + { + messageId: 'optionalChainSuggest', + fix: (fixer): TSESLint.RuleFix[] => [ + fixer.replaceText(previous, optionallyChainedCode), + ], + }, + ], + }); + } }, }; - interface BreakIfInvalidResult { - leftText: string; - rightText: string; - shouldBreak: boolean; - } - - interface BreakIfInvalidOptions { - previousLeftText: string; - rightNode: ValidChainTarget; - } - - function breakIfInvalid({ - previousLeftText, - rightNode, - }: BreakIfInvalidOptions): BreakIfInvalidResult { - let shouldBreak = false; - - const rightText = getText(rightNode); - // can't just use startsWith because of cases like foo && fooBar.baz; - const matchRegex = new RegExp( - `^${ - // escape regex characters - previousLeftText.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') - }[^a-zA-Z0-9_$]`, - ); - if ( - !matchRegex.test(rightText) && - // handle redundant cases like foo.bar && foo.bar - previousLeftText !== rightText - ) { - shouldBreak = true; - } - return { shouldBreak, leftText: previousLeftText, rightText }; - } - function getText(node: ValidChainTarget): string { if (node.type === AST_NODE_TYPES.BinaryExpression) { return getText( @@ -445,132 +387,6 @@ const ALLOWED_NON_COMPUTED_PROP_TYPES: ReadonlySet = new Set([ AST_NODE_TYPES.Identifier, ]); -interface ReportIfMoreThanOneOptions { - expressionCount: number; - previous: TSESTree.LogicalExpression; - optionallyChainedCode: string; - sourceCode: Readonly; - context: Readonly< - TSESLint.RuleContext< - 'preferOptionalChain' | 'optionalChainSuggest', - never[] - > - >; - shouldHandleChainedAnds: boolean; -} - -function reportIfMoreThanOne({ - expressionCount, - previous, - optionallyChainedCode, - sourceCode, - context, - shouldHandleChainedAnds, -}: ReportIfMoreThanOneOptions): void { - if (expressionCount > 1) { - if ( - shouldHandleChainedAnds && - previous.right.type === AST_NODE_TYPES.BinaryExpression - ) { - // case like foo && foo.bar !== someValue - optionallyChainedCode += ` ${ - previous.right.operator - } ${sourceCode.getText(previous.right.right)}`; - } - - context.report({ - node: previous, - messageId: 'preferOptionalChain', - suggest: [ - { - messageId: 'optionalChainSuggest', - fix: (fixer): TSESLint.RuleFix[] => [ - fixer.replaceText( - previous, - `${shouldHandleChainedAnds ? '' : '!'}${optionallyChainedCode}`, - ), - ], - }, - ], - }); - } -} - -interface NormalizedPattern { - expressionCount: number; - previousLeftText: string; - optionallyChainedCode: string; - previous: TSESTree.LogicalExpression; - current: TSESTree.Node; -} - -function normalizeRepeatingPatterns( - rightText: string, - expressionCount: number, - previousLeftText: string, - optionallyChainedCode: string, - previous: TSESTree.Node, - current: TSESTree.Node, -): NormalizedPattern { - const leftText = previousLeftText; - // omit weird doubled up expression that make no sense like foo.bar && foo.bar - if (rightText !== previousLeftText) { - expressionCount += 1; - previousLeftText = rightText; - - /* - Diff the left and right text to construct the fix string - There are the following cases: - - 1) - rightText === 'foo.bar.baz.buzz' - leftText === 'foo.bar.baz' - diff === '.buzz' - - 2) - rightText === 'foo.bar.baz.buzz()' - leftText === 'foo.bar.baz' - diff === '.buzz()' - - 3) - rightText === 'foo.bar.baz.buzz()' - leftText === 'foo.bar.baz.buzz' - diff === '()' - - 4) - rightText === 'foo.bar.baz[buzz]' - leftText === 'foo.bar.baz' - diff === '[buzz]' - - 5) - rightText === 'foo.bar.baz?.buzz' - leftText === 'foo.bar.baz' - diff === '?.buzz' - */ - const diff = rightText.replace(leftText, ''); - if (diff.startsWith('?')) { - // item was "pre optional chained" - optionallyChainedCode += diff; - } else { - const needsDot = diff.startsWith('(') || diff.startsWith('['); - optionallyChainedCode += `?${needsDot ? '.' : ''}${diff}`; - } - } - - previous = current as TSESTree.LogicalExpression; - current = util.nullThrows( - current.parent, - util.NullThrowsReasons.MissingParent, - ); - return { - expressionCount, - previousLeftText, - optionallyChainedCode, - previous, - current, - }; -} - function isValidChainTarget( node: TSESTree.Node, allowIdentifier: boolean, diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts index 99c8803f3fd..289e2c83fa5 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts @@ -64,16 +64,6 @@ const baseCases = [ 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()', @@ -132,11 +122,6 @@ const baseCases = [ code: 'foo.bar && foo.bar?.() && foo.bar?.().baz', output: 'foo.bar?.()?.baz', }, - // TODO: deepest left node already pre-optional chained - // { - // code: 'foo?.bar && foo.bar?.() && foo.bar?.().baz', - // output: 'foo?.bar?.()?.baz', - // }, ].map( c => ({ @@ -161,14 +146,6 @@ const baseCases = [ ruleTester.run('prefer-optional-chain', rule, { valid: [ - '!a || !b;', - '!a || a.b;', - '!a && a.b;', - '!a && !a.b;', - '!a.b || a.b?.();', - '!a.b || a.b();', - '!foo() || !foo().bar;', - 'foo || {};', 'foo || ({} as any);', '(foo || {})?.bar;', @@ -185,7 +162,6 @@ ruleTester.run('prefer-optional-chain', rule, { 'foo ?? {};', '(foo ?? {})?.bar;', 'foo ||= bar ?? {};', - 'foo && bar;', 'foo && foo;', 'foo || bar;', @@ -203,9 +179,6 @@ ruleTester.run('prefer-optional-chain', rule, { 'foo && foo[bar as string] && foo[bar as string].baz;', 'foo && foo[1 + 2] && foo[1 + 2].baz;', 'foo && foo[typeof bar] && foo[typeof bar].baz;', - // currently do not handle 'this' as the first part of a chain - 'this && this.foo;', - '!this || !this.foo;', ], invalid: [ ...baseCases, @@ -473,22 +446,6 @@ foo?.bar(/* comment */a, }, ], }, - // case with this keyword at the start of expression - { - code: 'this.bar && this.bar.baz;', - output: null, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: 'this.bar?.baz;', - }, - ], - }, - ], - }, // other weird cases { code: 'foo && foo?.();', @@ -1186,151 +1143,5 @@ foo?.bar(/* comment */a, }, ], }, - { - code: '(this || {}).foo;', - errors: [ - { - messageId: 'optionalChainSuggest', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: 'this?.foo;', - }, - ], - }, - ], - }, - ...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;', - output: null, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: '!this.bar?.baz;', - }, - ], - }, - ], - }, - { - code: '!a.b || !a.b();', - output: null, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: '!a.b?.();', - }, - ], - }, - ], - }, - { - code: '!foo.bar || !foo.bar.baz;', - output: null, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: '!foo.bar?.baz;', - }, - ], - }, - ], - }, - { - code: '!foo[bar] || !foo[bar]?.[baz];', - output: null, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: '!foo[bar]?.[baz];', - }, - ], - }, - ], - }, - { - code: '!foo || !foo?.bar.baz;', - output: null, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: '!foo?.bar.baz;', - }, - ], - }, - ], - }, - // two errors - { - code: noFormat`(!foo || !foo.bar || !foo.bar.baz) && (!baz || !baz.bar || !baz.bar.foo);`, - output: null, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: noFormat`(!foo?.bar?.baz) && (!baz || !baz.bar || !baz.bar.foo);`, - }, - ], - }, - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: noFormat`(!foo || !foo.bar || !foo.bar.baz) && (!baz?.bar?.foo);`, - }, - ], - }, - ], - }, - // TODO: deepest left node already pre-optional chained - // { - // code: '!foo?.bar || !foo?.bar.baz;', - // output: null, - // errors: [ - // { - // messageId: 'preferOptionalChain', - // suggestions: [ - // { - // messageId: 'optionalChainSuggest', - // output: '!foo?.bar?.baz;', - // }, - // ], - // }, - // ], - // }, ], });