feat(eslint-plugin): add no-implicit-this-methods rule

[Retsam]

Copying description from the readme here for convenience. This rule closes a class of potential runtime errors when dealing with this keywords.

No implicit this type for methods

This rule requires that all class and object methods that use the this keyword must explicitly specify the this type, so that the typescript compiler will ensure that the function is always called with the right this. For example:

class Example {
  value = 'val';
  implicit() {
    return this.value.toUpperCase();
  }
  explicit(this: this) {
    return this.value.toUpperCase();
  }
}
const implicit = new Example().implicit;
implicit(); // Runtime error, but not a compiler error
const explicit = new Example().explicit;
explicit(); // Compile error

The compiler has a built-in --noImplicitThis option, but it allows this to be implicit in classes and object methods.

[typescript-eslint]

Thanks for the PR, @Retsam!

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.

[glen-84]

Adding this: this to every method of every class seems like a lot of effort/noise for potentially not a lot of benefit. I don't think that I'd use this rule myself.

I feel like maybe such rules should start as suggestion issues, to gather feedback/upvotes from the community before implementation. There are a certain set of rules that are perhaps too specific for general usage, and should maybe be included in separate packages.

[Retsam]

@glen-84 It may be worth clarifying that it's not every method: it's only methods that use this that aren't already written as arrow function properties - at least in our codebase that left relatively few methods where this rule applied; (though admittedly we widely use arrow functions properties because we've been bitten by the this in the past).

I'd defend this rule's inclusion in the main typescript-eslint package, as it closes one of the few sources of runtime errors I've found in strict Typescript - but I'm fine with moving this to a suggestion issue and gathering feedback in the meantime. (I don't feel the time spent implementing it is wasted, if it doesn't get used here I'll likely move the code elsewhere)

[glen-84]

... it's only methods that use this that aren't already written as arrow function properties

That's most methods in the codebases that I've worked on.

---

Doesn't the unbound-method rule cover this? (if you ignore the fairly large number of issues)

I prefer the idea of flagging unbound methods and fixing only those cases, as opposed to requiring all methods to be declared this way, even if they're never called without the correct this.

Anyway, this is just my opinion, other developers may also want this rule.

[codecov]

Codecov Report

Merging #1279 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1279      +/-   ##
==========================================
- Coverage   94.06%   94.03%   -0.03%     
==========================================
  Files         131      132       +1     
  Lines        5745     5819      +74     
  Branches     1617     1645      +28     
==========================================
+ Hits         5404     5472      +68     
- Misses        183      186       +3     
- Partials      158      161       +3

Impacted Files	Coverage Δ
...slint-plugin/src/rules/no-implicit-this-methods.ts	100% <100%> (ø)
packages/eslint-plugin/src/rules/index.ts	100% <100%> (ø)	⬆️
...s/eslint-plugin/src/rules/no-non-null-assertion.ts	84.37% <0%> (-15.63%)	⬇️
packages/eslint-plugin/src/util/astUtils.ts	92.85% <0%> (-7.15%)	⬇️
...slint-plugin/src/rules/no-unnecessary-condition.ts	100% <0%> (ø)	⬆️
...s/eslint-plugin/src/rules/no-unused-expressions.ts	100% <0%> (ø)	⬆️
...lint-plugin/src/rules/prefer-nullish-coalescing.ts	93.61% <0%> (ø)	⬆️

[Retsam]

This does do fairly similar things to unbound-method, but I think this method is a little more reliable: it insists on a little more noise when declaring methods, but there won't be false positives on usage, and it doesn't rely on type information - it just enforces the code is written in such a way that the typescript compiler will do the right checks.

Would it change opinions at all if this rule had an auto-fixer? That would make it much easier for existing codebases to adopt this change.

[glen-84]

Would it change opinions at all if this rule had an auto-fixer?

Not for me personally.

[bradzacher]

I didn't realise that typescript has such good support for this: this. (a few more cool examples)

const explicit = new Example().explicit;
// typeof explicit === (this: Example) => string

const boundExplicit = explicit.bind(new Example());
// typeof boundExplicit === () => string

However, there is one case that this feature does not catch: passing a method with a this: this to a function will not trigger any errors.
This is by far the most common cause of bugs due to unbound this. eg:

• Passing unbound methods as a callback to things like window.addEventListener
• Passing unbound methods down the react tree as JSX props.

function acceptsStringFn(fn: () => string): string {
  return fn();
}

// runtime errors for both, but no compile-time errors
acceptsStringFn(new Example().explicit);
const x = <div onClick={new Example().explicit} />

On the other hand, the unbound-method rule catches this.
Which leads me to the question - why would I use this rule instead of unbound-method?

---

I've always been of the opinion that you should use arrow functions for class methods using this, so that the this is implicitly bound correctly always.

Out of curiosity - may I ask why you use methods instead of arrow functions for this case?

[bradzacher]

(RC for now to remove from queue whilst questions are answered)

[glen-84]

@bradzacher,

I think your questions have already been answered (?).

Which leads me to the question - why would I use this rule instead of unbound-method?

This does do fairly similar things to unbound-method, but I think this method is a little more reliable: it insists on a little more noise when declaring methods, but there won't be false positives on usage, and it doesn't rely on type information - it just enforces the code is written in such a way that the typescript compiler will do the right checks.

---

Out of curiosity - may I ask why you use methods instead of arrow functions for this case?

It may be worth clarifying that it's not every method: it's only methods that use this that aren't already written as arrow function properties - at least in our codebase that left relatively few methods where this rule applied; (though admittedly we widely use arrow functions properties because we've been bitten by the this in the past).

[bradzacher]

Kind of does.

The latter one is still open - you mention you had "relatively few methods where this rule applied", what were those cases and why couldn't they be arrow functions instead?

---

The former, I get not wanting to use type information, but I don't think this is an amazing alternative, due to the "pass as argument" hole I mentioned earlier.

Personally, I'd rather false positives which I can assess and silence, rather than false negatives which are missed and cause production bugs.

I wonder if a better rule would be - "always use an arrow function if referencing this"

[Retsam]

Yeah, after playing with this rule a bit more, I'm not satisfied with it myself. I also discovered the "pass as argument" hole, and found a few annoying edge cases. (e.g. it's a little non-trivial to annotate the type of this when dealing with generic classes)

Our codebase has a few places where we've specifically avoided arrow functions for memory reasons - places where we create thousands of instances and the memory pressure of each instance having its own copies of every method was noticeable, compared to a single shared instance on the prototype - but that's probably an anomaly.

I'm still not a huge fan of unbound-method's solution either; but I don't think this approach panned out, either.

Thanks for the discussion and the feedback!

[OliverJAsh]

passing a method with a this: this to a function will not trigger any errors

Note that if you define this: void on the receiver then you will get an error:

function acceptsStringFn(fn: (this: void) => string): string {
  return fn();
}

// runtime *and* compile-time errors
acceptsStringFn(new Example().explicit);

But I think it's a very big ask that:

1. all class methods must have this: this and
2. all higher-order functions that may receive class methods as a parameter must have this: void.

Ideally TypeScript would infer this: this for class methods (microsoft/TypeScript#36100, microsoft/TypeScript#35451), but that would only get half us of the way—we'd still need to add this: void to "receivers".

[bradzacher]

That's interesting, I haven't ever used the this argument, so I didn't know you could explicitly declare no this context.

I wouldn't completely be opposed to a rule which enforces that every single function includes a this argument.

I'm not sure if people would use it (because it's a weird thing to require), but it seems like it could be a pretty useful rule if it had a fixer for all cases.

[Retsam]

Ah, I didn't realize that either. I think you've got an error in the example, though. I think it's supposed to be

function acceptsStringFn(fn: (this: void) => string): string

not

function acceptsStringFn(this: void, fn: () => string): string

(Which means not every function needs this for type-safety: just all higher-order functions).

---

I guess in theory, a slightly more useful version of this linter rule would also check that higher order functions specify this: void?

Though, in my experience the biggest mistake in terms of this is .then, which would require mucking with DOM types. I wonder if this: void introduces any performance penalties in the way that this: this does, if not, perhaps the typescript team could be convinced to add this: void to stuff like Promise#then.

(Also, sorry, I guess I could have saved you some time if I had linked #microsoft/TypeScript#35451 here)

[OliverJAsh]

I wonder if this: void introduces any performance penalties in the way that this: this does, if not, perhaps the typescript team could be convinced to add this: void to stuff like Promise#then.

I was also curious about this. If there is no this type, they could default to void 🤔 It might be worth filing an issue to see what they think?