From 923d486c8c9c9096deac425e7a6cb0b6457eacbd Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Mon, 7 Nov 2022 23:00:38 +0200 Subject: [PATCH] feat(eslint-plugin): [prefer-optional-chain] support suggesting `!foo || !foo.bar` as a valid match for the rule (#5594) * feat/issue5245-negated-or-optional-chaining---fixed * Not supported mixing with TSNonNullExpression * complex computed properties * CR fix comment on unsupported cases and remove TODO comments * CR: Remove unreachable uncovered check --- .../docs/rules/prefer-optional-chain.md | 12 +- .../src/rules/prefer-optional-chain.ts | 350 ++++++++++++++---- .../tests/rules/prefer-optional-chain.test.ts | 175 +++++++++ 3 files changed, 455 insertions(+), 82 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.md b/packages/eslint-plugin/docs/rules/prefer-optional-chain.md index cff46dfdf49..4a9ada8b08f 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.' +description: 'Enforce using concise optional chain expressions instead of chained logical ands, negated logical ors, or empty objects.' --- > 🛑 This file is source code, not the primary documentation location! 🛑 @@ -23,9 +23,15 @@ 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 && @@ -43,6 +49,10 @@ foo?.['a']?.b?.c; foo?.a?.b?.method?.(); foo?.a?.b?.c?.d?.e; + +!foo?.bar; +!foo?.[bar]; +!foo?.bar?.baz?.(); ``` diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index dc9b514e353..89eb72094b6 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -35,7 +35,7 @@ export default util.createRule({ type: 'suggestion', docs: { description: - 'Enforce using concise optional chain expressions instead of chained logical ands', + 'Enforce using concise optional chain expressions instead of chained logical ands, negated logical ors, or empty objects', recommended: 'strict', }, hasSuggestions: true, @@ -111,6 +111,81 @@ 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', @@ -155,94 +230,76 @@ export default util.createRule({ ) { break; } - - 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 - ) { + const { rightText, shouldBreak } = breakIfInvalid({ + rightNode: current.right, + previousLeftText, + }); + if (shouldBreak) { break; } - // 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, - ); + ({ + expressionCount, + previousLeftText, + optionallyChainedCode, + previous, + current, + } = normalizeRepeatingPatterns( + rightText, + expressionCount, + previousLeftText, + optionallyChainedCode, + previous, + current, + )); } - 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), - ], - }, - ], - }); - } + reportIfMoreThanOne({ + expressionCount, + previous, + optionallyChainedCode, + sourceCode, + context, + shouldHandleChainedAnds: true, + }); }, }; + 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( @@ -299,6 +356,11 @@ export default util.createRule({ return getText(node.expression); } + if (node.object.type === AST_NODE_TYPES.TSNonNullExpression) { + // Not supported mixing with TSNonNullExpression + return ''; + } + return getMemberExpressionText(node); } @@ -389,6 +451,132 @@ 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 0358c065870..58b309edcc8 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts @@ -65,6 +65,16 @@ 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()', @@ -147,6 +157,14 @@ 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;', @@ -180,6 +198,17 @@ 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;', + '!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;', + // intentionally do not handle mixed TSNonNullExpression in properties + '!entity.__helper!.__initialized || options.refresh;', + '!foo!.bar || !foo!.bar.baz;', + '!foo!.bar!.baz || !foo!.bar!.baz!.paz;', + '!foo.bar!.baz || !foo.bar!.baz!.paz;', ], invalid: [ ...baseCases, @@ -447,6 +476,22 @@ 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?.();', @@ -1144,5 +1189,135 @@ 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);`, + }, + ], + }, + ], + }, ], });