Allow expression decomposition in a few places where we previously we…

…ren't allowing. These decompositions are only allowed if a new flag, --allow_method_call_decomposing, is passed, because they produce code that will not run on some browsers. This means expressions like x.someMethod(yield 1); can now be transpiled. See https://github.com/google/closure-compiler/wiki/FAQ#i-get-an-undecomposable-expression-error-for-my-yield-or-await-expression-what-do-i-do (which will need to be updated after this is submitted) ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=175870681
google · Nov 15, 2017 · 1de0f4b · 1de0f4b
1 parent 78816ee
commit 1de0f4b
Show file tree

Hide file tree

Showing 10 changed files with 324 additions and 61 deletions.
diff --git a/src/com/google/javascript/jscomp/CommandLineRunner.java b/src/com/google/javascript/jscomp/CommandLineRunner.java
@@ -758,6 +758,11 @@ private static class Flags {
         usage = "Rewrite ES6 library calls to use polyfills provided by the compiler's runtime.")
     private boolean rewritePolyfills = true;
 
+    @Option(name = "--allow_method_call_decomposing",
+        handler = BooleanOptionHandler.class,
+        usage = "Allow decomposing x.y(); to: var tmp = x.y; tmp.call(x); Unsafe on IE 8 and 9")
+    private boolean allowMethodCallDecomposing = false;
+
     @Option(
       name = "--print_source_after_each_pass",
       handler = BooleanOptionHandler.class,
@@ -1728,6 +1733,8 @@ protected CompilerOptions createOptions() {
     options.rewritePolyfills =
         flags.rewritePolyfills && options.getLanguageIn().toFeatureSet().contains(FeatureSet.ES6);
 
+    options.setAllowMethodCallDecomposing(flags.allowMethodCallDecomposing);
+
     if (!flags.translationsFile.isEmpty()) {
       try {
         options.messageBundle = new XtbMessageBundle(

diff --git a/src/com/google/javascript/jscomp/CompilerOptions.java b/src/com/google/javascript/jscomp/CompilerOptions.java
@@ -979,6 +979,20 @@ public void setTrustedStrings(boolean yes) {
     trustedStrings = yes;
   }
 
+  private boolean allowMethodCallDecomposing;
+
+  boolean allowMethodCallDecomposing() {
+    return allowMethodCallDecomposing;
+  }
+
+  /**
+   * Setting this to true indicates that it's safe to rewrite x.y() as: fn = x.y; fn.call(x);
+   * This should be usually be false if supporting IE 8 or IE 9 is necessary.
+   */
+  public void setAllowMethodCallDecomposing(boolean value) {
+    this.allowMethodCallDecomposing = value;
+  }
+
   // Should only be used when debugging compiler bugs.
   boolean printSourceAfterEachPass;
 

diff --git a/src/com/google/javascript/jscomp/Es6ExtractClasses.java b/src/com/google/javascript/jscomp/Es6ExtractClasses.java
@@ -65,7 +65,8 @@ public final class Es6ExtractClasses
         compiler,
         compiler.getUniqueNameIdSupplier(),
         consts,
-        Scope.createGlobalScope(new Node(Token.SCRIPT)));
+        Scope.createGlobalScope(new Node(Token.SCRIPT)),
+        compiler.getOptions().allowMethodCallDecomposing());
   }
 
   @Override

diff --git a/src/com/google/javascript/jscomp/Es6RewriteGenerators.java b/src/com/google/javascript/jscomp/Es6RewriteGenerators.java
@@ -1116,7 +1116,8 @@ private final class DecomposeYields extends NodeTraversal.AbstractPreOrderCallba
               compiler,
               compiler.getUniqueNameIdSupplier(),
               consts,
-              Scope.createGlobalScope(new Node(Token.SCRIPT)));
+              Scope.createGlobalScope(new Node(Token.SCRIPT)),
+              compiler.getOptions().allowMethodCallDecomposing());
     }
 
     @Override

diff --git a/src/com/google/javascript/jscomp/ExpressionDecomposer.java b/src/com/google/javascript/jscomp/ExpressionDecomposer.java
@@ -59,18 +59,26 @@ enum DecompositionType {
   private final Set<String> knownConstants;
   private final Scope scope;
 
+  /**
+   * Whether to allow decomposing foo.bar to "var fn = foo.bar; fn.call(foo);"
+   * Should be false if targetting IE8 or IE9.
+   */
+  private final boolean allowMethodCallDecomposing;
+
   ExpressionDecomposer(
       AbstractCompiler compiler,
       Supplier<String> safeNameIdSupplier,
       Set<String> constNames,
-      Scope scope) {
+      Scope scope,
+      boolean allowMethodCallDecomposing) {
     checkNotNull(compiler);
     checkNotNull(safeNameIdSupplier);
     checkNotNull(constNames);
     this.compiler = compiler;
     this.safeNameIdSupplier = safeNameIdSupplier;
     this.knownConstants = constNames;
     this.scope = scope;
+    this.allowMethodCallDecomposing = allowMethodCallDecomposing;
   }
 
   // An arbitrary limit to prevent catch infinite recursion.
@@ -89,7 +97,7 @@ void maybeExposeExpression(Node expression) {
       i++;
       if (i > MAX_ITERATIONS) {
         throw new IllegalStateException(
-            "DecomposeExpression depth exceeded on :\n" + expression.toStringTree());
+            "DecomposeExpression depth exceeded on:\n" + expression.toStringTree());
       }
     }
   }
@@ -196,18 +204,14 @@ private void exposeExpression(Node expressionRoot, Node subExpression) {
               decomposeSubExpressions(left.getFirstChild(), null, state);
             }
           }
-      } else if (parentType == Token.CALL
+      } else if (parentType == Token.CALL && NodeUtil.isGet(parent.getFirstChild())) {
-          && NodeUtil.isGet(parent.getFirstChild())) {
         Node functionExpression = parent.getFirstChild();
         decomposeSubExpressions(functionExpression.getNext(), child, state);
         // Now handle the call expression
         if (isExpressionTreeUnsafe(functionExpression, state.sideEffects)
             && functionExpression.getFirstChild() != grandchild) {
-          // TODO(johnlenz): In Internet Explorer, non-JavaScript objects such
+          checkState(allowMethodCallDecomposing, "Object method calls can not be decomposed.");
-          // as DOM objects can not be decomposed.
+          // Either there were preexisting side-effects, or this node has side-effects.
-          checkState(allowObjectCallDecomposing(), "Object method calls can not be decomposed.");
-          // Either there were preexisting side-effects, or this node has
-          // side-effects.
           state.sideEffects = true;
 
           // Rewrite the call so "this" is preserved.
@@ -239,18 +243,6 @@ private void exposeExpression(Node expressionRoot, Node subExpression) {
     }
   }
 
-  private static boolean allowObjectCallDecomposing() {
-    return false;
-  }
-
-  /**
-   * @return Whether the node may represent an external method.
-   */
-  private static boolean maybeExternMethod(Node node) {
-    // TODO(johnlenz): Provide some mechanism for determining this.
-    return true;
-  }
-
   /**
    * @return "expression" or the node closest to "expression", that does not
    * have a conditional ancestor.
@@ -519,20 +511,19 @@ private Node extractExpression(Node expr, Node injectionPoint) {
    * @return The replacement node.
    */
   private Node rewriteCallExpression(Node call, DecompositionState state) {
-    checkArgument(call.isCall());
+    checkArgument(call.isCall(), call);
     Node first = call.getFirstChild();
-    checkArgument(NodeUtil.isGet(first));
+    checkArgument(NodeUtil.isGet(first), first);
 
     // Extracts the expression representing the function to call. For example:
     //   "a['b'].c" from "a['b'].c()"
-    Node getVarNode = extractExpression(
+    Node getVarNode = extractExpression(first, state.extractBeforeStatement);
-        first, state.extractBeforeStatement);
     state.extractBeforeStatement = getVarNode;
 
     // Extracts the object reference to be used as "this". For example:
     //   "a['b']" from "a['b'].c"
     Node getExprNode = getVarNode.getFirstFirstChild();
-    checkArgument(NodeUtil.isGet(getExprNode));
+    checkArgument(NodeUtil.isGet(getExprNode), getExprNode);
     Node thisVarNode = extractExpression(
         getExprNode.getFirstChild(), state.extractBeforeStatement);
     state.extractBeforeStatement = thisVarNode;
@@ -551,9 +542,8 @@ private Node rewriteCallExpression(Node call, DecompositionState state) {
     //   ...
     Node newCall = IR.call(
         IR.getprop(
-            functionNameNode.cloneNode(),
+            functionNameNode.cloneNode(), IR.string("call")),
-            IR.string("call")),
+        thisNameNode.cloneNode()).useSourceInfoIfMissingFromForTree(call);
-        thisNameNode.cloneNode()).srcref(call);
 
     // Throw away the call name
     call.removeFirstChild();
@@ -562,9 +552,7 @@ private Node rewriteCallExpression(Node call, DecompositionState state) {
       newCall.addChildrenToBack(call.removeChildren());
     }
 
-    // Replace the call.
+    call.replaceWith(newCall);
-    Node callParent = call.getParent();
-    callParent.replaceChild(call, newCall);
 
     return newCall;
   }
@@ -824,10 +812,10 @@ private DecompositionType isSubexpressionMovable(
           //
           Node first = parent.getFirstChild();
           if (requiresDecomposition && parent.isCall() && NodeUtil.isGet(first)) {
-            if (maybeExternMethod(first)) {
+            if (allowMethodCallDecomposing) {
-              return DecompositionType.UNDECOMPOSABLE;
-            } else {
               return DecompositionType.DECOMPOSABLE;
+            } else {
+              return DecompositionType.UNDECOMPOSABLE;
             }
           }
         }

diff --git a/src/com/google/javascript/jscomp/FunctionInjector.java b/src/com/google/javascript/jscomp/FunctionInjector.java
@@ -449,8 +449,12 @@ private CallSiteType classifyCallSite(Reference ref) {
     } else {
       Node expressionRoot = ExpressionDecomposer.findExpressionRoot(callNode);
       if (expressionRoot != null) {
+        // TODO(tbreisacher): Change this to compiler.getOptions().allowMethodCallDecomposing().
+        // Doing so currently causes a "DecomposeExpression depth exceeded" error.
+        boolean allowMethodCallDecomposing = false;
+
         ExpressionDecomposer decomposer = new ExpressionDecomposer(
-            compiler, safeNameIdSupplier, knownConstants, ref.scope);
+            compiler, safeNameIdSupplier, knownConstants, ref.scope, allowMethodCallDecomposing);
         DecompositionType type = decomposer.canExposeExpression(
             callNode);
         if (type == DecompositionType.MOVABLE) {
@@ -467,8 +471,12 @@ private CallSiteType classifyCallSite(Reference ref) {
   }
 
   private ExpressionDecomposer getDecomposer(Scope scope) {
+    // TODO(tbreisacher): Change this to compiler.getOptions().allowMethodCallDecomposing().
+    // Doing so currently causes a "DecomposeExpression depth exceeded" error.
+    boolean allowMethodCallDecomposing = false;
+
     return new ExpressionDecomposer(
-        compiler, safeNameIdSupplier, knownConstants, scope);
+        compiler, safeNameIdSupplier, knownConstants, scope, allowMethodCallDecomposing);
   }
 
   /**

diff --git a/test/com/google/javascript/jscomp/Es6RewriteGeneratorsTest.java b/test/com/google/javascript/jscomp/Es6RewriteGeneratorsTest.java
@@ -21,10 +21,12 @@
 
 /** Unit tests for {@link Es6RewriteGenerators}. */
 public final class Es6RewriteGeneratorsTest extends CompilerTestCase {
+  private boolean allowMethodCallDecomposing;
 
   @Override
   protected void setUp() throws Exception {
     super.setUp();
+    allowMethodCallDecomposing = false;
     setAcceptedLanguage(LanguageMode.ECMASCRIPT_2015);
     enableRunTypeCheckAfterProcessing();
   }
@@ -33,6 +35,7 @@ protected void setUp() throws Exception {
   protected CompilerOptions getOptions() {
     CompilerOptions options = super.getOptions();
     options.setLanguageOut(LanguageMode.ECMASCRIPT3);
+    options.setAllowMethodCallDecomposing(allowMethodCallDecomposing);
     return options;
   }
 
@@ -406,8 +409,35 @@ public void testWhileLoopsGenerator() {
   }
 
   public void testUndecomposableExpression() {
-    testError("function *f() { obj.bar(yield 5); }",
+    testError("function *f() { obj.bar(yield 5); }", Es6ToEs3Util.CANNOT_CONVERT);
-        Es6ToEs3Util.CANNOT_CONVERT);
+  }
+
+  public void testDecomposableExpression() {
+    allowMethodCallDecomposing = true;
+    rewriteGeneratorBodyWithVars(
+        "obj.bar(yield 5);",
+        LINE_JOINER.join(
+            "var $jscomp$generator$next$arg2;",
+            "var JSCompiler_temp_const$jscomp$0;",
+            "var JSCompiler_temp_const$jscomp$1;"),
+        LINE_JOINER.join(
+            "case 0:",
+            "  JSCompiler_temp_const$jscomp$1 = obj;",
+            "  JSCompiler_temp_const$jscomp$0 = JSCompiler_temp_const$jscomp$1.bar;",
+            "  $jscomp$generator$state = 1;",
+            "  return { value: 5, done: false };",
+            "case 1:",
+            "  if (!($jscomp$generator$action$arg ==1 )) {",
+            "    $jscomp$generator$state = 2;",
+            "    break;",
+            "  }",
+            "  $jscomp$generator$state = -1;",
+            "  throw $jscomp$generator$throw$arg;",
+            "case 2:",
+            "  $jscomp$generator$next$arg2 = $jscomp$generator$next$arg;",
+            "  JSCompiler_temp_const$jscomp$0.call(",
+            "      JSCompiler_temp_const$jscomp$1, $jscomp$generator$next$arg2);",
+            "  $jscomp$generator$state = -1;"));
   }
 
   public void testGeneratorCannotConvertYet() {