Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong 'this' context in class methods #3927

Closed
agrafix opened this issue Jul 19, 2015 · 16 comments

Comments

Projects
None yet
6 participants
@agrafix
Copy link

commented Jul 19, 2015

Currently, the type system included in TypeScript does not allow proper typing of the this context. There are attempts to fix that (See #3694 for example), but these will take a while to ripe and until then I would like to suggest a "quick fix" for class methods, because the following pitfall appears often when interacting with web-libraries. Consider the following code:

class Foo {
    private bar: string = "Bar";

    logBar(): void {
        console.log("Bar's value is: " + this.bar);
    }
}

// many javascript frameworks rebind the this context for callbacks,
// see for example jQuery's $("foo").click or React's onClick
function fireCallback(cb: (() => any)): void {
    let someObj = {
        hello: "42"
    };
    cb.call(someObj);
}

let x = new Foo();
fireCallback(x.logBar);

This code typechecks perfectly:

$ tsc --version
message TS6029: Version 1.5.0-beta
$ tsc --noImplicitAny main.ts

But does not produce the desired result:

$ node main.js
Bar's value is: undefined

Many libraries rebind the this context for callbacks, so my suggestion would be that the tsc transforms methods passed as arguments by wrapping them in an anonymous function like so:

fireCallback((...args: any[]) => x.logBar.call(x, args));

This should be okay, be cause inside a method the compiler assumes that this is of the classes type so there's no way to interact with later bound this objects anyhow.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

commented Jul 19, 2015

We can't possibly do this. A non-exhaustive list of reasons:

  • This would be different behavior from ES6 classes, which is an absolute no-go
  • This would be a breaking change for existing TypeScript code (what if using a different this was intentional?)
  • What if x.logBar is null or undefined ?
  • For methods that don't need this, this proposal has huge perf impact in tight loops. How would you "opt out" ?
@agrafix

This comment has been minimized.

Copy link
Author

commented Jul 19, 2015

  • Maybe, but in the current state the type checker indicates my program is sound, but it is not.
  • You can not use a different this in your class method due to the type checker? (Inside the method this has the type of the class?)
  • x.logBar can not be null or undefined iff x.logBar is a class method of x? I would expect the type system to catch this if this is possible
  • I would expect very few class methods to not need this is practice.
@kitsonk

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2015

@agrafix you are arguing for this typing, which is a #229 and I think few people are disagreeing with it, but the challenge is how to implement it in the way that causes the least damage which is what @RyanCavanaugh is getting at.

Also for the third point...

class Foo {
    private bar: string = "Bar";

    logBar(): void {
        console.log("Bar's value is: " + this.bar);
    }
}

let x = new Foo();

x.logBar = undefined;

Is perfectly valid TypeScript (let alone valid ES6). This is because (I think) TypeScript has to support assigning undefined or null otherwise you can't have uninitialised values.

@agrafix

This comment has been minimized.

Copy link
Author

commented Jul 20, 2015

@kitsonk Hm, if that example is valid TypeScript I find that odd... Wouldn't that mean that x have different types before and after x.logBar = undefined?

@RyanCavanaugh While I don't know too much about the ES6 Spec, I found 4.3.31 method stating:

When a function is called as a method of an object, the object is 
passed to the function as its this value.

I agree that my first proposal is very hacky, so we came up with another solution together with @skogsbaer :
How about compiling class methods to something like this:

class Foo {
    private bar: string = "Bar";

    logBar = () => {
        console.log("Bar's value is: " + this.bar);
    }
}

That way the typing of this is correct inside a class method.

@skogsbaer

This comment has been minimized.

Copy link

commented Jul 20, 2015

So the question really is: what is the semantics of this inside methods definitions with respect to ecmascript 6? Can you rebind this inside of methods with ecmascript 6 or not?

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

When the spec says this

When a function is called as a method of an object,

the relevant part is this:

When a function is called as a method of an object,

Only invocations of the form x.f() are "method" calls. var y = x.f; y() is a non-method call that will have the wrong this.

You can try this in the Chrome console today
image

How about compiling class methods to something like this:

This has very serious perf impact because now you're allocating one closure per function per class instance, instead of one closure per function per class definition. It also makes super calls impossible. If you want to take that trade-off, you can simply writelogBar the way you're suggesting it be compiled to.

@agrafix

This comment has been minimized.

Copy link
Author

commented Jul 21, 2015

Hm, in this case that's sad news for the ES6 standard that the 'this' binding is messed up for method calls... :-(

I don't understand the perf difference between

class Foo {
    private bar: string = "Bar";

    logBar(): void {
        console.log("Bar's value is: " + this.bar);
    }

    logBar2 = () => {
        console.log("Bar's value is: " + this.bar);
    }

    logBar3 = function () {
        console.log("Bar's value is: " + this.bar);
    }.bind(this)
}

Wouldn't this be equivalent to something like this:

function Foo2() {
        this.bar = "Bar";
        this.prototype.logBar = function () {
        console.log("Bar's value is: " + this.bar);
    };
        this.prototype.logBar2 = function () {
        console.log("Bar's value is: " + this.bar);
    }.bind(this);
        this.prototype.logBar3 = function () {
        console.log("Bar's value is: " + this.bar);
    }.bind(this);
}
@RyanCavanaugh

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

That code doesn't even run?

It makes zero sense to try to put a this-bound object on prototype. The entire point of prototype is that there's only one per class (instead of per instance) -- how could a function there possibly have the correct pre-bound value for this if the same object is acting as a method for multiple class instances?

@agrafix

This comment has been minimized.

Copy link
Author

commented Jul 21, 2015

Sorry, that was a mistake this morning. I now copied the output of the typescript compiler (and removed the console.log and added a simple incr operation) to benchmark the differences:

var Foo = (function () {
        function Foo() {
            var _this = this;
            this.bar = "Bar";
            this.ctr = 0;
            this.logBar2 = function () {
                _this.ctr++;
            };
            this.logBar3 = function () {
                this.ctr++;
            }.bind(this);
        }
        Foo.prototype.logBar = function () {
            this.ctr++;
        };
        return Foo;
    })();

Benchmark in Google Chrome (Version 42.0.2311.135) with http://benchmarkjs.com/ :

var suite = new Benchmark.Suite;
    var f = new Foo();
    suite.add('logBar method call', function() {
        f.logBar();
    })
    .add('logBar arrow binding', function() {
        f.logBar2();
    })
    .add('logBar .bind(this)', function() {
        f.logBar3();
    })
    // add listeners
    .on('cycle', function(event) {
      console.log(String(event.target));
    })
    .on('complete', function() {
      console.log('Fastest is ' + this.filter('fastest').pluck('name'));
    })
    // run async
    .run({ 'async': true });

The classic method call is fastest, and the arrow binding is 25% percent slower. The .bind(this) is terrible, as you already said. It would be nice if you could opt-in for the 25% percent slower and get correct this semantics inside class methods :-)

logBar method call x 462,053,175 ops/sec ±4.48% (88 runs sampled)
logBar arrow binding x 313,023,641 ops/sec ±0.59% (88 runs sampled)
logBar .bind(this) x 7,297,088 ops/sec ±0.92% (96 runs sampled)
@RyanCavanaugh

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

It would be nice if you could opt-in for the 25% percent slower and get correct this semantics inside class methods

You can! Simply write your method as an arrow function and this will happen.

You should also understand that it isn't just a case of a 25% function invocation hit. Let's say you have a class with 30 methods and you allocate 200 instances of it. If all methods are on the prototype, you'll allocate 30 closures. If all methods are put on the instance, you'll allocate 6,000 closures. That's going to have a broad impact - the GC will run slower and more frequently, you'll have more overall memory pressure, and decreased memory locality.

@agrafix

This comment has been minimized.

Copy link
Author

commented Jul 21, 2015

Thanks for pointing that out - I now see why the suggested work-arounds are probably not a good idea in general case. I will go with the arrow function methods for now (as needed) and hope that maybe proper this typing ( #3694 , #229 ) will land in typescript soon! Thank you for your time!

@ststeiger

This comment has been minimized.

Copy link

commented Jun 26, 2017

You can ? And what about this in properties ? Like get/set ?
No arrow-functions there...

@Deilan

This comment has been minimized.

Copy link

commented Sep 16, 2017

@RyanCavanaugh So what's a proposed practice to overcome the issue?

@ststeiger

This comment has been minimized.

Copy link

commented Sep 16, 2017

For this issue, I would suggest binding the method to "this" in the constructor.
This works "this.showMessage = this.showMessage.bind(this);" .

Some nice info about it here:
https://daveceddia.com/avoid-bind-when-passing-props/


class foo
{
    private m_Length: number = 0.0;
    private m_Width: number = 0.0;
    private m_Height: number = 0.0;

    constructor(x, y, z) { 
        this.m_Length = x;
        this.m_Width = y;
        this.m_Height = z;

        this.showMessage = this.showMessage.bind(this);
        this.calculateVolume = this.calculateVolume.bind(this);
    }

   public showMessage(callback) { 
        console.log(callback());
    }

   get volume():number {
        return this.calculateVolume();
    }


    public calculateVolume():number {
        return (this.m_Length * this.m_Width * this.m_Height);
    }

}

var x = new foo();
x.showMessage(x.calculateVolume);

@Deilan

This comment has been minimized.

Copy link

commented Sep 16, 2017

Here's a solution I've come up to: https://stackoverflow.com/a/46256398/3095779.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.