feat(eslint-plugin): [unbound-method] improve error message (#3203)

* docs: highlight `this: void` and arrow function for unbound-method close #3201 * Update packages/eslint-plugin/docs/rules/unbound-method.md Co-authored-by: Brad Zacher <brad.zacher@gmail.com> * feat: check if the first param is `this` add new message `unboundWithoutThisAnnotation` Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
typescript-eslint · Mar 21, 2021 · 5cc5d2e · 5cc5d2e
1 parent 0557a43
commit 5cc5d2e
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 54 deletions.
diff --git a/packages/eslint-plugin/docs/rules/unbound-method.md b/packages/eslint-plugin/docs/rules/unbound-method.md
@@ -4,6 +4,8 @@ Warns when a method is used outside of a method call.
 
 Class functions don't preserve the class scope when passed as standalone variables.
 
+If your function does not access `this`, [you can annotate it with `this: void`](https://www.typescriptlang.org/docs/handbook/2/functions.html#declaring-this-in-a-function), or consider using an arrow function instead.
+
 ## Rule Details
 
 Examples of **incorrect** code for this rule

diff --git a/packages/eslint-plugin/src/rules/unbound-method.ts b/packages/eslint-plugin/src/rules/unbound-method.ts
@@ -16,7 +16,7 @@ interface Config {
 
 export type Options = [Config];
 
-export type MessageIds = 'unbound';
+export type MessageIds = 'unbound' | 'unboundWithoutThisAnnotation';
 
 /**
  * The following is a list of exceptions to the rule
@@ -121,6 +121,9 @@ const getNodeName = (node: TSESTree.Node): string | null =>
 const getMemberFullName = (node: TSESTree.MemberExpression): string =>
   `${getNodeName(node.object)}.${getNodeName(node.property)}`;
 
+const BASE_MESSAGE =
+  'Avoid referencing unbound methods which may cause unintentional scoping of `this`.';
+
 export default util.createRule<Options, MessageIds>({
   name: 'unbound-method',
   meta: {
@@ -132,8 +135,11 @@ export default util.createRule<Options, MessageIds>({
       requiresTypeChecking: true,
     },
     messages: {
-      unbound:
-        'Avoid referencing unbound methods which may cause unintentional scoping of `this`.',
+      unbound: BASE_MESSAGE,
+      unboundWithoutThisAnnotation:
+        BASE_MESSAGE +
+        '\n' +
+        'If your function does not access `this`, you can annotate it with `this: void`, or consider using an arrow function instead.',
     },
     schema: [
       {
@@ -160,6 +166,26 @@ export default util.createRule<Options, MessageIds>({
       context.getFilename(),
     );
 
+    function checkMethodAndReport(
+      node: TSESTree.Node,
+      symbol: ts.Symbol | undefined,
+    ): void {
+      if (!symbol) {
+        return;
+      }
+
+      const { dangerous, firstParamIsThis } = checkMethod(symbol, ignoreStatic);
+      if (dangerous) {
+        context.report({
+          messageId:
+            firstParamIsThis === false
+              ? 'unboundWithoutThisAnnotation'
+              : 'unbound',
+          node,
+        });
+      }
+    }
+
     return {
       MemberExpression(node: TSESTree.MemberExpression): void {
         if (isSafeUse(node)) {
@@ -179,14 +205,8 @@ export default util.createRule<Options, MessageIds>({
         }
 
         const originalNode = parserServices.esTreeNodeToTSNodeMap.get(node);
-        const symbol = checker.getSymbolAtLocation(originalNode);
 
-        if (symbol && isDangerousMethod(symbol, ignoreStatic)) {
-          context.report({
-            messageId: 'unbound',
-            node,
-          });
-        }
+        checkMethodAndReport(node, checker.getSymbolAtLocation(originalNode));
       },
       'VariableDeclarator, AssignmentExpression'(
         node: TSESTree.VariableDeclarator | TSESTree.AssignmentExpression,
@@ -219,13 +239,10 @@ export default util.createRule<Options, MessageIds>({
                 return;
               }
 
-              const symbol = initTypes.getProperty(property.key.name);
-              if (symbol && isDangerousMethod(symbol, ignoreStatic)) {
-                context.report({
-                  messageId: 'unbound',
-                  node,
-                });
-              }
+              checkMethodAndReport(
+                node,
+                initTypes.getProperty(property.key.name),
+              );
             }
           });
         }
@@ -234,44 +251,52 @@ export default util.createRule<Options, MessageIds>({
   },
 });
 
-function isDangerousMethod(symbol: ts.Symbol, ignoreStatic: boolean): boolean {
+function checkMethod(
+  symbol: ts.Symbol,
+  ignoreStatic: boolean,
+): { dangerous: boolean; firstParamIsThis?: boolean } {
   const { valueDeclaration } = symbol;
   if (!valueDeclaration) {
     // working around https://github.com/microsoft/TypeScript/issues/31294
-    return false;
+    return { dangerous: false };
   }
 
   switch (valueDeclaration.kind) {
     case ts.SyntaxKind.PropertyDeclaration:
-      return (
-        (valueDeclaration as ts.PropertyDeclaration).initializer?.kind ===
-        ts.SyntaxKind.FunctionExpression
-      );
+      return {
+        dangerous:
+          (valueDeclaration as ts.PropertyDeclaration).initializer?.kind ===
+          ts.SyntaxKind.FunctionExpression,
+      };
     case ts.SyntaxKind.MethodDeclaration:
     case ts.SyntaxKind.MethodSignature: {
       const decl = valueDeclaration as
         | ts.MethodDeclaration
         | ts.MethodSignature;
       const firstParam = decl.parameters[0];
-      const thisArgIsVoid =
+      const firstParamIsThis =
         firstParam?.name.kind === ts.SyntaxKind.Identifier &&
-        firstParam?.name.escapedText === 'this' &&
+        firstParam?.name.escapedText === 'this';
+      const thisArgIsVoid =
+        firstParamIsThis &&
         firstParam?.type?.kind === ts.SyntaxKind.VoidKeyword;
 
-      return (
-        !thisArgIsVoid &&
-        !(
-          ignoreStatic &&
-          tsutils.hasModifier(
-            valueDeclaration.modifiers,
-            ts.SyntaxKind.StaticKeyword,
-          )
-        )
-      );
+      return {
+        dangerous:
+          !thisArgIsVoid &&
+          !(
+            ignoreStatic &&
+            tsutils.hasModifier(
+              valueDeclaration.modifiers,
+              ts.SyntaxKind.StaticKeyword,
+            )
+          ),
+        firstParamIsThis,
+      };
     }
   }
 
-  return false;
+  return { dangerous: false };
 }
 
 function isSafeUse(node: TSESTree.Node): boolean {

diff --git a/packages/eslint-plugin/tests/rules/unbound-method.test.ts b/packages/eslint-plugin/tests/rules/unbound-method.test.ts
@@ -42,7 +42,7 @@ function addContainsMethodsClassInvalid(
     errors: [
       {
         line: 18,
-        messageId: 'unbound',
+        messageId: 'unboundWithoutThisAnnotation',
       },
     ],
   }));
@@ -298,7 +298,7 @@ Promise.resolve().then(console.log);
       errors: [
         {
           line: 10,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -310,7 +310,7 @@ const x = console.log;
       errors: [
         {
           line: 3,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -325,15 +325,15 @@ function foo(arg: ContainsMethods | null) {
       errors: [
         {
           line: 20,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
         {
           line: 21,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
         {
           line: 22,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -375,7 +375,7 @@ ContainsMethods.unboundStatic;
       errors: [
         {
           line: 8,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -390,7 +390,7 @@ const x = CommunicationError.prototype.foo;
       errors: [
         {
           line: 5,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -400,7 +400,7 @@ const x = CommunicationError.prototype.foo;
       errors: [
         {
           line: 1,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -419,7 +419,7 @@ instance.unbound = x; // THIS SHOULD NOT
       errors: [
         {
           line: 9,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -447,7 +447,7 @@ const { unbound } = new Foo();
       errors: [
         {
           line: 5,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -476,7 +476,7 @@ let unbound;
       errors: [
         {
           line: 6,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -505,7 +505,7 @@ const { foo } = CommunicationError.prototype;
       errors: [
         {
           line: 5,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -520,7 +520,7 @@ let foo;
       errors: [
         {
           line: 6,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -532,7 +532,7 @@ const { log } = console;
       errors: [
         {
           line: 3,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -541,7 +541,7 @@ const { log } = console;
       errors: [
         {
           line: 1,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -562,7 +562,7 @@ class OtherClass extends BaseClass {
         {
           line: 8,
           column: 15,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },
@@ -584,7 +584,7 @@ class OtherClass extends BaseClass {
         {
           line: 9,
           column: 9,
-          messageId: 'unbound',
+          messageId: 'unboundWithoutThisAnnotation',
         },
       ],
     },