From b78e9d690bca36ba0b78ac1330980142a613b8bd Mon Sep 17 00:00:00 2001 From: tbreisacher Date: Tue, 21 Jun 2016 14:33:54 -0700 Subject: [PATCH] Rewrite internal references to classes so that we don't crash on things like exports = class Foo { someMethod() { Foo.someStaticMethod(); } }; in a goog.module. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=125496086 --- .../javascript/jscomp/Es6ExtractClasses.java | 88 ++++++++++++-- .../javascript/jscomp/Es6ToEs3Converter.java | 7 +- .../jscomp/CommandLineRunnerTest.java | 2 +- .../jscomp/Es6ExtractClassesTest.java | 107 ++++++++++++++++-- .../jscomp/Es6ToEs3ConverterTest.java | 92 +++++++++++---- 5 files changed, 257 insertions(+), 39 deletions(-) diff --git a/src/com/google/javascript/jscomp/Es6ExtractClasses.java b/src/com/google/javascript/jscomp/Es6ExtractClasses.java index 8e961c0bf20..af0f5cc578e 100644 --- a/src/com/google/javascript/jscomp/Es6ExtractClasses.java +++ b/src/com/google/javascript/jscomp/Es6ExtractClasses.java @@ -16,12 +16,17 @@ package com.google.javascript.jscomp; +import static com.google.javascript.jscomp.Es6ToEs3Converter.CANNOT_CONVERT; + +import com.google.common.base.Preconditions; import com.google.javascript.jscomp.ExpressionDecomposer.DecompositionType; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; +import java.util.Deque; import java.util.HashSet; +import java.util.LinkedList; import java.util.Set; /** @@ -61,11 +66,13 @@ public final class Es6ExtractClasses @Override public void process(Node externs, Node root) { NodeTraversal.traverseRootsEs6(compiler, this, externs, root); + NodeTraversal.traverseRootsEs6(compiler, new SelfReferenceRewriter(), externs, root); } @Override public void hotSwapScript(Node scriptRoot, Node originalRoot) { NodeTraversal.traverseEs6(compiler, scriptRoot, this); + NodeTraversal.traverseEs6(compiler, scriptRoot, new SelfReferenceRewriter()); } @Override @@ -75,18 +82,85 @@ public void visit(NodeTraversal t, Node n, Node parent) { } } + private class SelfReferenceRewriter implements NodeTraversal.Callback { + private class ClassDescription { + Node nameNode; + String outerName; + + ClassDescription(Node nameNode, String outerName) { + this.nameNode = nameNode; + this.outerName = outerName; + } + } + + private Deque classStack = new LinkedList<>(); + + private boolean needsInnerNameRewriting(Node classNode, Node parent) { + Preconditions.checkArgument(classNode.isClass()); + return classNode.getFirstChild().isName() && parent.isName(); + } + + @Override + public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { + if (n.isClass() && needsInnerNameRewriting(n, parent)) { + classStack.addFirst(new ClassDescription(n.getFirstChild(), parent.getString())); + } + return true; + } + + @Override + public void visit(NodeTraversal t, Node n, Node parent) { + switch (n.getType()) { + case CLASS: + if (needsInnerNameRewriting(n, parent)) { + classStack.removeFirst(); + n.replaceChild(n.getFirstChild(), IR.empty().useSourceInfoFrom(n.getFirstChild())); + compiler.reportCodeChange(); + } + break; + case NAME: + maybeUpdateClassSelfRef(t, n, parent); + break; + default: + break; + } + } + + private void maybeUpdateClassSelfRef(NodeTraversal t, Node nameNode, Node parent) { + for (ClassDescription klass : classStack) { + if (nameNode != klass.nameNode && nameNode.matchesQualifiedName(klass.nameNode)) { + Var var = t.getScope().getVar(nameNode.getString()); + if (var != null && var.getNameNode() == klass.nameNode) { + parent.replaceChild(nameNode, IR.name(klass.outerName).useSourceInfoFrom(nameNode)); + compiler.reportCodeChange(); + return; + } + } + } + } + } + private boolean shouldExtractClass(Node classNode, Node parent) { + boolean isAnonymous = classNode.getFirstChild().isEmpty(); if (NodeUtil.isClassDeclaration(classNode) - || parent.isName() - || (parent.isAssign() && parent.getParent().isExprResult())) { + || isAnonymous && parent.isName() + || (isAnonymous && parent.isAssign() && parent.getParent().isExprResult())) { // No need to extract. Handled directly by Es6ToEs3Converter.ClassDeclarationMetadata#create. return false; } - // Don't extract the class if it's not safe to do so. For example, - // var c = maybeTrue() && class extends someSideEffect() {}; - // TODO(brndn): it is possible to be less conservative. If the classNode is DECOMPOSABLE, - // we could use the expression decomposer to move it out of the way. - return expressionDecomposer.canExposeExpression(classNode) == DecompositionType.MOVABLE; + + if (NodeUtil.mayHaveSideEffects(classNode) + // Don't extract the class if it's not safe to do so. For example, + // var c = maybeTrue() && class extends someSideEffect() {}; + // TODO(brndn): it is possible to be less conservative. If the classNode is DECOMPOSABLE, + // we could use the expression decomposer to move it out of the way. + || expressionDecomposer.canExposeExpression(classNode) != DecompositionType.MOVABLE) { + compiler.report( + JSError.make(classNode, CANNOT_CONVERT, "class expression that cannot be extracted")); + return false; + } + + return true; } private void extractClass(Node classNode, Node parent) { diff --git a/src/com/google/javascript/jscomp/Es6ToEs3Converter.java b/src/com/google/javascript/jscomp/Es6ToEs3Converter.java index 2dc7af3acdc..16a4bad6eae 100644 --- a/src/com/google/javascript/jscomp/Es6ToEs3Converter.java +++ b/src/com/google/javascript/jscomp/Es6ToEs3Converter.java @@ -45,6 +45,7 @@ * * @author tbreisacher@google.com (Tyler Breisacher) */ +// TODO(tbreisacher): This class does too many things. Break it into smaller passes. public final class Es6ToEs3Converter implements NodeTraversal.Callback, HotSwapCompilerPass { private final AbstractCompiler compiler; @@ -544,9 +545,9 @@ private void visitClass(final Node classNode, final Node parent) { ClassDeclarationMetadata metadata = ClassDeclarationMetadata.create(classNode, parent); if (metadata == null || metadata.fullClassName == null) { - cannotConvert(parent, "Can only convert classes that are declarations or the right hand" - + " side of a simple assignment."); - return; + throw new IllegalStateException( + "Can only convert classes that are declarations or the right hand" + + " side of a simple assignment: " + classNode); } if (metadata.hasSuperClass() && !metadata.superClassNameNode.isQualifiedName()) { compiler.report(JSError.make(metadata.superClassNameNode, DYNAMIC_EXTENDS_TYPE)); diff --git a/test/com/google/javascript/jscomp/CommandLineRunnerTest.java b/test/com/google/javascript/jscomp/CommandLineRunnerTest.java index 38c7a79b02e..7e48ab757c0 100644 --- a/test/com/google/javascript/jscomp/CommandLineRunnerTest.java +++ b/test/com/google/javascript/jscomp/CommandLineRunnerTest.java @@ -1494,7 +1494,7 @@ public void testES3() { } public void testES6TranspiledByDefault() { - test("var x = class X {};", "var x = function() {};"); + test("var x = class {};", "var x = function() {};"); } public void testES5ChecksByDefault() { diff --git a/test/com/google/javascript/jscomp/Es6ExtractClassesTest.java b/test/com/google/javascript/jscomp/Es6ExtractClassesTest.java index 67c92b0d4eb..2a97c6b75ac 100644 --- a/test/com/google/javascript/jscomp/Es6ExtractClassesTest.java +++ b/test/com/google/javascript/jscomp/Es6ExtractClassesTest.java @@ -16,6 +16,8 @@ package com.google.javascript.jscomp; +import static com.google.javascript.jscomp.Es6ToEs3Converter.CANNOT_CONVERT; + import com.google.javascript.jscomp.CompilerOptions.LanguageMode; public final class Es6ExtractClassesTest extends CompilerTestCase { @@ -36,7 +38,86 @@ public void testExtractionFromCall() { test( "f(class{});", LINE_JOINER.join( - "const testcode$classdecl$var0 = class {};", "f(testcode$classdecl$var0);")); + "const testcode$classdecl$var0 = class {};", + "f(testcode$classdecl$var0);")); + } + + public void testSelfReference1() { + test( + "var Outer = class Inner { constructor() { alert(Inner); } };", + LINE_JOINER.join( + "const testcode$classdecl$var0 = class {", + " constructor() { alert(testcode$classdecl$var0); }", + "};", + "var Outer=testcode$classdecl$var0")); + + test( + "let Outer = class Inner { constructor() { alert(Inner); } };", + LINE_JOINER.join( + "const testcode$classdecl$var0 = class {", + " constructor() { alert(testcode$classdecl$var0); }", + "};", + "let Outer=testcode$classdecl$var0")); + + test( + "const Outer = class Inner { constructor() { alert(Inner); } };", + LINE_JOINER.join( + "const testcode$classdecl$var0 = class {", + " constructor() { alert(testcode$classdecl$var0); }", + "};", + "const Outer=testcode$classdecl$var0")); + } + + public void testSelfReference2() { + test( + "alert(class C { constructor() { alert(C); } });", + LINE_JOINER.join( + "const testcode$classdecl$var0 = class {", + " constructor() { alert(testcode$classdecl$var0); }", + "};", + "alert(testcode$classdecl$var0)")); + } + + public void testSelfReference3() { + test( + LINE_JOINER.join( + "alert(class C {", + " m1() { class C {}; alert(C); }", + " m2() { alert(C); }", + "});"), + LINE_JOINER.join( + "const testcode$classdecl$var0 = class {", + " m1() { class C {}; alert(C); }", + " m2() { alert(testcode$classdecl$var0); }", + "};", + "alert(testcode$classdecl$var0)")); + } + + public void testSelfReference_googModule() { + test( + LINE_JOINER.join( + "goog.module('example');", + "exports = class Inner { constructor() { alert(Inner); } };"), + LINE_JOINER.join( + "goog.module('example');", + "const testcode$classdecl$var0 = class {", + " constructor() {", + " alert(testcode$classdecl$var0);", + " }", + "};", + "exports = testcode$classdecl$var0;")); + } + + public void testSelfReference_qualifiedName() { + test( + "outer.qual.Name = class Inner { constructor() { alert(Inner); } };", + LINE_JOINER.join( + "const testcode$classdecl$var0 = class {", + " constructor() {", + " alert(testcode$classdecl$var0);", + " }", + "};", + "outer.qual.Name = testcode$classdecl$var0;")); } public void testConstAssignment() { @@ -64,22 +145,34 @@ public void testVarAssignment() { } public void testConditionalBlocksExtractionFromCall() { - testSame("maybeTrue() && f(class{});"); + testError("maybeTrue() && f(class{});", CANNOT_CONVERT); } public void testExtractionFromArrayLiteral() { test( "var c = [class C {}];", LINE_JOINER.join( - "const testcode$classdecl$var0 = class C {};", "var c = [testcode$classdecl$var0];")); + "const testcode$classdecl$var0 = class {};", + "var c = [testcode$classdecl$var0];")); } - public void testConditionalBlocksExtractionFromArrayLiteral() { - testSame("var c = maybeTrue() && [class {}]"); + public void testTernaryOperatorBlocksExtraction() { + testError("var c = maybeTrue() ? class A {} : anotherExpr", CANNOT_CONVERT); + testError("var c = maybeTrue() ? anotherExpr : class B {}", CANNOT_CONVERT); } - public void testTernaryOperatorBlocksExtraction() { - testSame("var c = maybeTrue() ? class A {} : class B {}"); + public void testCannotExtract() { + testError( + "var c = maybeTrue() && class A extends sideEffect() {}", + CANNOT_CONVERT); + + testError( + LINE_JOINER.join( + "var x;", + "function f(x, y) {}", + + "f(x = 2, class Foo { [x=3]() {} });"), + CANNOT_CONVERT); } public void testClassesHandledByEs6ToEs3Converter() { diff --git a/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java b/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java index 222717e2c84..1df4abac01e 100644 --- a/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java +++ b/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java @@ -25,10 +25,13 @@ import java.util.Set; /** - * Test case for {@link Es6ToEs3Converter}. + * Test cases for ES6 transpilation. Despite the name, this isn't just testing {@link + * Es6ToEs3Converter}, but also some other ES6 transpilation passes. See #getProcessor. * * @author tbreisacher@google.com (Tyler Breisacher) */ +// TODO(tbreisacher): Rename this to Es6TranspilationIntegrationTest since it's really testing +// a lot of different passes. Also create a unit test for Es6ToEs3Converter. public final class Es6ToEs3ConverterTest extends CompilerTestCase { private static final String EXTERNS_BASE = @@ -130,8 +133,8 @@ public CompilerPass getProcessor(final Compiler compiler) { new Es6RenameVariablesInParamLists(compiler))); optimizer.addOneTimePass( makePassFactory("es6ConvertSuper", new Es6ConvertSuper(compiler))); - optimizer.addOneTimePass( - makePassFactory("convertEs6", new Es6ToEs3Converter(compiler))); + optimizer.addOneTimePass(makePassFactory("es6ExtractClasses", new Es6ExtractClasses(compiler))); + optimizer.addOneTimePass(makePassFactory("convertEs6", new Es6ToEs3Converter(compiler))); optimizer.addOneTimePass( makePassFactory("Es6RewriteBlockScopedDeclaration", new Es6RewriteBlockScopedDeclaration(compiler))); @@ -246,7 +249,14 @@ public void testClassWithNgInject() { } public void testAnonymousSuper() { - testError("f(class extends D { f() { super.g() } })", CANNOT_CONVERT); + test( + "f(class extends D { f() { super.g() } })", + LINE_JOINER.join( + "/** @constructor @struct @const @extends {D} */", + "var testcode$classdecl$var0 = function(var_args) { D.apply(this,arguments); };", + "$jscomp.inherits(testcode$classdecl$var0, D);", + "testcode$classdecl$var0.prototype.f = function() {super.g(); };", + "f(testcode$classdecl$var0)")); } public void testNewTarget() { @@ -448,15 +458,21 @@ public void testClassExpressionInVar() { "", "C.prototype.foo = function() {}")); - test("var C = class C { }", - "/** @constructor @struct */ var C = function() {}"); + test( + "var C = class C { }", + LINE_JOINER.join( + "/** @constructor @struct @const */", + "var testcode$classdecl$var0 = function() {};", + "var C = testcode$classdecl$var0;")); test( "var C = class C { foo() {} }", LINE_JOINER.join( - "/** @constructor @struct */ var C = function() {}", + "/** @constructor @struct @const */", + "var testcode$classdecl$var0 = function() {}", + "testcode$classdecl$var0.prototype.foo = function() {};", "", - "C.prototype.foo = function() {};")); + "var C = testcode$classdecl$var0;")); } /** @@ -473,15 +489,27 @@ public void testClassExpressionInAssignment() { "goog.example.C.prototype.foo = function() {};")); } + public void testClassExpression() { + test( + "var C = new (class {})();", + LINE_JOINER.join( + "/** @constructor @struct @const */", + "var testcode$classdecl$var0=function(){};", + "var C=new testcode$classdecl$var0")); + test( + "(condition ? obj1 : obj2).prop = class C { };", + LINE_JOINER.join( + "/** @constructor @struct @const */", + "var testcode$classdecl$var0 = function(){};", + "(condition ? obj1 : obj2).prop = testcode$classdecl$var0;")); + } + /** - * Class expressions that are not in a 'var' or simple 'assign' node. - * We don't bother transpiling these cases because the transpiled code - * will be very difficult to typecheck. + * We don't bother transpiling this case because the transpiled code will be very difficult to + * typecheck. */ - public void testClassExpression() { - testError("var C = new (class {})();", CANNOT_CONVERT); + public void testClassExpression_cannotConvert() { testError("var C = new (foo || (foo = class { }))();", CANNOT_CONVERT); - testError("(condition ? obj1 : obj2).prop = class C { };", CANNOT_CONVERT); } public void testExtends() { @@ -681,11 +709,19 @@ public void testComputedSuper() { } public void testMultiNameClass() { - test("var F = class G {}", - "/** @constructor @struct */ var F = function() {};"); + test( + "var F = class G {}", + LINE_JOINER.join( + "/** @constructor @struct @const */", + "var testcode$classdecl$var0 = function(){};", + "var F = testcode$classdecl$var0;")); - test("F = class G {}", - "/** @constructor @struct */ F = function() {};"); + test( + "F = class G {}", + LINE_JOINER.join( + "/** @constructor @struct @const */", + "var testcode$classdecl$var0 = function(){};", + "F = testcode$classdecl$var0;")); } public void testClassNested() { @@ -900,10 +936,24 @@ public void testMockingInFunction() { // Make sure we don't crash on this code. // https://github.com/google/closure-compiler/issues/752 public void testGithub752() { - testError("function f() { var a = b = class {};}", CANNOT_CONVERT); + test( + "function f() { var a = b = class {};}", + LINE_JOINER.join( + "function f() {", + " /** @constructor @struct @const */", + " var testcode$classdecl$var0 = function() {};", + " var a = b = testcode$classdecl$var0;", + "}")); - testError("var ns = {}; function f() { var self = ns.Child = class {};}", - CANNOT_CONVERT); + test( + "var ns = {}; function f() { var self = ns.Child = class {};}", + LINE_JOINER.join( + "var ns = {};", + "function f() {", + " /** @constructor @struct @const */", + " var testcode$classdecl$var0 = function() {};", + " var self = ns.Child = testcode$classdecl$var0", + "}")); } public void testInvalidClassUse() {