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

No compile error thrown when this referenced before call to super completes #8060

Open
tomdye opened this issue Apr 13, 2016 · 13 comments
Open
Labels
Effort: Difficult Good luck. Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@tomdye
Copy link

tomdye commented Apr 13, 2016

TypeScript Version:

1.8.9

Code
JSFiddle: https://jsfiddle.net/kitsonk/fs9t96ep/
TS Playground: http://goo.gl/X7cgvV

class A {
    constructor(fn: () => void) {
        fn.call(this);
    }
    foo: string = 'foo';
}

class B extends A {
    constructor() {
        super(() => {
            console.log(this);
        });
    }
    bar: string = 'bar';
}

const b = new B();

Expected behavior:
Typescript should guard against the use of this as the call to super has not completed. Thus it should not compile.

Actual behavior:
Typescript compiles this successfully. It works fine when the target is set to es5 but breaks the browser when target is set to es6.
Error is: VM89:55 Uncaught ReferenceError: this is not defined

@RyanCavanaugh RyanCavanaugh added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Apr 13, 2016
@RyanCavanaugh
Copy link
Member

This code isn't necessarily invalid -- it's entirely possible (i.e. we can't see its implementation) that the super constructor doesn't invoke the method immediately, in which case this would be valid to access at a later date.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 13, 2016

@RyanCavanaugh the ES6 code:

class A {
    constructor(fn) {
        fn.call(this);
    this.foo = 'foo';
    }
}

class B extends A {
    constructor() {
        super(() => {
            console.log(this);
        });
    this.bar = 'bar';
    }
}

const b = new B();

Fails in Chrome with:

Uncaught ReferenceError: this is not defined

And in Edge:

SCRIPT5113: Use before declaration

And Firefox:

ReferenceError: |this| used uninitialized in arrow function in class constructor

It is because it is a lambda function and therefore it is trying to access the lexical this, which isn't available until after super completes, irrespective of what is actually in the super constructor. Therefore, you cannot use lambda function as arguments of a super call if they reference this in ES6 (or all 3 browser manufactures have mis-implemented the spec).

@RyanCavanaugh
Copy link
Member

This works for me in Chrome and Edge?

image

class A {
    constructor(fn) {
       this.foo = 'foo';
       this.print = fn;
    }
}

class B extends A {
    constructor() {
        super(() => {
            console.log(this.bar);
        });
        this.bar = 'bar';
    }
}

const b = new B();
console.log(b.foo);
b.print();

@kitsonk
Copy link
Contributor

kitsonk commented Apr 13, 2016

Hmmm... wow, ok, I can break it again if I call it within the constructor:

class A {
    constructor(fn) {
       this.foo = 'foo';
       this.print = fn;
       fn();
    }
}

class B extends A {
    constructor() {
        super(() => {
            console.log(this.bar);
        });
        this.bar = 'bar';
    }
}

const b = new B();
console.log(b.foo);
b.print();

The specific use case that @tomdye and I ran into was a situation where we were extending Promise and passing in a lambda function as the executor, which gets invoked immediately. So we were going merrily along, targeting ES5 and then when we tried to target ES6, it broke.

But that is really difficult to guard against, I suspect.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 13, 2016

@bryanforbes just suggest that given:

class B extends A {
    constructor() {
        super(() => {
            console.log(this);
        });
    }
    bar: string = 'bar';
}

If the emit was:

var B = (function (_super) {
    __extends(B, _super);
    function B() {
        var _this;
        _super.call(this, function () {
            console.log(_this);
        });
        _this = this;
        this.bar = 'bar';
    }
    return B;
}(A));

Then the runtime behaviour of ES5 would match ES6.

@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript and removed By Design Deprecated - use "Working as Intended" or "Design Limitation" instead labels Apr 13, 2016
@RyanCavanaugh RyanCavanaugh reopened this Apr 13, 2016
@RyanCavanaugh
Copy link
Member

That seems like a good solution, though I'm slightly afraid of breaking existing working code.

@tomdye
Copy link
Author

tomdye commented Apr 13, 2016

It will only break code that's invalid ES6 in the first place though. If they were ever to try setting the compile target to 'es6' they would see this error at runtime. Must be better to catch it at compile time now rather than down the line should they choose to compile to 'es6' at a later date.

@RyanCavanaugh
Copy link
Member

The problem is we won't be catching at compile time -- people will upgrade to the next TypeScript version, recompile to ES5, and their code will start breaking at runtime with some very confusing errors and emit that looks like a bug.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 13, 2016

Yes, agreed. It is clearly a breaking change, but one that preserves the runtime semantics. Maybe there is room for a linter to flag uses of lexical this within the scope of a super call (@adidahiya), since it could be uninitialized?

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed Bug A bug in TypeScript labels Jun 7, 2016
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Effort: Difficult Good luck. and removed In Discussion Not yet reached consensus labels Jun 9, 2016
@RyanCavanaugh
Copy link
Member

Accepting PRs once the new transform-based emitter merges. Warning, this is likely a difficult change.

@mhegazy mhegazy added this to the Community milestone Jun 9, 2016
@kitsonk
Copy link
Contributor

kitsonk commented Nov 4, 2016

Just a note on this, the emit in master (TS 2.1) now breaks these examples in ES5 at run-time, so this now has parity. It still does not warn you that it is invalid.

Given:

class A {
    constructor(fn: () => void) {
        fn.call(this);
    }
    foo: string = 'foo';
}

class B extends A {
    constructor() {
        super(() => {
            console.log(this);
        });
    }
    bar: string = 'bar';
}

The emit is...

Version 2.1.0-dev.20161103

var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
var A = (function () {
    function A(fn) {
        this.foo = 'foo';
        fn.call(this);
    }
    return A;
}());
var B = (function (_super) {
    __extends(B, _super);
    function B() {
        var _this = _super.call(this, function () {
            console.log(_this);
        }) || this;
        _this.bar = 'bar';
        return _this;
    }
    return B;
}(A));

Version 2.0.6

var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
var A = (function () {
    function A(fn) {
        this.foo = 'foo';
        fn.call(this);
    }
    return A;
}());
var B = (function (_super) {
    __extends(B, _super);
    function B() {
        var _this = this;
        _super.call(this, function () {
            console.log(_this);
        });
        this.bar = 'bar';
    }
    return B;
}(A));

@zemlanin
Copy link

One more case (https://gist.github.com/zemlanin/a306e8498d05c923cac405c047014029):

// this.ts
class A {
    constructor(protected cb: () => void) { }
}

class Greeter extends A {
    constructor() {
        // ^ there should be `var _this = this;`
        super(() => { this })
    }
}
// compiled_with_ts2_0_10.js
var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
var A = (function () {
    function A(cb) {
        this.cb = cb;
    }
    return A;
}());
var Greeter = (function (_super) {
    __extends(Greeter, _super);
    function Greeter() {
        var _this = this;
        // ^ there should be `var _this = this;`
        _super.call(this, function () { _this; });
    }
    return Greeter;
}(A));
// compiled_with_ts2_1_1.js
var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
var A = (function () {
    function A(cb) {
        this.cb = cb;
    }
    return A;
}());
var Greeter = (function (_super) {
    __extends(Greeter, _super);
    function Greeter() {
        return
        // ^ there should be `var _this = this;`
        _super.call(this, function () { _this; }) || this;
    }
    return Greeter;
}(A));

HamletDRC pushed a commit to microsoft/tslint-microsoft-contrib that referenced this issue Nov 20, 2016
closes #312

bump typescript version to 2.1.1

update node flags check in AstUtils

accept `JsxOpeningLikeElement`

skip OmittedExpressions in prefer-const

accept `NewExpression` alongside with `CallExpression`

correct `validateUnaryExpression` signature

ES6 => ES2015

visit ts.moduleBlock

skip omitted expressions

skip string literal in JSX elements

don’t check left and right operands on *unary* operator

make test-data compilable

fix `this` referencing

microsoft/TypeScript#8060

fix AstUtils.isExported

In rev. 9385bba , I’ve assumed that `Export` flag was moved to `ModifiedFlags`. In fact, it was renamed to `ExportContext`

custom elements without <img /> in them

update line positions

fixing linter

closes #312
@Harley-Adams
Copy link
Member

I've also found this with creating an arrow function before calling super.

class Foo extends Object {
    public constructor() {
        const test = () => {
            console.log(this);
        };
        test();
        super();
    }
}

You can see here that we the output has var _this = this; which shouldn't be valid

@RyanCavanaugh RyanCavanaugh removed this from the Community milestone Mar 7, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: Difficult Good luck. Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants