The compiler should error when unbinding a method

[felixfbecker]

Search Terms

unbound method bind

Suggestion

When referencing a method like this:

const subject = new Subject()
<input onChange={subject.next} />

the compiler should error.

I imagine this would work something like that the this type of any method is its class by default, i.e. next(this: Subject) here.

Assigning or passing a method somewhere means that at runtime the function is "casted" to next(this: undefined), therefor the compiler should check if that "cast" is valid by checking if Subject is assignable to undefined, and emitting an error.

To make the error go away, one would have to bind it:

const subject = new Subject()
<input onChange={bindThis(subject.next)} />

A simplified bindThis() that only takes care of this and does no partial application would be defined as (with tuple types):

function bindThis<A, R>(fn: (...args: A) => R): (this: never, ...args: A) => R

An arrow function's this type would always be never, so wrapping it works too.

We could then also type lodash's bindAll():

function bindAll(obj: T): { [K in keyof T]: T extends Function ? ((this: never, ...args: Args<T[K]>): Return<T[K]>) : T[K] }

const subject = bindAll(new Subject())
<input onChange={subject.next} />

(the method whitelist would be easy to type too)

Use Cases

Currently when using RxJS in React components, its extremely cumbersome to convert callback or lifecycle events to Subjects. You end up with writing a bound version for every next() function of every Subject:

    private mouseMoves = new Subject<React.MouseEvent<HTMLElement>>()
    private nextMouseMove = (event: React.MouseEvent<HTMLElement>) => this.mouseMoves.next(event)

    private clicks = new Subject<React.MouseEvent<HTMLElement>>()
    private nextClick = (event: React.MouseEvent<HTMLElement>) => this.clicks.next(event)

just so you can e.g. pass nextClick to onClick in the render function. If I also need to call error, I need to bind that too.
There are even packages like https://www.npmjs.com/package/bound-subject-decorator to solve it, but there is no way to make this type safe in a way that the compiler will tell you when you forgot to use it.
With this proposal, I could just wrap the subject in bindAll().

TSLint has a no-unbound-method rule but it is not very smart. This would better come from the type checker itself.

Checklist

My suggestion meets these guidelines:

• This wouldn't be a breaking change in existing TypeScript / JavaScript code (it could be a flag)
• This wouldn't change the runtime behavior of existing JavaScript code
• This could be implemented without emitting different JS based on the types of the expressions
• This isn't a runtime feature (e.g. new expression-level syntax)

[RyanCavanaugh]

Duplicate #15

You can do this today by annotating sufficiently:

class A {
    x = 3;
    foo(this: A) {
        this.x = 10;
    }
}

function f(y: (this: void) => void) { }

const a = new A();
// Error
f(a.foo);

[felixfbecker]

Didn't find that issue - thanks. But that issue was closed with the introduction of the this type - this proposal acknowledges the existence of the this type and proposes stricter type checking.

So far the this type is really only useful to type what kind of this type is provided to a callback from a function, but not that useful in practice to validate what this type is accepted (specifically, not accepting undefined).

Yes, you can annotate it like that, but its not feasible to do so for every method. When the types come from a library you usually don't have control over it.

I propose that the containing class/interface becomes the this type by default (maybe when enabling a flag like strictMethodThisTypes).
Do you see any situation in which this would not be correct?

Casting can always be used as an escape hatch.

[RyanCavanaugh]

We tried to detect this globally, but the performance implications were too much (> 15% hit to compilations that use classes at all).

There are also more subtle design issues that are apparent at first glance -- many class methods don't actually use this and can be freely used without this capture, for example. Or a class in a .d.ts file actually has this bound (via an arrow function or .bind or just not being a prototypal class) and its methods were commonly used in apparently-unbound contexts without actually being a runtime error.

Between the perf hit and the imprecision, we decided the current solution of opting-in with annotation was the best compromise.

[felixfbecker]

Okay, didn't know this would have perf implications. Curious, what makes it so complex? What part do you mean with "detect this globally"? Checking for methods that are assigned/passed and therefor unbound?

many class methods don't actually use this and can be freely used without this capture

I would argue you still shouldn't unbind it. Just because a method doesn't use this right now doesn't mean it won't use this in the next minor or patch release, and I would not consider that a breaking change - whether a method uses this is an internal implementation detail of the method a consumer shouldn't rely on.

Or a class in a .d.ts file actually has this bound (via an arrow function or .bind or just not being a prototypal class) and its methods were commonly used in apparently-unbound contexts without actually being a runtime error.

In that case the types for that method could explicitly declare that by using this: any or arrow function syntax.

Between the perf hit and the imprecision, we decided the current solution of opting-in with annotation was the best compromise.

I have no problem with it being opt-in, but I currently don't see a practical way to opt in as a user. The only ones who can opt-in are library/types authors.

There is some mismatch right now because even when not explicitely declaring this for a method, TypeScript will tell you that this inside the method is the type of the containing class - whereas if it truly does not prevent unbinding the method if the this type is not given this should really be type unknown inside the method.

But imo it makes way more sense to have a mode where it makes this the type of the containing class (both inside of the method and when using the method), cause that is the 99% case, than to default all this types to unknown (it would always be possible to explicitely make it unknown).

[mhegazy]

#10288 tracks this now.

[felixfbecker]

So I just tried to hack around in TSLint's no-unbound-this rule a bit - my goal was to find out if it could detect bound functions by just looking at the this type of the function. I couldn't figure out though how to actually get the this type of a function symbol from the type checker. How would I do that?

[typescript-bot]

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.