Fix a bunch of bugs in global scope names:

- Make sure it works in es5 strict mode by adding an explicit 'this' reference in the wrapper - Make sure global-scope-name and output-wrapper work together - Make sure they both work on modularized and non-modularized binaries - Set source map prefix correctly
bolinfest · Jun 1, 2015 · ab85203 · ab85203
1 parent 8555d26
commit ab85203
Show file tree

Hide file tree

Showing 3 changed files with 126 additions and 70 deletions.
diff --git a/src/org/plovr/Compilation.java b/src/org/plovr/Compilation.java
@@ -151,21 +151,10 @@ public String getCompiledCode() {
 
     String compiledCode = inputJsConcatenatedInOrder != null ?
         inputJsConcatenatedInOrder : compiler.toSource();
-    String outputWrapper = config.getOutputWrapper();
-    if (outputWrapper != null) {
-      String outputWrapperMarker = config.getOutputWrapperMarker();
-      int pos = outputWrapper.indexOf(outputWrapperMarker);
-      if (pos >= 0) {
-        compiledCode = outputWrapper.substring(0, pos) +
-            compiledCode +
-            outputWrapper.substring(pos + outputWrapperMarker.length());
-      } else {
-        throw new RuntimeException(
-            "output-wrapper did not contain placeholder: " +
-            outputWrapperMarker);
-      }
-    }
-    return compiledCode;
+
+    String sourceUrl = config.getOutputFile() == null ? "" : config.getOutputFile().toString();
+    String outputWrapper = config.getOutputAndGlobalScopeWrapper(true, sourceUrl);
+    return interpolateOutputWrapper(compiledCode, outputWrapper);
   }
 
   public String getRootModuleName() {
@@ -202,10 +191,27 @@ private synchronized String getCodeForModule(
     Preconditions.checkState(modules != null,
         "This compilation does not use modules");
 
-    StringBuilder builder = new StringBuilder();
+    JSModule module = nameToModule.get(moduleName);
+    String moduleCode = compiler.toSource(module);
+
+    String outputWrapper = getModuleOutputWrapper(moduleName, isDebugMode, moduleNameToUri);
+    String result = interpolateOutputWrapper(moduleCode, outputWrapper);
+
+    if (resetSourceMap) {
+      SourceMap sourceMap = compiler.getSourceMap();
+      if (sourceMap != null) sourceMap.reset();
+    }
+
+    return result;
+  }
+
+  String getModuleOutputWrapper(
+      String moduleName,
+      boolean isDebugMode,
+      Function<String, String> moduleNameToUri) {
+    StringBuilder rootModuleInfoBuilder = new StringBuilder();
     ModuleConfig moduleConfig = config.getModuleConfig();
     String rootModule = moduleConfig.getRootModule();
-
     boolean isRootModule = rootModule.equals(moduleName);
 
     if (isRootModule) {
@@ -226,55 +232,39 @@ private synchronized String getCodeForModule(
       // which (as much as it pains me) is why "var" is omitted.
       if (!moduleConfig.excludeModuleInfoFromRootModule()) {
         try {
-          appendRootModuleInfo(builder, isDebugMode, moduleNameToUri);
+          appendRootModuleInfo(rootModuleInfoBuilder, isDebugMode, moduleNameToUri);
         } catch (IOException e) {
           // This should not occur because data is being appended to an
           // in-memory StringBuilder rather than a file.
           throw new RuntimeException(e);
         }
       }
     }
+    String sourceUrl = moduleNameToUri.apply(moduleName);
+    return rootModuleInfoBuilder.toString() +
+        config.getOutputAndGlobalScopeWrapper(isRootModule, sourceUrl);
+  }
 
-    JSModule module = nameToModule.get(moduleName);
-    String moduleCode = compiler.toSource(module);
-
-    boolean hasGlobalScopeName =
-        !Strings.isNullOrEmpty(config.getGlobalScopeName()) &&
-        config.getCompilationMode() != CompilationMode.WHITESPACE;
-
-    // Optionally wrap the module in an anonymous function, with the
-    // requisite wrapper to make it work.
-    if (hasGlobalScopeName) {
-      if (isRootModule) {
-        // Initialize the global scope in the root module.
-        builder.append(config.getGlobalScopeName());
-        builder.append("={};");
-      }
-      builder.append("(function(");
-      builder.append(Config.GLOBAL_SCOPE_NAME);
-      // Including a newline makes the offset into the source map easier to calculate.
-      builder.append("){\n");
-    }
-    builder.append(moduleCode);
-    if (hasGlobalScopeName) {
-      builder.append("})(");
-      builder.append(config.getGlobalScopeName());
-      builder.append(");");
+  private String interpolateOutputWrapper(String code, String outputWrapper) {
+    String outputWrapperMarker = config.getOutputWrapperMarker();
+    if (outputWrapper.equals(outputWrapperMarker)) {
+      return code;
     }
 
-    if (resetSourceMap) {
+    int pos = outputWrapper.indexOf(outputWrapperMarker);
+    if (pos >= 0) {
+      String prefix = outputWrapper.substring(0, pos);
+      String suffix = outputWrapper.substring(pos + outputWrapperMarker.length());
       SourceMap sourceMap = compiler.getSourceMap();
-      if (sourceMap != null) sourceMap.reset();
-    }
-
-    // http://code.google.com/p/closure-library/issues/detail?id=196
-    // http://blog.getfirebug.com/2009/08/11/give-your-eval-a-name-with-sourceurl/
-    // non-root modules are loaded with eval, give it a sourceURL for better debugging
-    if (!isRootModule) {
-        builder.append("\n//@ sourceURL=" + moduleNameToUri.apply(moduleName));
+      if (sourceMap != null) {
+        sourceMap.setWrapperPrefix(prefix);
+      }
+      return prefix + code + suffix;
+    } else {
+      throw new RuntimeException(
+          "output-wrapper did not contain placeholder: " +
+          outputWrapperMarker);
     }
-
-    return builder.toString();
   }
 
   public void appendRootModuleInfo(Appendable appendable, boolean isDebugMode,

diff --git a/src/org/plovr/Config.java b/src/org/plovr/Config.java
@@ -357,6 +357,43 @@ public String getOutputWrapper() {
     return outputWrapper;
   }
 
+  /**
+   * @return A complete output wrapper, including the wrapper for global re-scoping.
+   */
+  public String getOutputAndGlobalScopeWrapper(boolean isRootModule, String sourceUrl) {
+    String outputWrapper = getOutputWrapper();
+    String outputWrapperMarker = getOutputWrapperMarker();
+    if (Strings.isNullOrEmpty(outputWrapper)) {
+      outputWrapper = outputWrapperMarker;
+    }
+
+    boolean hasGlobalScopeName =
+        !Strings.isNullOrEmpty(getGlobalScopeName()) &&
+        getCompilationMode() != CompilationMode.WHITESPACE;
+
+    if (hasGlobalScopeName) {
+      // Initialize the global scope if not initialized yet.
+      String globalScopeNameWrapper = "";
+      if (isRootModule) {
+        globalScopeNameWrapper += "var " + getGlobalScopeName() + "={};";
+      }
+      globalScopeNameWrapper +=
+          "(function(" + GLOBAL_SCOPE_NAME + "){\n" +
+          outputWrapperMarker +
+          "}).call(this, " + getGlobalScopeName() + ");";
+      outputWrapper = outputWrapper.replace(outputWrapperMarker, globalScopeNameWrapper);
+    }
+
+    // http://code.google.com/p/closure-library/issues/detail?id=196
+    // http://blog.getfirebug.com/2009/08/11/give-your-eval-a-name-with-sourceurl/
+    // non-root modules are loaded with eval, give it a sourceURL for better debugging
+    if (!isRootModule && !Strings.isNullOrEmpty(sourceUrl)) {
+      outputWrapper += "\n//@ sourceURL=" + sourceUrl;
+    }
+
+    return outputWrapper;
+  }
+
   public Charset getOutputCharset() {
     return outputCharset;
   }
@@ -614,15 +651,16 @@ public PlovrCompilerOptions getCompilerOptions(
     if (moduleConfig != null) {
       options.crossModuleCodeMotion = true;
       options.crossModuleMethodMotion = true;
-      if (!Strings.isNullOrEmpty(globalScopeName)) {
-        Preconditions.checkState(
-            options.collapseAnonymousFunctions == true ||
-            level != CompilationLevel.ADVANCED_OPTIMIZATIONS,
-            "For reasons unknown, setting this to false ends up " +
-            "with a fairly larger final output, even though we just go " +
-            "and re-anonymize the functions a few steps later.");
-        options.renamePrefixNamespace = GLOBAL_SCOPE_NAME;
-      }
+    }
+
+    if (!Strings.isNullOrEmpty(globalScopeName)) {
+      Preconditions.checkState(
+          options.collapseAnonymousFunctions == true ||
+          level != CompilationLevel.ADVANCED_OPTIMIZATIONS,
+          "For reasons unknown, setting this to false ends up " +
+          "with a fairly larger final output, even though we just go " +
+          "and re-anonymize the functions a few steps later.");
+      options.renamePrefixNamespace = GLOBAL_SCOPE_NAME;
     }
 
     // Now that custom passes have registered with the PlovrDiagnosticGroups,

diff --git a/test/org/plovr/ConfigTest.java b/test/org/plovr/ConfigTest.java
@@ -5,6 +5,7 @@
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
+import java.io.File;
 import org.junit.Test;
 
 import com.google.common.base.Charsets;
@@ -39,13 +40,13 @@ public void testApplyExperimentalCompilerOptions() {
 
     JsonParser parser = new JsonParser();
     JsonObject experimentalOptions = parser.parse("{" +
-    		"\"processObjectPropertyString\": true, " +
-    		"\"externExports\": true, " +
-    		"\"checkMissingGetCssNameBlacklist\": \"hello world\", " +
-    		"\"acceptConstKeyword\": true, " +
-    		"\"outputCharset\": \"UTF-8\", " +
-    		"\"languageIn\": \"ECMASCRIPT5\"" +
-    		"}").getAsJsonObject();
+        "\"processObjectPropertyString\": true, " +
+        "\"externExports\": true, " +
+        "\"checkMissingGetCssNameBlacklist\": \"hello world\", " +
+        "\"acceptConstKeyword\": true, " +
+        "\"outputCharset\": \"UTF-8\", " +
+        "\"languageIn\": \"ECMASCRIPT5\"" +
+        "}").getAsJsonObject();
     Config.applyExperimentalCompilerOptions(experimentalOptions, options);
 
     assertTrue(options.getProcessObjectPropertyString());
@@ -55,4 +56,31 @@ public void testApplyExperimentalCompilerOptions() {
     assertEquals(Charsets.UTF_8, options.getOutputCharset());
     assertEquals(LanguageMode.ECMASCRIPT5, options.getLanguageIn());
   }
+
+  @Test
+  public void testOutputAndGlobalScopeWrapper() {
+    Config.Builder builder = Config.builderForTesting();
+    builder.addInput(new File("fake-input.js"), "fake-input.js");
+    builder.setOutputWrapper("(function () { %output% })()");
+    builder.setCompilationMode(CompilationMode.ADVANCED);
+
+    assertEquals("(function () { %output% })()",
+                 builder.build().getOutputAndGlobalScopeWrapper(false, ""));
+    assertEquals("(function () { %output% })()\n//@ sourceURL=foo.js",
+                 builder.build().getOutputAndGlobalScopeWrapper(false, "foo.js"));
+
+    builder.setOutputWrapper("");
+    builder.setGlobalScopeName("_mdm");
+
+    assertEquals("(function(z){\n%output%}).call(this, _mdm);",
+                 builder.build().getOutputAndGlobalScopeWrapper(false, ""));
+    assertEquals("var _mdm={};(function(z){\n%output%}).call(this, _mdm);",
+                 builder.build().getOutputAndGlobalScopeWrapper(true, ""));
+
+    builder.setOutputWrapper("(function () { %output% })()");
+    builder.setGlobalScopeName("_mdm");
+
+    assertEquals("(function () { (function(z){\n%output%}).call(this, _mdm); })()",
+                 builder.build().getOutputAndGlobalScopeWrapper(false, ""));
+  }
 }