From 5486e6068b5943d7d217a9509ca9c72308bc5b09 Mon Sep 17 00:00:00 2001 From: JounQin Date: Fri, 19 Mar 2021 16:08:15 +0800 Subject: [PATCH 1/3] docs: highlight `this: void` and arrow function for unbound-method close #3201 --- packages/eslint-plugin/docs/rules/unbound-method.md | 2 ++ packages/eslint-plugin/src/rules/unbound-method.ts | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/unbound-method.md b/packages/eslint-plugin/docs/rules/unbound-method.md index a9a4fe396a2..8ef51924940 100644 --- 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 you think it is intended, you can try to add `this: void` as the first function parameter or use 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 index 2ec66bc2aa3..62a4ac2bd91 100644 --- a/packages/eslint-plugin/src/rules/unbound-method.ts +++ b/packages/eslint-plugin/src/rules/unbound-method.ts @@ -132,8 +132,8 @@ export default util.createRule({ requiresTypeChecking: true, }, messages: { - unbound: - 'Avoid referencing unbound methods which may cause unintentional scoping of `this`.', + unbound: `Avoid referencing unbound methods which may cause unintentional scoping of \`this\`. +If you think it is intended, you can try to add \`this: void\` as the first function parameter or use arrow function instead.`, }, schema: [ { From 2a6026f0bd2f38ddb8431e1a6a3f00f6af3f1673 Mon Sep 17 00:00:00 2001 From: JounQin Date: Sat, 20 Mar 2021 08:59:34 +0800 Subject: [PATCH 2/3] Update packages/eslint-plugin/docs/rules/unbound-method.md Co-authored-by: Brad Zacher --- packages/eslint-plugin/docs/rules/unbound-method.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/unbound-method.md b/packages/eslint-plugin/docs/rules/unbound-method.md index 8ef51924940..15063dbb4f3 100644 --- a/packages/eslint-plugin/docs/rules/unbound-method.md +++ b/packages/eslint-plugin/docs/rules/unbound-method.md @@ -4,7 +4,7 @@ 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 you think it is intended, you can try to add `this: void` as the first function parameter or use arrow function instead. +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 From f55107cd5dbf46578a1644e4a1618a5c4242de90 Mon Sep 17 00:00:00 2001 From: JounQin Date: Sat, 20 Mar 2021 09:50:55 +0800 Subject: [PATCH 3/3] feat: check if the first param is `this` add new message `unboundWithoutThisAnnotation` --- .../eslint-plugin/src/rules/unbound-method.ts | 97 ++++++++++++------- .../tests/rules/unbound-method.test.ts | 36 +++---- 2 files changed, 79 insertions(+), 54 deletions(-) diff --git a/packages/eslint-plugin/src/rules/unbound-method.ts b/packages/eslint-plugin/src/rules/unbound-method.ts index 62a4ac2bd91..ecf9e652819 100644 --- 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({ name: 'unbound-method', meta: { @@ -132,8 +135,11 @@ export default util.createRule({ requiresTypeChecking: true, }, messages: { - unbound: `Avoid referencing unbound methods which may cause unintentional scoping of \`this\`. -If you think it is intended, you can try to add \`this: void\` as the first function parameter or use arrow function instead.`, + 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 @@ If you think it is intended, you can try to add \`this: void\` as the first func 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 @@ If you think it is intended, you can try to add \`this: void\` as the first func } 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 @@ If you think it is intended, you can try to add \`this: void\` as the first func 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 @@ If you think it is intended, you can try to add \`this: void\` as the first func }, }); -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 index 911206b1f21..3075e93fe67 100644 --- 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', }, ], },