Decompose class extends calls

[ChadKillingsworth]

When the extends clause of a class is not a qualified name, alias the expression so that it can be transpiled.

Allows GETELEM and Mixin functions extends to be correctly transpiled.

[lauraharker]

@ChadKillingsworth I merged a4d3536 in today which made your unit test start failing. sorry :)

testMixinFunction(com.google.javascript.jscomp.Es6RewriteClassExtendsExpressionsTest)java.lang.IllegalStateException: SCRIPT node should be marked as containing feature const declaration.

You should be able to fix in Es6RewriteClassExtendsExpressions with something like:

NodeUtil.markFeatureAsAdded(t.getEnclosingFile(), Feature.CONST_DECLARATIONS)

Also, could this cause problems with the order in which side effects occur?

e.g. converting

f(somethingWithSideEffects1(), class extends somethingWithSideEffects2() {});

to

const tmpExtendsVar = somethingWithSideEffects2();
f(somethingWithSideEffects1(), class extends tmpExtendsVar {});

[ChadKillingsworth]

Yes - the side effects could be problematic. Here's how Babel transpiles your example:

"use strict";

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

function _possibleConstructorReturn(self, call) { if (!self) { throw new ReferenceError("this hasn't been initialised - super() hasn't been called"); } return call && (typeof call === "object" || typeof call === "function") ? call : self; }

function _inherits(subClass, superClass) { if (typeof superClass !== "function" && superClass !== null) { throw new TypeError("Super expression must either be null or a function, not " + typeof superClass); } subClass.prototype = Object.create(superClass && superClass.prototype, { constructor: { value: subClass, enumerable: false, writable: true, configurable: true } }); if (superClass) Object.setPrototypeOf ? Object.setPrototypeOf(subClass, superClass) : subClass.__proto__ = superClass; }

f(somethingWithSideEffects1(), function (_somethingWithSideEff) {
  _inherits(_class, _somethingWithSideEff);

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

    return _possibleConstructorReturn(this, (_class.__proto__ || Object.getPrototypeOf(_class)).apply(this, arguments));
  }

  return _class;
}(somethingWithSideEffects2()));

I can do something similar for this case - but it will be almost completely un-typecheckable without tremendous work.

The other option would be to restrict this to the common case where the class is not used in an expression. That alone would get us a large win.

[lauraharker]

Just fixing this for class declarations now sounds good to me. After moving class transpilation post-typechecking, we could do something like your Babel example.

[ChadKillingsworth]

Fixed review issues and added the restriction that the class must not be used as part of an expression. Rebased to master and commits squashed.

[ChadKillingsworth]

I realized there were several more common cases where this is safe to do. I updated this PR to include them (and tests).

[lauraharker]

Importing for internal review.

[lauraharker]

Copying @concavelenz 's comments from internal review

[lauraharker]

needs some tests for things we can't/won't move.

[brad4d]

I think we have the tests we need now, but will address this with @lauraharker in internal review.

[lauraharker]

We would need to check the LHS of the expression to see if it has side-effects:

get(x++).foo = class extends get(x++).bar {}

[lauraharker]

Why here instead of immediate before class rewriting? Alternately, why isn't it part of class rewriting?

[ChadKillingsworth]

See the comment at https://github.com/google/closure-compiler/pull/2997/files#diff-fa51555d8bc30554f2cbba232a8033bdR41

This must be performed before transpilation of super calls. So much of class rewriting depends on the extends clause being a qualified name.

[ChadKillingsworth]

@lauraharker I addressed the review issues.

One note: I decided to go ahead an handle all the cases. When possible, the decomposition happens with a simple alias. However in complex expressions, the class node is now returned decomposed in an IIFE. While these nodes will probably not have very good type checking, that's better than simply failing to transpile valid syntax.

[brad4d]

Laura and I agreed I should take this PR over, since I have some concerns.

[brad4d]

here are my concerns.

1. We're now actively working to move class transpilation to happen after all of the checks passes.
   That's pretty tricky, and I'm reluctant to add yet another pass that will have to be moved.
2. We already have a pass that does something similar, Es6ExtractClasses, and I feel like the extraction done here would be better done in that pass.

I know number 2 is a no-go right now for sure, because apparently this extraction needs to happen before Es6ConvertSuper, which happens quite early.

I believe we'd be better off waiting until we get the transpilation passes moved before attempting this change.

1. I think some of the problems this is trying to solve may disappear once transpilation moves after type checking.
2. I think the pass itself can be simpler if we don't have to worry about type checking happening after it, as mentioned in earlier comments on this PR.

Exactly how long it takes to complete the move of class transpilation passes depends on how many issues we hit, but I'm expecting 2 to 4 weeks.

@ChadKillingsworth can you wait that long?

[ChadKillingsworth]

The lack of support for transpilation of these classes is very frustrating. It's common (in webpack), for ES6 imports/exports to be transpiled as GETELEMS. So if you have a class that extends a class imported from an ES6 module, it can't be transpiled. This comes up very frequently in real world code.

import {Foo} from './foo.js';

class Bar extends Foo {
}

Transpiles to something like:

const fooModule = require('foo');

class Bar extends fooModule['Foo'] {}

This transpilation step occurs before the compiler, thus rendering this code unusable by Closure Compiler.

I've taken multiple stabs at trying to solve this over the years and finally settled on a new small unique pass. I do not think this pass should complicate type checking - because these clauses aren't even allowed right now. And by decomposing simply where possible with a const, type inference should provide very good results in the vast majority of cases.

In addition, I have a strong suspicion that this pass will be needed even in cases where classes aren't transpiled. For instance, solving #3015 probably requires the extends clause to be a qualified name.

In short - I'd rather not wait for type checking. It shouldn't make that scenario worse for any currently supported code. And moving this class should be trivial - it should be movable at the same time that Es6ConvertSuper is moved.

[brad4d]

OK, I'm OK with this change, but I want to make some tweaks.
I've made them in my internal copy and am running them through tests.

@ChadKillingsworth I'll push my changes back out to this PR so you can review them.

I've taken this PR over from Laura

[ChadKillingsworth]

@brad4d your changes lgtm. Thanks!

[brad4d]

Regular tests look fine. Now testing all the things.
Sent to @blickly for internal review.

[brad4d]

Submitted internally, should be in GitHub tomorrow.

[aomarks]

@ChadKillingsworth How would you tell Closure here that Bar should be augmented with the interface of the internal class? The way we've typically been doing that is to manually define an interface corresponding to the inner mixin class, and then assert that the base class @implements it:

/** @interface */
MixinInterface = function(){};
/** @return {string} */
MixinInterface.prototype.bar = function(){};

/**
 * @constructor
 * @extends {SuperClass}
 * @implements {MixinInterface}
 */
const base = Mixin(SuperClass);

class Foo extends base {
}

My understanding is that if you were to put the @implements annotation directly on the Foo class, Closure would require that Foo implements it, rather than trusting that it is implemented as in the separate @constructor case.

[ChadKillingsworth]

This isn't fully supported yet - this is just step one. This allows the compiler to successfully transpile this code. Type checking is yet to come.