fix(eslint-plugin): [prefer-function-type] handle this return (#2437)

Co-authored-by: Tadhg McDonald-Jensen <[email protected]>

Author: tadhgmister

Diff for: packages/eslint-plugin/docs/rules/prefer-function-type.md

@@ -24,6 +24,13 @@ interface Foo extends Function {
 }
 ```
 
+```ts
+interface MixinMethod {
+  // returns the function itself, not the `this` argument.
+  (arg: string): this;
+}
+```
+
 Examples of **correct** code for this rule:
 
 ```ts
@@ -48,6 +55,23 @@ interface Bar extends Foo {
 }
 ```
 
+```ts
+// returns the `this` argument of function, retaining it's type.
+type MixinMethod = <TSelf>(this: TSelf, arg: string) => TSelf;
+// a function that returns itself is much clearer in this form.
+type ReturnsSelf = (arg: string) => ReturnsSelf;
+```
+
+```ts
+// multiple call signatures (overloads) is allowed:
+interface Overloaded {
+  (data: string): number;
+  (id: number): string;
+}
+// this is equivelent to Overloaded interface.
+type Intersection = ((data: string) => number) & ((id: number) => string);
+```
+
 ## When Not To Use It
 
 If you specifically want to use an interface or type literal with a single call signature for stylistic reasons, you can disable this rule.

Diff for: packages/eslint-plugin/src/rules/prefer-function-type.ts

@@ -5,6 +5,11 @@ import {
 } from '@typescript-eslint/experimental-utils';
 import * as util from '../util';
 
+export const phrases = {
+  [AST_NODE_TYPES.TSTypeLiteral]: 'Type literal',
+  [AST_NODE_TYPES.TSInterfaceDeclaration]: 'Interface',
+} as const;
+
 export default util.createRule({
   name: 'prefer-function-type',
   meta: {
@@ -17,7 +22,9 @@ export default util.createRule({
     fixable: 'code',
     messages: {
       functionTypeOverCallableType:
-        "{{ type }} has only a call signature - use '{{ sigSuggestion }}' instead.",
+        '{{ literalOrInterface }} only has a call signature, you should use a function type instead.',
+      unexpectedThisOnFunctionOnlyInterface:
+        "`this` refers to the function type '{{ interfaceName }}', did you intend to use a generic `this` parameter like `<Self>(this: Self, ...) => Self` instead?",
     },
     schema: [],
     type: 'suggestion',
@@ -104,13 +111,30 @@ export default util.createRule({
      */
     function checkMember(
       member: TSESTree.TypeElement,
-      node: TSESTree.Node,
+      node: TSESTree.TSInterfaceDeclaration | TSESTree.TSTypeLiteral,
+      tsThisTypes: TSESTree.TSThisType[] | null = null,
     ): void {
       if (
         (member.type === AST_NODE_TYPES.TSCallSignatureDeclaration ||
           member.type === AST_NODE_TYPES.TSConstructSignatureDeclaration) &&
         typeof member.returnType !== 'undefined'
       ) {
+        if (
+          tsThisTypes !== null &&
+          tsThisTypes.length > 0 &&
+          node.type === AST_NODE_TYPES.TSInterfaceDeclaration
+        ) {
+          // the message can be confusing if we don't point directly to the `this` node instead of the whole member
+          // and in favour of generating at most one error we'll only report the first occurrence of `this` if there are multiple
+          context.report({
+            node: tsThisTypes[0],
+            messageId: 'unexpectedThisOnFunctionOnlyInterface',
+            data: {
+              interfaceName: node.id.name,
+            },
+          });
+          return;
+        }
         const suggestion = renderSuggestion(member, node);
         const fixStart =
           node.type === AST_NODE_TYPES.TSTypeLiteral
@@ -127,11 +151,7 @@ export default util.createRule({
           node: member,
           messageId: 'functionTypeOverCallableType',
           data: {
-            type:
-              node.type === AST_NODE_TYPES.TSTypeLiteral
-                ? 'Type literal'
-                : 'Interface',
-            sigSuggestion: suggestion,
+            literalOrInterface: phrases[node.type],
           },
           fix(fixer) {
             return fixer.replaceTextRange(
@@ -142,12 +162,36 @@ export default util.createRule({
         });
       }
     }
-
+    let tsThisTypes: TSESTree.TSThisType[] | null = null;
+    let literalNesting = 0;
     return {
-      TSInterfaceDeclaration(node): void {
+      TSInterfaceDeclaration(): void {
+        // when entering an interface reset the count of `this`s to empty.
+        tsThisTypes = [];
+      },
+      'TSInterfaceDeclaration TSThisType'(node: TSESTree.TSThisType): void {
+        // inside an interface keep track of all ThisType references.
+        // unless it's inside a nested type literal in which case it's invalid code anyway
+        // we don't want to incorrectly say "it refers to name" while typescript says it's completely invalid.
+        if (literalNesting === 0 && tsThisTypes !== null) {
+          tsThisTypes.push(node);
+        }
+      },
+      'TSInterfaceDeclaration:exit'(
+        node: TSESTree.TSInterfaceDeclaration,
+      ): void {
         if (!hasOneSupertype(node) && node.body.body.length === 1) {
-          checkMember(node.body.body[0], node);
+          checkMember(node.body.body[0], node, tsThisTypes);
         }
+        // on exit check member and reset the array to nothing.
+        tsThisTypes = null;
+      },
+      // keep track of nested literals to avoid complaining about invalid `this` uses
+      'TSInterfaceDeclaration TSTypeLiteral'(): void {
+        literalNesting += 1;
+      },
+      'TSInterfaceDeclaration TSTypeLiteral:exit'(): void {
+        literalNesting -= 1;
       },
       'TSTypeLiteral[members.length = 1]'(node: TSESTree.TSTypeLiteral): void {
         checkMember(node.members[0], node);

Diff for: packages/eslint-plugin/tests/rules/prefer-function-type.test.ts

@@ -1,6 +1,6 @@
 import { AST_NODE_TYPES } from '@typescript-eslint/experimental-utils';
-import rule from '../../src/rules/prefer-function-type';
-import { RuleTester } from '../RuleTester';
+import rule, { phrases } from '../../src/rules/prefer-function-type';
+import { noFormat, RuleTester } from '../RuleTester';
 
 const ruleTester = new RuleTester({
   parserOptions: {
@@ -56,6 +56,9 @@ interface Foo {
         {
           messageId: 'functionTypeOverCallableType',
           type: AST_NODE_TYPES.TSCallSignatureDeclaration,
+          data: {
+            literalOrInterface: phrases[AST_NODE_TYPES.TSInterfaceDeclaration],
+          },
         },
       ],
       output: `
@@ -72,6 +75,9 @@ type Foo = {
         {
           messageId: 'functionTypeOverCallableType',
           type: AST_NODE_TYPES.TSCallSignatureDeclaration,
+          data: {
+            literalOrInterface: phrases[AST_NODE_TYPES.TSTypeLiteral],
+          },
         },
       ],
       output: `
@@ -88,6 +94,9 @@ function foo(bar: { (s: string): number }): number {
         {
           messageId: 'functionTypeOverCallableType',
           type: AST_NODE_TYPES.TSCallSignatureDeclaration,
+          data: {
+            literalOrInterface: phrases[AST_NODE_TYPES.TSTypeLiteral],
+          },
         },
       ],
       output: `
@@ -106,6 +115,9 @@ function foo(bar: { (s: string): number } | undefined): number {
         {
           messageId: 'functionTypeOverCallableType',
           type: AST_NODE_TYPES.TSCallSignatureDeclaration,
+          data: {
+            literalOrInterface: phrases[AST_NODE_TYPES.TSTypeLiteral],
+          },
         },
       ],
       output: `
@@ -124,6 +136,9 @@ interface Foo extends Function {
         {
           messageId: 'functionTypeOverCallableType',
           type: AST_NODE_TYPES.TSCallSignatureDeclaration,
+          data: {
+            literalOrInterface: phrases[AST_NODE_TYPES.TSInterfaceDeclaration],
+          },
         },
       ],
       output: `
@@ -140,11 +155,118 @@ interface Foo<T> {
         {
           messageId: 'functionTypeOverCallableType',
           type: AST_NODE_TYPES.TSCallSignatureDeclaration,
+          data: {
+            literalOrInterface: phrases[AST_NODE_TYPES.TSInterfaceDeclaration],
+          },
         },
       ],
       output: `
 type Foo<T> = (bar: T) => string;
       `,
     },
+    {
+      code: `
+interface Foo<T> {
+  (this: T): void;
+}
+      `,
+      errors: [
+        {
+          messageId: 'functionTypeOverCallableType',
+          type: AST_NODE_TYPES.TSCallSignatureDeclaration,
+          data: {
+            literalOrInterface: phrases[AST_NODE_TYPES.TSInterfaceDeclaration],
+          },
+        },
+      ],
+      output: `
+type Foo<T> = (this: T) => void;
+      `,
+    },
+    {
+      code: `
+type Foo<T> = { (this: string): T };
+      `,
+      errors: [
+        {
+          messageId: 'functionTypeOverCallableType',
+          type: AST_NODE_TYPES.TSCallSignatureDeclaration,
+          data: {
+            literalOrInterface: phrases[AST_NODE_TYPES.TSTypeLiteral],
+          },
+        },
+      ],
+      output: `
+type Foo<T> = (this: string) => T;
+      `,
+    },
+    {
+      code: `
+interface Foo {
+  (arg: this): void;
+}
+      `,
+      errors: [
+        {
+          messageId: 'unexpectedThisOnFunctionOnlyInterface',
+          type: AST_NODE_TYPES.TSThisType,
+          data: {
+            interfaceName: 'Foo',
+          },
+        },
+      ],
+    },
+    {
+      code: `
+interface Foo {
+  (arg: number): this | undefined;
+}
+      `,
+      errors: [
+        {
+          messageId: 'unexpectedThisOnFunctionOnlyInterface',
+          type: AST_NODE_TYPES.TSThisType,
+          data: {
+            interfaceName: 'Foo',
+          },
+        },
+      ],
+    },
+    {
+      code: `
+interface Foo {
+  // isn't actually valid ts but want to not give message saying it refers to Foo.
+  (): {
+    a: {
+      nested: this;
+    };
+    between: this;
+    b: {
+      nested: string;
+    };
+  };
+}
+      `,
+      errors: [
+        {
+          messageId: 'functionTypeOverCallableType',
+          type: AST_NODE_TYPES.TSCallSignatureDeclaration,
+          data: {
+            literalOrInterface: phrases[AST_NODE_TYPES.TSInterfaceDeclaration],
+          },
+        },
+      ],
+      output: noFormat`
+type Foo = () => {
+    a: {
+      nested: this;
+    };
+    between: this;
+    b: {
+      nested: string;
+    };
+  };
+      `,
+    },
   ],
 });