[method-signature-style] Shouldn't affect methods that use `this`

[fregante]

Repro

{
  "rules": {
    "@typescript-eslint/method-signature-style": ["error"]
  }
}

interface Foo {
	cloneThis(): this; // ❌ reported
	cloneThis: () => this; //  🤔 fixed to this
}

Expected Result

Methods should not be affected, this should not be autofixed:

interface Foo {
	cloneThis(): this;
}

Actual Result

That type is changed to an arrow function, which doesn't have this

interface Foo {
	cloneThis: () => this; //  Fixed to this, broken
}

Additional Info

It causes errors elsewhere in the code

Versions

package	version
@typescript-eslint/eslint-plugin	2.27.0
@typescript-eslint/parser	2.27.0
TypeScript	3.8.3
ESLint	6.8.0
node	13.8.0
npm	6.13.6

[bradzacher]

Could you please show me an example of the code that it breaks?

Looking at the types that TS generates, it looks like it works fine:

interface Foo {
	cloneThis1(): this;
	cloneThis2: () => this;
}

declare const x: Foo;
const one = x.cloneThis1(); // typeof === Foo
const two = x.cloneThis2(); // typeof === Foo

repl

[fregante]

There are a few errors here: https://github.com/sindresorhus/refined-github/runs/655021611#step:4:19

And this is the commit that fixed it: https://github.com/sindresorhus/refined-github/pull/3072/commits/b8fb6ee2a534b7badc437cf416da46fb8ad26af2

I guess it breaks extended classes:

interface Node extends EventTarget {
	cloneNode: (deep?: boolean) => this;
}

[fregante]

In the Repl that last example fails (however this is not what I saw before)

This passes:

interface Node extends EventTarget {
	cloneNode(deep?: boolean): this;
}

[bradzacher]

Hmm - this is weird. I'm not sure why TS treats the two differently.

In essence the two signatures are actually the same - both define a function that returns this. Whilst it looks like an arrow function - that syntax is just a coincidence, and the function type has no relation to an arrow function whatsoever.

I suspect that this is either:

• a bug in TS
• an intentional difference due to the difference in variance of the two signatures

Unsure. I'd need to do more investigation (I always forget how variance works exactly)

[DuncanWalter]

Bringing this back from the dead- I do think there's a missing, very useful feature here. The weirdness is caused by an intentional difference in the strictness of assignments between methods and properties

interface WithMethod {
  getName(animal: Dog): string
}

interface WithProperty {
  getName: (animal: Dog) => string
}

declare const withMethod: WithMethod
declare const withProperty: WithProperty

const arbitraryAssignment: {
  getName (animal: Animal) => string
} = withMethod // doesn't throw, but does with withProperty. The second behavior is better

In TS it is also possible to express function signatures with restrictions on the this type:

interface Dog {
  name: string
  getName(this: Dog): string
}

I think that it would be useful to allow the method syntax for functions that specify a this type in the parameters slot even when the rule is configured to only allow property signatures more broadly. There are cases where I might want to define types for method-like behavior even if in general property-style signatures give stricter checks

[javiertury]

Perhaps method-signature-style fixer and documentation should be updated to use method: (this: this, ...input) => Output as a replacement of method(...input): Output. I think it provides additional safety against unbound method calls and incompatible this bindings.

Example

interface Foo {
  cloneThis(): this;
  foo(input: 'foo'): Output;
}

would be fixed to

interface Foo {
  cloneThis: (this: this) => this;
  foo: (this: this, input: 'foo') => Output;
}

[Josh-Cena]

So, what is the fix exactly? Do we exempt method signatures involving this, or do we fix to an explicit this: this parameter? Honestly the latter looks very artificial and not what most people would write.

[bradzacher]

or do we fix to an explicit this: this parameter

Definitely not - 100% no. Adding a this parameter essentially makes a function generic and that comes at a performance cost. It's also far, far, far, far, FAR from idiomatic TS to write that.

Do we exempt method signatures involving this

I think if we were going to do anything - this would be what we would do.

---

It's also worth noting that it's possible to manually fix a lot of cases to use the name of the type instead of this - but that does break some usecases so it couldn't be an autofix.

interface Foo {
  methodFoo(): Foo;
  methodThis(): this;
}
interface Bar extends Foo {
}

declare const x: Bar;
x.methodFoo(); // === Foo
x.methodThis(); // === Bar

I say "manually fix" because this specific breakage relies upon (a) using this as a return type and (b) subtyping the type - which is a usecase but it's rarer - hence people could manually fix many cases from method(): this to method: () => Foo.

[Josh-Cena]

Okay; do we still report it and just not autofix, or should we consider it a valid pattern? It is still unsound though!

[fregante]

If this is intentional behavior by typescript (which it appears to be), then I don't see why it should be reported. It you report it, I'll fix it, have it fail (in my project or in a dependent) and then revert it.

A linter should not suggest to change the behavior of a piece of code that is correct (according to the compiler)

[bradzacher]

It should ignore it. My note aboht manual fix was just a side-note about how one might fix it. Things are complicated to fix so we would probably just be better to avoid it.

People can ban the this type if they want but that's orthogonal to this issue.

[bradzacher]

A linter should not suggest to change the behavior of a piece of code that is correct (according to the compiler)

@fregante this is an incorrect take.

foo!!!!!!!!!!!!

This code is correct according to the compiler. But clearly it's bad code. Should a rule (a) ignore it or (b) report and fix it to foo!

What about if the code is this:

const foo = 1;
foo!.toString();

Again this is fine according to TS - it doesn't care if you've got provably useless code. But the correct code is to drop the useless non-null assertion.

This same thing can be extrapolated to all of our rules really. Just because TS thinks something is correct doesn't mean it's correct or safe code. The point of the linter is to augment TS's checks - provide extra checks that TS doesn't do.

Based on our analysis we can find and flag code that looks wrong even if the types say there is no issue. We can also suggest patterns which are more safe and work around unsoundness in TS.

The latter is what this rule does - the variance of method signatures is bad and function types are generally safer.

[fregante]

A linter should not suggest to change the behavior of a piece of code that is correct (according to the compiler)

@fregante this is an incorrect take.

Sorry I should have finished the sentence, and I stand by it.

A linter should not suggest to change the behavior of a piece of code that is correct (according to the compiler) to something that breaks it.

The second part was implied because this is what's happening here. The plugin is breaking the code by suggesting that it's not correct, while it is (because the alternative is not)

Also in this case it's super hard to argue that this style fix should affect the behavior.

Merry Christmas! 🤗

[bradzacher]

I still don't fully agree - just because a suggested fix may break the code doesn't mean it's the wrong thing to do.

For example suggesting someone doesn't use any is a good thing and more often than not will lead to much better code - even if the immediate act of removing the any maybe cause type errors.

Instead if you change your statement to say "A linter should not autofix a piece of code that is correct (according to the compiler) to something that breaks it." - then I fully agree! Autofixes shouldn't break the build.

In this instance the suggestion isn't necessarily wrong to use a function type as its got a safer variance and (barring the subtype example I mentioned earlier) there is an alternative that can be used without introducing an error. But it's quite hard for us to detect the subtype case thus in this instance it's probably better if we just ingore this case.

[kirkwaiblinger]

So, is this actionable? My interpretation of the conversation so far is: With the "property" option enabled, when encountering method signatures that make use of the this type (except possibly if there is a this: this parameter?), either do not flag them, or do flag them, but change the autofix to a suggestion? Is that correct?

[bradzacher]

The action would be:

1. If the signature includes a this parameter - do not fix it (but still report).
2. Perhaps update the error message in this case to describe the edge case?
3. Document the edge case in the rule doc.