Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from Mar 21, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5486e60
docs: highlight `this: void` and arrow function for unbound-method
JounQin Mar 19, 2021
2a6026f
Update packages/eslint-plugin/docs/rules/unbound-method.md
JounQin Mar 20, 2021
f55107c
feat: check if the first param is `this`
JounQin Mar 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/eslint-plugin/docs/rules/unbound-method.md
View file
Open in desktop
Expand Up @@ -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
Expand Down
97 changes: 61 additions & 36 deletions packages/eslint-plugin/src/rules/unbound-method.ts
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -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: {
Expand All @@ -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: [
{
Expand All @@ -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)) {
Expand All @@ -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,
Expand Down Expand Up @@ -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),
);
}
});
}
Expand All @@ -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 {
Expand Down
36 changes: 18 additions & 18 deletions packages/eslint-plugin/tests/rules/unbound-method.test.ts
View file
Open in desktop
Expand Up @@ -42,7 +42,7 @@ function addContainsMethodsClassInvalid(
errors: [
{
line: 18,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
}));
Expand Down Expand Up @@ -298,7 +298,7 @@ Promise.resolve().then(console.log);
errors: [
{
line: 10,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand All @@ -310,7 +310,7 @@ const x = console.log;
errors: [
{
line: 3,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand All @@ -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',
},
],
},
Expand Down Expand Up @@ -375,7 +375,7 @@ ContainsMethods.unboundStatic;
errors: [
{
line: 8,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand All @@ -390,7 +390,7 @@ const x = CommunicationError.prototype.foo;
errors: [
{
line: 5,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand All @@ -400,7 +400,7 @@ const x = CommunicationError.prototype.foo;
errors: [
{
line: 1,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand All @@ -419,7 +419,7 @@ instance.unbound = x; // THIS SHOULD NOT
errors: [
{
line: 9,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand Down Expand Up @@ -447,7 +447,7 @@ const { unbound } = new Foo();
errors: [
{
line: 5,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand Down Expand Up @@ -476,7 +476,7 @@ let unbound;
errors: [
{
line: 6,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand Down Expand Up @@ -505,7 +505,7 @@ const { foo } = CommunicationError.prototype;
errors: [
{
line: 5,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand All @@ -520,7 +520,7 @@ let foo;
errors: [
{
line: 6,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand All @@ -532,7 +532,7 @@ const { log } = console;
errors: [
{
line: 3,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand All @@ -541,7 +541,7 @@ const { log } = console;
errors: [
{
line: 1,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand All @@ -562,7 +562,7 @@ class OtherClass extends BaseClass {
{
line: 8,
column: 15,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand All @@ -584,7 +584,7 @@ class OtherClass extends BaseClass {
{
line: 9,
column: 9,
messageId: 'unbound',
messageId: 'unboundWithoutThisAnnotation',
},
],
},
Expand Down