From caaa8599284d02ab3341e282cad35a52d0fb86c7 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Sat, 30 May 2020 14:59:47 -0700 Subject: [PATCH] feat(eslint-plugin): [explicit-module-boundary-types] improve accuracy and coverage (#2135) --- .vscode/launch.json | 60 ++- .../rules/explicit-module-boundary-types.md | 181 +++---- .../rules/explicit-module-boundary-types.ts | 488 +++++++++-------- .../src/util/explicitReturnTypeUtils.ts | 34 +- .../explicit-module-boundary-types.test.ts | 500 ++++++++++++++---- .../experimental-utils/src/ts-eslint/Scope.ts | 7 +- 6 files changed, 826 insertions(+), 444 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index eee0937502c..951609b55e3 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -18,7 +18,17 @@ ], "sourceMaps": true, "console": "integratedTerminal", - "internalConsoleOptions": "neverOpen" + "internalConsoleOptions": "neverOpen", + "skipFiles": [ + "${workspaceFolder}/packages/experimental-utils/src/index.ts", + "${workspaceFolder}/packages/experimental-utils/dist/index.js", + "${workspaceFolder}/packages/experimental-utils/src/ts-estree.ts", + "${workspaceFolder}/packages/experimental-utils/dist/ts-estree.js", + "${workspaceFolder}/packages/parser/src/index.ts", + "${workspaceFolder}/packages/parser/dist/index.js", + "${workspaceFolder}/packages/typescript-estree/src/index.ts", + "${workspaceFolder}/packages/typescript-estree/dist/index.js", + ], }, { "type": "node", @@ -34,7 +44,17 @@ ], "sourceMaps": true, "console": "integratedTerminal", - "internalConsoleOptions": "neverOpen" + "internalConsoleOptions": "neverOpen", + "skipFiles": [ + "${workspaceFolder}/packages/experimental-utils/src/index.ts", + "${workspaceFolder}/packages/experimental-utils/dist/index.js", + "${workspaceFolder}/packages/experimental-utils/src/ts-estree.ts", + "${workspaceFolder}/packages/experimental-utils/dist/ts-estree.js", + "${workspaceFolder}/packages/parser/src/index.ts", + "${workspaceFolder}/packages/parser/dist/index.js", + "${workspaceFolder}/packages/typescript-estree/src/index.ts", + "${workspaceFolder}/packages/typescript-estree/dist/index.js", + ], }, { "type": "node", @@ -50,7 +70,17 @@ ], "sourceMaps": true, "console": "integratedTerminal", - "internalConsoleOptions": "neverOpen" + "internalConsoleOptions": "neverOpen", + "skipFiles": [ + "${workspaceFolder}/packages/experimental-utils/src/index.ts", + "${workspaceFolder}/packages/experimental-utils/dist/index.js", + "${workspaceFolder}/packages/experimental-utils/src/ts-estree.ts", + "${workspaceFolder}/packages/experimental-utils/dist/ts-estree.js", + "${workspaceFolder}/packages/parser/src/index.ts", + "${workspaceFolder}/packages/parser/dist/index.js", + "${workspaceFolder}/packages/typescript-estree/src/index.ts", + "${workspaceFolder}/packages/typescript-estree/dist/index.js", + ], }, { "type": "node", @@ -66,7 +96,17 @@ ], "sourceMaps": true, "console": "integratedTerminal", - "internalConsoleOptions": "neverOpen" + "internalConsoleOptions": "neverOpen", + "skipFiles": [ + "${workspaceFolder}/packages/experimental-utils/src/index.ts", + "${workspaceFolder}/packages/experimental-utils/dist/index.js", + "${workspaceFolder}/packages/experimental-utils/src/ts-estree.ts", + "${workspaceFolder}/packages/experimental-utils/dist/ts-estree.js", + "${workspaceFolder}/packages/parser/src/index.ts", + "${workspaceFolder}/packages/parser/dist/index.js", + "${workspaceFolder}/packages/typescript-estree/src/index.ts", + "${workspaceFolder}/packages/typescript-estree/dist/index.js", + ], }, { "type": "node", @@ -82,7 +122,17 @@ ], "sourceMaps": true, "console": "integratedTerminal", - "internalConsoleOptions": "neverOpen" + "internalConsoleOptions": "neverOpen", + "skipFiles": [ + "${workspaceFolder}/packages/experimental-utils/src/index.ts", + "${workspaceFolder}/packages/experimental-utils/dist/index.js", + "${workspaceFolder}/packages/experimental-utils/src/ts-estree.ts", + "${workspaceFolder}/packages/experimental-utils/dist/ts-estree.js", + "${workspaceFolder}/packages/parser/src/index.ts", + "${workspaceFolder}/packages/parser/dist/index.js", + "${workspaceFolder}/packages/typescript-estree/src/index.ts", + "${workspaceFolder}/packages/typescript-estree/dist/index.js", + ], } ] } diff --git a/packages/eslint-plugin/docs/rules/explicit-module-boundary-types.md b/packages/eslint-plugin/docs/rules/explicit-module-boundary-types.md index b91036ec643..1054fbcdeef 100644 --- a/packages/eslint-plugin/docs/rules/explicit-module-boundary-types.md +++ b/packages/eslint-plugin/docs/rules/explicit-module-boundary-types.md @@ -71,16 +71,9 @@ The rule accepts an options object with the following properties: ```ts type Options = { /** - * If true, type annotations are also allowed on the variable of a function expression - * rather than on the function arguments/return value directly. - */ - allowTypedFunctionExpressions?: boolean; - /** - * If true, functions immediately returning another function expression will not - * require an explicit return value annotation. - * You must still type the parameters of the function. + * If true, the rule will not report for arguments that are explicitly typed as `any` */ - allowHigherOrderFunctions?: boolean; + allowArgumentsExplicitlyTypedAsAny?: boolean; /** * If true, body-less arrow functions that return an `as const` type assertion will not * require an explicit return value annotation. @@ -92,16 +85,24 @@ type Options = { */ allowedNames?: string[]; /** - * If true, track references to exported variables as well as direct exports. + * If true, functions immediately returning another function expression will not + * require an explicit return value annotation. + * You must still type the parameters of the function. + */ + allowHigherOrderFunctions?: boolean; + /** + * If true, type annotations are also allowed on the variable of a function expression + * rather than on the function arguments/return value directly. */ - shouldTrackReferences?: boolean; + allowTypedFunctionExpressions?: boolean; }; const defaults = { - allowTypedFunctionExpressions: true, - allowHigherOrderFunctions: true, + allowArgumentsExplicitlyTypedAsAny: false, + allowDirectConstAssertionInArrowFunctions: true, allowedNames: [], - shouldTrackReferences: true, + allowHigherOrderFunctions: true, + allowTypedFunctionExpressions: true, }; ``` @@ -127,83 +128,20 @@ If you are working on a codebase within which you lint non-TypeScript code (i.e. } ``` -### `allowTypedFunctionExpressions` - -Examples of **incorrect** code for this rule with `{ allowTypedFunctionExpressions: true }`: - -```ts -export let arrowFn = () => 'test'; - -export let funcExpr = function () { - return 'test'; -}; - -export let objectProp = { - foo: () => 1, -}; - -export const foo = bar => {}; -``` +### `allowArgumentsExplicitlyTypedAsAny` -Examples of additional **correct** code for this rule with `{ allowTypedFunctionExpressions: true }`: +Examples of **incorrect** code for this rule with `{ allowArgumentsExplicitlyTypedAsAny: true }`: ```ts -type FuncType = () => string; - -export let arrowFn: FuncType = () => 'test'; - -export let funcExpr: FuncType = function () { - return 'test'; -}; - -export let asTyped = (() => '') as () => string; -export let castTyped = <() => string>(() => ''); - -interface ObjectType { - foo(): number; -} -export let objectProp: ObjectType = { - foo: () => 1, -}; -export let objectPropAs = { - foo: () => 1, -} as ObjectType; -export let objectPropCast = { - foo: () => 1, -}; - -type FooType = (bar: string) => void; -export const foo: FooType = bar => {}; -``` - -### `allowHigherOrderFunctions` - -Examples of **incorrect** code for this rule with `{ allowHigherOrderFunctions: true }`: - -```ts -export var arrowFn = () => () => {}; - -export function fn() { - return function () {}; -} - -export function foo(outer) { - return function (inner): void {}; -} +export const func = (value: any): void => ({ type: 'X', value }); +export function foo(value: any): void {} ``` -Examples of **correct** code for this rule with `{ allowHigherOrderFunctions: true }`: +Examples of **correct** code for this rule with `{ allowArgumentsExplicitlyTypedAsAny: true }`: ```ts -export var arrowFn = () => (): void => {}; - -export function fn() { - return function (): void {}; -} - -export function foo(outer: string) { - return function (inner: string): void {}; -} +export const func = (value: number): void => ({ type: 'X', value }); +export function foo(value: number): void {} ``` ### `allowDirectConstAssertionInArrowFunctions` @@ -248,26 +186,83 @@ You may pass function/method names you would like this rule to ignore, like so: } ``` -### `shouldTrackReferences` +### `allowHigherOrderFunctions` -Examples of **incorrect** code for this rule with `{ shouldTrackReferences: true }`: +Examples of **incorrect** code for this rule with `{ allowHigherOrderFunctions: true }`: ```ts -function foo(bar) { - return bar; +export var arrowFn = () => () => {}; + +export function fn() { + return function () {}; } -export default foo; +export function foo(outer) { + return function (inner): void {}; +} ``` -Examples of **correct** code for this rule with `{ shouldTrackReferences: true }`: +Examples of **correct** code for this rule with `{ allowHigherOrderFunctions: true }`: ```ts -function foo(bar: string): string { - return bar; +export var arrowFn = () => (): void => {}; + +export function fn() { + return function (): void {}; +} + +export function foo(outer: string) { + return function (inner: string): void {}; } +``` + +### `allowTypedFunctionExpressions` + +Examples of **incorrect** code for this rule with `{ allowTypedFunctionExpressions: true }`: -export default foo; +```ts +export let arrowFn = () => 'test'; + +export let funcExpr = function () { + return 'test'; +}; + +export let objectProp = { + foo: () => 1, +}; + +export const foo = bar => {}; +``` + +Examples of additional **correct** code for this rule with `{ allowTypedFunctionExpressions: true }`: + +```ts +type FuncType = () => string; + +export let arrowFn: FuncType = () => 'test'; + +export let funcExpr: FuncType = function () { + return 'test'; +}; + +export let asTyped = (() => '') as () => string; +export let castTyped = <() => string>(() => ''); + +interface ObjectType { + foo(): number; +} +export let objectProp: ObjectType = { + foo: () => 1, +}; +export let objectPropAs = { + foo: () => 1, +} as ObjectType; +export let objectPropCast = { + foo: () => 1, +}; + +type FooType = (bar: string) => void; +export const foo: FooType = bar => {}; ``` ## When Not To Use It 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 63f94d1d1a4..3d5cafcb2e5 100644 --- a/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts +++ b/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts @@ -1,26 +1,34 @@ import { TSESTree, AST_NODE_TYPES, - TSESLint, } from '@typescript-eslint/experimental-utils'; import * as util from '../util'; import { + ancestorHasReturnType, checkFunctionExpressionReturnType, checkFunctionReturnType, + doesImmediatelyReturnFunctionExpression, + FunctionExpression, + FunctionNode, isTypedFunctionExpression, - ancestorHasReturnType, } from '../util/explicitReturnTypeUtils'; type Options = [ { - allowTypedFunctionExpressions?: boolean; - allowHigherOrderFunctions?: boolean; + allowArgumentsExplicitlyTypedAsAny?: boolean; allowDirectConstAssertionInArrowFunctions?: boolean; allowedNames?: string[]; + allowHigherOrderFunctions?: boolean; + allowTypedFunctionExpressions?: boolean; shouldTrackReferences?: boolean; }, ]; -type MessageIds = 'missingReturnType' | 'missingArgType'; +type MessageIds = + | 'missingReturnType' + | 'missingArgType' + | 'missingArgTypeUnnamed' + | 'anyTypedArg' + | 'anyTypedArgUnnamed'; export default util.createRule({ name: 'explicit-module-boundary-types', @@ -35,15 +43,16 @@ export default util.createRule({ messages: { missingReturnType: 'Missing return type on function.', missingArgType: "Argument '{{name}}' should be typed.", + missingArgTypeUnnamed: '{{type}} argument should be typed.', + anyTypedArg: "Argument '{{name}}' should be typed with a non-any type.", + anyTypedArgUnnamed: + '{{type}} argument should be typed with a non-any type.', }, schema: [ { type: 'object', properties: { - allowTypedFunctionExpressions: { - type: 'boolean', - }, - allowHigherOrderFunctions: { + allowArgumentsExplicitlyTypedAsAny: { type: 'boolean', }, allowDirectConstAssertionInArrowFunctions: { @@ -55,6 +64,13 @@ export default util.createRule({ type: 'string', }, }, + allowHigherOrderFunctions: { + type: 'boolean', + }, + allowTypedFunctionExpressions: { + type: 'boolean', + }, + // DEPRECATED - To be removed in next major shouldTrackReferences: { type: 'boolean', }, @@ -65,87 +81,136 @@ export default util.createRule({ }, defaultOptions: [ { - allowTypedFunctionExpressions: true, - allowHigherOrderFunctions: true, + allowArgumentsExplicitlyTypedAsAny: false, allowDirectConstAssertionInArrowFunctions: true, allowedNames: [], - shouldTrackReferences: true, + allowHigherOrderFunctions: true, + allowTypedFunctionExpressions: true, }, ], create(context, [options]) { const sourceCode = context.getSourceCode(); - /** - * Steps up the nodes parents to check if any ancestor nodes are export - * declarations. - */ - function isUnexported(node: TSESTree.Node | undefined): boolean { - let isReturnedValue = false; - while (node) { - if ( - node.type === AST_NODE_TYPES.ExportDefaultDeclaration || - node.type === AST_NODE_TYPES.ExportNamedDeclaration || - node.type === AST_NODE_TYPES.ExportSpecifier - ) { - return false; - } + // tracks all of the functions we've already checked + const checkedFunctions = new Set(); - if (node.type === AST_NODE_TYPES.JSXExpressionContainer) { - return true; - } + // tracks functions that were found whilst traversing + const foundFunctions: FunctionNode[] = []; + + /* + # How the rule works: + + As the rule traverses the AST, it immediately checks every single function that it finds is exported. + "exported" means that it is either directly exported, or that its name is exported. - if (node.type === AST_NODE_TYPES.ReturnStatement) { - isReturnedValue = true; + It also collects a list of every single function it finds on the way, but does not check them. + After it's finished traversing the AST, it then iterates through the list of found functions, and checks to see if + any of them are part of a higher-order function + */ + + return { + ExportDefaultDeclaration(node): void { + checkNode(node.declaration); + }, + 'ExportNamedDeclaration:not([source])'( + node: TSESTree.ExportNamedDeclaration, + ): void { + if (node.declaration) { + checkNode(node.declaration); + } else { + for (const specifier of node.specifiers) { + followReference(specifier.local); + } } + }, + TSExportAssignment(node): void { + checkNode(node.expression); + }, - if ( - node.type === AST_NODE_TYPES.ArrowFunctionExpression || - node.type === AST_NODE_TYPES.FunctionDeclaration || - node.type === AST_NODE_TYPES.FunctionExpression - ) { - isReturnedValue = false; + 'ArrowFunctionExpression, FunctionDeclaration, FunctionExpression'( + node: FunctionNode, + ): void { + foundFunctions.push(node); + }, + 'Program:exit'(): void { + for (const func of foundFunctions) { + if (isExportedHigherOrderFunction(func)) { + checkNode(func); + } } + }, + }; - if (node.type === AST_NODE_TYPES.BlockStatement && !isReturnedValue) { - return true; + function checkParameters( + node: TSESTree.TSEmptyBodyFunctionExpression | FunctionNode, + ): void { + function checkParameter(param: TSESTree.Parameter): void { + function report( + namedMessageId: MessageIds, + unnamedMessageId: MessageIds, + ): void { + if (param.type === AST_NODE_TYPES.Identifier) { + context.report({ + node: param, + messageId: namedMessageId, + data: { name: param.name }, + }); + } else if (param.type === AST_NODE_TYPES.ArrayPattern) { + context.report({ + node: param, + messageId: unnamedMessageId, + data: { type: 'Array pattern' }, + }); + } else if (param.type === AST_NODE_TYPES.ObjectPattern) { + context.report({ + node: param, + messageId: unnamedMessageId, + data: { type: 'Object pattern' }, + }); + } else if (param.type === AST_NODE_TYPES.RestElement) { + if (param.argument.type === AST_NODE_TYPES.Identifier) { + context.report({ + node: param, + messageId: namedMessageId, + data: { name: param.argument.name }, + }); + } else { + context.report({ + node: param, + messageId: unnamedMessageId, + data: { type: 'Rest' }, + }); + } + } } - node = node.parent; - } + switch (param.type) { + case AST_NODE_TYPES.ArrayPattern: + case AST_NODE_TYPES.Identifier: + case AST_NODE_TYPES.ObjectPattern: + case AST_NODE_TYPES.RestElement: + if (!param.typeAnnotation) { + report('missingArgType', 'missingArgTypeUnnamed'); + } else if ( + options.allowArgumentsExplicitlyTypedAsAny !== true && + param.typeAnnotation.typeAnnotation.type === + AST_NODE_TYPES.TSAnyKeyword + ) { + report('anyTypedArg', 'anyTypedArgUnnamed'); + } + return; - return true; - } + case AST_NODE_TYPES.TSParameterProperty: + return checkParameter(param.parameter); - /** - * Returns true when the argument node is not typed. - */ - function isArgumentUntyped(node: TSESTree.Identifier): boolean { - return ( - !node.typeAnnotation || - node.typeAnnotation.typeAnnotation.type === AST_NODE_TYPES.TSAnyKeyword - ); - } + case AST_NODE_TYPES.AssignmentPattern: // ignored as it has a type via its assignment + return; + } + } - /** - * Checks if a function declaration/expression has a return type. - */ - function checkArguments( - node: - | TSESTree.ArrowFunctionExpression - | TSESTree.FunctionDeclaration - | TSESTree.FunctionExpression, - ): void { - const paramIdentifiers = node.params.filter(util.isIdentifier); - const untypedArgs = paramIdentifiers.filter(isArgumentUntyped); - untypedArgs.forEach(untypedArg => - context.report({ - node, - messageId: 'missingArgType', - data: { - name: untypedArg.name, - }, - }), - ); + for (const arg of node.params) { + checkParameter(arg); + } } /** @@ -185,106 +250,151 @@ export default util.createRule({ return false; } - /** - * Finds an array of a function expression node referred by a variable passed from parameters - */ - function findFunctionExpressionsInScope( - variable: TSESLint.Scope.Variable, - ): - | (TSESTree.FunctionExpression | TSESTree.ArrowFunctionExpression)[] - | undefined { - const writeExprs = variable.references - .map(ref => ref.writeExpr) - .filter( - ( - expr, - ): expr is - | TSESTree.ArrowFunctionExpression - | TSESTree.FunctionExpression => - expr?.type === AST_NODE_TYPES.FunctionExpression || - expr?.type === AST_NODE_TYPES.ArrowFunctionExpression, - ); + function isExportedHigherOrderFunction(node: FunctionNode): boolean { + let current = node.parent; + while (current) { + if (current.type === AST_NODE_TYPES.ReturnStatement) { + // the parent of a return will always be a block statement, so we can skip over it + current = current.parent?.parent; + continue; + } + + if ( + !util.isFunction(current) || + !doesImmediatelyReturnFunctionExpression(current) + ) { + return false; + } + + if (checkedFunctions.has(current)) { + return true; + } + + current = current.parent; + } - return writeExprs; + return false; } - /** - * Finds a function node referred by a variable passed from parameters - */ - function findFunctionInScope( - variable: TSESLint.Scope.Variable, - ): TSESTree.FunctionDeclaration | undefined { - if (variable.defs[0].type !== 'FunctionName') { + function followReference(node: TSESTree.Identifier): void { + const scope = context.getScope(); + const variable = scope.set.get(node.name); + /* istanbul ignore if */ if (!variable) { return; } - const functionNode = variable.defs[0].node; + // check all of the definitions + for (const definition of variable.defs) { + // cases we don't care about in this rule + if ( + definition.type === 'ImplicitGlobalVariable' || + definition.type === 'ImportBinding' || + // eslint-disable-next-line @typescript-eslint/internal/prefer-ast-types-enum + definition.type === 'CatchClause' || + definition.type === 'Parameter' + ) { + continue; + } - if (functionNode?.type !== AST_NODE_TYPES.FunctionDeclaration) { - return; + checkNode(definition.node); } - return functionNode; + // follow references to find writes to the variable + for (const reference of variable.references) { + if ( + // we don't want to check the initialization ref, as this is handled by the declaration check + !reference.init && + reference.writeExpr + ) { + checkNode(reference.writeExpr); + } + } } - /** - * Checks if a function referred by the identifier passed from parameters follow the rule - */ - function checkWithTrackingReferences(node: TSESTree.Identifier): void { - const scope = context.getScope(); - const variable = scope.set.get(node.name); - - if (!variable) { + function checkNode(node: TSESTree.Node | null): void { + if (node == null) { return; } - if (variable.defs[0].type === 'ClassName') { - const classNode = variable.defs[0].node; - for (const classElement of classNode.body.body) { - if ( - classElement.type === AST_NODE_TYPES.MethodDefinition && - classElement.value.type === AST_NODE_TYPES.FunctionExpression - ) { - checkFunctionExpression(classElement.value); + switch (node.type) { + case AST_NODE_TYPES.ArrowFunctionExpression: + case AST_NODE_TYPES.FunctionExpression: + return checkFunctionExpression(node); + + case AST_NODE_TYPES.ArrayExpression: + for (const element of node.elements) { + checkNode(element); } + return; - if ( - classElement.type === AST_NODE_TYPES.ClassProperty && - (classElement.value?.type === AST_NODE_TYPES.FunctionExpression || - classElement.value?.type === - AST_NODE_TYPES.ArrowFunctionExpression) - ) { - checkFunctionExpression(classElement.value); + case AST_NODE_TYPES.ClassProperty: + case AST_NODE_TYPES.TSAbstractClassProperty: + if (node.accessibility === 'private') { + return; } - } - } + return checkNode(node.value); - const functionNode = findFunctionInScope(variable); - if (functionNode) { - checkFunction(functionNode); - } + case AST_NODE_TYPES.ClassDeclaration: + case AST_NODE_TYPES.ClassExpression: + for (const element of node.body.body) { + checkNode(element); + } + return; - const functionExpressions = findFunctionExpressionsInScope(variable); - if (functionExpressions && functionExpressions.length > 0) { - for (const functionExpression of functionExpressions) { - checkFunctionExpression(functionExpression); - } + case AST_NODE_TYPES.FunctionDeclaration: + return checkFunction(node); + + case AST_NODE_TYPES.MethodDefinition: + case AST_NODE_TYPES.TSAbstractMethodDefinition: + if (node.accessibility === 'private') { + return; + } + return checkNode(node.value); + + case AST_NODE_TYPES.Identifier: + return followReference(node); + + case AST_NODE_TYPES.ObjectExpression: + for (const property of node.properties) { + checkNode(property); + } + return; + + case AST_NODE_TYPES.Property: + return checkNode(node.value); + + case AST_NODE_TYPES.TSEmptyBodyFunctionExpression: + return checkEmptyBodyFunctionExpression(node); + + case AST_NODE_TYPES.VariableDeclaration: + for (const declaration of node.declarations) { + checkNode(declaration); + } + return; + + case AST_NODE_TYPES.VariableDeclarator: + return checkNode(node.init); } } - /** - * Checks if a function expression follow the rule. - */ - function checkFunctionExpression( - node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, + function checkEmptyBodyFunctionExpression( + node: TSESTree.TSEmptyBodyFunctionExpression, ): void { - if ( - node.parent?.type === AST_NODE_TYPES.MethodDefinition && - node.parent.accessibility === 'private' - ) { - // don't check private methods as they aren't part of the public signature + if (!node.returnType) { + context.report({ + node, + messageId: 'missingReturnType', + }); + } + + checkParameters(node); + } + + function checkFunctionExpression(node: FunctionExpression): void { + if (checkedFunctions.has(node)) { return; } + checkedFunctions.add(node); if ( isAllowedName(node.parent) || @@ -294,21 +404,23 @@ export default util.createRule({ return; } - checkFunctionExpressionReturnType(node, options, sourceCode, loc => + checkFunctionExpressionReturnType(node, options, sourceCode, loc => { context.report({ node, loc, messageId: 'missingReturnType', - }), - ); + }); + }); - checkArguments(node); + checkParameters(node); } - /** - * Checks if a function follow the rule - */ function checkFunction(node: TSESTree.FunctionDeclaration): void { + if (checkedFunctions.has(node)) { + return; + } + checkedFunctions.add(node); + if ( isAllowedName(node.parent) || ancestorHasReturnType(node.parent, options) @@ -316,75 +428,15 @@ export default util.createRule({ return; } - checkFunctionReturnType(node, options, sourceCode, loc => + checkFunctionReturnType(node, options, sourceCode, loc => { context.report({ node, loc, messageId: 'missingReturnType', - }), - ); + }); + }); - checkArguments(node); + checkParameters(node); } - - return { - 'ArrowFunctionExpression, FunctionExpression'( - node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, - ): void { - if (isUnexported(node)) { - return; - } - - checkFunctionExpression(node); - }, - FunctionDeclaration(node): void { - if (isUnexported(node)) { - return; - } - - checkFunction(node); - }, - 'ExportDefaultDeclaration, TSExportAssignment'( - node: TSESTree.ExportDefaultDeclaration | TSESTree.TSExportAssignment, - ): void { - if (!options.shouldTrackReferences) { - return; - } - - let exported: TSESTree.Node; - - if (node.type === AST_NODE_TYPES.ExportDefaultDeclaration) { - exported = node.declaration; - } else { - exported = node.expression; - } - - switch (exported.type) { - case AST_NODE_TYPES.Identifier: { - checkWithTrackingReferences(exported); - break; - } - case AST_NODE_TYPES.ArrayExpression: { - for (const element of exported.elements) { - if (element.type === AST_NODE_TYPES.Identifier) { - checkWithTrackingReferences(element); - } - } - break; - } - case AST_NODE_TYPES.ObjectExpression: { - for (const property of exported.properties) { - if ( - property.type === AST_NODE_TYPES.Property && - property.value.type === AST_NODE_TYPES.Identifier - ) { - checkWithTrackingReferences(property.value); - } - } - break; - } - } - }, - }; }, }); diff --git a/packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts b/packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts index 220cd081c0a..473a9d1549f 100644 --- a/packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts +++ b/packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts @@ -7,10 +7,10 @@ import { import { isTypeAssertion, isConstructor, isSetter } from './astUtils'; import { nullThrows, NullThrowsReasons } from './nullThrows'; -type FunctionNode = +type FunctionExpression = | TSESTree.ArrowFunctionExpression - | TSESTree.FunctionDeclaration | TSESTree.FunctionExpression; +type FunctionNode = FunctionExpression | TSESTree.FunctionDeclaration; /** * Creates a report location for the given function. @@ -158,10 +158,7 @@ function isPropertyOfObjectWithType( */ function doesImmediatelyReturnFunctionExpression({ body, -}: - | TSESTree.ArrowFunctionExpression - | TSESTree.FunctionDeclaration - | TSESTree.FunctionExpression): boolean { +}: FunctionNode): boolean { // Should always have a body; really checking just in case /* istanbul ignore if */ if (!body) { return false; @@ -196,7 +193,7 @@ function doesImmediatelyReturnFunctionExpression({ */ function isFunctionArgument( parent: TSESTree.Node, - callee?: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, + callee?: FunctionExpression, ): parent is TSESTree.CallExpression | TSESTree.OptionalCallExpression { return ( (parent.type === AST_NODE_TYPES.CallExpression || @@ -243,7 +240,7 @@ interface Options { * True when the provided function expression is typed. */ function isTypedFunctionExpression( - node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, + node: FunctionExpression, options: Options, ): boolean { const parent = nullThrows(node.parent, NullThrowsReasons.MissingParent); @@ -267,7 +264,7 @@ function isTypedFunctionExpression( * with the provided options. */ function isValidFunctionExpressionReturnType( - node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, + node: FunctionExpression, options: Options, ): boolean { if (isTypedFunctionExpression(node, options)) { @@ -301,10 +298,7 @@ function isValidFunctionExpressionReturnType( * Check that the function expression or declaration is valid. */ function isValidFunctionReturnType( - node: - | TSESTree.ArrowFunctionExpression - | TSESTree.FunctionDeclaration - | TSESTree.FunctionExpression, + node: FunctionNode, options: Options, isParentCheck = false, ): boolean { @@ -327,10 +321,7 @@ function isValidFunctionReturnType( * Checks if a function declaration/expression has a return type. */ function checkFunctionReturnType( - node: - | TSESTree.ArrowFunctionExpression - | TSESTree.FunctionDeclaration - | TSESTree.FunctionExpression, + node: FunctionNode, options: Options, sourceCode: TSESLint.SourceCode, report: (loc: TSESTree.SourceLocation) => void, @@ -346,7 +337,7 @@ function checkFunctionReturnType( * Checks if a function declaration/expression has a return type. */ function checkFunctionExpressionReturnType( - node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, + node: FunctionExpression, options: Options, sourceCode: TSESLint.SourceCode, report: (loc: TSESTree.SourceLocation) => void, @@ -395,8 +386,11 @@ function ancestorHasReturnType( } export { - checkFunctionReturnType, + ancestorHasReturnType, checkFunctionExpressionReturnType, + checkFunctionReturnType, + doesImmediatelyReturnFunctionExpression, + FunctionExpression, + FunctionNode, isTypedFunctionExpression, - ancestorHasReturnType, }; 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 313fe533e72..3f619f1819d 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 @@ -34,32 +34,51 @@ export var arrowFn = (): string => 'test'; `, }, { + // not exported code: ` class Test { - constructor() {} - get prop() { + constructor(one) {} + get prop(one) { return 1; } - set prop() {} - method() { + set prop(one) {} + method(one) { return; } - arrow = (): string => 'arrow'; + arrow = one => 'arrow'; + abstract abs(one); } `, }, { code: ` export class Test { - constructor() {} - get prop(): number { + constructor(one: string) {} + get prop(one: string): void { return 1; } - set prop() {} + set prop(one: string): void {} + method(one: string): void { + return; + } + arrow = (one: string): string => 'arrow'; + abstract abs(one: string): void; +} + `, + }, + { + code: ` +export class Test { + private constructor(one) {} + private get prop(one) { + return 1; + } + private set prop(one) {} private method(one) { return; } - arrow = (): string => 'arrow'; + private arrow = one => 'arrow'; + private abstract abs(one); } `, }, @@ -284,7 +303,6 @@ export const func2 = (value: number) => ({ type: 'X', value }); { code: ` export class Test { - constructor() {} get prop() { return 1; } @@ -292,7 +310,11 @@ export class Test { method() { return; } - arrow = (): string => 'arrow'; + // prettier-ignore + 'method'() {} + ['prop']() {} + [\`prop\`]() {} + [\`\${v}\`](): void {} } `, options: [ @@ -355,7 +377,6 @@ const test = (): void => { }; export default test; `, - options: [{ shouldTrackReferences: true }], }, { code: ` @@ -364,7 +385,6 @@ function test(): void { } export default test; `, - options: [{ shouldTrackReferences: true }], }, { code: ` @@ -373,7 +393,6 @@ const test = (): void => { }; export default [test]; `, - options: [{ shouldTrackReferences: true }], }, { code: ` @@ -382,7 +401,6 @@ function test(): void { } export default [test]; `, - options: [{ shouldTrackReferences: true }], }, { code: ` @@ -391,7 +409,6 @@ const test = (): void => { }; export default { test }; `, - options: [{ shouldTrackReferences: true }], }, { code: ` @@ -400,14 +417,12 @@ function test(): void { } export default { test }; `, - options: [{ shouldTrackReferences: true }], }, { code: ` const foo = (arg => arg) as Foo; export default foo; `, - options: [{ shouldTrackReferences: true }], }, { code: ` @@ -415,7 +430,6 @@ let foo = (arg => arg) as Foo; foo = 3; export default foo; `, - options: [{ shouldTrackReferences: true }], }, { code: ` @@ -424,7 +438,6 @@ class Foo { } export default { Foo }; `, - options: [{ shouldTrackReferences: true }], }, { code: ` @@ -435,7 +448,6 @@ class Foo { } export default { Foo }; `, - options: [{ shouldTrackReferences: true }], }, { code: ` @@ -520,6 +532,73 @@ export function foo(outer: string) { } `, }, + // shouldn't check functions that aren't directly exported - https://github.com/typescript-eslint/typescript-eslint/issues/2134 + ` +export function foo(): unknown { + return new Proxy(apiInstance, { + get: (target, property) => { + // implementation + }, + }); +} + `, + { + code: 'export default (() => true)();', + options: [ + { + allowTypedFunctionExpressions: false, + }, + ], + }, + // explicit assertions are allowed + { + code: 'export const x = (() => {}) as Foo;', + options: [{ allowTypedFunctionExpressions: false }], + }, + { + code: ` +interface Foo {} +export const x = { + foo: () => {}, +} as Foo; + `, + options: [{ allowTypedFunctionExpressions: false }], + }, + // allowArgumentsExplicitlyTypedAsAny + { + code: ` +export function foo(foo: any): void {} + `, + options: [{ allowArgumentsExplicitlyTypedAsAny: true }], + }, + { + code: ` +export function foo({ foo }: any): void {} + `, + options: [{ allowArgumentsExplicitlyTypedAsAny: true }], + }, + { + code: ` +export function foo([bar]: any): void {} + `, + options: [{ allowArgumentsExplicitlyTypedAsAny: true }], + }, + { + code: ` +export function foo(...bar: any): void {} + `, + options: [{ allowArgumentsExplicitlyTypedAsAny: true }], + }, + { + code: ` +export function foo(...[a]: any): void {} + `, + options: [{ allowArgumentsExplicitlyTypedAsAny: true }], + }, + // assignment patterns are ignored + ` +export function foo(arg = 1): void {} + `, ], invalid: [ { @@ -599,6 +678,7 @@ export class Test { private method() { return; } + abstract abs(arg); } `, errors: [ @@ -613,8 +693,11 @@ export class Test { messageId: 'missingArgType', line: 7, endLine: 7, - column: 11, - endColumn: 21, + column: 12, + endColumn: 17, + data: { + name: 'value', + }, }, { messageId: 'missingReturnType', @@ -635,7 +718,27 @@ export class Test { line: 11, endLine: 11, column: 11, - endColumn: 25, + endColumn: 14, + data: { + name: 'arg', + }, + }, + { + messageId: 'missingReturnType', + line: 15, + column: 15, + endLine: 15, + endColumn: 21, + }, + { + messageId: 'missingArgType', + line: 15, + column: 16, + endLine: 15, + endColumn: 19, + data: { + name: 'arg', + }, }, ], }, @@ -698,13 +801,6 @@ export class Foo { column: 16, endColumn: 21, }, - { - messageId: 'missingReturnType', - line: 1, - endLine: 1, - column: 30, - endColumn: 35, - }, ], }, { @@ -737,37 +833,6 @@ export var funcExpr = function () { }, ], }, - { - code: 'export const x = (() => {}) as Foo;', - options: [{ allowTypedFunctionExpressions: false }], - errors: [ - { - messageId: 'missingReturnType', - line: 1, - endLine: 1, - column: 19, - endColumn: 24, - }, - ], - }, - { - code: ` -interface Foo {} -export const x = { - foo: () => {}, -} as Foo; - `, - options: [{ allowTypedFunctionExpressions: false }], - errors: [ - { - messageId: 'missingReturnType', - line: 4, - endLine: 4, - column: 8, - endColumn: 13, - }, - ], - }, { code: ` interface Foo {} @@ -925,23 +990,6 @@ export default () => () => { }, ], }, - { - code: 'export default (() => true)();', - options: [ - { - allowTypedFunctionExpressions: false, - }, - ], - errors: [ - { - messageId: 'missingReturnType', - line: 1, - endLine: 1, - column: 17, - endColumn: 22, - }, - ], - }, { code: ` export const func1 = (value: number) => ({ type: 'X', value } as any); @@ -1019,6 +1067,31 @@ export class Test { }, { code: ` +export class Test { + constructor(public foo, private ...bar) {} +} + `, + errors: [ + { + messageId: 'missingArgType', + line: 3, + column: 22, + data: { + name: 'foo', + }, + }, + { + messageId: 'missingArgType', + line: 3, + column: 27, + data: { + name: 'bar', + }, + }, + ], + }, + { + code: ` export const func1 = (value: number) => value; export const func2 = (value: number) => value; `, @@ -1047,35 +1120,46 @@ export function fn(test): string { { messageId: 'missingArgType', line: 2, - endLine: 4, - column: 8, - endColumn: 2, + endLine: 2, + column: 20, + endColumn: 24, + data: { + name: 'test', + }, }, ], }, { - code: "export const fn = (one: number, two): string => '123';", + code: ` +export const fn = (one: number, two): string => '123'; + `, errors: [ { messageId: 'missingArgType', - line: 1, - endLine: 1, - column: 19, - endColumn: 54, + line: 2, + endLine: 2, + column: 33, + endColumn: 36, + data: { + name: 'two', + }, }, ], }, { code: ` - export function foo(outer) { - return function (inner) {}; - } +export function foo(outer) { + return function (inner) {}; +} `, options: [{ allowHigherOrderFunctions: true }], errors: [ { messageId: 'missingArgType', line: 2, + data: { + name: 'outer', + }, }, { messageId: 'missingReturnType', @@ -1084,6 +1168,9 @@ export function fn(test): string { { messageId: 'missingArgType', line: 3, + data: { + name: 'inner', + }, }, ], }, @@ -1094,6 +1181,9 @@ export function fn(test): string { { messageId: 'missingArgType', line: 1, + data: { + name: 'arg', + }, }, ], }, @@ -1102,7 +1192,6 @@ export function fn(test): string { const foo = arg => arg; export default foo; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1111,6 +1200,9 @@ export default foo; { messageId: 'missingArgType', line: 2, + data: { + name: 'arg', + }, }, ], }, @@ -1119,7 +1211,6 @@ export default foo; const foo = arg => arg; export = foo; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1128,6 +1219,9 @@ export = foo; { messageId: 'missingArgType', line: 2, + data: { + name: 'arg', + }, }, ], }, @@ -1137,7 +1231,6 @@ let foo = (arg: number): number => arg; foo = arg => arg; export default foo; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1146,6 +1239,9 @@ export default foo; { messageId: 'missingArgType', line: 3, + data: { + name: 'arg', + }, }, ], }, @@ -1154,7 +1250,6 @@ export default foo; const foo = arg => arg; export default [foo]; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1163,6 +1258,9 @@ export default [foo]; { messageId: 'missingArgType', line: 2, + data: { + name: 'arg', + }, }, ], }, @@ -1171,7 +1269,6 @@ export default [foo]; const foo = arg => arg; export default { foo }; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1180,6 +1277,9 @@ export default { foo }; { messageId: 'missingArgType', line: 2, + data: { + name: 'arg', + }, }, ], }, @@ -1190,7 +1290,6 @@ function foo(arg) { } export default foo; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1199,6 +1298,9 @@ export default foo; { messageId: 'missingArgType', line: 2, + data: { + name: 'arg', + }, }, ], }, @@ -1209,7 +1311,6 @@ function foo(arg) { } export default [foo]; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1218,6 +1319,9 @@ export default [foo]; { messageId: 'missingArgType', line: 2, + data: { + name: 'arg', + }, }, ], }, @@ -1228,7 +1332,6 @@ function foo(arg) { } export default { foo }; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1237,6 +1340,9 @@ export default { foo }; { messageId: 'missingArgType', line: 2, + data: { + name: 'arg', + }, }, ], }, @@ -1247,7 +1353,6 @@ const bar = function foo(arg) { }; export default { bar }; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1256,6 +1361,9 @@ export default { bar }; { messageId: 'missingArgType', line: 2, + data: { + name: 'arg', + }, }, ], }, @@ -1268,7 +1376,6 @@ class Foo { } export default Foo; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1277,6 +1384,9 @@ export default Foo; { messageId: 'missingArgType', line: 3, + data: { + name: 'arg', + }, }, ], }, @@ -1289,7 +1399,6 @@ class Foo { } export default Foo; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1298,6 +1407,9 @@ export default Foo; { messageId: 'missingArgType', line: 3, + data: { + name: 'arg', + }, }, ], }, @@ -1310,7 +1422,6 @@ class Foo { } export default Foo; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1319,6 +1430,9 @@ export default Foo; { messageId: 'missingArgType', line: 3, + data: { + name: 'arg', + }, }, ], }, @@ -1331,7 +1445,6 @@ class Foo { } export default [Foo]; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1340,6 +1453,9 @@ export default [Foo]; { messageId: 'missingArgType', line: 3, + data: { + name: 'arg', + }, }, ], }, @@ -1351,7 +1467,6 @@ test = (): void => { }; export default test; `, - options: [{ shouldTrackReferences: true }], errors: [ { messageId: 'missingReturnType', @@ -1360,6 +1475,31 @@ export default test; { messageId: 'missingArgType', line: 2, + data: { + name: 'arg', + }, + }, + ], + }, + { + code: ` +let test = arg => argl; +test = (): void => { + return; +}; +export { test }; + `, + errors: [ + { + messageId: 'missingReturnType', + line: 2, + }, + { + messageId: 'missingArgType', + line: 2, + data: { + name: 'arg', + }, }, ], }, @@ -1419,12 +1559,160 @@ export function foo(outer) { { messageId: 'missingArgType', line: 2, - column: 8, + column: 21, + data: { + name: 'outer', + }, }, { messageId: 'missingArgType', line: 3, - column: 10, + column: 20, + data: { + name: 'inner', + }, + }, + ], + }, + // test a few different argument patterns + { + code: ` +export function foo({ foo }): void {} + `, + errors: [ + { + messageId: 'missingArgTypeUnnamed', + line: 2, + column: 21, + data: { + type: 'Object pattern', + }, + }, + ], + }, + { + code: ` +export function foo([bar]): void {} + `, + errors: [ + { + messageId: 'missingArgTypeUnnamed', + line: 2, + column: 21, + data: { + type: 'Array pattern', + }, + }, + ], + }, + { + code: ` +export function foo(...bar): void {} + `, + errors: [ + { + messageId: 'missingArgType', + line: 2, + column: 21, + data: { + name: 'bar', + }, + }, + ], + }, + { + code: ` +export function foo(...[a]): void {} + `, + errors: [ + { + messageId: 'missingArgTypeUnnamed', + line: 2, + column: 21, + data: { + type: 'Rest', + }, + }, + ], + }, + // allowArgumentsExplicitlyTypedAsAny + { + code: ` +export function foo(foo: any): void {} + `, + options: [{ allowArgumentsExplicitlyTypedAsAny: false }], + errors: [ + { + messageId: 'anyTypedArg', + line: 2, + column: 21, + data: { + name: 'foo', + }, + }, + ], + }, + { + code: ` +export function foo({ foo }: any): void {} + `, + options: [{ allowArgumentsExplicitlyTypedAsAny: false }], + errors: [ + { + messageId: 'anyTypedArgUnnamed', + line: 2, + column: 21, + data: { + type: 'Object pattern', + }, + }, + ], + }, + { + code: ` +export function foo([bar]: any): void {} + `, + options: [{ allowArgumentsExplicitlyTypedAsAny: false }], + errors: [ + { + messageId: 'anyTypedArgUnnamed', + line: 2, + column: 21, + data: { + type: 'Array pattern', + }, + }, + ], + }, + { + code: ` +export function foo(...bar: any): void {} + `, + options: [{ allowArgumentsExplicitlyTypedAsAny: false }], + errors: [ + { + messageId: 'anyTypedArg', + line: 2, + column: 21, + data: { + name: 'bar', + }, + }, + ], + }, + { + code: ` +export function foo(...[a]: any): void {} + `, + options: [{ allowArgumentsExplicitlyTypedAsAny: false }], + errors: [ + { + messageId: 'anyTypedArgUnnamed', + line: 2, + column: 21, + data: { + type: 'Rest', + }, }, ], }, diff --git a/packages/experimental-utils/src/ts-eslint/Scope.ts b/packages/experimental-utils/src/ts-eslint/Scope.ts index bb5e5accc51..9dcae37f060 100644 --- a/packages/experimental-utils/src/ts-eslint/Scope.ts +++ b/packages/experimental-utils/src/ts-eslint/Scope.ts @@ -81,7 +81,11 @@ namespace Scope { node: TSESTree.FunctionDeclaration | TSESTree.FunctionExpression; parent: null; } - | { type: 'ImplicitGlobalVariable'; node: TSESTree.Program; parent: null } + | { + type: 'ImplicitGlobalVariable'; + node: TSESTree.Program; + parent: null; + } | { type: 'ImportBinding'; node: @@ -98,7 +102,6 @@ namespace Scope { | TSESTree.ArrowFunctionExpression; parent: null; } - | { type: 'TDZ'; node: unknown; parent: null } | { type: 'Variable'; node: TSESTree.VariableDeclarator;