From 5e0a57057eac2e518151df6d3df5bae495904726 Mon Sep 17 00:00:00 2001 From: Chad Killingsworth Date: Tue, 14 Feb 2017 11:15:08 -0800 Subject: [PATCH] When rewriting CommonJS named functions to expressions, hoist the function definitions. Closes https://github.com/google/closure-compiler/pull/2283 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=147496517 --- .../jscomp/ProcessCommonJSModules.java | 77 +++++++++++++++---- .../jscomp/ProcessCommonJSModulesTest.java | 13 ++++ 2 files changed, 77 insertions(+), 13 deletions(-) diff --git a/src/com/google/javascript/jscomp/ProcessCommonJSModules.java b/src/com/google/javascript/jscomp/ProcessCommonJSModules.java index f8e8032c667..71d87f2204d 100644 --- 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 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() { * *

If all of the assignments are simply property assignments, initialize the module name * variable as a namespace. + * + *

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 exports; private final List imports = new ArrayList<>(); private final List rewrittenClassExpressions = new ArrayList<>(); + private final List functionsToHoist = new ArrayList<>(); + private final Node hoistInsertionPoint; - public RewriteModule(boolean allowFullRewrite, ImmutableCollection exports) { + public RewriteModule( + boolean allowFullRewrite, + ImmutableCollection 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 index d0ea9187ef3..8644736b1c7 100644 --- 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() {