When rewriting CommonJS named functions to expressions, hoist the fun…

…ction definitions. Closes #2283 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=147496517
google · Feb 15, 2017 · 5e0a570 · 5e0a570
1 parent 6ac6a7e
commit 5e0a570
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 13 deletions.
diff --git a/src/com/google/javascript/jscomp/ProcessCommonJSModules.java b/src/com/google/javascript/jscomp/ProcessCommonJSModules.java
@@ -95,6 +95,7 @@ public void process(Node externs, Node root) {
     NodeTraversal.traverseEs6(compiler, root, finder);
 
     ImmutableList.Builder<ExportInfo> exports = ImmutableList.builder();
+    Node hoistInsertionReference = null;
     if (finder.isCommonJsModule()) {
       finder.reportModuleErrors();
 
@@ -121,12 +122,14 @@ public void process(Node externs, Node root) {
         }
       }
 
-      finder.addGoogProvide();
+      hoistInsertionReference = finder.addGoogProvide();
       compiler.reportCodeChange();
     }
 
     NodeTraversal.traverseEs6(
-        compiler, root, new RewriteModule(finder.isCommonJsModule(), exports.build()));
+        compiler,
+        root,
+        new RewriteModule(finder.isCommonJsModule(), exports.build(), hoistInsertionReference));
   }
 
   /**
@@ -228,6 +231,24 @@ private boolean removeIIFEWrapper(Node root) {
     return true;
   }
 
+  /**
+   * Given a scope root, return an insertion reference after any goog.require or goot.provide
+   * functions.
+   */
+  private static Node getScopeInsertionPoint(Node scopeRoot, Node startPoint) {
+    Node insertionPoint = startPoint;
+    for (Node next = startPoint != null ? startPoint.getNext() : scopeRoot.getFirstChild();
+        next != null
+            && next.getFirstChild().isCall()
+            && (next.getFirstFirstChild().matchesQualifiedName("goog.require")
+                || next.getFirstFirstChild().matchesQualifiedName("goog.provide"));
+        next = next.getNext()) {
+      insertionPoint = next;
+    }
+
+    return insertionPoint;
+  }
+
   /**
    * Traverse the script. Find all references to CommonJS require (import) and module.exports or
    * export statements. Add goog.require statements for any require statements. Rewrites any require
@@ -465,12 +486,15 @@ void reportModuleErrors() {
      *
      * <p>If all of the assignments are simply property assignments, initialize the module name
      * variable as a namespace.
+     *
+     * <p>Returns a node reference after which hoisted functions within the module should be
+     * inserted.
      */
-    void addGoogProvide() {
+    Node addGoogProvide() {
       CompilerInput ci = compiler.getInput(this.script.getInputId());
       ModulePath modulePath = ci.getPath();
       if (modulePath == null) {
-        return;
+        return null;
       }
 
       String moduleName = modulePath.toModuleName();
@@ -529,16 +553,16 @@ void addGoogProvide() {
         }
         initModule.useSourceInfoIfMissingFromForTree(this.script);
 
-        Node refChild = this.script.getFirstChild();
-        while (refChild.getNext() != null
-            && refChild.getNext().isExprResult()
-            && refChild.getNext().getFirstChild().isCall()
-            && (refChild.getNext().getFirstFirstChild().matchesQualifiedName("goog.require")
-                || refChild.getNext().getFirstFirstChild().matchesQualifiedName("goog.provide"))) {
-          refChild = refChild.getNext();
+        Node refChild = getScopeInsertionPoint(this.script, null);
+        if (refChild == null) {
+          this.script.addChildToFront(initModule);
+        } else {
+          this.script.addChildAfter(initModule, refChild);
         }
-        this.script.addChildAfter(initModule, refChild);
+        return initModule;
       }
+
+      return script.getFirstChild();
     }
 
     /** Find the outermost if node ancestor for a node without leaving the function scope */
@@ -613,10 +637,16 @@ private class RewriteModule extends AbstractPostOrderCallback {
     private final ImmutableCollection<ExportInfo> exports;
     private final List<Node> imports = new ArrayList<>();
     private final List<Node> rewrittenClassExpressions = new ArrayList<>();
+    private final List<Node> functionsToHoist = new ArrayList<>();
+    private final Node hoistInsertionPoint;
 
-    public RewriteModule(boolean allowFullRewrite, ImmutableCollection<ExportInfo> exports) {
+    public RewriteModule(
+        boolean allowFullRewrite,
+        ImmutableCollection<ExportInfo> exports,
+        Node hoistInsertionPoint) {
       this.allowFullRewrite = allowFullRewrite;
       this.exports = exports;
+      this.hoistInsertionPoint = hoistInsertionPoint;
     }
 
     @Override
@@ -631,6 +661,26 @@ public void visit(NodeTraversal t, Node n, Node parent) {
             compiler.reportCodeChange();
           }
 
+          // Hoist functions in reverse order so that they maintain the same relative
+          // order after hoisting.
+          for (int i = functionsToHoist.size() - 1; i >= 0; i--) {
+            Node functionExpr = functionsToHoist.get(i);
+            Node scopeRoot = t.getClosestHoistScope().getRootNode();
+            Node insertionRef = null;
+            if (hoistInsertionPoint != null
+                && t.getEnclosingFunction() == NodeUtil.getEnclosingFunction(hoistInsertionPoint)) {
+              insertionRef = hoistInsertionPoint;
+            }
+            Node insertionPoint = getScopeInsertionPoint(scopeRoot, insertionRef);
+            if (insertionPoint == null) {
+              if (scopeRoot.getFirstChild() != functionExpr) {
+                scopeRoot.addChildToFront(functionExpr.detach());
+              }
+            } else if (insertionPoint != functionExpr && insertionPoint.getNext() != functionExpr) {
+              scopeRoot.addChildAfter(functionExpr.detach(), insertionPoint);
+            }
+          }
+
           for (ExportInfo export : exports) {
             visitExport(t, export.node);
           }
@@ -999,6 +1049,7 @@ private void updateNameReference(
             } else {
               expr.getFirstChild().replaceChild(expr.getFirstChild().getSecondChild(), parent);
             }
+            functionsToHoist.add(expr);
           } else {
             nameRef.setString(newName);
             nameRef.setOriginalName(originalName);

diff --git a/test/com/google/javascript/jscomp/ProcessCommonJSModulesTest.java b/test/com/google/javascript/jscomp/ProcessCommonJSModulesTest.java
@@ -483,6 +483,19 @@ public void testFunctionHoisting() {
             "goog.provide('module$test');",
             "var module$test = function() {};",
             "module$test.prototype = new Date();"));
+
+    testModules(
+        LINE_JOINER.join(
+            "function foo() {}",
+            "Object.assign(foo, { bar: foobar });",
+            "function foobar() {}",
+            "module.exports = foo;",
+            "module.exports.bar = foobar;"),
+        LINE_JOINER.join(
+            "goog.provide('module$test');",
+            "var module$test = function () {};",
+            "module$test.bar = function() {};",
+            "Object.assign(module$test, { bar: module$test.bar });"));
   }
 
   public void testClassRewriting() {