Skip to content

Commit 5cc5d2e

Browse files
JounQinbradzacher
JounQin
and
bradzacher
authored
feat(eslint-plugin): [unbound-method] improve error message (typescript-eslint#3203)
* docs: highlight `this: void` and arrow function for unbound-method close typescript-eslint#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>
1 parent 0557a43 commit 5cc5d2e

File tree

3 files changed

+81
-54
lines changed

3 files changed

+81
-54
lines changed

‎packages/eslint-plugin/docs/rules/unbound-method.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ Warns when a method is used outside of a method call.
44

55
Class functions don't preserve the class scope when passed as standalone variables.
66

7+
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.
8+
79
## Rule Details
810

911
Examples of **incorrect** code for this rule

‎packages/eslint-plugin/src/rules/unbound-method.ts

Lines changed: 61 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ interface Config {
1616

1717
export type Options = [Config];
1818

19-
export type MessageIds = 'unbound';
19+
export type MessageIds = 'unbound' | 'unboundWithoutThisAnnotation';
2020

2121
/**
2222
* The following is a list of exceptions to the rule
@@ -121,6 +121,9 @@ const getNodeName = (node: TSESTree.Node): string | null =>
121121
const getMemberFullName = (node: TSESTree.MemberExpression): string =>
122122
`${getNodeName(node.object)}.${getNodeName(node.property)}`;
123123

124+
const BASE_MESSAGE =
125+
'Avoid referencing unbound methods which may cause unintentional scoping of `this`.';
126+
124127
export default util.createRule<Options, MessageIds>({
125128
name: 'unbound-method',
126129
meta: {
@@ -132,8 +135,11 @@ export default util.createRule<Options, MessageIds>({
132135
requiresTypeChecking: true,
133136
},
134137
messages: {
135-
unbound:
136-
'Avoid referencing unbound methods which may cause unintentional scoping of `this`.',
138+
unbound: BASE_MESSAGE,
139+
unboundWithoutThisAnnotation:
140+
BASE_MESSAGE +
141+
'\n' +
142+
'If your function does not access `this`, you can annotate it with `this: void`, or consider using an arrow function instead.',
137143
},
138144
schema: [
139145
{
@@ -160,6 +166,26 @@ export default util.createRule<Options, MessageIds>({
160166
context.getFilename(),
161167
);
162168

169+
function checkMethodAndReport(
170+
node: TSESTree.Node,
171+
symbol: ts.Symbol | undefined,
172+
): void {
173+
if (!symbol) {
174+
return;
175+
}
176+
177+
const { dangerous, firstParamIsThis } = checkMethod(symbol, ignoreStatic);
178+
if (dangerous) {
179+
context.report({
180+
messageId:
181+
firstParamIsThis === false
182+
? 'unboundWithoutThisAnnotation'
183+
: 'unbound',
184+
node,
185+
});
186+
}
187+
}
188+
163189
return {
164190
MemberExpression(node: TSESTree.MemberExpression): void {
165191
if (isSafeUse(node)) {
@@ -179,14 +205,8 @@ export default util.createRule<Options, MessageIds>({
179205
}
180206

181207
const originalNode = parserServices.esTreeNodeToTSNodeMap.get(node);
182-
const symbol = checker.getSymbolAtLocation(originalNode);
183208

184-
if (symbol && isDangerousMethod(symbol, ignoreStatic)) {
185-
context.report({
186-
messageId: 'unbound',
187-
node,
188-
});
189-
}
209+
checkMethodAndReport(node, checker.getSymbolAtLocation(originalNode));
190210
},
191211
'VariableDeclarator, AssignmentExpression'(
192212
node: TSESTree.VariableDeclarator | TSESTree.AssignmentExpression,
@@ -219,13 +239,10 @@ export default util.createRule<Options, MessageIds>({
219239
return;
220240
}
221241

222-
const symbol = initTypes.getProperty(property.key.name);
223-
if (symbol && isDangerousMethod(symbol, ignoreStatic)) {
224-
context.report({
225-
messageId: 'unbound',
226-
node,
227-
});
228-
}
242+
checkMethodAndReport(
243+
node,
244+
initTypes.getProperty(property.key.name),
245+
);
229246
}
230247
});
231248
}
@@ -234,44 +251,52 @@ export default util.createRule<Options, MessageIds>({
234251
},
235252
});
236253

237-
function isDangerousMethod(symbol: ts.Symbol, ignoreStatic: boolean): boolean {
254+
function checkMethod(
255+
symbol: ts.Symbol,
256+
ignoreStatic: boolean,
257+
): { dangerous: boolean; firstParamIsThis?: boolean } {
238258
const { valueDeclaration } = symbol;
239259
if (!valueDeclaration) {
240260
// working around https://github.com/microsoft/TypeScript/issues/31294
241-
return false;
261+
return { dangerous: false };
242262
}
243263

244264
switch (valueDeclaration.kind) {
245265
case ts.SyntaxKind.PropertyDeclaration:
246-
return (
247-
(valueDeclaration as ts.PropertyDeclaration).initializer?.kind ===
248-
ts.SyntaxKind.FunctionExpression
249-
);
266+
return {
267+
dangerous:
268+
(valueDeclaration as ts.PropertyDeclaration).initializer?.kind ===
269+
ts.SyntaxKind.FunctionExpression,
270+
};
250271
case ts.SyntaxKind.MethodDeclaration:
251272
case ts.SyntaxKind.MethodSignature: {
252273
const decl = valueDeclaration as
253274
| ts.MethodDeclaration
254275
| ts.MethodSignature;
255276
const firstParam = decl.parameters[0];
256-
const thisArgIsVoid =
277+
const firstParamIsThis =
257278
firstParam?.name.kind === ts.SyntaxKind.Identifier &&
258-
firstParam?.name.escapedText === 'this' &&
279+
firstParam?.name.escapedText === 'this';
280+
const thisArgIsVoid =
281+
firstParamIsThis &&
259282
firstParam?.type?.kind === ts.SyntaxKind.VoidKeyword;
260283

261-
return (
262-
!thisArgIsVoid &&
263-
!(
264-
ignoreStatic &&
265-
tsutils.hasModifier(
266-
valueDeclaration.modifiers,
267-
ts.SyntaxKind.StaticKeyword,
268-
)
269-
)
270-
);
284+
return {
285+
dangerous:
286+
!thisArgIsVoid &&
287+
!(
288+
ignoreStatic &&
289+
tsutils.hasModifier(
290+
valueDeclaration.modifiers,
291+
ts.SyntaxKind.StaticKeyword,
292+
)
293+
),
294+
firstParamIsThis,
295+
};
271296
}
272297
}
273298

274-
return false;
299+
return { dangerous: false };
275300
}
276301

277302
function isSafeUse(node: TSESTree.Node): boolean {

‎packages/eslint-plugin/tests/rules/unbound-method.test.ts

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ function addContainsMethodsClassInvalid(
4242
errors: [
4343
{
4444
line: 18,
45-
messageId: 'unbound',
45+
messageId: 'unboundWithoutThisAnnotation',
4646
},
4747
],
4848
}));
@@ -298,7 +298,7 @@ Promise.resolve().then(console.log);
298298
errors: [
299299
{
300300
line: 10,
301-
messageId: 'unbound',
301+
messageId: 'unboundWithoutThisAnnotation',
302302
},
303303
],
304304
},
@@ -310,7 +310,7 @@ const x = console.log;
310310
errors: [
311311
{
312312
line: 3,
313-
messageId: 'unbound',
313+
messageId: 'unboundWithoutThisAnnotation',
314314
},
315315
],
316316
},
@@ -325,15 +325,15 @@ function foo(arg: ContainsMethods | null) {
325325
errors: [
326326
{
327327
line: 20,
328-
messageId: 'unbound',
328+
messageId: 'unboundWithoutThisAnnotation',
329329
},
330330
{
331331
line: 21,
332-
messageId: 'unbound',
332+
messageId: 'unboundWithoutThisAnnotation',
333333
},
334334
{
335335
line: 22,
336-
messageId: 'unbound',
336+
messageId: 'unboundWithoutThisAnnotation',
337337
},
338338
],
339339
},
@@ -375,7 +375,7 @@ ContainsMethods.unboundStatic;
375375
errors: [
376376
{
377377
line: 8,
378-
messageId: 'unbound',
378+
messageId: 'unboundWithoutThisAnnotation',
379379
},
380380
],
381381
},
@@ -390,7 +390,7 @@ const x = CommunicationError.prototype.foo;
390390
errors: [
391391
{
392392
line: 5,
393-
messageId: 'unbound',
393+
messageId: 'unboundWithoutThisAnnotation',
394394
},
395395
],
396396
},
@@ -400,7 +400,7 @@ const x = CommunicationError.prototype.foo;
400400
errors: [
401401
{
402402
line: 1,
403-
messageId: 'unbound',
403+
messageId: 'unboundWithoutThisAnnotation',
404404
},
405405
],
406406
},
@@ -419,7 +419,7 @@ instance.unbound = x; // THIS SHOULD NOT
419419
errors: [
420420
{
421421
line: 9,
422-
messageId: 'unbound',
422+
messageId: 'unboundWithoutThisAnnotation',
423423
},
424424
],
425425
},
@@ -447,7 +447,7 @@ const { unbound } = new Foo();
447447
errors: [
448448
{
449449
line: 5,
450-
messageId: 'unbound',
450+
messageId: 'unboundWithoutThisAnnotation',
451451
},
452452
],
453453
},
@@ -476,7 +476,7 @@ let unbound;
476476
errors: [
477477
{
478478
line: 6,
479-
messageId: 'unbound',
479+
messageId: 'unboundWithoutThisAnnotation',
480480
},
481481
],
482482
},
@@ -505,7 +505,7 @@ const { foo } = CommunicationError.prototype;
505505
errors: [
506506
{
507507
line: 5,
508-
messageId: 'unbound',
508+
messageId: 'unboundWithoutThisAnnotation',
509509
},
510510
],
511511
},
@@ -520,7 +520,7 @@ let foo;
520520
errors: [
521521
{
522522
line: 6,
523-
messageId: 'unbound',
523+
messageId: 'unboundWithoutThisAnnotation',
524524
},
525525
],
526526
},
@@ -532,7 +532,7 @@ const { log } = console;
532532
errors: [
533533
{
534534
line: 3,
535-
messageId: 'unbound',
535+
messageId: 'unboundWithoutThisAnnotation',
536536
},
537537
],
538538
},
@@ -541,7 +541,7 @@ const { log } = console;
541541
errors: [
542542
{
543543
line: 1,
544-
messageId: 'unbound',
544+
messageId: 'unboundWithoutThisAnnotation',
545545
},
546546
],
547547
},
@@ -562,7 +562,7 @@ class OtherClass extends BaseClass {
562562
{
563563
line: 8,
564564
column: 15,
565-
messageId: 'unbound',
565+
messageId: 'unboundWithoutThisAnnotation',
566566
},
567567
],
568568
},
@@ -584,7 +584,7 @@ class OtherClass extends BaseClass {
584584
{
585585
line: 9,
586586
column: 9,
587-
messageId: 'unbound',
587+
messageId: 'unboundWithoutThisAnnotation',
588588
},
589589
],
590590
},

0 commit comments

Comments
 (0)