Skip to content

Emit more efficient/concise "empty" ES6 ctor #10189

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

Merged

Conversation

chancancode
Copy link
Contributor

@chancancode chancancode commented Aug 6, 2016

Fixes #10175

Our setup is to have tsc compile into ES6 and run the output through our existing build pipeline (babel plugins, etc). I understand this is not strictly speaking a bug in tsc, but it is definitely a problem for us. This is a case where both sides, independently, are doing an acceptable thing but the combination of the two is not, so I figured I'll give this a shot and see if we can get this addressed in tsc.


When there are property assignments in a the class body of an inheriting class, tsc current emit the following compilation:

class Foo extends Bar {
  public foo = 1;
}
class Foo extends Bar {
  constructor(…args) {
    super(…args);
    this.foo = 1;
  }
}

This introduces an unneeded local variable and might force a reification of the arguments object (or otherwise reify the arguments into an array).

This is particularly bad when that output is fed into another transpiler like Babel. In Babel, you get something like this today:

var Foo = (function (_Bar) {
  _inherits(Foo, _Bar);

  function Foo() {
    _classCallCheck(this, Foo);

    for (var _len = arguments.length, args = Array(_len), _key = 0; _key < _len; _key++) {
      args[_key] = arguments[_key];
    }

    _Bar.call.apply(_Bar, [this].concat(args));
    this.foo = 1;
  }

  return Foo;
})(Bar);

This causes a lot of needless work/allocations and some very strange code (.call.apply o_0).

Admittedly, this is not strictly tsc’s problem; it could have done a deeper analysis of the code and optimized out the extra dance. However, tsc could also have emitted this simpler, more concise and semantically equivalent code in the first place:

class Foo extends Bar {
  constructor() {
    super(…arguments);
    this.foo = 1;
  }
}

Which compiles into the following in Babel:

var Foo = (function (_Bar) {
  _inherits(Foo, _Bar);

  function Foo() {
    _classCallCheck(this, Foo);

    _Bar.apply(this, arguments);
    this.foo = 1;
  }

  return Foo;
})(Bar);

Which is well-optimized (today) in most engines and much less confusing
to read.

As far as I can tell, the proposed compilation has exactly the same
semantics as before.

@msftclas
Copy link

msftclas commented Aug 6, 2016

Hi @chancancode, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

@chancancode
Copy link
Contributor Author

@DanielRosenwasser
Copy link
Member

Interesting - I know that the spec is very explicit about how the constructor body that gets generated. Step 10 on https://tc39.github.io/ecma262/#sec-runtime-semantics-classdefinitionevaluation right now says that it's

constructor(...args){ super (...args);}

But I can't imagine what differences there could be. They literally even use the same iterator by using %ArrayProto_values%. From Step 23 on https://tc39.github.io/ecma262/#sec-createmappedargumentsobject right now:

Perform ! DefinePropertyOrThrow(obj, @@iterator, PropertyDescriptor {[[Value]]: %ArrayProto_values%, [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true}).

Pinging @bterlson for any input on this.

When there are property assignments in a the class body of an inheriting
class, tsc current emit the following compilation:

```ts
class Foo extends Bar {
  public foo = 1;
}
```

```js
class Foo extends Bar {
  constructor(…args) {
    super(…args);
    this.foo = 1;
  }
}
```

This introduces an unneeded local variable and might force a reification
of the `arguments` object (or otherwise reify the arguments into an
array).

This is particularly bad when that output is fed into another transpiler
like Babel. In Babel, you get something like this today:


```js
var Foo = (function (_Bar) {
  _inherits(Foo, _Bar);

  function Foo() {
    _classCallCheck(this, Foo);

    for (var _len = arguments.length, args = Array(_len), _key = 0; _key < _len; _key++) {
      args[_key] = arguments[_key];
    }

    _Bar.call.apply(_Bar, [this].concat(args));
    this.foo = 1;
  }

  return Foo;
})(Bar);
```

This causes a lot of needless work/allocations and some very strange
code (`.call.apply` o_0).

Admittedly, this is not strictly tsc’s problem; it could have done a
deeper analysis of the code and optimized out the extra dance. However,
tsc could also have emitted this simpler, more concise and semantically
equivalent code in the first place:


```js
class Foo extends Bar {
  constructor() {
    super(…arguments);
    this.foo = 1;
  }
}
```

Which compiles into the following in Babel:

```js
var Foo = (function (_Bar) {
  _inherits(Foo, _Bar);

  function Foo() {
    _classCallCheck(this, Foo);

    _Bar.apply(this, arguments);
    this.foo = 1;
  }

  return Foo;
})(Bar);
```

Which is well-optimized (today) in most engines and much less confusing
to read.

As far as I can tell, the proposed compilation has exactly the same
semantics as before.

Fixes microsoft#10175
@chancancode chancancode force-pushed the constructor-splat-arguments branch from 09e72bf to cc2dc3a Compare August 7, 2016 06:25
@DanielRosenwasser
Copy link
Member

(Might be a separate bug) TypeScript does not allow super(...arguments):

Yup, see #7596.

@bterlson
Copy link
Member

bterlson commented Aug 7, 2016

I don't know about claims of efficiency as arguments object comes with its own amount of overhead but @chancancode's suggestion is semantically equivalent (with the exception of the constructor's toString() output).

@chancancode
Copy link
Contributor Author

Any updates on this? 😄

We are getting close to shipping so we need a solution for this. We can use my branch, write an AST transform etc, but if an official fix is on the horizon (even just in nightly), then we won't bother coming up with our own solution.

@DanielRosenwasser
Copy link
Member

👍, @mhegazy?

@mhegazy
Copy link
Contributor

mhegazy commented Aug 15, 2016

👍

@mhegazy
Copy link
Contributor

mhegazy commented Aug 15, 2016

@rbuckton any comments?

@mhegazy
Copy link
Contributor

mhegazy commented Aug 15, 2016

@yuit we need to port this to transforms branch

@rbuckton
Copy link
Contributor

This looks like an acceptable change to me.

@RyanCavanaugh
Copy link
Member

@bterlson seems good?

@bterlson
Copy link
Member

@RyanCavanaugh seems fine to me, although a comment documenting the seeming deviation from the letter of ES6 would be appreciated I think.

@DanielRosenwasser
Copy link
Member

a comment documenting the seeming deviation from the letter of ES6 would be appreciated I think.

Good idea.

@chancancode can you leave the original comment and document the deviation?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Aug 16, 2016

Merged, we'll add the comment for you 😄

@chancancode chancancode deleted the constructor-splat-arguments branch August 16, 2016 22:42
@chancancode
Copy link
Contributor Author

Thank you @DanielRosenwasser!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants