Roll forward of "Move weak sources into a separate module at the bott…

…om of the graph." Reason: internal breakage fixed. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=213369181
google · Sep 18, 2018 · 91dbb31 · 91dbb31
1 parent 42409c2
commit 91dbb31
Show file tree

Hide file tree

Showing 8 changed files with 288 additions and 61 deletions.
diff --git a/src/com/google/javascript/jscomp/AbstractCommandLineRunner.java b/src/com/google/javascript/jscomp/AbstractCommandLineRunner.java
@@ -32,7 +32,6 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Maps;
 import com.google.common.io.ByteStreams;
 import com.google.gson.Gson;
 import com.google.gson.stream.JsonReader;
@@ -927,10 +926,11 @@ protected void checkModuleName(String name) {
    * @return A map from module name to module wrapper. Modules with no wrapper will have the empty
    *     string as their value in this map.
    */
-  public static Map<String, String> parseModuleWrappers(List<String> specs, List<JSModule> chunks) {
+  public static Map<String, String> parseModuleWrappers(
+      List<String> specs, Iterable<JSModule> chunks) {
     checkState(specs != null);
 
-    Map<String, String> wrappers = Maps.newHashMapWithExpectedSize(chunks.size());
+    Map<String, String> wrappers = new HashMap<>();
 
     // Prepopulate the map with module names.
     for (JSModule c : chunks) {
@@ -1372,7 +1372,7 @@ int processResults(Result result, List<JSModule> modules, B options) throws IOEx
           outputSourceMap(options, config.jsOutputFile);
         }
       } else {
-        DiagnosticType error = outputModuleBinaryAndSourceMaps(modules, options);
+        DiagnosticType error = outputModuleBinaryAndSourceMaps(compiler.getModules(), options);
         if (error != null) {
           compiler.report(JSError.make(error));
           return 1;
@@ -1475,7 +1475,7 @@ void outputJsonStream() throws IOException {
   }
 
   @GwtIncompatible("Unnecessary")
-  private DiagnosticType outputModuleBinaryAndSourceMaps(List<JSModule> modules, B options)
+  private DiagnosticType outputModuleBinaryAndSourceMaps(Iterable<JSModule> modules, B options)
       throws IOException {
     parsedModuleWrappers = parseModuleWrappers(
         config.moduleWrapper, modules);

diff --git a/src/com/google/javascript/jscomp/Compiler.java b/src/com/google/javascript/jscomp/Compiler.java
@@ -461,7 +461,7 @@ protected void reconcileOptionsWithGuards() {
   /** Initializes the instance state needed for a compile job. */
   public final <T1 extends SourceFile, T2 extends SourceFile> void init(
       List<T1> externs, List<T2> sources, CompilerOptions options) {
-    JSModule module = new JSModule(JSModule.SINGLETON_MODULE_NAME);
+    JSModule module = new JSModule(JSModule.STRONG_MODULE_NAME);
     for (SourceFile source : sources) {
       module.add(new CompilerInput(source));
     }
@@ -481,6 +481,7 @@ public <T extends SourceFile> void initModules(
     initOptions(options);
 
     checkFirstModule(modules);
+    modules = moveWeakSources(modules);
     fillEmptyModules(modules);
 
     this.externs = makeExternInputs(externs);
@@ -564,6 +565,52 @@ private void checkFirstModule(List<JSModule> modules) {
     }
   }
 
+  /** Moves all weak sources into a separate weak module that depends on every other module. */
+  private List<JSModule> moveWeakSources(List<JSModule> modules) {
+    // Collect weak sources.
+    List<CompilerInput> weakInputs = new ArrayList<>();
+    for (JSModule module : modules) {
+      if (module.getName().equals(JSModule.WEAK_MODULE_NAME)) {
+        // Skip an already existing weak module - see below.
+        continue;
+      }
+      for (int i = 0; i < module.getInputCount(); ) {
+        CompilerInput input = module.getInput(i);
+        if (input.getSourceFile().isWeak()) {
+          module.remove(input);
+          weakInputs.add(input);
+        } else {
+          i++;
+        }
+      }
+    }
+
+    // If a weak module already exists (e.g. in a stage 2 compilation), make sure it contains all
+    // weak sources, but leave the module graph otherwise untouched.
+    if (moduleGraph != null
+        && moduleGraph.getModuleByName(JSModule.WEAK_MODULE_NAME) != null
+        && !weakInputs.isEmpty()) {
+      throw new RuntimeException(
+          "A weak module already exists but weak sources were found in other modules.");
+    }
+
+    // Create the weak module and make it depend on every other module.
+    JSModule weakModule = new JSModule(JSModule.WEAK_MODULE_NAME);
+    for (JSModule module : modules) {
+      weakModule.addDependency(module);
+    }
+
+    // Move the weak sources.
+    for (CompilerInput input : weakInputs) {
+      weakModule.add(input);
+    }
+
+    // Make a copy in case the original list is immutable.
+    modules = ImmutableList.<JSModule>builder().addAll(modules).add(weakModule).build();
+
+    return modules;
+  }
+
   /**
    * Empty modules get an empty "fill" file, so that we can move code into
    * an empty module.
@@ -587,9 +634,8 @@ public static String joinPathParts(String... pathParts) {
   /** Fill any empty modules with a place holder file. It makes any cross module motion easier. */
   private static void fillEmptyModules(Iterable<JSModule> modules) {
     for (JSModule module : modules) {
-      if (module.getInputs().isEmpty()) {
+      if (!module.getName().equals(JSModule.WEAK_MODULE_NAME) && module.getInputs().isEmpty()) {
-        module.add(SourceFile.fromCode(
+        module.add(SourceFile.fromCode(createFillFileName(module.getName()), ""));
-            createFillFileName(module.getName()), ""));
       }
     }
   }
@@ -1421,10 +1467,8 @@ boolean addNewSourceAst(JsAst ast) {
     CompilerInput newInput = new CompilerInput(ast);
 
     // TODO(tylerg): handle this for multiple modules at some point.
-    Iterable<JSModule> modules = getModules();
+    JSModule firstModule = Iterables.getFirst(getModules(), null);
-    if (Iterables.size(modules) == 1) {
+    if (firstModule.getName().equals(JSModule.STRONG_MODULE_NAME)) {
-      // singleton module
-      JSModule firstModule = Iterables.getFirst(modules, null);
       firstModule.add(newInput);
     }
 

diff --git a/src/com/google/javascript/jscomp/JSModule.java b/src/com/google/javascript/jscomp/JSModule.java
@@ -40,8 +40,16 @@
  *
  */
 public final class JSModule extends DependencyInfo.Base implements Serializable {
-  // The name of the artificial module containing all sources when no module spec is specified.
+  // The name of the artificial module containing all strong sources when there is no module spec.
-  public static final String SINGLETON_MODULE_NAME = "$singleton$";
+  // If there is a module spec, strong sources go in their respective modules, and this module does
+  // not exist.
+  public static final String STRONG_MODULE_NAME = "$strong$";
+
+  // The name of the artificial module containing all weak sources. Regardless of the module spec,
+  // weak sources are moved into this module, which is made to depend on every other module. This is
+  // necessary so that removing weak sources (as an optimization) does not accidentally remove
+  // namespace declarations whose existence strong sources rely upon.
+  public static final String WEAK_MODULE_NAME = "$weak$";
 
   private static final long serialVersionUID = 1;
 
@@ -222,6 +230,16 @@ public Set<JSModule> getThisAndAllDependencies() {
     return deps;
   }
 
+  /** Returns the number of source code inputs. */
+  public int getInputCount() {
+    return inputs.size();
+  }
+
+  /** Returns the i-th source code input. */
+  public CompilerInput getInput(int i) {
+    return inputs.get(i);
+  }
+
   /**
    * Gets this module's list of source code inputs.
    *
@@ -258,6 +276,14 @@ public boolean removeByName(String name) {
     return found;
   }
 
+  /**
+   * Returns whether this module is synthetic (i.e. one of the special strong or weak modules
+   * created by the compiler.
+   */
+  public boolean isSynthetic() {
+    return name.equals(STRONG_MODULE_NAME) || name.equals(WEAK_MODULE_NAME);
+  }
+
   /** Returns the module name (primarily for debugging). */
   @Override
   public String toString() {

diff --git a/src/com/google/javascript/jscomp/JSModuleGraph.java b/src/com/google/javascript/jscomp/JSModuleGraph.java
@@ -48,6 +48,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import javax.annotation.Nullable;
 
 /**
  * A {@link JSModule} dependency graph that assigns a depth to each module and can answer
@@ -198,7 +199,7 @@ Iterable<CompilerInput> getAllInputs() {
   int getInputCount() {
     int count = 0;
     for (JSModule module : modules) {
-      count += module.getInputs().size();
+      count += module.getInputCount();
     }
     return count;
   }
@@ -210,6 +211,21 @@ Iterable<JSModule> getAllModules() {
     return Arrays.asList(modules);
   }
 
+  /**
+   * Gets a single module by name.
+   *
+   * @return The module, or null if no such module exists.
+   */
+  @Nullable
+  JSModule getModuleByName(String name) {
+    for (JSModule m : modules) {
+      if (m.getName().equals(name)) {
+        return m;
+      }
+    }
+    return null;
+  }
+
   /**
    * Gets all modules indexed by name.
    */

diff --git a/src/com/google/javascript/jscomp/RecordFunctionInformation.java b/src/com/google/javascript/jscomp/RecordFunctionInformation.java
@@ -71,14 +71,12 @@ public void visit(NodeTraversal t, Node n, Node parent) {
 
     String compiledSource = compiler.toSource(n);
     JSModule module = t.getModule();
-    String moduleName =
-        module.getName().equals(JSModule.SINGLETON_MODULE_NAME) ? "" : module.getName();
     mapBuilder.addEntry(
         FunctionInformationMap.Entry.newBuilder()
             .setId(id)
             .setSourceName(NodeUtil.getSourceName(n))
             .setLineNumber(n.getLineno())
-            .setModuleName(moduleName)
+            .setModuleName(module.isSynthetic() ? "" : module.getName())
             .setSize(compiledSource.length())
             .setName(functionNames.getFunctionName(n))
             .setCompiledSource(compiledSource)

diff --git a/test/com/google/javascript/jscomp/CommandLineRunnerTest.java b/test/com/google/javascript/jscomp/CommandLineRunnerTest.java
@@ -1472,19 +1472,23 @@ public void testChainModuleManifest() throws Exception {
     lastCommandLineRunner.printModuleGraphManifestOrBundleTo(
         lastCompiler.getModuleGraph(), builder, true);
     assertThat(builder.toString())
-        .isEqualTo(Joiner.on('\n').join(
+        .isEqualTo(
-            "{m0}",
+            Joiner.on('\n')
-            "i0.js",
+                .join(
-            "",
+                    "{m0}",
-            "{m1:m0}",
+                    "i0.js",
-            "i1.js",
+                    "",
-            "",
+                    "{m1:m0}",
-            "{m2:m1}",
+                    "i1.js",
-            "i2.js",
+                    "",
-            "",
+                    "{m2:m1}",
-            "{m3:m2}",
+                    "i2.js",
-            "i3.js",
+                    "",
-            ""));
+                    "{m3:m2}",
+                    "i3.js",
+                    "",
+                    "{$weak$:m0,m1,m2,m3}",
+                    ""));
   }
 
   @Test
@@ -1497,19 +1501,23 @@ public void testStarModuleManifest() throws Exception {
     lastCommandLineRunner.printModuleGraphManifestOrBundleTo(
         lastCompiler.getModuleGraph(), builder, true);
     assertThat(builder.toString())
-        .isEqualTo(Joiner.on('\n').join(
+        .isEqualTo(
-            "{m0}",
+            Joiner.on('\n')
-            "i0.js",
+                .join(
-            "",
+                    "{m0}",
-            "{m1:m0}",
+                    "i0.js",
-            "i1.js",
+                    "",
-            "",
+                    "{m1:m0}",
-            "{m2:m0}",
+                    "i1.js",
-            "i2.js",
+                    "",
-            "",
+                    "{m2:m0}",
-            "{m3:m0}",
+                    "i2.js",
-            "i3.js",
+                    "",
-            ""));
+                    "{m3:m0}",
+                    "i3.js",
+                    "",
+                    "{$weak$:m0,m1,m2,m3}",
+                    ""));
   }
 
   @Test
@@ -2281,12 +2289,17 @@ public void testJsonStreamAllowsAnyChunkName() {
       fail("Unexpected exception " + e);
     }
     String output = new String(outReader.toByteArray(), UTF_8);
-    assertThat(output).isEqualTo("[{\"src\":\"alert(\\\"foo\\\");\\n\","
+    assertThat(output)
-        + "\"path\":\"./foo/bar/baz.js\",\"source_map\":\"{\\n\\\"version\\\":3,"
+        .isEqualTo(
-        + "\\n\\\"file\\\":\\\"./foo/bar/baz.js\\\",\\n\\\"lineCount\\\":1,"
+            "[{\"src\":\"alert(\\\"foo\\\");\\n\","
-        + "\\n\\\"mappings\\\":\\\"AAAAA,KAAA,CAAM,KAAN;\\\","
+                + "\"path\":\"./foo/bar/baz.js\",\"source_map\":\"{\\n\\\"version\\\":3,"
-        + "\\n\\\"sources\\\":[\\\"foo.js\\\"],"
+                + "\\n\\\"file\\\":\\\"./foo/bar/baz.js\\\",\\n\\\"lineCount\\\":1,"
-        + "\\n\\\"names\\\":[\\\"alert\\\"]\\n}\\n\"}]");
+                + "\\n\\\"mappings\\\":\\\"AAAAA,KAAA,CAAM,KAAN;\\\","
+                + "\\n\\\"sources\\\":[\\\"foo.js\\\"],\\n\\\"names\\\":[\\\"alert\\\"]\\n}\\n\"},"
+                + "{\"src\":\"\\n\",\"path\":\"./$weak$.js\",\"source_map\":"
+                + "\"{\\n\\\"version\\\":3,\\n\\\"file\\\":\\\"./$weak$.js\\\","
+                + "\\n\\\"lineCount\\\":1,\\n\\\"mappings\\\":\\\";\\\",\\n\\\"sources\\\":[],"
+                + "\\n\\\"names\\\":[]\\n}\\n\"}]");
   }
 
   @Test
@@ -2310,12 +2323,17 @@ public void testOutputModuleNaming() {
       fail("Unexpected exception " + e);
     }
     String output = new String(outReader.toByteArray(), UTF_8);
-    assertThat(output).isEqualTo("[{\"src\":\"alert(\\\"foo\\\");\\n\","
+    assertThat(output)
-        + "\"path\":\"./foo--bar.baz.js\",\"source_map\":\"{\\n\\\"version\\\":3,"
+        .isEqualTo(
-        + "\\n\\\"file\\\":\\\"./foo--bar.baz.js\\\",\\n\\\"lineCount\\\":1,"
+            "[{\"src\":\"alert(\\\"foo\\\");\\n\","
-        + "\\n\\\"mappings\\\":\\\"AAAAA,KAAA,CAAM,KAAN;\\\","
+                + "\"path\":\"./foo--bar.baz.js\",\"source_map\":\"{\\n\\\"version\\\":3,"
-        + "\\n\\\"sources\\\":[\\\"foo.js\\\"],"
+                + "\\n\\\"file\\\":\\\"./foo--bar.baz.js\\\",\\n\\\"lineCount\\\":1,"
-        + "\\n\\\"names\\\":[\\\"alert\\\"]\\n}\\n\"}]");
+                + "\\n\\\"mappings\\\":\\\"AAAAA,KAAA,CAAM,KAAN;\\\","
+                + "\\n\\\"sources\\\":[\\\"foo.js\\\"],\\n\\\"names\\\":[\\\"alert\\\"]\\n}\\n\"},"
+                + "{\"src\":\"\\n\",\"path\":\"./$weak$.js\","
+                + "\"source_map\":\"{\\n\\\"version\\\":3,\\n\\\"file\\\":\\\"./$weak$.js\\\","
+                + "\\n\\\"lineCount\\\":1,\\n\\\"mappings\\\":\\\";\\\",\\n\\\"sources\\\":[],"
+                + "\\n\\\"names\\\":[]\\n}\\n\"}]");
   }
 
   @Test