Skip to content

Commit

Permalink
Rewrite internal references to classes so that we don't crash on thin…
Browse files Browse the repository at this point in the history 
…gs like

exports = class Foo { someMethod() { Foo.someStaticMethod(); } };

in a goog.module.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125496086
  • Loading branch information
@blickly
tbreisacher authored and blickly committed Jun 22, 2016
1 parent 41dd503 commit b78e9d6
Show file tree
Hide file tree
Showing 5 changed files with 257 additions and 39 deletions.
88 changes: 81 additions & 7 deletions src/com/google/javascript/jscomp/Es6ExtractClasses.java
View file
Expand Up @@ -16,12 +16,17 @@


package com.google.javascript.jscomp; 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.jscomp.ExpressionDecomposer.DecompositionType;
import com.google.javascript.rhino.IR; import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token; import com.google.javascript.rhino.Token;


import java.util.Deque;
import java.util.HashSet; import java.util.HashSet;
import java.util.LinkedList;
import java.util.Set; import java.util.Set;


/** /**
Expand Down Expand Up @@ -61,11 +66,13 @@ public final class Es6ExtractClasses
@Override @Override
public void process(Node externs, Node root) { public void process(Node externs, Node root) {
NodeTraversal.traverseRootsEs6(compiler, this, externs, root); NodeTraversal.traverseRootsEs6(compiler, this, externs, root);
NodeTraversal.traverseRootsEs6(compiler, new SelfReferenceRewriter(), externs, root);
} }


@Override @Override
public void hotSwapScript(Node scriptRoot, Node originalRoot) { public void hotSwapScript(Node scriptRoot, Node originalRoot) {
NodeTraversal.traverseEs6(compiler, scriptRoot, this); NodeTraversal.traverseEs6(compiler, scriptRoot, this);
NodeTraversal.traverseEs6(compiler, scriptRoot, new SelfReferenceRewriter());
} }


@Override @Override
Expand All @@ -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<ClassDescription> 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) { private boolean shouldExtractClass(Node classNode, Node parent) {
boolean isAnonymous = classNode.getFirstChild().isEmpty();
if (NodeUtil.isClassDeclaration(classNode) if (NodeUtil.isClassDeclaration(classNode)
|| parent.isName() || isAnonymous && parent.isName()
|| (parent.isAssign() && parent.getParent().isExprResult())) { || (isAnonymous && parent.isAssign() && parent.getParent().isExprResult())) {
// No need to extract. Handled directly by Es6ToEs3Converter.ClassDeclarationMetadata#create. // No need to extract. Handled directly by Es6ToEs3Converter.ClassDeclarationMetadata#create.
return false; return false;
} }
// Don't extract the class if it's not safe to do so. For example,
// var c = maybeTrue() && class extends someSideEffect() {}; if (NodeUtil.mayHaveSideEffects(classNode)
// TODO(brndn): it is possible to be less conservative. If the classNode is DECOMPOSABLE, // Don't extract the class if it's not safe to do so. For example,
// we could use the expression decomposer to move it out of the way. // var c = maybeTrue() && class extends someSideEffect() {};
return expressionDecomposer.canExposeExpression(classNode) == DecompositionType.MOVABLE; // 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) { private void extractClass(Node classNode, Node parent) {
Expand Down
7 changes: 4 additions & 3 deletions src/com/google/javascript/jscomp/Es6ToEs3Converter.java
View file
Expand Up @@ -45,6 +45,7 @@
* *
* @author tbreisacher@google.com (Tyler Breisacher) * @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 { public final class Es6ToEs3Converter implements NodeTraversal.Callback, HotSwapCompilerPass {
private final AbstractCompiler compiler; private final AbstractCompiler compiler;


Expand Down Expand Up @@ -544,9 +545,9 @@ private void visitClass(final Node classNode, final Node parent) {
ClassDeclarationMetadata metadata = ClassDeclarationMetadata.create(classNode, parent); ClassDeclarationMetadata metadata = ClassDeclarationMetadata.create(classNode, parent);


if (metadata == null || metadata.fullClassName == null) { if (metadata == null || metadata.fullClassName == null) {
cannotConvert(parent, "Can only convert classes that are declarations or the right hand" throw new IllegalStateException(
+ " side of a simple assignment."); "Can only convert classes that are declarations or the right hand"
return; + " side of a simple assignment: " + classNode);
} }
if (metadata.hasSuperClass() && !metadata.superClassNameNode.isQualifiedName()) { if (metadata.hasSuperClass() && !metadata.superClassNameNode.isQualifiedName()) {
compiler.report(JSError.make(metadata.superClassNameNode, DYNAMIC_EXTENDS_TYPE)); compiler.report(JSError.make(metadata.superClassNameNode, DYNAMIC_EXTENDS_TYPE));
Expand Down
2 changes: 1 addition & 1 deletion test/com/google/javascript/jscomp/CommandLineRunnerTest.java
View file
Expand Up @@ -1494,7 +1494,7 @@ public void testES3() {
} }


public void testES6TranspiledByDefault() { public void testES6TranspiledByDefault() {
test("var x = class X {};", "var x = function() {};"); test("var x = class {};", "var x = function() {};");
} }


public void testES5ChecksByDefault() { public void testES5ChecksByDefault() {
Expand Down
107 changes: 100 additions & 7 deletions test/com/google/javascript/jscomp/Es6ExtractClassesTest.java
View file
Expand Up @@ -16,6 +16,8 @@


package com.google.javascript.jscomp; package com.google.javascript.jscomp;


import static com.google.javascript.jscomp.Es6ToEs3Converter.CANNOT_CONVERT;

import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.jscomp.CompilerOptions.LanguageMode;


public final class Es6ExtractClassesTest extends CompilerTestCase { public final class Es6ExtractClassesTest extends CompilerTestCase {
Expand All @@ -36,7 +38,86 @@ public void testExtractionFromCall() {
test( test(
"f(class{});", "f(class{});",
LINE_JOINER.join( 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() { public void testConstAssignment() {
Expand Down Expand Up @@ -64,22 +145,34 @@ public void testVarAssignment() {
} }


public void testConditionalBlocksExtractionFromCall() { public void testConditionalBlocksExtractionFromCall() {
testSame("maybeTrue() && f(class{});"); testError("maybeTrue() && f(class{});", CANNOT_CONVERT);
} }


public void testExtractionFromArrayLiteral() { public void testExtractionFromArrayLiteral() {
test( test(
"var c = [class C {}];", "var c = [class C {}];",
LINE_JOINER.join( 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() { public void testTernaryOperatorBlocksExtraction() {
testSame("var c = maybeTrue() && [class {}]"); testError("var c = maybeTrue() ? class A {} : anotherExpr", CANNOT_CONVERT);
testError("var c = maybeTrue() ? anotherExpr : class B {}", CANNOT_CONVERT);
} }


public void testTernaryOperatorBlocksExtraction() { public void testCannotExtract() {
testSame("var c = maybeTrue() ? class A {} : class B {}"); 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() { public void testClassesHandledByEs6ToEs3Converter() {
Expand Down

0 comments on commit b78e9d6

Please sign in to comment.