From cebb785a39c8d82c792bfd928076b8bad3815344 Mon Sep 17 00:00:00 2001 From: bradfordcsmith Date: Wed, 27 Jun 2018 15:06:34 -0700 Subject: [PATCH] Generated constructors should call `super(...arguments);` They were using `super.apply(this, arguments)` which is invalid ES6 syntax. Also cleans up some obsolete code that was only needed for NTI. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=202377340 --- .../javascript/jscomp/Es6ConvertSuper.java | 9 +++--- .../Es6ConvertSuperConstructorCalls.java | 18 +++++------ .../javascript/jscomp/Es6RewriteClass.java | 10 +----- .../jscomp/TranspilationPasses.java | 2 +- .../jscomp/Es6ConvertSuperTest.java | 2 +- .../jscomp/Es6RewriteClassTest.java | 32 ++++++------------- .../jscomp/Es6ToEs3ConverterTest.java | 7 +++- 7 files changed, 31 insertions(+), 49 deletions(-) diff --git a/src/com/google/javascript/jscomp/Es6ConvertSuper.java b/src/com/google/javascript/jscomp/Es6ConvertSuper.java index a76e9b4b76f..43cf240fb45 100644 --- a/src/com/google/javascript/jscomp/Es6ConvertSuper.java +++ b/src/com/google/javascript/jscomp/Es6ConvertSuper.java @@ -84,10 +84,11 @@ private void addSyntheticConstructor(Node classNode) { // A call to super() shouldn't actually exist for these cases and is problematic to // transpile, so don't generate it. if (!classNode.isFromExterns() && !isInterface(classNode)) { - Node exprResult = IR.exprResult(IR.call( - IR.getprop(IR.superNode(), IR.string("apply")), - IR.thisNode(), - IR.name("arguments"))); + // Generate required call to super() + // `super(...arguments);` + // Note that transpilation of spread args must occur after this pass for this to work. + Node exprResult = + IR.exprResult(NodeUtil.newCallNode(IR.superNode(), IR.spread(IR.name("arguments")))); body.addChildToFront(exprResult); } Node constructor = IR.function( diff --git a/src/com/google/javascript/jscomp/Es6ConvertSuperConstructorCalls.java b/src/com/google/javascript/jscomp/Es6ConvertSuperConstructorCalls.java index d6df9bb8075..6d0d5e9fa07 100644 --- a/src/com/google/javascript/jscomp/Es6ConvertSuperConstructorCalls.java +++ b/src/com/google/javascript/jscomp/Es6ConvertSuperConstructorCalls.java @@ -276,7 +276,10 @@ private Node createNewSuperCall(String superClassQName, Node superCall) { newSuperCall.putBooleanProp(Node.FREE_CALL, false); // callee is now a getprop newSuperCall.addChildAfter(IR.thisNode().useSourceInfoFrom(callee), superClassDotCall); } else { - // super.apply(null|this, ...) -> SuperClass.apply(this, ...) + // spread transpilation does + // super(...arguments) -> super.apply(null, arguments) + // Now we must do + // super.apply(null, arguments) -> SuperClass.apply(this, arguments) checkState(callee.isGetProp(), callee); Node applyNode = checkNotNull(callee.getSecondChild()); checkState(applyNode.getString().equals("apply"), applyNode); @@ -286,15 +289,10 @@ private Node createNewSuperCall(String superClassQName, Node superCall) { callee.replaceChild( superNode, NodeUtil.newQName(compiler, superClassQName).useSourceInfoFromForTree(superNode)); - // super.apply(null, ...) is generated by spread transpilation - // super.apply(this, arguments) is used by Es6ConvertSuper in automatically-generated - // constructors. - Node nullOrThisNode = superCall.getFirstChild(); - if (!nullOrThisNode.isThis()) { - checkState(nullOrThisNode.isNull(), nullOrThisNode); - superCall.removeChild(nullOrThisNode); - newSuperCall.addChildToBack(IR.thisNode().useSourceInfoFrom(nullOrThisNode)); - } + Node nullNode = superCall.getFirstChild(); + checkState(nullNode.isNull(), nullNode); + superCall.removeChild(nullNode); + newSuperCall.addChildToBack(IR.thisNode().useSourceInfoFrom(nullNode)); } while (superCall.hasChildren()) { newSuperCall.addChildToBack(superCall.removeFirstChild()); diff --git a/src/com/google/javascript/jscomp/Es6RewriteClass.java b/src/com/google/javascript/jscomp/Es6RewriteClass.java index 333374e975e..db5d11d55f1 100644 --- a/src/com/google/javascript/jscomp/Es6RewriteClass.java +++ b/src/com/google/javascript/jscomp/Es6RewriteClass.java @@ -44,9 +44,6 @@ public final class Es6RewriteClass implements NodeTraversal.Callback, HotSwapCom private static final FeatureSet features = FeatureSet.BARE_MINIMUM.with(Feature.CLASSES, Feature.NEW_TARGET); - // Whether to add $jscomp.inherits(Parent, Child) for each subclass. - private final boolean shouldAddInheritsPolyfill; - static final DiagnosticType DYNAMIC_EXTENDS_TYPE = DiagnosticType.error( "JSC_DYNAMIC_EXTENDS_TYPE", "The class in an extends clause must be a qualified name."); @@ -63,12 +60,7 @@ public final class Es6RewriteClass implements NodeTraversal.Callback, HotSwapCom static final String INHERITS = "$jscomp.inherits"; public Es6RewriteClass(AbstractCompiler compiler) { - this(compiler, true); - } - - public Es6RewriteClass(AbstractCompiler compiler, boolean shouldAddInheritsPolyfill) { this.compiler = compiler; - this.shouldAddInheritsPolyfill = shouldAddInheritsPolyfill; } @Override @@ -224,7 +216,7 @@ private void visitClass(final NodeTraversal t, final Node classNode, final Node IR.string(superClassString)), metadata.superClassNameNode.getSourceFileName())); } else { - if (shouldAddInheritsPolyfill && !classNode.isFromExterns()) { + if (!classNode.isFromExterns()) { Node classNameNode = NodeUtil.newQName(compiler, metadata.fullClassName) .useSourceInfoIfMissingFrom(metadata.classNameNode); Node superClassNameNode = metadata.superClassNameNode.cloneTree(); diff --git a/src/com/google/javascript/jscomp/TranspilationPasses.java b/src/com/google/javascript/jscomp/TranspilationPasses.java index 73cde10e031..01ed58e3cf0 100644 --- a/src/com/google/javascript/jscomp/TranspilationPasses.java +++ b/src/com/google/javascript/jscomp/TranspilationPasses.java @@ -266,7 +266,7 @@ protected FeatureSet featureSet() { new HotSwapPassFactory("Es6RewriteClass") { @Override protected HotSwapCompilerPass create(AbstractCompiler compiler) { - return new Es6RewriteClass(compiler, true); + return new Es6RewriteClass(compiler); } @Override diff --git a/test/com/google/javascript/jscomp/Es6ConvertSuperTest.java b/test/com/google/javascript/jscomp/Es6ConvertSuperTest.java index c233c31270d..a971d5a5005 100644 --- a/test/com/google/javascript/jscomp/Es6ConvertSuperTest.java +++ b/test/com/google/javascript/jscomp/Es6ConvertSuperTest.java @@ -425,7 +425,7 @@ public void testSynthesizingConstructorOfDerivedClassInSource() { "", "class B extends A {", " /** @param {...?} var_args */", - " constructor(var_args) { super.apply(this, arguments); }", + " constructor(var_args) { super(...arguments); }", "}"))); } diff --git a/test/com/google/javascript/jscomp/Es6RewriteClassTest.java b/test/com/google/javascript/jscomp/Es6RewriteClassTest.java index 220713c795c..724abdc5d0d 100644 --- a/test/com/google/javascript/jscomp/Es6RewriteClassTest.java +++ b/test/com/google/javascript/jscomp/Es6RewriteClassTest.java @@ -28,8 +28,6 @@ public final class Es6RewriteClassTest extends CompilerTestCase { - private boolean shouldAddInheritsPolyfill; - private static final String EXTERNS_BASE = lines( "/** @constructor @template T */", @@ -73,7 +71,6 @@ protected void setUp() throws Exception { setAcceptedLanguage(LanguageMode.ECMASCRIPT_2015); setLanguageOut(LanguageMode.ECMASCRIPT3); enableRunTypeCheckAfterProcessing(); - shouldAddInheritsPolyfill = true; } protected final PassFactory makePassFactory( @@ -97,13 +94,15 @@ protected CompilerPass getProcessor(final Compiler compiler) { optimizer.addOneTimePass( makePassFactory("es6ConvertSuper", new Es6ConvertSuper(compiler))); optimizer.addOneTimePass(makePassFactory("es6ExtractClasses", new Es6ExtractClasses(compiler))); - optimizer.addOneTimePass(makePassFactory("es6RewriteClass", - new Es6RewriteClass(compiler, shouldAddInheritsPolyfill))); - if (shouldAddInheritsPolyfill) { - optimizer.addOneTimePass( - makePassFactory( - "Es6ConvertSuperConstructorCalls", new Es6ConvertSuperConstructorCalls(compiler))); - } + optimizer.addOneTimePass(makePassFactory("es6RewriteClass", new Es6RewriteClass(compiler))); + // Automatically generated constructor calls will contain a call to super() using spread. + // super(...arguments); + // We depend on that getting rewritten before we do the super constructor call rewriting. + optimizer.addOneTimePass( + makePassFactory("es6RewriteRestAndSpread", new Es6RewriteRestAndSpread(compiler))); + optimizer.addOneTimePass( + makePassFactory( + "Es6ConvertSuperConstructorCalls", new Es6ConvertSuperConstructorCalls(compiler))); return optimizer; } @@ -622,19 +621,6 @@ public void testExtends() { "let C = function(var_args) {};")); } - public void testExtendsWithoutInheritsPolyfill() { - shouldAddInheritsPolyfill = false; - test("class D {} class C extends D {}", - lines( - "/** @constructor @struct */", - "let D = function() {};", - "/** @constructor @struct", - " * @extends {D}", - " * @param {...?} var_args", - " */", - "let C = function(var_args) {super.apply(this,arguments); };")); - } - public void testExtendNonNativeError() { test( lines( diff --git a/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java b/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java index 499841ffa13..1003ecc1367 100644 --- a/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java +++ b/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java @@ -128,6 +128,11 @@ protected CompilerPass getProcessor(final Compiler compiler) { optimizer.addOneTimePass(makePassFactory("es6RewriteClass", new Es6RewriteClass(compiler))); optimizer.addOneTimePass( makePassFactory("es6InjectRuntimeLibraries", new Es6InjectRuntimeLibraries(compiler))); + // Automatically generated constructor calls will contain a call to super() using spread. + // super(...arguments); + // We depend on that getting rewritten before we do the super constructor call rewriting. + optimizer.addOneTimePass( + makePassFactory("es6RewriteRestAndSpread", new Es6RewriteRestAndSpread(compiler))); optimizer.addOneTimePass( makePassFactory("convertEs6Late", new LateEs6ToEs3Converter(compiler))); optimizer.addOneTimePass(makePassFactory("es6ForOf", new Es6ForOfConverter(compiler))); @@ -151,7 +156,7 @@ public void testObjectLiteralStringKeysWithNoValue() { } public void testSpreadLibInjection() { - testSame("var x = [...a];"); + test("var x = [...a];", "var x=[].concat($jscomp.arrayFromIterable(a))"); assertThat(getLastCompiler().injected).containsExactly("es6/util/arrayfromiterable"); }