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

[tadhgmister]

Fixes #1756

This isn't quite ready yet, it is missing update to docs and a few cases described below. Just need some feedback for how to handle some edge cases.

When this is used in a call signature of an interface the auto-fix will replace the occurrences of this with the name of the interface so the suggested fix is still valid. However: If the interface defines type parameters and uses this then the necessary replacement becomes much more tricky, for example:

interface Foo<A extends string, B = number> {
  (a: A, b: B): this;
}
// correct replacement would be:
type Foo<A extends string, B = number> = (a: A, b: B) => Foo<A,B>;

The current handling for generics can get away with just coping the generics declaration without knowing how to reconstruct Foo<A,B> as the valid reference to it's own interface name, so handling this would take some additional effort that I personally don't think is really worth it. (Also I don't trust myself to produce code to do it that is bug free)

Right now my implementation never reports on an interface with both type parameters and refers to this but I don't think that is the best course of action, I'd appreciate some advice on what the best course of action would be in this case.

The second part is that I think the intent was to offer a suggestion instead of auto fix when this is used since technically the suggested fix isn't strictly equivalent. I'd like some advice on whether the message should be the same or if there is a different message that should show.

Thank you! 😄

[typescript-eslint]

Thanks for the PR, @tadhgmister!

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]

Codecov Report

Merging #2437 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2437   +/-   ##
=======================================
  Coverage   92.81%   92.82%           
=======================================
  Files         290      290           
  Lines        9453     9466   +13     
  Branches     2647     2650    +3     
=======================================
+ Hits         8774     8787   +13     
  Misses        322      322           
  Partials      357      357           

Flag	Coverage Δ
#unittest	92.82% <ø> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
...es/eslint-plugin/src/rules/prefer-function-type.ts	96.29% <ø> (+1.17%)	⬆️

[bradzacher]

See prior art: #1783

[tadhgmister]

second question, the coverage went down slightly because tsThisTypes?.push and tsThisTypes ?? undefined is only hit in cases where tsThisTypes is an actual list so the optional part is never checked, I assume that is fine? If there is something different I should do that would also be welcome feedback.

[bradzacher]

Suggestion fixers are a good alternative for risky fixers, but here I would probably err on the side of "fix it properly or not at all". We don't want to provide a suggestion that's slightly wrong and then break all of their types.

Note that we don't have to provide a fixer for all cases.
If it's too complicated to produce valid code automatically then it's perfectly fine for us to just report without a fixer.
The user can always manually fix up their code. ESP in this sort of case where it's already pretty advanced TS type - the user should be well-versed enough to be able to manually convert it.

---

codecov doesn't handle optional chaining well. The test coverage is just a guide, if you can cover all of the branches, fine, if there are cases that can't be hit, then feel free to ignore. I will make a judgement when merging to decide if the tests are good enough.

[tadhgmister]

I have thought about this more thoroughly and have come to the conclusion that using this doesn't make any sense here. It will refer to the function type which doesn't seem like a useful construct, when people do return this in javascript to do method chaining it's rarely

I think it'd be likely that if people wanted to write a mixin-style method (independent of class but uses this) they would want to use a generic this parameter instead of referring to the this of the function itself.

function logArg<Self>(this: Self, arg: string): Self {
  console.log(`logging ${arg} from ${this.toString()}`);
  return this;
}
const thing = {logArg, data: "hello"};
// the function returns `thing` in this case, not the `logArg` function:
const data = thing.logArg("logged to console").data;

this is a useful construct, and in this case using type MemberMixin = <Self>(this: Self, ...) => Self would be the correct way to do it, I can absolutely see someone accidentally ending up with just using the this type and be surprised when it refers to the function.

So I think if there is an interface that defines only a call signature and uses this they should get an error message like:

this refers to the function type {{ interfaceName }}, did you intend to use a generic this parameter like <Self>(this: Self, ...) => Self instead?

(Offer no suggestion or fix in this case, it represents a semantic change that is application specific, I imagine every application would have some constraint on the Self type for the method to be useful.)

This will probably help people move towards what they almost certainly intended in the first place, and if they do replace all occurrences of this then the current implementation of the auto fix is absolutely applicable.

Does this seem reasonable?

---

I'm also not sure how to approach this in the docs, is an update needed to mention this or does it fall under "obscure enough edge case we don't have to publicize it"?

[tadhgmister]

@bradzacher I think it's ready now, still not really sure if or how to mention this in the docs, if you think something should be added please tell me and I'll come up with something, otherwise this should be good to go.

Thanks.

[bradzacher]

logic mostly LGTM.
One bug that needs to be fixed.

thanks for your work.

---

Responses to your questions:

So I think if there is an interface that defines only a call signature and uses this they should get an error message like:
.....
Does this seem reasonable?

Yup, that seems perfectly reasonable to me. I'm all for having lint rules clearly educate users about how TS actually works (see ban-types where I added some education that people constantly butt up against).

I'm also not sure how to approach this in the docs, is an update needed to mention this or does it fall under "obscure enough edge case we don't have to publicize it"?

I would probably just add it as a examples and add some comments to it.
I don't see a problem of clearly educating people ahead of time as well. I.e.

---

Examples of incorrect code for this rule:

<snip>

interface Foo {
  (arg: number): this | undefined;
}

interface Foo {
  (arg: this): void;
}

Examples of correct code for this rule:

<snip>

type Foo<TSelf> = (this: TSelf, arg: number) => TSelf | undefined;

type Foo<TSelf> = (this: TSelf, arg: TSelf) => void

[bradzacher]

LGTM - thanks for your contribution!