From 8295b94bf7aeace65c362d4a28e5850feadd7446 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Wed, 8 May 2019 10:03:31 -0700 Subject: [PATCH 1/3] fix(eslint-plugin): [explicit-function-return-type] Add handling for class properties --- .../rules/explicit-function-return-type.md | 42 +++++-- .../rules/explicit-function-return-type.ts | 107 +++++++++++------- .../explicit-function-return-type.test.ts | 26 +++++ 3 files changed, 123 insertions(+), 52 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/explicit-function-return-type.md b/packages/eslint-plugin/docs/rules/explicit-function-return-type.md index 46cd2b86077..2812023ffba 100644 --- a/packages/eslint-plugin/docs/rules/explicit-function-return-type.md +++ b/packages/eslint-plugin/docs/rules/explicit-function-return-type.md @@ -61,12 +61,19 @@ class Test { The rule accepts an options object with the following properties: -- `allowExpressions` if true, only functions which are part of a declaration will be checked -- `allowTypedFunctionExpressions` if true, type annotations are also allowed on the variable - of a function expression rather than on the function directly. +```ts +type Options = { + // if true, only functions which are part of a declaration will be checked + allowExpressions?: boolean; + // if true, type annotations are also allowed on the variable of a function expression rather than on the function directly. + allowTypedFunctionExpressions?: boolean; +}; -By default, `allowExpressions: false` and `allowTypedFunctionExpressions: false` are used, -meaning all declarations and expressions _must_ have a return type. +const defaults = { + allowExpressions: false, + allowTypedFunctionExpressions: false, +}; +``` ### allowExpressions @@ -88,6 +95,20 @@ const foo = arr.map(i => i * i); ### allowTypedFunctionExpressions +Examples of **incorrect** code for this rule with `{ allowTypedFunctionExpressions: true }`: + +```ts +let arrowFn = () => 'test'; + +let funcExpr = function() { + return 'test'; +}; + +let objectProp = { + foo: () => 1, +}; +``` + Examples of additional **correct** code for this rule with `{ allowTypedFunctionExpressions: true }`: ```ts @@ -100,6 +121,7 @@ let funcExpr: FuncType = function() { }; let asTyped = (() => '') as () => string; +let caasTyped = <() => string>(() => ''); interface ObjectType { foo(): number; @@ -107,14 +129,12 @@ interface ObjectType { let objectProp: ObjectType = { foo: () => 1, }; - -interface ObjectType { - foo(): number; -} - -let asObjectProp = { +let objectPropAs = { foo: () => 1, } as ObjectType; +let objectPropCast = { + foo: () => 1, +}; ``` ## When Not To Use It 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 9186ef3cab6..5a1f51d7643 100644 --- a/packages/eslint-plugin/src/rules/explicit-function-return-type.ts +++ b/packages/eslint-plugin/src/rules/explicit-function-return-type.ts @@ -5,6 +5,7 @@ type Options = [ { allowExpressions?: boolean; allowTypedFunctionExpressions?: boolean; + allowUntypedSetters?: boolean; } ]; type MessageIds = 'missingReturnType'; @@ -41,6 +42,7 @@ export default util.createRule({ { allowExpressions: false, allowTypedFunctionExpressions: false, + allowUntypedSetters: true, }, ], create(context, [options]) { @@ -48,8 +50,9 @@ export default util.createRule({ * Checks if a node is a constructor. * @param node The node to check */ - function isConstructor(node: TSESTree.Node): boolean { + function isConstructor(node: TSESTree.Node | undefined): boolean { return ( + !!node && node.type === AST_NODE_TYPES.MethodDefinition && node.kind === 'constructor' ); @@ -58,14 +61,17 @@ export default util.createRule({ /** * Checks if a node is a setter. */ - function isSetter(node: TSESTree.Node): boolean { + function isSetter(node: TSESTree.Node | undefined): boolean { return ( - node.type === AST_NODE_TYPES.MethodDefinition && node.kind === 'set' + !!node && + node.type === AST_NODE_TYPES.MethodDefinition && + node.kind === 'set' ); } /** * Checks if a node is a variable declarator with a type annotation. + * `const x: Foo = ...` */ function isVariableDeclaratorWithTypeAnnotation( node: TSESTree.Node, @@ -76,14 +82,37 @@ export default util.createRule({ ); } + /** + * Checks if a node is a class property with a type annotation. + * `public x: Foo = ...` + */ + function isClassPropertyWithTypeAnnotation(node: TSESTree.Node): boolean { + return ( + node.type === AST_NODE_TYPES.ClassProperty && !!node.typeAnnotation + ); + } + + /** + * Checks if a node is a type cast + * `(() => {}) as Foo` + * `(() => {})` + */ + function isTypeCast(node: TSESTree.Node): boolean { + return ( + node.type === AST_NODE_TYPES.TSAsExpression || + node.type === AST_NODE_TYPES.TSTypeAssertion + ); + } + /** * Checks if a node belongs to: - * const x: Foo = { prop: () => {} } + * `const x: Foo = { prop: () => {} }` + * `const x = { prop: () => {} } as Foo` + * `const x = { prop: () => {} }` */ - function isPropertyOfObjectVariableDeclaratorWithTypeAnnotation( - node: TSESTree.Node, + function isPropertyOfObjectWithType( + parent: TSESTree.Node | undefined, ): boolean { - let parent = node.parent; if (!parent || parent.type !== AST_NODE_TYPES.Property) { return false; } @@ -91,26 +120,21 @@ export default util.createRule({ if (!parent || parent.type !== AST_NODE_TYPES.ObjectExpression) { return false; } - parent = parent.parent; - return !!parent && isVariableDeclaratorWithTypeAnnotation(parent); - } - function isPropertyOfObjectInAsExpression(node: TSESTree.Node): boolean { - let parent = node.parent; - if (!parent || parent.type !== AST_NODE_TYPES.Property) { - return false; - } parent = parent.parent; - if (!parent || parent.type !== AST_NODE_TYPES.ObjectExpression) { + if (!parent) { return false; } - parent = parent.parent; - return !!parent && parent.type === AST_NODE_TYPES.TSAsExpression; + + return ( + isTypeCast(parent) || + isClassPropertyWithTypeAnnotation(parent) || + isVariableDeclaratorWithTypeAnnotation(parent) + ); } /** * Checks if a function declaration/expression has a return type. - * @param node The node representing a function. */ function checkFunctionReturnType( node: @@ -119,22 +143,14 @@ export default util.createRule({ | TSESTree.FunctionExpression, ): void { if ( - options.allowExpressions && - node.type !== AST_NODE_TYPES.FunctionDeclaration && - node.parent && - node.parent.type !== AST_NODE_TYPES.VariableDeclarator && - node.parent.type !== AST_NODE_TYPES.MethodDefinition + node.returnType || + isConstructor(node.parent) || + isSetter(node.parent) ) { return; } - if ( - !node.returnType && - node.parent && - !isConstructor(node.parent) && - !isSetter(node.parent) && - util.isTypeScriptFile(context.getFilename()) - ) { + if (util.isTypeScriptFile(context.getFilename())) { context.report({ node, messageId: 'missingReturnType', @@ -144,20 +160,29 @@ export default util.createRule({ /** * Checks if a function declaration/expression has a return type. - * @param {ASTNode} node The node representing a function. */ function checkFunctionExpressionReturnType( node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, ): void { - if ( - options.allowTypedFunctionExpressions && - node.parent && - (isVariableDeclaratorWithTypeAnnotation(node.parent) || - isPropertyOfObjectVariableDeclaratorWithTypeAnnotation(node) || - node.parent.type === AST_NODE_TYPES.TSAsExpression || - isPropertyOfObjectInAsExpression(node)) - ) { - return; + if (node.parent) { + if (options.allowTypedFunctionExpressions) { + if ( + isTypeCast(node.parent) || + isVariableDeclaratorWithTypeAnnotation(node.parent) || + isClassPropertyWithTypeAnnotation(node.parent) || + isPropertyOfObjectWithType(node.parent) + ) { + return; + } + } + + if ( + options.allowExpressions && + node.parent.type !== AST_NODE_TYPES.VariableDeclarator && + node.parent.type !== AST_NODE_TYPES.MethodDefinition + ) { + return; + } } checkFunctionReturnType(node); 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 4c242b484cc..efb98d1dd34 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 @@ -125,6 +125,11 @@ var funcExpr: Foo = function() { return 'test'; }; code: `const x = (() => {}) as Foo`, options: [{ allowTypedFunctionExpressions: true }], }, + { + filename: 'test.ts', + code: `const x = (() => {})`, + options: [{ allowTypedFunctionExpressions: true }], + }, { filename: 'test.ts', code: ` @@ -137,8 +142,29 @@ const x = { { filename: 'test.ts', code: ` +const x = { + foo: () => {}, +} + `, + options: [{ allowTypedFunctionExpressions: true }], + }, + { + filename: 'test.ts', + code: ` const x: Foo = { foo: () => {}, +} + `, + options: [{ allowTypedFunctionExpressions: true }], + }, + // https://github.com/typescript-eslint/typescript-eslint/issues/484 + { + filename: 'test.ts', + code: ` +type MethodType = () => void; + +class App { + private method: MethodType = () => {} } `, options: [{ allowTypedFunctionExpressions: true }], From 8444e25e6a425444fd5e1fac4e3274ea247bdc82 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Wed, 8 May 2019 10:23:47 -0700 Subject: [PATCH 2/3] coverage --- .../eslint-plugin/src/rules/explicit-function-return-type.ts | 2 ++ 1 file changed, 2 insertions(+) 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 5a1f51d7643..432a679eca2 100644 --- a/packages/eslint-plugin/src/rules/explicit-function-return-type.ts +++ b/packages/eslint-plugin/src/rules/explicit-function-return-type.ts @@ -117,11 +117,13 @@ export default util.createRule({ return false; } parent = parent.parent; + /* istanbul ignore if */// this shouldn't happen, checking just in case if (!parent || parent.type !== AST_NODE_TYPES.ObjectExpression) { return false; } parent = parent.parent; + /* istanbul ignore if */// this shouldn't happen, checking just in case if (!parent) { return false; } From ac9c1d5f7dbb2710381c3efbd2b66fd6de4e2f93 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Wed, 8 May 2019 10:29:21 -0700 Subject: [PATCH 3/3] format --- .../src/rules/explicit-function-return-type.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 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 432a679eca2..6228f014311 100644 --- a/packages/eslint-plugin/src/rules/explicit-function-return-type.ts +++ b/packages/eslint-plugin/src/rules/explicit-function-return-type.ts @@ -116,15 +116,16 @@ export default util.createRule({ if (!parent || parent.type !== AST_NODE_TYPES.Property) { return false; } - parent = parent.parent; - /* istanbul ignore if */// this shouldn't happen, checking just in case - if (!parent || parent.type !== AST_NODE_TYPES.ObjectExpression) { + parent = parent.parent; // this shouldn't happen, checking just in case + /* istanbul ignore if */ if ( + !parent || + parent.type !== AST_NODE_TYPES.ObjectExpression + ) { return false; } - parent = parent.parent; - /* istanbul ignore if */// this shouldn't happen, checking just in case - if (!parent) { + parent = parent.parent; // this shouldn't happen, checking just in case + /* istanbul ignore if */ if (!parent) { return false; }