[unbound-method] support binding methods inside constructor

[chrisblossom]

Repro

{
  "rules": {
    "@typescript-eslint/unbound-method": "error"
  }
}

class MyClass {
    constructor() {
        // will permanently bind and should not be an error (but currently is)
        this.log = this.log.bind(this)
    }

    public log(): void {
        console.log(this);
    }
}

const instance = new MyClass();

// this should not error because it is bound in the class's constructor so it will log the expected this
const myLog = instance.log;
myLog();

// This log might later be called with an incorrect scope
const { log } = instance;

Expected Result
No @typescript-eslint/unbound-method errors.

Actual Result
Two @typescript-eslint/unbound-method errors.

Additional Info

Versions

package	version
@typescript-eslint/eslint-plugin	1.10.2
@typescript-eslint/parser	1.10.2
TypeScript	3.5.2
ESLint	5.16.0
node	10.14.1
npm	6.9.0

[nickineering]

I successfully replicated this.

[JounQin]

Any news?

[bradzacher]

happy to accept a PR!

However, it will not be easy to implement.
The solution to this would have to be done completely manually, because AFAIK TypeScript does not track binding information anywhere.

---

Take this aside with a pinch of salt, because even a naive solution is an improvement over the base implementation, and no doubt the below is an edge case that can be ignored.

As an aside, this isn't really possible to do 100% correctly, because there's the problem of things like reassignment outside of the constructor (because methods are not readonly).
So you'd need to do some global control-flow analysis shenanigans, which wouldn't work.

class MyClass {
    constructor() {
        // will permanently bind and should not be an error (but currently is)
        this.log = this.log.bind(this)
    }

    public log(): void {
        console.log(this);
    }
}

const instance = new MyClass();

// this should not error, because the method is bound
const myLog1 = instance.log;

//////////

instance.log = function methodThatsNoLongerBound() { console.log(this); }
// this should error because it is no longer bound
const myLog2 = instance.log;

//////////

instance.log = instance.log.bind(instance);
// this should not error, because the method is bound
const myLog3 = instance.log;

//////////

class MyClass2 {
    constructor() {
        // will permanently bind and should not be an error (but currently is)
        this.log = this.log.bind(this);
        
        if (process.env.DEV) {
            this.doSomethingStupid();
        }
    }

    public log(): void {
        console.log(this);
    }

    private doSomethingStupid() {
        this.log = function methodThatsNoLongerBound() { console.log(this); };
    }
}

const instance2 = new MyClass2();

// this should error, because the method is potentially not bound
const myLog4 = instance.log;

[piotrgajow]

I have just encountered this error when migrating from tslint to eslint. In my team we are using convention, that fields/variables/etc. that have Bind suffix contain methods that were previously bound. Maybe as a solution (or just as a temporary workaround :) ) an ignorePattern option could be added (something similar to how no-unused-vars rule works)?

If you think this would be okey, I could try to create a pull request with this.

[bradzacher]

Options to ignore names are a bad solution to the problem. They lack context, which can easily cause false negatives for a rule.

False negatives are much more insidious and worse than false positives because they are invisible and they cause production bugs. This harms trust in the linting tooling, causing users to second guess lint results - negatively impacting the ecosystem.

There's no such thing as a temporary option. Once it's in, people depend on it and then they will want it. It's a breaking change to remove an option and people will be upset if you remove something they rely on.

[JoshuaKGoldberg]

Coming back to this issue: it's been marked as accepting PRs for about a year and hasn't had much interaction in over a year and a half. The comments it does have are discussing how difficult it will be to get right, and all the fun little edge cases that make it hard.

We're a small maintenance team and already are swamped. So even if someone were to send in a perfect PR now, it would be hard for us to support the rule.

I'm going to close this as wontfix. If anybody is very interested in implemented it, I'd encourage them to release it as a separate open source plugin+rule. If it gets popular we can always talk about moving it into typescript-eslint/eslint-plugin.

Thanks all!