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

Return value of super() calls not used for this #7574

Closed
justinfagnani opened this issue Mar 18, 2016 · 17 comments · Fixed by #10762
Closed

Return value of super() calls not used for this #7574

justinfagnani opened this issue Mar 18, 2016 · 17 comments · Fixed by #10762
Assignees
Labels
Bug A bug in TypeScript ES6 Relates to the ES6 Spec Fixed A PR has been merged for this issue

Comments

@justinfagnani
Copy link

If a constructor returns a value other than this, then in subclasses super() calls to that constructor should use the result as this in the subclass constructor.

I believe this is specified in 12.3.5.1, step 10 of SuperCall: http://www.ecma-international.org/ecma-262/6.0/index.html#sec-super-keyword

Getting this behavior correct is going to be very important for supporting Custom Elements, which takes advantage of this to initialize browser-allocated elements with user-written constructors. See https://w3c.github.io/webcomponents/spec/custom/#htmlelement-constructor

Note, Babel 6 implements this correctly.

TypeScript Version:

1.8.9

Code

class Foo {

  x : number;

  constructor() {
    return {
      x: 1,
    };
  }
}

class Bar extends Foo {

  y : number;

  constructor() {
    super();
    this.y = 2;
  }
}

let o = new Bar();
console.assert(o.x === 1 && o.y === 2);

Expected behavior:

No assertion error

The constructor for Bar should compile to this:

var Bar = (function (_super) {
    __extends(Bar, _super);
    function Bar() {
        var _this = _super.call(this);
        _this.x = 1;
        return _this;
    }
    return Bar;
}(Foo));

Actual behavior:

Assertion error

The constructor for Bar compiles to this:

var Bar = (function (_super) {
    __extends(Bar, _super);
    function Bar() {
        _super.call(this);
        this.x = 1;
    }
    return Bar;
}(Foo));
@justinfagnani
Copy link
Author

Looking at the Typescript spec, it seems like there's an explicit incompatibility between section 4.9.1 and ECMA-262 12.3.5.1

Specifically the line "The type of a super call expression is Void": https://github.com/Microsoft/TypeScript/blob/master/doc/spec.md#4.9.1

@jeffreymorlan
Copy link
Contributor

If you return an object from a constructor like that, you're throwing away the subclass prototype. So it doesn't make sense to make a super() call to such a constructor - if it worked you'd end up with new Bar() creating an object that doesn't have any methods of Bar.

@mhegazy mhegazy added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Mar 18, 2016
@justinfagnani
Copy link
Author

if it worked you'd end up with new Bar() creating an object that doesn't have any methods of Bar.

@jeffmcaffer This isn't necessarily true, new Bar() can return a Bar even if it's not returning this. It can easily type check - this has nothing to do with types. The return type of super() should be the return type of new in the super constructors interface.

For instance, this should fully type check:

class A {
  static _singleton : A;
  constructor() : A {
    if (!_singleton) {
      _singleton = this;
    }
    return _singleton;
  }
}

@mhegazy I'd like to point out that Typescript is in direct violation of the ECMAScript spec, and because of this you won't be able to write Custom Elements in Typescript compiled to ES5*. I hope that if this behavior is "By Design" that the design can still be changed so that Typescript correctly implements ES2016 and is actually a superset rather a separate language.

*Targeting ES2015 should probably work, because ES2015 just works this way.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 18, 2016

and because of this you won't be able to write Custom Elements in Typescript compiled to ES5*.

can you elaborate?

@justinfagnani
Copy link
Author

Here's the spec for the callable HTMLElement constructor: https://w3c.github.io/webcomponents/spec/custom/#htmlelement-constructor

The relevant parts are step 5-10

  1. Let instance be...
  2. Return instance

The native implementations of Custom Elements will return something that needs to become this from the HTMLElement constructor.

Separately, the initial polyfill code I'm writing looks something like this:

  window.HTMLElement = function() {
    if (_newInstance) {
      var i = _newInstance;
      _newInstance = null;
      _newTagName = null;
      return i;
    }
    var tagName = // some stuff to emulate new.target
    return document.createElement(tagName);
  }

(actual code here: https://github.com/webcomponents/webcomponentsjs/blob/v1/src/CustomElements/v1/CustomElements.js#L48 )

Test of Typescript ES5 output that fails: https://github.com/webcomponents/webcomponentsjs/blob/v1/tests/CustomElements/v1/js/typescript.js#L15

Corresponding test of Babel output that passes: https://github.com/webcomponents/webcomponentsjs/blob/v1/tests/CustomElements/v1/js/babel.js#L15

@sebmck
Copy link

sebmck commented Mar 21, 2016

Compiling to var _this = _super.call(this); isn't exactly accurate. Note step 13.a of 9.2.2. The return value of the constructor is only used if the returning value is an ECMAScript object.

@mhegazy mhegazy added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. ES6 Relates to the ES6 Spec Suggestion An idea for TypeScript and removed By Design Deprecated - use "Working as Intended" or "Design Limitation" instead labels Mar 21, 2016
@falsandtru
Copy link
Contributor

falsandtru commented Apr 19, 2016

Simplified another case:

new class extends class {
    constructor() {
        return function () {};
    }
} {
    constructor() {
        super();
    }
}

This code returns a function on chrome, firefox, and edge, but TypeScript is not.

@justinfagnani
Copy link
Author

Any news here? Chrome, Safari, and the Web Components polyfills are all making progress on implementing custom elements v1 APIs, which rely on spec-compliant SuperCall behavior.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 1, 2016

@DanielRosenwasser should be presenting this again and investigating issues we need to address.

@supermoos
Copy link

Has this been fixed for TypeScript 2.0@beta? Would really like to use TypeScript with Custom Elements V1.

@justinfagnani
Copy link
Author

@supermoos if you target ES6 it works fine. Then, if you need, you can compile to ES5 with Babel.

@supermoos
Copy link

@justinfagnani oh yeah, you're right. I tried the build from this readme.md:
https://github.com/webcomponents/webcomponentsjs/tree/v1/src/CustomElements/v1 - it work's with extending HTMLElement, but if I try HTMLButtonElement I get the Illegal constructor error. Also, the constructor seems to be fired twice with this code:

        this.testButton = new ButtonTest();
        document.body.appendChild(this.testButton);

I know it's a 14 days old build, just wanted to put it out there :-)

@DanielRosenwasser DanielRosenwasser removed the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label Aug 6, 2016
@DanielRosenwasser
Copy link
Member

One issue I'm seeing now is that even if you set the value of this in the constructor to the potential return value of super(), the prototype in the current class gets ignored. That means any methods you define in a derived class get ignored.

class A {
    constructor() {
        return { a: 10 };
    }
}

class B extends A {
    foo() {
        return this.a;
    }
}

new B().foo()
> Uncaught TypeError: (intermediate value).foo is not a function

I think that behavior would be very surprising for TypeScript users, but I don't know of a workaround for that.

@justinfagnani
Copy link
Author

justinfagnani commented Sep 4, 2016

That would also be surprising for a JavaScript user, so I'd recommend that people don't do it :)

More seriously, TypeScript could catch this:

class A {
    constructor() {
        return { a: 10 }; // OK, because {a: number} is assignable to A
    }
}

class B extends A {
    foo() {
        return this.a; // Warning: {a: number} not assignable to B
    }
}

new B().foo()

Again, the most common use-case for this behavior is to return an instance of the class with the "interesting" constructor. For browsers, the HTMLElement constructor returns a non-this instance of HTMLElement during parsing.

Since return something other that this does work in TypeScript when targeting ES6, what's the behavior there? Can that just be matched?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Sep 4, 2016

My original comment wasn't about the fact that a wasn't declared on A, but rather that new B().foo won't be defined.

TypeScript currently can't handle that behavior at the type level. There's no way to know from a construct signature whether a super() call is going to make a new this value whose prototype chain doesn't use the prototype of the current class (meaning that any methods defined on the current class will get ignored).

So people might extend from HTMLElement, and define methods thinking that everything is okay. But their methods will get ignored, and they'll be confused, and ask why TypeScript didn't warn them about this issue.

In fact, from reading this page I'm not sure how createdCallback, attachedCallback, etc. are supposed to be defined if the prototype of the current class isn't used following a super calls of things like HTMLElement. Maybe it is used and then an HTMLElement is returned?

There's definitely more to this than I currently understand. Any chance you can elaborate on that last part @justinfagnani?

@Arnavion
Copy link
Contributor

Arnavion commented Sep 4, 2016

In fact, from reading this page I'm not sure how createdCallback, attachedCallback, etc. are supposed to be defined if the prototype of the current class isn't used following a super calls of things like HTMLElement.

The custom element spec specially makes the result of new MyElement() have a MyElement prototype. That's why there's an explicit "register the function MyElement to produce <my-element> elements" step so that document.createElement("my-element") can return a MyElement.

In the general case of constructors returning new objects, the returned object does not have the prototype chain set to the constructor.

@justinfagnani
Copy link
Author

justinfagnani commented Sep 5, 2016

@Arnavion is correct. In the HTML spec, parser-created elements work like this:

  1. Create an element object
  2. Look up its constructor by tagname
  3. Set the prototype of the element to the constructors .prototype
  4. Store the element in a global
  5. Invoke the constructor to initialize it, which will eventually super() into HTMLElement which
  6. Returns the element from the global
  7. The constructor chain runs, initializing the element

You can see the same steps in the polyfill here: https://github.com/webcomponents/webcomponentsjs/blob/v1/src/CustomElements/v1/CustomElements.js#L568

The important bits with some added comments:

    // the element instance has already been created, and the definition looked up
    _upgradeElement(element, definition, callConstructor) {
      const prototype = definition.constructor.prototype;
      element.__proto__ = prototype; // 3. Set the prototype
      if (callConstructor) {
        this._setNewInstance(element); // 4. Store the element in a global
        new (definition.constructor)(); // 5. Invoke constructor
        element['_upgradedProp'] = true;
        console.assert(this._newInstance == null);
      }

Inside HTMLElement's constructor we have:

    if (customElements._newInstance) {
      const i = customElements._newInstance;
      customElements._newInstance = null;
      return i; // 6. Return the element from the global
    }

Even though in the general case an object returned from a constructor might not have the correct prototype chain, in this critical case it will.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript ES6 Relates to the ES6 Spec Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants