Skip to content

Bug or breaking change in super constructors? #12669

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

Closed
Elephant-Vessel opened this issue Dec 5, 2016 · 6 comments
Closed

Bug or breaking change in super constructors? #12669

Elephant-Vessel opened this issue Dec 5, 2016 · 6 comments
Assignees
Labels
Bug A bug in TypeScript High Priority

Comments

@Elephant-Vessel
Copy link

Elephant-Vessel commented Dec 5, 2016

TypeScript Version: 2.1.1 and nightly (2.2.0-dev.20161205)

Code

abstract class Animal
{
    constructor(someStr) { }

    public GetName() {
        return "test";
    }
}

class Dog extends Animal
{
    constructor()
    {
        super(super.GetName());
        var test = super.GetName();
    }
}

Expected behavior: (As per 2.0.10 and Playground)

var Dog = (function (_super) {
    __extends(Dog, _super);
    function Dog() {
        _super.call(this, _super.prototype.GetName.call(this)); // <= Look here
        var test = _super.prototype.GetName.call(this);
    }
    return Dog;
}(Animal));

Actual behavior:

var Dog = (function (_super) {
    __extends(Dog, _super);
    function Dog() {
        var _this = _super.call(this, _super.GetName.call(_this)) || this; // <= Look here
        var test = _super.prototype.GetName.call(_this);
        return _this;
    }
    return Dog;
}(Animal));

Notice the difference in the compiled call to _super.GetName/_super.prototype.GetName when super.GetName occurs in the super-constructor, while calls to _super.GetName outside of the constructor always gets compiled to _super.prototype.GetName.

GetName is always attached to the prototype, so the _super.GetName is breaking runtime.

Is there an intention, which would make sense, to disallow calls to super until after the constructor is run, only allowing static calls? Nevertheless, the generated code don't make sense and no compiler error is shown.

@mhegazy mhegazy added Bug A bug in TypeScript High Priority labels Dec 5, 2016
@mhegazy mhegazy added this to the TypeScript 2.1.3 milestone Dec 5, 2016
@rbuckton
Copy link
Contributor

rbuckton commented Dec 5, 2016

I'll take a look, though this may be an error at runtime in native ES6.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 5, 2016

This should be an error. the emit is rather secondary (though we should fix it).

@mhegazy
Copy link
Contributor

mhegazy commented Dec 5, 2016

also this should be an error, though it emits correctly:

class Dog extends Animal {
    constructor() {
        var x = super.GetName();  // implicit use of `this` before it is defined
    }
}

@Elephant-Vessel
Copy link
Author

Elephant-Vessel commented Dec 5, 2016

Would you still be able to allow calls to super directly after the super-constructor is called, even if inside the derived constructor? The absence of any call to super in your above example made me unsure about your intentions.

class Dog extends Animal {
    constructor() {
        super("foo");
        var superName = super.GetName(); // Ok?
    }
}

@mhegazy
Copy link
Contributor

mhegazy commented Dec 5, 2016

Would you still be able to allow calls to super directly after the super-constructor is called, even if inside the derived constructor?

This is valid code, and should work now anyways.

the issue is the order. this is not defined until super() is called. The emit would look something like:

 var _this = _super.call(this)) || this;

so before this line, _this is undefined. any call to a super method is just sugar for _super.prototype.GetName.call(_this); and thus it needs to pass the value of _this. if this is done before _this is defined it will fail at runtime.

the rule the compiler should enforce is that this is never used (neither directly nor indirectly through super) before the super() constructor call.

@rbuckton
Copy link
Contributor

rbuckton commented Dec 5, 2016

We should definitely make this an error. Tested in NodeJS REPL:

> class B { constructor(o) {} getName() {} }
[Function: B]
> class C extends B { constructor() { super(super.getName()); } }
[Function: C]
> new C
ReferenceError: this is not defined
    at C (repl:1:43)

@mhegazy mhegazy modified the milestones: TypeScript 2.1.3, TypeScript 2.2 Dec 6, 2016
@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.
Labels
Bug A bug in TypeScript High Priority
Projects
None yet
Development

No branches or pull requests

3 participants