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.

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

feat(eslint-plugin): [class-methods-use-this] add extension rule #6457

Merged
merged 3 commits into from Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions packages/eslint-plugin/TSLINT_RULE_ALTERNATIVES.md
Expand Up @@ -185,7 +185,7 @@ It lists all TSLint rules along side rules from the ESLint ecosystem that are th
| [`one-line`] | 🌟 | [`brace-style`][brace-style] or [Prettier] |
| [`one-variable-per-declaration`] | 🌟 | [`one-var`][one-var] |
| [`ordered-imports`] | 🌓 | [`import/order`] |
| [`prefer-function-over-method`] | 🌟 | [`class-methods-use-this`][class-methods-use-this] |
bradzacher marked this conversation as resolved.
Show resolved Hide resolved
| [`prefer-function-over-method`] | 🌟 | [`@typescript-eslint/class-methods-use-this`] |
| [`prefer-method-signature`] | ✅ | [`@typescript-eslint/method-signature-style`] |
| [`prefer-switch`] | 🛑 | N/A |
| [`prefer-template`] | 🌟 | [`prefer-template`][prefer-template] |
Expand Down Expand Up @@ -566,7 +566,6 @@ Relevant plugins: [`chai-expect-keywords`](https://github.com/gavinaiken/eslint-
[object-shorthand]: https://eslint.org/docs/rules/object-shorthand
[brace-style]: https://eslint.org/docs/rules/brace-style
[one-var]: https://eslint.org/docs/rules/one-var
[class-methods-use-this]: https://eslint.org/docs/rules/class-methods-use-this
[prefer-template]: https://eslint.org/docs/rules/prefer-template
[quotes]: https://eslint.org/docs/rules/quotes
[semi]: https://eslint.org/docs/rules/semi
Expand Down Expand Up @@ -598,6 +597,7 @@ Relevant plugins: [`chai-expect-keywords`](https://github.com/gavinaiken/eslint-
[`@typescript-eslint/await-thenable`]: https://typescript-eslint.io/rules/await-thenable
[`@typescript-eslint/ban-types`]: https://typescript-eslint.io/rules/ban-types
[`@typescript-eslint/ban-ts-comment`]: https://typescript-eslint.io/rules/ban-ts-comment
[`@typescript-eslint/class-methods-use-this`]: https://typescript-eslint.io/rules/class-methods-use-this
[`@typescript-eslint/consistent-type-assertions`]: https://typescript-eslint.io/rules/consistent-type-assertions
[`@typescript-eslint/consistent-type-definitions`]: https://typescript-eslint.io/rules/consistent-type-definitions
[`@typescript-eslint/explicit-member-accessibility`]: https://typescript-eslint.io/rules/explicit-member-accessibility
Expand Down
6 changes: 5 additions & 1 deletion packages/eslint-plugin/docs/rules/TEMPLATE.md
@@ -1,6 +1,10 @@
---
description: '<Description from rule metadata here>'
---

> 🛑 This file is source code, not the primary documentation location! 🛑
>
> See **https://typescript-eslint.io/rules/your-rule-name** for documentation.
> See **https://typescript-eslint.io/rules/RULE_NAME_REPLACEME** for documentation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was just me fleshing out the template
i noticed this was super easy to miss this so I made it more obvious!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it :)

## Examples

Expand Down
57 changes: 57 additions & 0 deletions packages/eslint-plugin/docs/rules/class-methods-use-this.md
@@ -0,0 +1,57 @@
---
description: 'Enforce that class methods utilize `this`.'
---

> 🛑 This file is source code, not the primary documentation location! 🛑
>
> See **https://typescript-eslint.io/rules/class-methods-use-this** for documentation.

## Examples

This rule extends the base [`eslint/class-methods-use-this`](https://eslint.org/docs/rules/class-methods-use-this) rule.
It adds support for ignoring `override` methods or methods on classes that implement an interface.

## Options

This rule adds the following options:

```ts
interface Options extends BaseClassMethodsUseThisOptions {
ignoreOverrideMethods?: boolean;
ignoreClassesThatImplementAnInterface?: boolean;
}

const defaultOptions: Options = {
...baseClassMethodsUseThisOptions,
ignoreOverrideMethods: false,
ignoreClassesThatImplementAnInterface: false,
};
```

### `ignoreOverrideMethods`

Makes the rule to ignores any class member explicitly marked with `override`.

Example of a correct code when `ignoreOverrideMethods` is set to `true`:

```ts
class X {
override method() {}
override property = () => {};
}
```

### `ignoreClassesThatImplementAnInterface`

Makes the rule ignore all class members that are defined within a class that `implements` a type.

It's important to note that this option does not only apply to members defined in the interface as that would require type information.

Example of a correct code when `ignoreClassesThatImplementAnInterface` is set to `true`:

```ts
class X implements Y {
method() {}
property = () => {};
}
```
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/configs/all.ts
Expand Up @@ -19,6 +19,8 @@ export = {
'brace-style': 'off',
'@typescript-eslint/brace-style': 'error',
'@typescript-eslint/class-literal-property-style': 'error',
'class-methods-use-this': 'off',
'@typescript-eslint/class-methods-use-this': 'error',
'comma-dangle': 'off',
'@typescript-eslint/comma-dangle': 'error',
'comma-spacing': 'off',
Expand Down
265 changes: 265 additions & 0 deletions packages/eslint-plugin/src/rules/class-methods-use-this.ts
@@ -0,0 +1,265 @@
import type { TSESTree } from '@typescript-eslint/utils';
import { AST_NODE_TYPES } from '@typescript-eslint/utils';

import * as util from '../util';

type Options = [
{
exceptMethods?: string[];
enforceForClassFields?: boolean;
ignoreOverrideMethods?: boolean;
ignoreClassesThatImplementAnInterface?: boolean;
},
];
type MessageIds = 'missingThis';

export default util.createRule<Options, MessageIds>({
name: 'class-methods-use-this',
meta: {
type: 'suggestion',
docs: {
description: 'Enforce that class methods utilize `this`',
extendsBaseRule: true,
requiresTypeChecking: false,
},
fixable: 'code',
hasSuggestions: false,
schema: [
{
type: 'object',
properties: {
exceptMethods: {
type: 'array',
description:
'Allows specified method names to be ignored with this rule',
items: {
type: 'string',
},
},
enforceForClassFields: {
type: 'boolean',
description:
'Enforces that functions used as instance field initializers utilize `this`',
default: true,
},
ignoreOverrideMethods: {
type: 'boolean',
description: 'Ingore members marked with the `override` modifier',
},
ignoreClassesThatImplementAnInterface: {
type: 'boolean',
description:
'Ignore classes that specifically implement some interface',
},
},
additionalProperties: false,
},
],
messages: {
missingThis: "Expected 'this' to be used by class {{name}}.",
},
},
defaultOptions: [
{
enforceForClassFields: true,
exceptMethods: [],
ignoreClassesThatImplementAnInterface: false,
ignoreOverrideMethods: false,
},
],
create(
context,
[
{
enforceForClassFields,
exceptMethods: exceptMethodsRaw,
ignoreClassesThatImplementAnInterface,
ignoreOverrideMethods,
},
],
) {
const exceptMethods = new Set(exceptMethodsRaw);
type Stack =
| {
member: null;
class: null;
parent: Stack | undefined;
usesThis: boolean;
}
| {
member: TSESTree.MethodDefinition | TSESTree.PropertyDefinition;
class: TSESTree.ClassDeclaration | TSESTree.ClassExpression;
parent: Stack | undefined;
usesThis: boolean;
};
let stack: Stack | undefined;

const sourceCode = context.getSourceCode();

function pushContext(
member?: TSESTree.MethodDefinition | TSESTree.PropertyDefinition,
): void {
if (member?.parent.type === AST_NODE_TYPES.ClassBody) {
stack = {
member,
class: member.parent.parent as
| TSESTree.ClassDeclaration
| TSESTree.ClassExpression,
usesThis: false,
parent: stack,
};
} else {
stack = {
member: null,
class: null,
usesThis: false,
parent: stack,
};
}
}

function enterFunction(
node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression,
): void {
if (
node.parent.type === AST_NODE_TYPES.MethodDefinition ||
node.parent.type === AST_NODE_TYPES.PropertyDefinition
) {
pushContext(node.parent);
} else {
pushContext();
}
}

/**
* Pop `this` used flag from the stack.
*/
function popContext(): Stack | undefined {
const oldStack = stack;
stack = stack?.parent;
return oldStack;
}

/**
* Check if the node is an instance method not excluded by config
*/
function isIncludedInstanceMethod(
node: NonNullable<Stack['member']>,
): node is NonNullable<Stack['member']> {
if (
node.static ||
(node.type === AST_NODE_TYPES.MethodDefinition &&
node.kind === 'constructor') ||
(node.type === AST_NODE_TYPES.PropertyDefinition &&
!enforceForClassFields)
) {
return false;
}

if (node.computed || exceptMethods.size === 0) {
return true;
}

const hashIfNeeded =
node.key.type === AST_NODE_TYPES.PrivateIdentifier ? '#' : '';
const name =
node.key.type === AST_NODE_TYPES.Literal
? util.getStaticStringValue(node.key)
: node.key.name || '';

return !exceptMethods.has(hashIfNeeded + (name ?? ''));
}

/**
* Checks if we are leaving a function that is a method, and reports if 'this' has not been used.
* Static methods and the constructor are exempt.
* Then pops the context off the stack.
*/
function exitFunction(
node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression,
): void {
const stackContext = popContext();
if (
stackContext?.member == null ||
stackContext.class == null ||
stackContext.usesThis ||
(ignoreOverrideMethods && stackContext.member.override) ||
(ignoreClassesThatImplementAnInterface &&
stackContext.class.implements != null)
) {
return;
}

if (isIncludedInstanceMethod(stackContext.member)) {
context.report({
node,
loc: util.getFunctionHeadLoc(node, sourceCode),
messageId: 'missingThis',
data: {
name: util.getFunctionNameWithKind(node),
},
});
}
}

return {
// function declarations have their own `this` context
FunctionDeclaration(): void {
pushContext();

Check warning on line 208 in packages/eslint-plugin/src/rules/class-methods-use-this.ts

View check run for this annotation

Codecov / codecov/patch

packages/eslint-plugin/src/rules/class-methods-use-this.ts#L208

Added line #L208 was not covered by tests
},
'FunctionDeclaration:exit'(): void {
popContext();

Check warning on line 211 in packages/eslint-plugin/src/rules/class-methods-use-this.ts

View check run for this annotation

Codecov / codecov/patch

packages/eslint-plugin/src/rules/class-methods-use-this.ts#L211

Added line #L211 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Testing] Was it intentional that these bits aren't covered by unit tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got all the tests from the base rule - so either it's not covered by the base tests or code cov is wrong again. IDK

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ESLint core isn't testing these lines, who are we to dispute??

},

FunctionExpression(node): void {
enterFunction(node);
},
'FunctionExpression:exit'(node): void {
exitFunction(node);
},
...(enforceForClassFields
? {
'PropertyDefinition > ArrowFunctionExpression.value'(
node: TSESTree.ArrowFunctionExpression,
): void {
enterFunction(node);
},
'PropertyDefinition > ArrowFunctionExpression.value:exit'(
node: TSESTree.ArrowFunctionExpression,
): void {
exitFunction(node);
},
}
: {}),

/*
* Class field value are implicit functions.
*/
'PropertyDefinition > *.key:exit'(): void {
pushContext();
},
'PropertyDefinition:exit'(): void {
popContext();
},

/*
* Class static blocks are implicit functions. They aren't required to use `this`,
* but we have to push context so that it captures any use of `this` in the static block
* separately from enclosing contexts, because static blocks have their own `this` and it
* shouldn't count as used `this` in enclosing contexts.
*/
StaticBlock(): void {
pushContext();
},
'StaticBlock:exit'(): void {
popContext();
},

'ThisExpression, Super'(): void {
if (stack) {
stack.usesThis = true;
}
},
};
},
});
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Expand Up @@ -7,6 +7,7 @@ import banTypes from './ban-types';
import blockSpacing from './block-spacing';
import braceStyle from './brace-style';
import classLiteralPropertyStyle from './class-literal-property-style';
import classMethodsUseThis from './class-methods-use-this';
import commaDangle from './comma-dangle';
import commaSpacing from './comma-spacing';
import consistentGenericConstructors from './consistent-generic-constructors';
Expand Down Expand Up @@ -141,6 +142,7 @@ export default {
'block-spacing': blockSpacing,
'brace-style': braceStyle,
'class-literal-property-style': classLiteralPropertyStyle,
'class-methods-use-this': classMethodsUseThis,
'comma-dangle': commaDangle,
'comma-spacing': commaSpacing,
'consistent-generic-constructors': consistentGenericConstructors,
Expand Down