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

Cannot do anything before super() #3311

Closed
NekR opened this issue May 30, 2015 · 32 comments
Closed

Cannot do anything before super() #3311

NekR opened this issue May 30, 2015 · 32 comments
Labels
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@NekR
Copy link

NekR commented May 30, 2015

Consider this code:

class Super {
  _view:Element;

  constructor(view?:Element) {
    this._view = view || document.createElement('div');
  };
}

class Inherits extends Super {
  test = 1;
  view:Element;

  constructor() {
    let view = document.createElement('span');
    super(view);

    this.view = view;
  };
}

And it says: 2376 A 'super' call must be the first statement in the constructor when a class contains initialized properties or has parameter properties.

I understand that it's because of test property. but can you allow not-this code before super() call how ES6 does?

Also, this won't work too, of course:

class Inherits extends Super {
  test = 1;
  view:Element = document.createElement('span');

  constructor() {
    super(this.view); // 2332 'this' cannot be referenced in current location.
  };
}
@qc00
Copy link

qc00 commented Jun 1, 2015

There are similar restrictions in most OO languages so that the super-class constructor does not run with fields set to unexpected values.

You can however do this:

var view: Element;
class Inherits extends Super {
  test = 1;
  view:Element;

  constructor() {
    super(view = document.createElement('span'));

    this.view = view;
    view = undefined;
  };
}

Or provide appropriate getters in the parent class.

@NekR
Copy link
Author

NekR commented Jun 1, 2015

There are similar restrictions in most OO languages so that the super-class constructor does not run with fields set to unexpected values.

How this related to non-this code?

You can however do this:
Or provide appropriate getters in the parent class.

How this is different from non-this code in constructor?
Your both ways are ugly and looks more like a hack here. I do not actually ask what I can do here to workaround it, I ask why that case is not allowed.

Do you have valid point why it's not allowed?

@RyanCavanaugh
Copy link
Member

Linking to #1617 for bookkeeping purposes.

@RyanCavanaugh RyanCavanaugh added the Question An issue which isn't directly actionable in code label Jun 1, 2015
@RyanCavanaugh
Copy link
Member

You can call super whenever you want if you don't have member initializers:

// This code does not have any errors
class Inherits extends Super {
  test; // changed, was 'test = 1;'
  view: Element;

  constructor() {
    let view = document.createElement('span');
    super(view);
    this.test = 1;
    this.view = view;
  };
}

If you look at how class member initializers are emitted, the reasoning for this becomes fairly clear. Member initializers are emitted after the super call (otherwise your derived-class initializers wouldn't properly overwrite base class initializations); by enforcing the super call to be the first thing the compiler can guarantee that there's exactly one (correct) place to emit those initializers.

You move the super call to any position by removing member initializers -- this makes explicit what you expect the initialization order of each class member to be.

@NekR
Copy link
Author

NekR commented Jun 1, 2015

You can call super whenever you want if you don't have member initializers:

I know that, but it's not a reason to close this issue. It seems more like you was lazy to detect if super() was called before any this access or not. No offense here please, you are closing this issue -- I am saying truth as is.

Consider this code:

class Test extends Something {
  test:number;
  test2:number;
  test3:number;
  test4:number;
  test5:number;
  test6:number;
  test7:number;
  test8:number;
  test9:number;
  test10:number;
  test10:number;
  test12:number;
  test13:number;
  test14:number;
  test15:number;
  test16:number;
  test17:number;
  test18:number;
  test19:number;
  test20:number;
  test22:number;
  test23:number;
  test24:number;
  test25:number;
  test26:number;
  test27:number;
  test28:number;
  test29:number;
  test30:number;

  constructor() {
    this.test = 0;
    this.test2 = 0;
    this.test3 = 0;
    this.test4 = 0;
    this.test5 = 0;
    this.test6 = 0;
    this.test7 = 0;
    this.test8 = 0;
    this.test9 = 0;
    this.test10 = 0;
    this.test10 = 0;
    this.test12 = 0;
    this.test13 = 0;
    this.test14 = 0;
    this.test15 = 0;
    this.test16 = 0;
    this.test17 = 0;
    this.test18 = 0;
    this.test19 = 0;
    this.test20 = 0;
    this.test22 = 0;
    this.test23 = 0;
    this.test24 = 0;
    this.test25 = 0;
    this.test26 = 0;
    this.test27 = 0;
    this.test28 = 0;
    this.test29 = 0;
    this.test30 = 0;

   super();
  }
}

Do you think this is okay code? Why then I need those member definitions before constructor at all?

@RyanCavanaugh RyanCavanaugh reopened this Jun 1, 2015
@RyanCavanaugh
Copy link
Member

It's not just about this access. Consider code like this:

class Base {
  constructor(n) { }
}

var g = 10;
function f() {
  g = 15;
}

class Derived1 extends Base {
  i = f();
  constructor() {
    var x;
    var y = (x = g, super(0), g);
    console.log(x + ', ' + y)
  }
}

The code should (if we can make normative statements about things that aren't allowed) emit 10, 15. What do you propose the compiler emit here? Or by what rule would this code be disallowed?

@NekR
Copy link
Author

NekR commented Jun 1, 2015

I suggest ignore such side effects and follow ES2015 spec here, because you cannot save developers from all of them and this "save" looks pretty strange.

See this: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-super-keyword-runtime-semantics-evaluation and this: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-makesuperpropertyreference
Also test case here.

@RyanCavanaugh
Copy link
Member

Trading correctness for convenience is not something we do lightly.

I don't see the relevance of the linked test case -- there's no property initializer there; TypeScript's behavior is the same is 6to5's.

Again, what do you propose the compiler emit for the example I gave?

@NekR
Copy link
Author

NekR commented Jun 1, 2015

I don't see the relevance of the linked test case -- there's no property initializer there; TypeScript's behavior is the same is 6to5's.

Wrong. You cannot do the same in TS, you need to do this, which is not the same (see #3311 (comment)):

class Base {
  _test: number;
  constructor(v:number) {
    this._test = v;
  }
}

class Child extends Base {
  test: number;
  constructor() {
    let test = 2;
    super(test);
    this.test = test;
  }
}

(es6 example was a bit wrong, here is correct one)

Again, what do you propose the compiler emit for the example I gave?

I am pretty sure it should do this.

Again, I do not see difference between these two cases:

[1]

class Base {
  constructor(v) {
    this._test = v;
  }
}

class Child extends Base {
  constructor() {
    var test = 1;
    super(test);
    this.test = getSomething();
  }
}

function getSomething() {
    return 1;
}

[2]

class Base {
  constructor(v) {
    this._test = v;
  }
}

class Child extends Base {
  test = getSomething();
  constructor() {
    var test = 1;
    super(test);
  }
}

function getSomething() {
    return 1;
}

But in TS you need to have duplicate definitions for first case (see #3311 (comment)) and second is not allowed in TS at all.

@duanyao
Copy link

duanyao commented Jun 2, 2015

@RyanCavanaugh ,

Your example can be translated to an equivalent valid TS form (replace x with Derived1.x):

class Base {
  constructor(n) { }
}

var g = 10;
function f() {
  g = 15;
}

class Derived1 extends Base {
  i = f();
  constructor() {
    super((Derived1.x = g, 0));
    var y = g;
    console.log(Derived1.x + ', ' + y);
  }  
  private static x;
}

The compiler emits for Derived1:

var Derived1 = (function (_super) {
    __extends(Derived1, _super);
    function Derived1() {
        _super.call(this, (Derived1.x = g, 0));
        this.i = f();
        var y = g;
        console.log(Derived1.x + ', ' + y);
    }
    return Derived1;
})(Base);

Converting Derived1.x back to x gives:

var Derived1 = (function (_super) {
    __extends(Derived1, _super);
    function Derived1() {
        var x;
        _super.call(this, (x = g, 0));
        this.i = f();
        var y = g;
        console.log(x + ', ' + y);
    }
    return Derived1;
})(Base);

So I think the answer is clear: emit property initialization section of derived class immediately after _super.call(), i.e. I agree with @NekR .

by enforcing the super call to be the first thing the compiler can guarantee that there's exactly one (correct) place to emit those initializers.

Actually TS had failed to enforce "the super call to be the first thing", because developers can place almost arbitrary code in the parameter list of a super call, which executes before the super call. Java etc. also had failed to enforce it.

So the "super call statement first" rule is not quite useful; disallowing access to this/super before/in a super call should be sufficient.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 18, 2015

The proposal is to remove the restriction on super being the first statement in the constructor body, and only require the existence of at least one call to super.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed Question An issue which isn't directly actionable in code labels Aug 18, 2015
@NekR
Copy link
Author

NekR commented Aug 18, 2015

@mhegazy and access to the this before super() call should be still banned, otherwise it won't compile correctly to ES2015.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 18, 2015

this is kinda hard to do right all the time without control flow analysis.. this is why the restriction of super being the first statement.. consider this:

class  C {
    x = 1;
    constructor(v) {
         if (v) {
             super(); // tricks the compiler to think super was already called.
         }
         doSomething(this); // OK, but fails at runtime
         super();  
    }
}

@NekR
Copy link
Author

NekR commented Aug 18, 2015

Hmm.. seems reasonable. ES2015 will have such kind of errors reporting anyway, so people will get them at a runtime. Looks better than completely disallowing any statements before super() call.

@adidahiya
Copy link
Contributor

The argument of "users will get this error at runtime anyway" feels very weak -- it kind of defeats the purpose of static type checking. Compiler errors >> runtime errors.

@duanyao
Copy link

duanyao commented Aug 19, 2015

I think control flow analysis for super() is nice to have, just like control flow analysis for return in most static typed language:

int foo(int v) {
  if (v) {
    return 1;
  }
} // Error: not all control paths return a value

int bar(int v) {
  return 0;
  printf("bar returned"); // Warning: unreachable code
}

If it turns out too complicated for TS, how about just disallow super() in conditions and loops?

@adidahiya
Copy link
Contributor

@duanyao btw, side note: we do a bit of control flow analysis in tslint to detect unreachable code

@duanyao
Copy link

duanyao commented Aug 19, 2015

@adidahiya Glad to know tslint, thanks!

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". and removed In Discussion Not yet reached consensus labels Oct 5, 2015
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Oct 5, 2015
@RyanCavanaugh
Copy link
Member

Approved. Access to this (lexically) before the super call should be disallowed to match ES6 runtime behavior.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 22, 2016

should be addressed by #6860

@mhegazy mhegazy closed this as completed Feb 22, 2016
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Feb 22, 2016
@mhegazy mhegazy modified the milestones: TypeScript 1.8.2, Community, TypeScript 1.8 Feb 22, 2016
@jcalz
Copy link
Contributor

jcalz commented May 27, 2017

Sorry if I'm misunderstanding something, but is this issue actually fixed (as opposed to "by design")? Should we be allowed to initialize subclass members outside the constructor or not, if the super call comes later in the constructor than the first line? The code in the OP comment gives me the same error today as was reported two years ago.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented May 30, 2017

The code in the OP is correctly an error.

Because class member initializers are emitted immediately following the super call, the super call needs to be in a predictable location. Otherwise unpredictable effects would occur:

var x = 0;
class C extends B {
  m = ++x;
  constructor() {
    x++;
    super();
  }
}
const c = new C();

It looks like m would get the value 1 here, but it would actually get the value 2 if we unconditionally allowed pre-super code in classes with property initializers.

@jcalz
Copy link
Contributor

jcalz commented May 30, 2017

@RyanCavanaugh Thanks for your answer. So, was anything "fixed" in this issue? Seems like it should be "declined" or "working as intended".

Is the effect you describe actually unpredictable (i.e., it might change in future versions of TS) or just potentially confusing? If it is unambiguously true that member initializers are evaluated immediately after the super call, then I do expect m to get the value 2.

I see what you mean about it "looking" like it would be 1. But such confusion arises in other situations that are not considered to be errors. What do you make of this very slightly different example:

var x = 0;
class C extends B {
  m = ++x;
  constructor() {
    super(x++); // super constructor takes a parameter
  }
}
const c = new C();

No error from TS, and c.m == 2.

The problem I see here is not with the location of the super call, but with having a weird state dependency between initializer and constructor that makes reasoning correctly about its behavior difficult.

I know there's a permissiveness trade-off between allowing bad code and disallowing good code. There's good code that can be written by allowing both initializers and code before the super constructor. Do you have any more egregious examples of bad code that happens when you allow this?

Thanks!

@RyanCavanaugh
Copy link
Member

It's more that the super call has to be in a position where we can simply and reliably insert assignment statements. For example, you could write

class X extends Y {
  k = this.foo();
  constructor() {
    const j = [expr1, super(), this.k] || super();
  }
  foo() { }  
}

which would be a legal way to write super() calls according to the ES spec, but now we have to do all kinds of backflips to correctly emit code that accomplishes the same thing.

@jcalz
Copy link
Contributor

jcalz commented May 31, 2017

Ah, so are these backflips something you don't think TS should do in principle, or would they be something you'd consider a proposal/pull request for?

@RyanCavanaugh
Copy link
Member

It's more of a pragmatic thing - the amount of work we'd have to do to handle all the possible places in an expression a super call could appear and properly extract an equivalent program would be a substantial amount of complexity and testing, compared to just limiting users to choosing between property initializers or weird super call locations (which doesn't seem to be a big deal in practice).

If there were some clever 20-line thing to pluck out an equivalent program for any arbitrary set of super calls, sure, but I expect this would be hundreds if not thousands of lines of code + testcases for the sake of a pretty small improvement.

@jcalz
Copy link
Contributor

jcalz commented Jun 1, 2017

Right now TS emits super(…) as _this = _super.call(this, …) || this; and emits the property initializers as statements afterward:

test.tstest.js
class Derived extends Base {
  prop = 'expr';
  constructor(arg) {
    super(arg);
    this.prop = 'or not';
  }
}
var Derived = (function(_super) {
  __extends(Derived, _super);
  function Derived(arg) {
    var _this;
    // emit constructor body here
    // super(…) ⇒ _this = _super.call(this,…) || this;
    _this = _super.call(this, arg) || this;
    // emit initializers here
    _this.prop = 'expr';
    _this.prop = 'or not';
    return _this;
  }
  return Derived;
}(Base));

But, if we replace calls to super(…) with calls to another function which both calls the super constructor and then calls the initializers:

test.tstest.js
class Derived extends Base {
  prop = 'expr';
  constructor(arg) {
    super(arg);
    this.prop = 'or not';
  }
}
var Derived = (function(_super) {
  __extends(Derived, _super);
  function Derived(arg) {
    var _this;
    var __super = (function() {
      _this = _super.apply(this, arguments) || this;
      __super = function() {
        //ES6 throws if super(…) called twice
        throw new ReferenceError("super dupe");
      };
      // emit initializers here
      _this.prop = 'expr';
      return _this;
    }).bind(this);
    // emit constructor body here
    // super(…) ⇒ __super(…)
    __super(arg);
    _this.prop = 'or not';
    return _this;
  } return Derived;
}(Base));

then we don't have to worry about trying to shove initializer statements in multiple or crazy places. The constructor body can be transcribed mostly as-is, and initializers are evaluated exactly once, immediately after the super constructor is called.


This may or may not turn out to be a 20-liner does the right thing. Even if it is, I understand that you probably do not consider the issue important enough to tackle. In that case, could you please change the status of the issue to reflect what's actually going on (that this behavior is either intentional or not important enough to fix)?

If you go back and read this issue from top to bottom, it's clear that @NekR and @duanyao are asking to always allow code before super(…) in the constructor, and that until mid-August 2015 that's what the discussion is about. Then a few months go by. In October, the issue wakes up and switches to a related but different topic (lexical analysis to prevent this from being referenced before super), and it's marked as "fixed". That is confusing to me and probably will be to any other folks who run into this behavior and find themselves here in the land of TypeScript GitHub Issue #3311 looking for relevant information.

Whew, I wrote a lot. Thanks again for your time!

@NekR
Copy link
Author

NekR commented Jun 1, 2017

Wow, I thought this was fixed. Why so you mark this fixed if it's not? You're saying about should be, predictable location, etc. But as I stated before, Babel handles this right and was handling it right when I opened this issue, TypeScript still not. I stopped using TypeScript for a while ago because of such small issue which nobody wants to fix because it's their beautiful software (yet advertised to use) and they have so principles of not changing things. It's a shame anyway and I'm sad for your tool's users.

@NekR
Copy link
Author

NekR commented Jun 1, 2017

It looks like m would get the value 1 here, but it would actually get the value 2 if we unconditionally allowed pre-super code in classes with property initializers.

Evaluate class members first, then code before super, then super and everything will be as expected. Also that x is a side effect and not safe to use it in that way in general.

@jcalz
Copy link
Contributor

jcalz commented Jun 1, 2017

Evaluate class members first, then code before super, then super and everything will be as expected. Also that x is a side effect and not safe to use it in that way in general.

You have to initialize class members after super(…) or else the base class constructor may modify them. I'm fairly sure the required order is something like:

  • evaluate code before super(…)
  • evaluate super(…)
  • initialize class members
  • evaluate code after super(…)

The issue that bothers @RyanCavanaugh is how to emit class member initialization immediately after the call to super(…) if that call can show up in all sorts of weird places.

@NekR
Copy link
Author

NekR commented Jun 1, 2017

The issue that bothers @RyanCavanaugh is how to emit class member initialization immediately after the call to super(…) if that call can show up in all sorts of weird places.

It was actually discussed already, just like babel does that.

@jcalz
Copy link
Contributor

jcalz commented Jun 1, 2017

just like babel does that.

I mostly agree but the Babel REPL doesn't seem to know what do with super(…) in some expression locations when property initializers are present. I think my proposal above might work but I'm not sure.

@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
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants