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

if first line in constructor is 'somestring', "this" is called before calling super() in constructor #6901

Closed
Mischi opened this issue Feb 4, 2016 · 10 comments
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue High Priority

Comments

@Mischi
Copy link

Mischi commented Feb 4, 2016

Hi,

we have experienced a problem where this is called before super() in a constructor function. This happen if the first line in a constructor is a string (for e.g ngInject;)

Typescript

class A  {
    blub = 6;
}


class B extends A {

  blub = 12;

  constructor() {
    'someStringForEgngInject';
    super()
  }
}

console.log(new B().blub);

and here the transpiled output

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() {
        this.blub = 6;
    }
    return A;
})();
var B = (function (_super) {
    __extends(B, _super);
    function B() {
        'someStringForEgngInject';
        this.blub = 12;
        _super.call(this);
    }
    return B;
})(A);
console.log(new B().blub);

The transpiled output with 'someStringForEgngInject' removed works correct.

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() {
        this.blub = 6;
    }
    return A;
})();
var B = (function (_super) {
    __extends(B, _super);
    function B() {
        _super.call(this);
        this.blub = 12;
    }
    return B;
})(A);
console.log(new B().blub);

We are using Typescript 1.7.5 and compile to ES6 which is handed of to Babel which will print a error message like this

Module build failed: SyntaxError: C:/dev/a.js: 'this' is not allowed before super()

The code for transpiling constructors probably "assumes" that the first line is always the "super" call and insert all default values in the second line?!

Issue found by @loxy, CC'ing @davidreher

Cheers,
Fabian

@sandersn
Copy link
Member

sandersn commented Feb 4, 2016

CC: @yuit

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Feb 4, 2016
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 1.8.2 milestone Feb 4, 2016
@RyanCavanaugh
Copy link
Member

Proposing the following behavior (@ahejlsberg, @bterlson any thoughts?)

  • If super(...) is in an expression statement whose parent is a block, initializers are emitted as subsequent statements
  • If super(...) is in an expression statement whose parent is a control flow statement (i.g. if (true) super()), rewrite it as a block containing a super call and its initializers
  • If super(...) is in any other expression position, emit (super(...), this.prop1 = value1, this.prop2 = value2, ..., this)

@bterlson
Copy link
Member

bterlson commented Feb 4, 2016

@RyanCavanaugh I think what you propose works.

@yuit
Copy link
Contributor

yuit commented Feb 4, 2016

As talked with @RyanCavanaugh off-line, the two bullet points should be address separately as they will involve discussing whether we should allow super call to NOT be the first statement. The fix for this issue is not related to it

@ahejlsberg
Copy link
Member

@yuit Right, the issue here is that the checker skips prologue directives (expression statements consisting of a string literal) but the emitter doesn't. We need similar logic in the emitter where we locate the first super call.

@ahejlsberg
Copy link
Member

@bterlson Brian, are there any proposals in TC39 to support property declarations with initializers in classes?

@RyanCavanaugh Seems like there are so many ways this could go wrong. What about situations where there are multiple super calls, e.g. in both branches of an if statement? What if the super call isn't reachable? In general I'm not super excited about changing our rules here, and if we do I think we should be very conservative.

@bterlson
Copy link
Member

bterlson commented Feb 4, 2016

@ahejlsberg indeed, see https://github.com/jeffmo/es-class-fields-and-static-properties. Notably, "execution of the initializers happens at the end of the internal "initialization" process that occurs while executing super() in the derived constructor", which seems to align with @RyanCavanaugh's behavior. Also note the next sentence: "This means that if a derived constructor never calls super() , instance fields specified on the derived class will not be initialized".

@RyanCavanaugh
Copy link
Member

I don't think we can realistically pick and choose which ES6 classes are valid are allowed to use property initializers based on how someone wrote the body of their semantically valid constructor.

What about situations where there are multiple super calls, e.g. in both branches of an if statement?

That's fine - we emit the initializers twice. There's nothing wrong with that. If you call super twice, it's a runtime error, so we can be guaranteed the initializers won't actually run twice.

What if the super call isn't reachable?

If the super call isn't reachable, then it doesn't matter if the initializers don't run because you can't see them anyway -- this will never be valid, and the constructor will throw at the end of execution, so you can't ever access this.someInitializedProperty to see its uninitialized value.

@yuit yuit added the Fixed A PR has been merged for this issue label Feb 5, 2016
@yuit yuit closed this as completed Feb 5, 2016
@davidreher
Copy link

Honestly guys: your speed is amazing! Thx for the quick help to all of you :)

@yuit
Copy link
Contributor

yuit commented Feb 5, 2016

@davidreher you're welcome :). Just an fyi, the porting to master will be done today so if you would like to give it a try on today's nightly !

@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 Fixed A PR has been merged for this issue High Priority
Projects
None yet
Development

No branches or pull requests

8 participants