From 612875bced1cd5a197a97c58dc9a89bf9e59cd8a Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Mon, 18 Mar 2024 21:55:39 +0900 Subject: [PATCH] fix(eslint-plugin): [explicit-function-return-type, explicit-module-boundary-types] improved checking for allowHigherOrderFunctions option (#8508) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(eslint-plugin): [explicit-function-return-type] improved checking for allowHigherOrderFunctions option * refactor * fix lint errors * apply reviews * fix lint error * Update packages/eslint-plugin/src/rules/explicit-function-return-type.ts --------- Co-authored-by: Josh Goldberg ✨ --- .../rules/explicit-function-return-type.ts | 101 +++++++++++------ .../rules/explicit-module-boundary-types.ts | 103 +++++++++++++----- .../src/util/explicitReturnTypeUtils.ts | 57 +++++----- .../explicit-function-return-type.test.ts | 74 +++++++++++++ .../explicit-module-boundary-types.test.ts | 43 ++++++++ 5 files changed, 288 insertions(+), 90 deletions(-) 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 d135f4c1a0d..9904dc2e533 100644 --- a/packages/eslint-plugin/src/rules/explicit-function-return-type.ts +++ b/packages/eslint-plugin/src/rules/explicit-function-return-type.ts @@ -1,7 +1,8 @@ import type { TSESTree } from '@typescript-eslint/utils'; import { AST_NODE_TYPES } from '@typescript-eslint/utils'; -import { createRule } from '../util'; +import { createRule, nullThrows } from '../util'; +import type { FunctionInfo } from '../util/explicitReturnTypeUtils'; import { ancestorHasReturnType, checkFunctionReturnType, @@ -22,6 +23,11 @@ type Options = [ ]; type MessageIds = 'missingReturnType'; +type FunctionNode = + | TSESTree.ArrowFunctionExpression + | TSESTree.FunctionDeclaration + | TSESTree.FunctionExpression; + export default createRule({ name: 'explicit-function-return-type', meta: { @@ -98,6 +104,22 @@ export default createRule({ }, ], create(context, [options]) { + const functionInfoStack: FunctionInfo[] = []; + + function enterFunction(node: FunctionNode): void { + functionInfoStack.push({ + node, + returns: [], + }); + } + + function popFunctionInfo(exitNodeType: string): FunctionInfo { + return nullThrows( + functionInfoStack.pop(), + `Stack should exist on ${exitNodeType} exit`, + ); + } + function isAllowedFunction( node: | TSESTree.ArrowFunctionExpression @@ -168,41 +190,49 @@ export default createRule({ return node.parent.type === AST_NODE_TYPES.CallExpression; } - return { - 'ArrowFunctionExpression, FunctionExpression'( - node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, - ): void { - if ( - options.allowConciseArrowFunctionExpressionsStartingWithVoid && - node.type === AST_NODE_TYPES.ArrowFunctionExpression && - node.expression && - node.body.type === AST_NODE_TYPES.UnaryExpression && - node.body.operator === 'void' - ) { - return; - } + function exitFunctionExpression( + node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, + ): void { + const info = popFunctionInfo('function expression'); - if (isAllowedFunction(node)) { - return; - } + if ( + options.allowConciseArrowFunctionExpressionsStartingWithVoid && + node.type === AST_NODE_TYPES.ArrowFunctionExpression && + node.expression && + node.body.type === AST_NODE_TYPES.UnaryExpression && + node.body.operator === 'void' + ) { + return; + } - if ( - options.allowTypedFunctionExpressions && - (isValidFunctionExpressionReturnType(node, options) || - ancestorHasReturnType(node)) - ) { - return; - } + if (isAllowedFunction(node)) { + return; + } - checkFunctionReturnType(node, options, context.sourceCode, loc => - context.report({ - node, - loc, - messageId: 'missingReturnType', - }), - ); - }, - FunctionDeclaration(node): void { + if ( + options.allowTypedFunctionExpressions && + (isValidFunctionExpressionReturnType(node, options) || + ancestorHasReturnType(node)) + ) { + return; + } + + checkFunctionReturnType(info, options, context.sourceCode, loc => + context.report({ + node, + loc, + messageId: 'missingReturnType', + }), + ); + } + + return { + 'ArrowFunctionExpression, FunctionExpression, FunctionDeclaration': + enterFunction, + 'ArrowFunctionExpression:exit': exitFunctionExpression, + 'FunctionExpression:exit': exitFunctionExpression, + 'FunctionDeclaration:exit'(node): void { + const info = popFunctionInfo('function declaration'); if (isAllowedFunction(node)) { return; } @@ -210,7 +240,7 @@ export default createRule({ return; } - checkFunctionReturnType(node, options, context.sourceCode, loc => + checkFunctionReturnType(info, options, context.sourceCode, loc => context.report({ node, loc, @@ -218,6 +248,9 @@ export default createRule({ }), ); }, + ReturnStatement(node): void { + functionInfoStack.at(-1)?.returns.push(node); + }, }; }, }); diff --git a/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts b/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts index f04a71de8b6..c8981e40394 100644 --- a/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts +++ b/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts @@ -5,6 +5,7 @@ import { AST_NODE_TYPES } from '@typescript-eslint/utils'; import { createRule, isFunction } from '../util'; import type { FunctionExpression, + FunctionInfo, FunctionNode, } from '../util/explicitReturnTypeUtils'; import { @@ -101,13 +102,31 @@ export default createRule({ // tracks all of the functions we've already checked const checkedFunctions = new Set(); - // tracks functions that were found whilst traversing - const foundFunctions: FunctionNode[] = []; + const functionStack: FunctionNode[] = []; + const functionReturnsMap = new Map< + FunctionNode, + TSESTree.ReturnStatement[] + >(); // all nodes visited, avoids infinite recursion for cyclic references // (such as class member referring to itself) const alreadyVisited = new Set(); + function getReturnsInFunction( + node: FunctionNode, + ): TSESTree.ReturnStatement[] { + return functionReturnsMap.get(node) ?? []; + } + + function enterFunction(node: FunctionNode): void { + functionStack.push(node); + functionReturnsMap.set(node, []); + } + + function exitFunction(): void { + functionStack.pop(); + } + /* # How the rule works: @@ -120,10 +139,10 @@ export default createRule({ */ return { - ExportDefaultDeclaration(node): void { + 'ExportDefaultDeclaration:exit'(node): void { checkNode(node.declaration); }, - 'ExportNamedDeclaration:not([source])'( + 'ExportNamedDeclaration:not([source]):exit'( node: TSESTree.ExportNamedDeclaration, ): void { if (node.declaration) { @@ -134,21 +153,25 @@ export default createRule({ } } }, - TSExportAssignment(node): void { + 'TSExportAssignment:exit'(node): void { checkNode(node.expression); }, - 'ArrowFunctionExpression, FunctionDeclaration, FunctionExpression'( - node: FunctionNode, - ): void { - foundFunctions.push(node); - }, + 'ArrowFunctionExpression, FunctionDeclaration, FunctionExpression': + enterFunction, + 'ArrowFunctionExpression:exit': exitFunction, + 'FunctionDeclaration:exit': exitFunction, + 'FunctionExpression:exit': exitFunction, 'Program:exit'(): void { - for (const func of foundFunctions) { - if (isExportedHigherOrderFunction(func)) { - checkNode(func); + for (const [node, returns] of functionReturnsMap) { + if (isExportedHigherOrderFunction({ node, returns })) { + checkNode(node); } } }, + ReturnStatement(node): void { + const current = functionStack[functionStack.length - 1]; + functionReturnsMap.get(current)?.push(node); + }, }; function checkParameters( @@ -265,7 +288,9 @@ export default createRule({ return false; } - function isExportedHigherOrderFunction(node: FunctionNode): boolean { + function isExportedHigherOrderFunction({ + node, + }: FunctionInfo): boolean { let current: TSESTree.Node | undefined = node.parent; while (current) { if (current.type === AST_NODE_TYPES.ReturnStatement) { @@ -274,9 +299,12 @@ export default createRule({ continue; } + if (!isFunction(current)) { + return false; + } + const returns = getReturnsInFunction(current); if ( - !isFunction(current) || - !doesImmediatelyReturnFunctionExpression(current) + !doesImmediatelyReturnFunctionExpression({ node: current, returns }) ) { return false; } @@ -335,8 +363,10 @@ export default createRule({ switch (node.type) { case AST_NODE_TYPES.ArrowFunctionExpression: - case AST_NODE_TYPES.FunctionExpression: - return checkFunctionExpression(node); + case AST_NODE_TYPES.FunctionExpression: { + const returns = getReturnsInFunction(node); + return checkFunctionExpression({ node, returns }); + } case AST_NODE_TYPES.ArrayExpression: for (const element of node.elements) { @@ -360,8 +390,10 @@ export default createRule({ } return; - case AST_NODE_TYPES.FunctionDeclaration: - return checkFunction(node); + case AST_NODE_TYPES.FunctionDeclaration: { + const returns = getReturnsInFunction(node); + return checkFunction({ node, returns }); + } case AST_NODE_TYPES.MethodDefinition: case AST_NODE_TYPES.TSAbstractMethodDefinition: @@ -419,7 +451,10 @@ export default createRule({ checkParameters(node); } - function checkFunctionExpression(node: FunctionExpression): void { + function checkFunctionExpression({ + node, + returns, + }: FunctionInfo): void { if (checkedFunctions.has(node)) { return; } @@ -434,7 +469,7 @@ export default createRule({ } checkFunctionExpressionReturnType( - node, + { node, returns }, options, context.sourceCode, loc => { @@ -449,7 +484,10 @@ export default createRule({ checkParameters(node); } - function checkFunction(node: TSESTree.FunctionDeclaration): void { + function checkFunction({ + node, + returns, + }: FunctionInfo): void { if (checkedFunctions.has(node)) { return; } @@ -459,13 +497,18 @@ export default createRule({ return; } - checkFunctionReturnType(node, options, context.sourceCode, loc => { - context.report({ - node, - loc, - messageId: 'missingReturnType', - }); - }); + checkFunctionReturnType( + { node, returns }, + options, + context.sourceCode, + loc => { + context.report({ + node, + loc, + messageId: 'missingReturnType', + }); + }, + ); checkParameters(node); } diff --git a/packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts b/packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts index 512ff62202b..1cfc834c1c1 100644 --- a/packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts +++ b/packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts @@ -1,5 +1,9 @@ import type { TSESLint, TSESTree } from '@typescript-eslint/utils'; -import { AST_NODE_TYPES, ESLintUtils } from '@typescript-eslint/utils'; +import { + AST_NODE_TYPES, + ASTUtils, + ESLintUtils, +} from '@typescript-eslint/utils'; import { isConstructor, isSetter, isTypeAssertion } from './astUtils'; import { getFunctionHeadLoc } from './getFunctionHeadLoc'; @@ -9,6 +13,11 @@ type FunctionExpression = | TSESTree.FunctionExpression; type FunctionNode = FunctionExpression | TSESTree.FunctionDeclaration; +export interface FunctionInfo { + node: T; + returns: TSESTree.ReturnStatement[]; +} + /** * Checks if a node is a variable declarator with a type annotation. * ``` @@ -134,26 +143,22 @@ function isPropertyOfObjectWithType( * ``` */ function doesImmediatelyReturnFunctionExpression({ - body, -}: FunctionNode): boolean { - // Check if body is a block with a single statement - if (body.type === AST_NODE_TYPES.BlockStatement && body.body.length === 1) { - const [statement] = body.body; - - // Check if that statement is a return statement with an argument - if ( - statement.type === AST_NODE_TYPES.ReturnStatement && - !!statement.argument - ) { - // If so, check that returned argument as body - body = statement.argument; - } + node, + returns, +}: FunctionInfo): boolean { + if ( + node.type === AST_NODE_TYPES.ArrowFunctionExpression && + ASTUtils.isFunction(node.body) + ) { + return true; } - // Check if the body being returned is a function expression - return ( - body.type === AST_NODE_TYPES.ArrowFunctionExpression || - body.type === AST_NODE_TYPES.FunctionExpression + if (returns.length === 0) { + return false; + } + + return returns.every( + node => node.argument && ASTUtils.isFunction(node.argument), ); } @@ -251,12 +256,12 @@ function isValidFunctionExpressionReturnType( * Check that the function expression or declaration is valid. */ function isValidFunctionReturnType( - node: FunctionNode, + { node, returns }: FunctionInfo, options: Options, ): boolean { if ( options.allowHigherOrderFunctions && - doesImmediatelyReturnFunctionExpression(node) + doesImmediatelyReturnFunctionExpression({ node, returns }) ) { return true; } @@ -272,12 +277,12 @@ function isValidFunctionReturnType( * Checks if a function declaration/expression has a return type. */ function checkFunctionReturnType( - node: FunctionNode, + { node, returns }: FunctionInfo, options: Options, sourceCode: TSESLint.SourceCode, report: (loc: TSESTree.SourceLocation) => void, ): void { - if (isValidFunctionReturnType(node, options)) { + if (isValidFunctionReturnType({ node, returns }, options)) { return; } @@ -288,16 +293,16 @@ function checkFunctionReturnType( * Checks if a function declaration/expression has a return type. */ function checkFunctionExpressionReturnType( - node: FunctionExpression, + info: FunctionInfo, options: Options, sourceCode: TSESLint.SourceCode, report: (loc: TSESTree.SourceLocation) => void, ): void { - if (isValidFunctionExpressionReturnType(node, options)) { + if (isValidFunctionExpressionReturnType(info.node, options)) { return; } - checkFunctionReturnType(node, options, sourceCode, report); + checkFunctionReturnType(info, options, sourceCode, report); } /** diff --git a/packages/eslint-plugin/tests/rules/explicit-function-return-type.test.ts b/packages/eslint-plugin/tests/rules/explicit-function-return-type.test.ts index 8b0a05d6b5b..2036927e0ca 100644 --- a/packages/eslint-plugin/tests/rules/explicit-function-return-type.test.ts +++ b/packages/eslint-plugin/tests/rules/explicit-function-return-type.test.ts @@ -8,6 +8,7 @@ const ruleTester = new RuleTester({ ruleTester.run('explicit-function-return-type', rule, { valid: [ + 'return;', { code: ` function test(): void { @@ -267,6 +268,17 @@ const myObj = { }, { code: ` +() => { + const foo = 'foo'; + return function (): string { + return foo; + }; +}; + `, + options: [{ allowHigherOrderFunctions: true }], + }, + { + code: ` function fn() { return (): void => {}; } @@ -276,6 +288,31 @@ function fn() { { code: ` function fn() { + return function (): void {}; +} + `, + options: [{ allowHigherOrderFunctions: true }], + }, + { + code: ` +function fn() { + const bar = () => (): number => 1; + return function (): void {}; +} + `, + options: [{ allowHigherOrderFunctions: true }], + }, + { + code: ` +function fn(arg: boolean) { + if (arg) { + return () => (): number => 1; + } else { + return function (): string { + return 'foo'; + }; + } + return function (): void {}; } `, @@ -1230,6 +1267,43 @@ function fn() { }, { code: ` +function fn() { + const bar = () => (): number => 1; + const baz = () => () => 'baz'; + return function (): void {}; +} + `, + options: [{ allowHigherOrderFunctions: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 4, + endLine: 4, + column: 24, + endColumn: 26, + }, + ], + }, + { + code: ` +function fn(arg: boolean) { + if (arg) return 'string'; + return function (): void {}; +} + `, + options: [{ allowHigherOrderFunctions: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 2, + endLine: 2, + column: 1, + endColumn: 12, + }, + ], + }, + { + code: ` function FunctionDeclaration() { return function FunctionExpression_Within_FunctionDeclaration() { return function FunctionExpression_Within_FunctionExpression() { diff --git a/packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts b/packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts index 8a945ceca63..b96cb31d74e 100644 --- a/packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts +++ b/packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts @@ -316,6 +316,28 @@ export default () => () => { }, { code: ` +export default () => () => { + const foo = 'foo'; + return (): void => { + return; + }; +}; + `, + options: [{ allowHigherOrderFunctions: true }], + }, + { + code: ` +export default () => () => { + const foo = () => (): string => 'foo'; + return (): void => { + return; + }; +}; + `, + options: [{ allowHigherOrderFunctions: true }], + }, + { + code: ` export class Accumulator { private count: number = 0; @@ -1728,6 +1750,27 @@ export function foo(outer) { }, ], }, + { + code: ` +export function foo(outer: boolean) { + if (outer) { + return 'string'; + } + return function (inner): void {}; +} + `, + options: [{ allowHigherOrderFunctions: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 2, + column: 8, + data: { + name: 'inner', + }, + }, + ], + }, // test a few different argument patterns { code: `