diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.md b/packages/eslint-plugin/docs/rules/prefer-optional-chain.md index a0ff0a0ab22..16b6d31883f 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! 🛑 @@ -65,9 +65,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 && @@ -85,6 +91,10 @@ 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/explicit-function-return-type.ts b/packages/eslint-plugin/src/rules/explicit-function-return-type.ts index 9e5a377ee5f..4b18307281c 100644 --- a/packages/eslint-plugin/src/rules/explicit-function-return-type.ts +++ b/packages/eslint-plugin/src/rules/explicit-function-return-type.ts @@ -90,7 +90,7 @@ export default util.createRule({ | TSESTree.FunctionExpression | TSESTree.FunctionDeclaration, ): boolean { - if (!options.allowedNames || !options.allowedNames.length) { + if (!options.allowedNames?.length) { return false; } diff --git a/packages/eslint-plugin/src/rules/no-useless-constructor.ts b/packages/eslint-plugin/src/rules/no-useless-constructor.ts index 3f0b15c9efd..07bbc672d65 100644 --- a/packages/eslint-plugin/src/rules/no-useless-constructor.ts +++ b/packages/eslint-plugin/src/rules/no-useless-constructor.ts @@ -34,13 +34,10 @@ function checkAccessibility(node: TSESTree.MethodDefinition): boolean { * Check if method is not useless due to typescript parameter properties and decorators */ function checkParams(node: TSESTree.MethodDefinition): boolean { - return ( - !node.value.params || - !node.value.params.some( - param => - param.type === AST_NODE_TYPES.TSParameterProperty || - param.decorators?.length, - ) + return !node.value.params.some( + param => + param.type === AST_NODE_TYPES.TSParameterProperty || + param.decorators?.length, ); } diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 654624faaf7..aeb4ff9146d 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -34,7 +34,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, @@ -110,6 +110,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', @@ -154,99 +229,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 as ValidChainTarget, + 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( @@ -393,6 +445,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/src/rules/require-await.ts b/packages/eslint-plugin/src/rules/require-await.ts index 56067fe554a..f01ece89afe 100644 --- a/packages/eslint-plugin/src/rules/require-await.ts +++ b/packages/eslint-plugin/src/rules/require-await.ts @@ -104,7 +104,7 @@ export default util.createRule({ * function and the delegate is `true` */ function markAsHasDelegateGen(node: TSESTree.YieldExpression): void { - if (!scopeInfo || !scopeInfo.isGen || !node.argument) { + if (!scopeInfo?.isGen || !node.argument) { return; } diff --git a/packages/eslint-plugin/src/rules/return-await.ts b/packages/eslint-plugin/src/rules/return-await.ts index fbb78708bd4..bd16a9e4f53 100644 --- a/packages/eslint-plugin/src/rules/return-await.ts +++ b/packages/eslint-plugin/src/rules/return-await.ts @@ -306,7 +306,7 @@ export default util.createRule({ }, ReturnStatement(node): void { const scopeInfo = scopeInfoStack[scopeInfoStack.length - 1]; - if (!scopeInfo || !scopeInfo.hasAsync || !node.argument) { + if (!scopeInfo?.hasAsync || !node.argument) { return; } findPossiblyReturnedNodes(node.argument).forEach(node => { 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 289e2c83fa5..99c8803f3fd 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts @@ -64,6 +64,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()', @@ -122,6 +132,11 @@ 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 => ({ @@ -146,6 +161,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;', @@ -162,6 +185,7 @@ ruleTester.run('prefer-optional-chain', rule, { 'foo ?? {};', '(foo ?? {})?.bar;', 'foo ||= bar ?? {};', + 'foo && bar;', 'foo && foo;', 'foo || bar;', @@ -179,6 +203,9 @@ 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, @@ -446,6 +473,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?.();', @@ -1143,5 +1186,151 @@ 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;', + // }, + // ], + // }, + // ], + // }, ], }); diff --git a/packages/eslint-plugin/tests/rules/return-await.test.ts b/packages/eslint-plugin/tests/rules/return-await.test.ts index fd568272fe9..6a19ba3c223 100644 --- a/packages/eslint-plugin/tests/rules/return-await.test.ts +++ b/packages/eslint-plugin/tests/rules/return-await.test.ts @@ -15,6 +15,7 @@ const ruleTester = new RuleTester({ // default rule is in-try-catch ruleTester.run('return-await', rule, { valid: [ + 'return;', // No function in scope, so behave like return in a commonjs module ` function test() { return; diff --git a/packages/typescript-estree/src/convert.ts b/packages/typescript-estree/src/convert.ts index be09a2b387d..81c17185743 100644 --- a/packages/typescript-estree/src/convert.ts +++ b/packages/typescript-estree/src/convert.ts @@ -402,7 +402,7 @@ export class Converter { private convertParameters( parameters: ts.NodeArray, ): TSESTree.Parameter[] { - if (!parameters || !parameters.length) { + if (!parameters?.length) { return []; } return parameters.map(param => { @@ -688,7 +688,7 @@ export class Converter { result: TSESTree.TSEnumDeclaration | TSESTree.TSModuleDeclaration, modifiers?: ts.ModifiersArray, ): void { - if (!modifiers || !modifiers.length) { + if (!modifiers?.length) { return; } const remainingModifiers: TSESTree.Modifier[] = []; diff --git a/packages/utils/src/eslint-utils/getParserServices.ts b/packages/utils/src/eslint-utils/getParserServices.ts index 27ad579272b..f385b64fecb 100644 --- a/packages/utils/src/eslint-utils/getParserServices.ts +++ b/packages/utils/src/eslint-utils/getParserServices.ts @@ -17,8 +17,7 @@ function getParserServices< // backwards compatibility check // old versions of the parser would not return any parserServices unless parserOptions.project was set if ( - !context.parserServices || - !context.parserServices.program || + !context.parserServices?.program || !context.parserServices.esTreeNodeToTSNodeMap || !context.parserServices.tsNodeToESTreeNodeMap ) { diff --git a/packages/website/src/components/RulesTable/index.tsx b/packages/website/src/components/RulesTable/index.tsx index 1e9e2524601..08a55cda240 100644 --- a/packages/website/src/components/RulesTable/index.tsx +++ b/packages/website/src/components/RulesTable/index.tsx @@ -15,7 +15,7 @@ function interpolateCode(text: string): (JSX.Element | string)[] | string { } function RuleRow({ rule }: { rule: RulesMeta[number] }): JSX.Element | null { - if (!rule.docs || !rule.docs.url) { + if (!rule.docs?.url) { return null; } const { fixable, hasSuggestions } = rule;