Skip to content

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

Merged
DanielRosenwasser merged 1 commit into
microsoft:masterfrom
chancancode:constructor-splat-arguments
Aug 16, 2016
Merged

Emit more efficient/concise "empty" ES6 ctor#10189
DanielRosenwasser merged 1 commit into
microsoft:masterfrom
chancancode:constructor-splat-arguments

Conversation

@chancancode

@chancancode chancancode commented Aug 6, 2016

Copy link
Copy Markdown
Contributor

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

msftclas commented Aug 6, 2016

Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

@DanielRosenwasser

Copy link
Copy Markdown
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
Copy Markdown
Member

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

Yup, see #7596.

@bterlson

bterlson commented Aug 7, 2016

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member

👍, @mhegazy?

@mhegazy

mhegazy commented Aug 15, 2016

Copy link
Copy Markdown
Contributor

👍

@mhegazy

mhegazy commented Aug 15, 2016

Copy link
Copy Markdown
Contributor

@rbuckton any comments?

@mhegazy

mhegazy commented Aug 15, 2016

Copy link
Copy Markdown
Contributor

@yuit we need to port this to transforms branch

@rbuckton

Copy link
Copy Markdown
Contributor

This looks like an acceptable change to me.

@RyanCavanaugh

Copy link
Copy Markdown
Member

@bterlson seems good?

@bterlson

Copy link
Copy Markdown
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
Copy Markdown
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

DanielRosenwasser commented Aug 16, 2016

Copy link
Copy Markdown
Member

Merged, we'll add the comment for you 😄

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

Copy link
Copy Markdown
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