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

Rule proposal: enforce polymorphic this types #889

Closed
Zzzen opened this issue Aug 20, 2019 · 7 comments · Fixed by #3228
Closed

Rule proposal: enforce polymorphic this types #889

Zzzen opened this issue Aug 20, 2019 · 7 comments · Fixed by #3228
Labels
enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@Zzzen
Copy link
Contributor

Zzzen commented Aug 20, 2019

For those who may have not heard it, here's the doc.

The main point is that if a function always returns this, it should be typed as this explicitly or have implicit this type.

Realworld example that breaks this rule:
https://github.com/BabylonJS/Babylon.js/blob/master/src/Meshes/transformNode.ts#L396

@Zzzen Zzzen added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Aug 20, 2019
@bradzacher bradzacher added enhancement: new plugin rule New rule request for eslint-plugin and removed triage Waiting for maintainers to take a look labels Aug 20, 2019
@armano2 armano2 added the good first issue Good for newcomers label Jan 19, 2020
@armano2
Copy link
Member

armano2 commented Jan 19, 2020

This rule should not be hard to implement (and should not require type checking)

To implement this rule we should check all codepaths returns from method and validate if they return this and do so only if rule already had return type.

@Zzzen
Copy link
Contributor Author

Zzzen commented Jan 29, 2020

It's easy to find all return statements of a method and then validate. But not all codepath returns, how do we check if all codepath returns? @armano2

@anikethsaha
Copy link
Contributor

what's the status of this ? is anyone working in it?

@bradzacher bradzacher removed the good first issue Good for newcomers label Apr 6, 2020
@bradzacher
Copy link
Member

Nope, but I suspect that this isn't quite as simple as described.

a this return type doesn't have to specifically be derived from return this, it could also be derived from a case like the following:

class Foo {
  returnsThis1(): this {
    return this;
  }
  returnsThis2(): this {
    return this.returnsThis1();
  }
}

So simple code-path analysis won't work here. It will require type information, and be a decent chunk more complex to implement.


As an aside - a good place to look for issues to work on would be this search which sorts issues by the number of reactions (a good metric for how "popular"/"needed" an issue is):
https://github.com/typescript-eslint/typescript-eslint/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22+sort%3Areactions-desc+

@anikethsaha
Copy link
Contributor

class Foo {
returnsThis1(): this {
return this;
}
returnsThis2(): this {
return this.returnsThis1();
}
}

I actually planned to crawl the definition for the function in this case as a recursive as that function might be returning something else or this class's instances.
it did turn out to be a bit of complex.
I am thinking of reference tracker but not sure whether it would be compatible for ts !!

As an aside - a good place to look for issues to work on would be this search which sorts issues by the number of reactions (a good metric for how "popular"/"needed" an issue is):
https://github.com/typescript-eslint/typescript-eslint/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22+sort%3Areactions-desc+

on it 👍

@bradzacher
Copy link
Member

The problem with crawling definitions is that you don't always have direct access to them - eg a method might exist on the parent.

This will require type information to assess.

@anikethsaha
Copy link
Contributor

yea agreed 💯

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants