feat(eslint-plugin): [prefer-return-this-type] add a new rule #3228

Zzzen · 2021-03-26T16:15:20Z

close #889
The implementation is pretty clear, it only warns if:

the return type is written explicitly, and it is simple ( not union or intersection types )
all code paths return this type

TODO:

doc
maybe support PropertyDeclaration initialized with functions like

class Foo {
  f = (): Foo => {
    return this;
  }
}

typescript-eslint · 2021-03-26T16:15:23Z

Thanks for the PR, @Zzzen!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

codecov · 2021-03-26T16:27:18Z

Codecov Report

Merging #3228 (52df4db) into master (15f7184) will increase coverage by 0.87%.
The diff coverage is 91.54%.

@@            Coverage Diff             @@
##           master    #3228      +/-   ##
==========================================
+ Coverage   92.64%   93.51%   +0.87%     
==========================================
  Files         326      147     -179     
  Lines       11253     7848    -3405     
  Branches     3171     2485     -686     
==========================================
- Hits        10425     7339    -3086     
+ Misses        368      162     -206     
+ Partials      460      347     -113

Flag	Coverage Δ
unittest	`93.51% <91.54%> (+0.87%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/eslint-plugin/src/configs/all.ts	`100.00% <ø> (ø)`
...eslint-plugin/src/rules/prefer-return-this-type.ts	`90.47% <90.47%> (ø)`
packages/eslint-plugin/src/util/astUtils.ts	`89.47% <100.00%> (+7.65%)`	⬆️
...ckages/typescript-estree/src/jsx/xhtml-entities.ts
packages/scope-manager/src/lib/es2019.array.ts
packages/typescript-estree/src/ts-estree/index.ts
...es/experimental-utils/src/ts-eslint-scope/Scope.ts
packages/scope-manager/src/lib/esnext.array.ts
...ckages/scope-manager/src/lib/webworker.iterable.ts
packages/scope-manager/src/lib/dom.ts
... and 174 more

…lassProperty

bradzacher · 2021-04-10T19:43:29Z

packages/eslint-plugin/docs/rules/prefer-return-this-type.md

+  f2() {
+    return this;
+  }
+  f3(): Foo | undefined {


why not this | undefined?

packages/eslint-plugin/src/rules/prefer-return-this-type.ts

bradzacher · 2021-04-10T19:43:59Z

packages/eslint-plugin/src/rules/prefer-return-this-type.ts

+      description:
+        'Enforce that `this` is used when only `this` type is returned',
+      category: 'Best Practices',
+      recommended: 'error',


adding to the recommended set is a breaking change

Suggested change

recommended: 'error',

recommended: false,

packages/eslint-plugin/src/rules/prefer-return-this-type.ts

bradzacher · 2021-04-10T20:10:44Z

packages/eslint-plugin/src/rules/prefer-return-this-type.ts

+      'ClassBody > MethodDefinition'(node: TSESTree.MethodDefinition): void {
+        checkFunction(node.value, node.parent!.parent as ClassLikeDeclaration);
+      },
+      'ClassBody > ClassProperty'(node: TSESTree.ClassProperty): void {
+        if (
+          !(
+            node.value?.type === AST_NODE_TYPES.FunctionExpression ||
+            node.value?.type === AST_NODE_TYPES.ArrowFunctionExpression
+          )
+        ) {
+          return;
+        }
+
+        checkFunction(node.value, node.parent!.parent as ClassLikeDeclaration);
+      },


one easier way to do this is to use a stack to track your state.
It saves you doing manual traversal of the tree

interface Stack { classLike: ClassLikeDeclaration, methodLike: FunctionLike | null } const stack: Stack[] = []; function getCurrentStack(): Stack | undefined { return stack[stack.length - 1]; } return { 'ClassDeclaration, ClassExpression:enter'(node: ClassLikeDeclaration) { stack.push({ classLike: node, methodLike: null }); }, 'ClassDeclaration, ClassExpression:exit'(node: ClassLikeDeclaration) { stack.pop(); }, 'ClassBody > MethodDefinition:enter, ClassBody > ClassProperty > :matches(FunctionExpression, ArrowFunctionExpression).value:enter'(node: FunctionLike ) { const current = getCurrentStack(); if (current != null) { current.methodLike = node; } }, 'ClassBody > MethodDefinition, ClassBody > ClassProperty > :matches(FunctionExpression, ArrowFunctionExpression).value:exit'(node: FunctionLike ) { const current = getCurrentStack(); if (current != null) { current.methodLike = null; } }, ReturnStatement(node) { const current = getCurrentStack(); if (current != null && current.methodLike != null) { // check return and report } }, };

Cool, I haven't seen this pattern before, will try it.

This is more complicated than I have expected. We may have functions inside methods and need to track them too.

class Foo { bar() { const f = function() { return this; } } }

bradzacher

a few quick comments - haven't reviewed SUPER deeply yet.

what about abstract methods? Do we want to check those signatures?
what about interfaces? Do we want to check those signatures?

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

Zzzen · 2021-04-14T16:02:31Z

what about abstract methods? Do we want to check those signatures?
what about interfaces? Do we want to check those signatures?

Showerthought: we don't need to check them because this doesn't make a difference in interfaces/methods. Eg. Playgroud

interface IAnimalFixed {
    move(): IAnimalFixed;
}

interface IAnimalPolymophic {
    move(): this;
}

class CatFixed implements IAnimalFixed {
    // return type will be checked by this rule
    move(): this {
        return this;
    }

    meow(): this {
        return this;
    }
}

class CatPolymophic implements IAnimalPolymophic {
    move(): this {
        return this;
    }

    meow(): this {
        return this;
    }
}

function useInterface(fixed: IAnimalFixed, polymophic: IAnimalPolymophic) {
    // return type: IAnimalFixed
    fixed.move();

    // return type: IAnimalFixed
    polymophic.move();
}

function useImplementation(fixed: CatFixed, polymophic: CatPolymophic) {
    // return type: CatFixed
    fixed.move();

    // return type: CatPolymophic
    polymophic.move();
}

bradzacher · 2021-05-15T18:35:21Z

packages/eslint-plugin/src/configs/recommended-requiring-type-checking.ts

@@ -17,6 +17,7 @@ export = {
    '@typescript-eslint/no-unsafe-member-access': 'error',
    '@typescript-eslint/no-unsafe-return': 'error',
    '@typescript-eslint/prefer-regexp-exec': 'error',
+    '@typescript-eslint/prefer-return-this-type': 'error',


you need to regenerate the configs

bradzacher · 2021-05-15T18:36:00Z

packages/eslint-plugin/README.md

@@ -163,6 +163,7 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int
 | [`@typescript-eslint/prefer-readonly-parameter-types`](./docs/rules/prefer-readonly-parameter-types.md)               | Requires that function parameters are typed as readonly to prevent accidental mutation of inputs                        |                    |          | :thought_balloon: |
 | [`@typescript-eslint/prefer-reduce-type-parameter`](./docs/rules/prefer-reduce-type-parameter.md)                     | Prefer using type parameter when calling `Array#reduce` instead of casting                                              |                    | :wrench: | :thought_balloon: |
 | [`@typescript-eslint/prefer-regexp-exec`](./docs/rules/prefer-regexp-exec.md)                                         | Enforce that `RegExp#exec` is used instead of `String#match` if no global flag is provided                              | :white_check_mark: | :wrench: | :thought_balloon: |
+| [`@typescript-eslint/prefer-return-this-type`](./docs/rules/prefer-return-this-type.md)                               | Enforce that `this` is used when only `this` type is returned                                                           | :heavy_check_mark: | :wrench: | :thought_balloon: |


you need to regen the docs to remove the recommended tick

…nfigs

Zzzen · 2021-05-16T13:51:58Z

Why is CI not running?

nx-cloud · 2021-07-31T20:30:33Z

Nx Cloud Report

CI ran the following commands for commit 52df4db. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status	Command
	`nx run-many --target=build --all --parallel`
	`nx run-many --target=typecheck --all --parallel`
	`nx test @typescript-eslint/eslint-plugin`
	`nx test @typescript-eslint/eslint-plugin-internal`
	`nx test @typescript-eslint/eslint-plugin-tslint`
	`nx test @typescript-eslint/experimental-utils`
	`nx test @typescript-eslint/parser`
	`nx test @typescript-eslint/scope-manager`
	`nx test @typescript-eslint/typescript-estree`
	`nx test @typescript-eslint/visitor-keys`

Sent with 💌 from NxCloud.

bradzacher

LGTM - this is enough coverage for a v1.
We can always add more cases later.
Thanks for your contribution!

Zzzen added 2 commits March 26, 2021 13:43

feat(eslint-plugin): [prefer-return-this-type] prototype

ee36cf9

feat(eslint-plugin): [prefer-return-this-type] update tests and doc

d4d5a0a

bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Mar 26, 2021

Zzzen added 3 commits March 28, 2021 21:36

feat(eslint-plugin): [prefer-return-this-type] add tests and handle C…

698a787

…lassProperty

feat(eslint-plugin): [prefer-return-this-type] fix arrow function

59af3dd

feat(eslint-plugin): [prefer-return-this-type] add test

b3a5230

bradzacher marked this pull request as draft March 28, 2021 20:50

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Mar 28, 2021

feat(eslint-plugin): [prefer-return-this-type] update doc and test

8131158

Zzzen marked this pull request as ready for review April 5, 2021 16:17

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 7, 2021

bradzacher reviewed Apr 10, 2021

View reviewed changes

packages/eslint-plugin/src/rules/prefer-return-this-type.ts Outdated Show resolved Hide resolved

bradzacher reviewed Apr 10, 2021

View reviewed changes

packages/eslint-plugin/src/rules/prefer-return-this-type.ts Outdated Show resolved Hide resolved

bradzacher reviewed Apr 10, 2021

View reviewed changes

bradzacher requested changes Apr 10, 2021

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Apr 10, 2021

Apply suggestions from code review

31ef41e

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

Zzzen added 2 commits April 19, 2021 23:46

feat(eslint-plugin): [prefer-return-this-type] error on more cases

059f7f4

Merge remote-tracking branch 'origin/master' into this-type

e400889

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 19, 2021

bradzacher requested changes May 15, 2021

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label May 15, 2021

feat(eslint-plugin): [prefer-return-this-type] regenerate docs and co…

d19f340

…nfigs

Zzzen force-pushed the this-type branch from 2631fb3 to d19f340 Compare May 16, 2021 13:47

feat(eslint-plugin): [prefer-return-this-typetrigger CI

a84ef21

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label May 24, 2021

Merge branch 'master' into this-type

52df4db

bradzacher approved these changes Jul 31, 2021

View reviewed changes

bradzacher enabled auto-merge (squash) July 31, 2021 20:31

bradzacher added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Jul 31, 2021

bradzacher disabled auto-merge July 31, 2021 20:38

bradzacher merged commit 5e1a615 into typescript-eslint:master Jul 31, 2021

github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [prefer-return-this-type] add a new rule #3228

feat(eslint-plugin): [prefer-return-this-type] add a new rule #3228

Zzzen commented Mar 26, 2021 •

edited

typescript-eslint bot commented Mar 26, 2021

codecov bot commented Mar 26, 2021 •

edited

bradzacher Apr 10, 2021

Zzzen Apr 19, 2021

bradzacher Apr 10, 2021

bradzacher Apr 10, 2021

Zzzen Apr 11, 2021

Zzzen Apr 19, 2021

bradzacher left a comment

Zzzen commented Apr 14, 2021

bradzacher May 15, 2021

bradzacher May 15, 2021

Zzzen commented May 16, 2021

nx-cloud bot commented Jul 31, 2021 •

edited

bradzacher left a comment

feat(eslint-plugin): [prefer-return-this-type] add a new rule #3228

feat(eslint-plugin): [prefer-return-this-type] add a new rule #3228

Conversation

Zzzen commented Mar 26, 2021 • edited

typescript-eslint bot commented Mar 26, 2021

codecov bot commented Mar 26, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

Zzzen commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zzzen commented May 16, 2021

nx-cloud bot commented Jul 31, 2021 • edited

Nx Cloud Report

bradzacher left a comment

Choose a reason for hiding this comment

Zzzen commented Mar 26, 2021 •

edited

codecov bot commented Mar 26, 2021 •

edited

nx-cloud bot commented Jul 31, 2021 •

edited