From 2c36325c7797f84352b3f60cb3ed85c43c572a51 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Wed, 8 May 2019 18:41:52 -0700 Subject: [PATCH] fix(eslint-plugin): [explicit-function-return-type] Add handling for class properties (#502) --- .../rules/explicit-function-return-type.md | 42 +++++-- .../rules/explicit-function-return-type.ts | 116 +++++++++++------- .../explicit-function-return-type.test.ts | 26 ++++ 3 files changed, 129 insertions(+), 55 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..6228f014311 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,41 +82,62 @@ 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; } - parent = parent.parent; - 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; - return !!parent && isVariableDeclaratorWithTypeAnnotation(parent); - } - function isPropertyOfObjectInAsExpression(node: TSESTree.Node): boolean { - let parent = node.parent; - if (!parent || parent.type !== AST_NODE_TYPES.Property) { + parent = parent.parent; // this shouldn't happen, checking just in case + /* istanbul ignore if */ if (!parent) { return false; } - parent = parent.parent; - if (!parent || parent.type !== AST_NODE_TYPES.ObjectExpression) { - 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 +146,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 +163,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 }],