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 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           

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

See prior art: #1783

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.

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.

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"?

@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.