[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: refined-github/refined-github@b8fb6ee

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.