From 392c51a4ed37e1272ba836c0435633ae980630a1 Mon Sep 17 00:00:00 2001 From: Sosuke Suzuki Date: Sat, 21 Mar 2020 23:56:39 +0900 Subject: [PATCH 1/3] feat(eslint-plugin): [explicit-module-boundary-types] add new option --- .../rules/explicit-module-boundary-types.md | 27 ++ .../rules/explicit-module-boundary-types.ts | 222 +++++++++-- .../explicit-module-boundary-types.test.ts | 347 ++++++++++++++++++ 3 files changed, 565 insertions(+), 31 deletions(-) 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 a5327a1e547..72db897b900 100644 --- a/packages/eslint-plugin/docs/rules/explicit-module-boundary-types.md +++ b/packages/eslint-plugin/docs/rules/explicit-module-boundary-types.md @@ -86,12 +86,17 @@ type Options = { * An array of function/method names that will not have their arguments or their return values checked. */ allowedNames?: string[]; + /** + * If true, track references to exported variables as well as direct exports. + */ + shouldTrackReferences?: boolean; }; const defaults = { allowTypedFunctionExpressions: true, allowHigherOrderFunctions: true, allowedNames: [], + shouldTrackReferences: false, }; ``` @@ -238,6 +243,28 @@ You may pass function/method names you would like this rule to ignore, like so: } ``` +### `shouldTrackReferences` + +Examples of **incorrect** code for this rule with `{ shouldTrackReferences: true }`: + +```ts +function foo(bar) { + return bar; +} + +export default foo; +``` + +Examples of **correct** code for this rule with `{ shouldTrackReferences: true }`: + +```ts +function foo(bar: string): string { + return bar; +} + +export default foo; +``` + ## When Not To Use It If you wish to make sure all functions have explicit return types, as opposed to only the module boundaries, you can use [explicit-function-return-type](https://github.com/eslint/eslint/blob/master/docs/rules/explicit-function-return-type.md) 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 55c6bd29f99..44915bca553 100644 --- a/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts +++ b/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts @@ -1,6 +1,7 @@ import { TSESTree, AST_NODE_TYPES, + TSESLint, } from '@typescript-eslint/experimental-utils'; import * as util from '../util'; import { @@ -15,6 +16,7 @@ type Options = [ allowHigherOrderFunctions?: boolean; allowDirectConstAssertionInArrowFunctions?: boolean; allowedNames?: string[]; + shouldTrackReferences?: boolean; }, ]; type MessageIds = 'missingReturnType' | 'missingArgType'; @@ -52,6 +54,9 @@ export default util.createRule({ type: 'string', }, }, + shouldTrackReferences: { + type: 'boolean', + }, }, additionalProperties: false, }, @@ -63,6 +68,7 @@ export default util.createRule({ allowHigherOrderFunctions: true, allowDirectConstAssertionInArrowFunctions: true, allowedNames: [], + shouldTrackReferences: false, }, ], create(context, [options]) { @@ -171,50 +177,204 @@ 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, + ); + + return writeExprs; + } + + /** + * Finds a function node referred by a variable passed from parameters + */ + function findFunctionInScope( + varibale: TSESLint.Scope.Variable, + ): TSESTree.FunctionDeclaration | undefined { + if (varibale.defs[0].type !== 'FunctionName') { + return; + } + + const functionNode = varibale.defs[0].node; + + if ( + functionNode && + functionNode.type !== AST_NODE_TYPES.FunctionDeclaration + ) { + return; + } + + return functionNode; + } + + /** + * Checks if a function referred by the identifier passed from parameters follow the rule + */ + function checkWithTrackingReferences(node: TSESTree.Identifier): void { + if (!options.shouldTrackReferences) { + return; + } + + const scope = context.getScope(); + const variable = scope.set.get(node.name); + + if (!variable) { + 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); + } + + if ( + classElement.type === AST_NODE_TYPES.ClassProperty && + (classElement.value?.type === AST_NODE_TYPES.FunctionExpression || + classElement.value?.type === + AST_NODE_TYPES.ArrowFunctionExpression) + ) { + checkFunctionExpression(classElement.value); + } + } + } + + const functionNode = findFunctionInScope(variable); + if (functionNode) { + checkFunction(functionNode); + } + + const functionExpressions = findFunctionExpressionsInScope(variable); + if (functionExpressions && functionExpressions.length > 0) { + for (const functionExpression of functionExpressions) { + checkFunctionExpression(functionExpression); + } + } + } + + /** + * Checks if a function expression follow the rule + */ + function checkFunctionExpression( + node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, + ): 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 + return; + } + + if ( + isAllowedName(node.parent) || + isTypedFunctionExpression(node, options) + ) { + return; + } + + checkFunctionExpressionReturnType(node, options, sourceCode, loc => + context.report({ + node, + loc, + messageId: 'missingReturnType', + }), + ); + + checkArguments(node); + } + + /** + * Checks if a function follow the rule + */ + function checkFunction(node: TSESTree.FunctionDeclaration): void { + if (isAllowedName(node.parent)) { + return; + } + + checkFunctionReturnType(node, options, sourceCode, loc => + context.report({ + node, + loc, + messageId: 'missingReturnType', + }), + ); + + checkArguments(node); + } + return { 'ArrowFunctionExpression, FunctionExpression'( node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, ): 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 (isUnexported(node)) { return; } - if ( - isAllowedName(node.parent) || - isUnexported(node) || - isTypedFunctionExpression(node, options) - ) { - return; - } - - checkFunctionExpressionReturnType(node, options, sourceCode, loc => - context.report({ - node, - loc, - messageId: 'missingReturnType', - }), - ); - - checkArguments(node); + checkFunctionExpression(node); }, FunctionDeclaration(node): void { - if (isAllowedName(node.parent) || isUnexported(node)) { + if (isUnexported(node)) { return; } - checkFunctionReturnType(node, options, sourceCode, loc => - context.report({ - node, - loc, - messageId: 'missingReturnType', - }), - ); + checkFunction(node); + }, + 'ExportDefaultDeclaration, TSExportAssignment'( + node: TSESTree.ExportDefaultDeclaration | TSESTree.TSExportAssignment, + ): void { + let exported: TSESTree.Node; + + if (node.type === AST_NODE_TYPES.ExportDefaultDeclaration) { + exported = node.declaration; + } else { + exported = node.expression; + } - checkArguments(node); + 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/tests/rules/explicit-module-boundary-types.test.ts b/packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts index 388adb99431..d6275d8bb02 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 @@ -341,6 +341,95 @@ export const Foo: JSX.Element = ecmaFeatures: { jsx: true }, }, }, + { + code: ` +const test = (): void => { + return; +} +export default test; + `, + options: [{ shouldTrackReferences: true }], + }, + { + code: ` +function test(): void { + return; +} +export default test; + `, + options: [{ shouldTrackReferences: true }], + }, + { + code: ` +const test = (): void => { + return; +} +export default [ test ]; + `, + options: [{ shouldTrackReferences: true }], + }, + { + code: ` +function test(): void { + return; +} +export default [ test ]; + `, + options: [{ shouldTrackReferences: true }], + }, + { + code: ` +const test = (): void => { + return; +} +export default { test }; + `, + options: [{ shouldTrackReferences: true }], + }, + { + code: ` +function test(): void { + return; +} +export default { test }; + `, + options: [{ shouldTrackReferences: true }], + }, + { + code: ` +const foo = (arg => arg) as Foo; +export default foo; + `, + options: [{ shouldTrackReferences: true }], + }, + { + code: ` +let foo = (arg => arg) as Foo; +foo = 3; +export default foo; + `, + options: [{ shouldTrackReferences: true }], + }, + { + code: ` +class Foo { + bar = (arg: string): string => arg; +} +export default { Foo } + `, + options: [{ shouldTrackReferences: true }], + }, + { + code: ` +class Foo { + bar(): void { + return; + } +} +export default { Foo } + `, + options: [{ shouldTrackReferences: true }], + }, ], invalid: [ { @@ -889,5 +978,263 @@ export const func2 = (value: number) => value; }, ], }, + { + code: ` +const foo = arg => arg; +export default foo; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 2, + }, + { + messageId: 'missingArgType', + line: 2, + }, + ], + }, + { + code: ` +const foo = arg => arg; +export = foo; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 2, + }, + { + messageId: 'missingArgType', + line: 2, + }, + ], + }, + { + code: ` +let foo = (arg: number): number => arg; +foo = arg => arg; +export default foo; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 3, + }, + { + messageId: 'missingArgType', + line: 3, + }, + ], + }, + { + code: ` +const foo = arg => arg; +export default [ foo ]; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 2, + }, + { + messageId: 'missingArgType', + line: 2, + }, + ], + }, + { + code: ` +const foo = arg => arg; +export default { foo }; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 2, + }, + { + messageId: 'missingArgType', + line: 2, + }, + ], + }, + { + code: ` +function foo(arg) { return arg; } +export default foo; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 2, + }, + { + messageId: 'missingArgType', + line: 2, + }, + ], + }, + { + code: ` +function foo(arg) { return arg; } +export default [ foo ]; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 2, + }, + { + messageId: 'missingArgType', + line: 2, + }, + ], + }, + { + code: ` +function foo(arg) { return arg; } +export default { foo }; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 2, + }, + { + messageId: 'missingArgType', + line: 2, + }, + ], + }, + { + code: ` +const bar = function foo(arg) { return arg; } +export default { bar }; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 2, + }, + { + messageId: 'missingArgType', + line: 2, + }, + ], + }, + { + code: ` +class Foo { + bool(arg) { + return arg; + } +} +export default Foo; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 3, + }, + { + messageId: 'missingArgType', + line: 3, + }, + ], + }, + { + code: ` +class Foo { + bool = (arg) => { + return arg; + } +} +export default Foo; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 3, + }, + { + messageId: 'missingArgType', + line: 3, + }, + ], + }, + { + code: ` +class Foo { + bool = function(arg) { + return arg; + } +} +export default Foo; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 3, + }, + { + messageId: 'missingArgType', + line: 3, + }, + ], + }, + { + code: ` +class Foo { + bool = function(arg) { + return arg; + } +} +export default [ Foo ]; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 3, + }, + { + messageId: 'missingArgType', + line: 3, + }, + ], + }, + { + code: ` +let test = arg => argl +test = (): void => { + return; +}; +export default test; + `, + options: [{ shouldTrackReferences: true }], + errors: [ + { + messageId: 'missingReturnType', + line: 2, + }, + { + messageId: 'missingArgType', + line: 2, + }, + ], + }, ], }); From 90627f106f9c5e6f64f10f1f83d1874c696cf0a9 Mon Sep 17 00:00:00 2001 From: Sosuke Suzuki Date: Mon, 13 Apr 2020 05:58:27 +0900 Subject: [PATCH 2/3] fix(eslint-plugin): [explicit-module-boundary-types] fix reviewed points --- .../rules/explicit-module-boundary-types.ts | 19 ++-- .../explicit-module-boundary-types.test.ts | 102 ++++++++++-------- 2 files changed, 63 insertions(+), 58 deletions(-) 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 44915bca553..0e062d25a21 100644 --- a/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts +++ b/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts @@ -204,18 +204,15 @@ export default util.createRule({ * Finds a function node referred by a variable passed from parameters */ function findFunctionInScope( - varibale: TSESLint.Scope.Variable, + variable: TSESLint.Scope.Variable, ): TSESTree.FunctionDeclaration | undefined { - if (varibale.defs[0].type !== 'FunctionName') { + if (variable.defs[0].type !== 'FunctionName') { return; } - const functionNode = varibale.defs[0].node; + const functionNode = variable.defs[0].node; - if ( - functionNode && - functionNode.type !== AST_NODE_TYPES.FunctionDeclaration - ) { + if (functionNode?.type !== AST_NODE_TYPES.FunctionDeclaration) { return; } @@ -226,10 +223,6 @@ export default util.createRule({ * Checks if a function referred by the identifier passed from parameters follow the rule */ function checkWithTrackingReferences(node: TSESTree.Identifier): void { - if (!options.shouldTrackReferences) { - return; - } - const scope = context.getScope(); const variable = scope.set.get(node.name); @@ -342,6 +335,10 @@ export default util.createRule({ 'ExportDefaultDeclaration, TSExportAssignment'( node: TSESTree.ExportDefaultDeclaration | TSESTree.TSExportAssignment, ): void { + if (!options.shouldTrackReferences) { + return; + } + let exported: TSESTree.Node; if (node.type === AST_NODE_TYPES.ExportDefaultDeclaration) { 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 c1ff74d640c..5d63e8e1036 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 @@ -351,62 +351,62 @@ export const Foo: JSX.Element = ( { code: ` const test = (): void => { - return; -} + return; +}; export default test; - `, + `, options: [{ shouldTrackReferences: true }], }, { code: ` function test(): void { - return; + return; } export default test; - `, + `, options: [{ shouldTrackReferences: true }], }, { code: ` const test = (): void => { - return; -} -export default [ test ]; - `, + return; +}; +export default [test]; + `, options: [{ shouldTrackReferences: true }], }, { code: ` function test(): void { - return; + return; } -export default [ test ]; - `, +export default [test]; + `, options: [{ shouldTrackReferences: true }], }, { code: ` const test = (): void => { - return; -} + return; +}; export default { test }; - `, + `, options: [{ shouldTrackReferences: true }], }, { code: ` function test(): void { - return; + return; } export default { test }; - `, + `, options: [{ shouldTrackReferences: true }], }, { code: ` const foo = (arg => arg) as Foo; export default foo; - `, + `, options: [{ shouldTrackReferences: true }], }, { @@ -414,7 +414,7 @@ export default foo; let foo = (arg => arg) as Foo; foo = 3; export default foo; - `, + `, options: [{ shouldTrackReferences: true }], }, { @@ -422,7 +422,7 @@ export default foo; class Foo { bar = (arg: string): string => arg; } -export default { Foo } +export default { Foo }; `, options: [{ shouldTrackReferences: true }], }, @@ -433,7 +433,7 @@ class Foo { return; } } -export default { Foo } +export default { Foo }; `, options: [{ shouldTrackReferences: true }], }, @@ -1018,7 +1018,7 @@ export function fn(test): string { code: ` const foo = arg => arg; export default foo; - `, + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1035,7 +1035,7 @@ export default foo; code: ` const foo = arg => arg; export = foo; - `, + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1053,7 +1053,7 @@ export = foo; let foo = (arg: number): number => arg; foo = arg => arg; export default foo; - `, + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1069,8 +1069,8 @@ export default foo; { code: ` const foo = arg => arg; -export default [ foo ]; - `, +export default [foo]; + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1087,7 +1087,7 @@ export default [ foo ]; code: ` const foo = arg => arg; export default { foo }; - `, + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1102,9 +1102,11 @@ export default { foo }; }, { code: ` -function foo(arg) { return arg; } +function foo(arg) { + return arg; +} export default foo; - `, + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1119,9 +1121,11 @@ export default foo; }, { code: ` -function foo(arg) { return arg; } -export default [ foo ]; - `, +function foo(arg) { + return arg; +} +export default [foo]; + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1136,9 +1140,11 @@ export default [ foo ]; }, { code: ` -function foo(arg) { return arg; } +function foo(arg) { + return arg; +} export default { foo }; - `, + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1153,9 +1159,11 @@ export default { foo }; }, { code: ` -const bar = function foo(arg) { return arg; } +const bar = function foo(arg) { + return arg; +}; export default { bar }; - `, + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1176,7 +1184,7 @@ class Foo { } } export default Foo; - `, + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1192,12 +1200,12 @@ export default Foo; { code: ` class Foo { - bool = (arg) => { + bool = arg => { return arg; - } + }; } export default Foo; - `, + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1215,10 +1223,10 @@ export default Foo; class Foo { bool = function(arg) { return arg; - } + }; } export default Foo; - `, + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1236,10 +1244,10 @@ export default Foo; class Foo { bool = function(arg) { return arg; - } + }; } -export default [ Foo ]; - `, +export default [Foo]; + `, options: [{ shouldTrackReferences: true }], errors: [ { @@ -1254,12 +1262,12 @@ export default [ Foo ]; }, { code: ` -let test = arg => argl +let test = arg => argl; test = (): void => { return; }; export default test; - `, + `, options: [{ shouldTrackReferences: true }], errors: [ { From d21d52b43655a29bb1be1e96d4e9ffe3da3857e7 Mon Sep 17 00:00:00 2001 From: Sosuke Suzuki Date: Mon, 13 Apr 2020 08:18:55 +0900 Subject: [PATCH 3/3] fix(eslint-plugin): [explicit-module-boundary-types] fix default option --- .../eslint-plugin/docs/rules/explicit-module-boundary-types.md | 2 +- .../eslint-plugin/src/rules/explicit-module-boundary-types.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 72db897b900..3c077a75c88 100644 --- a/packages/eslint-plugin/docs/rules/explicit-module-boundary-types.md +++ b/packages/eslint-plugin/docs/rules/explicit-module-boundary-types.md @@ -96,7 +96,7 @@ const defaults = { allowTypedFunctionExpressions: true, allowHigherOrderFunctions: true, allowedNames: [], - shouldTrackReferences: false, + shouldTrackReferences: true, }; ``` 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 0e062d25a21..b00d8eb1087 100644 --- a/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts +++ b/packages/eslint-plugin/src/rules/explicit-module-boundary-types.ts @@ -68,7 +68,7 @@ export default util.createRule({ allowHigherOrderFunctions: true, allowDirectConstAssertionInArrowFunctions: true, allowedNames: [], - shouldTrackReferences: false, + shouldTrackReferences: true, }, ], create(context, [options]) {