Skip to content

Commit

Permalink
Roll back of "Move weak sources into a separate module at the bottom …
Browse files Browse the repository at this point in the history 
…of the graph."

Reason: internal breakage.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=213064114
  • Loading branch information
@tjgq
tjgq committed Sep 18, 2018
1 parent b725574 commit c43eaff
Show file tree
Hide file tree
Showing 8 changed files with 61 additions and 288 deletions.
10 changes: 5 additions & 5 deletions src/com/google/javascript/jscomp/AbstractCommandLineRunner.java
View file
Expand Up @@ -32,6 +32,7 @@
import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables; import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import com.google.common.io.ByteStreams; import com.google.common.io.ByteStreams;
import com.google.gson.Gson; import com.google.gson.Gson;
import com.google.gson.stream.JsonReader; import com.google.gson.stream.JsonReader;
Expand Down Expand Up @@ -926,11 +927,10 @@ protected void checkModuleName(String name) {
* @return A map from module name to module wrapper. Modules with no wrapper will have the empty * @return A map from module name to module wrapper. Modules with no wrapper will have the empty
* string as their value in this map. * string as their value in this map.
*/ */
public static Map<String, String> parseModuleWrappers( public static Map<String, String> parseModuleWrappers(List<String> specs, List<JSModule> chunks) {
List<String> specs, Iterable<JSModule> chunks) {
checkState(specs != null); checkState(specs != null);


Map<String, String> wrappers = new HashMap<>(); Map<String, String> wrappers = Maps.newHashMapWithExpectedSize(chunks.size());


// Prepopulate the map with module names. // Prepopulate the map with module names.
for (JSModule c : chunks) { for (JSModule c : chunks) {
Expand Down Expand Up @@ -1372,7 +1372,7 @@ int processResults(Result result, List<JSModule> modules, B options) throws IOEx
outputSourceMap(options, config.jsOutputFile); outputSourceMap(options, config.jsOutputFile);
} }
} else { } else {
DiagnosticType error = outputModuleBinaryAndSourceMaps(compiler.getModules(), options); DiagnosticType error = outputModuleBinaryAndSourceMaps(modules, options);
if (error != null) { if (error != null) {
compiler.report(JSError.make(error)); compiler.report(JSError.make(error));
return 1; return 1;
Expand Down Expand Up @@ -1475,7 +1475,7 @@ void outputJsonStream() throws IOException {
} }


@GwtIncompatible("Unnecessary") @GwtIncompatible("Unnecessary")
private DiagnosticType outputModuleBinaryAndSourceMaps(Iterable<JSModule> modules, B options) private DiagnosticType outputModuleBinaryAndSourceMaps(List<JSModule> modules, B options)
throws IOException { throws IOException {
parsedModuleWrappers = parseModuleWrappers( parsedModuleWrappers = parseModuleWrappers(
config.moduleWrapper, modules); config.moduleWrapper, modules);
Expand Down
60 changes: 8 additions & 52 deletions src/com/google/javascript/jscomp/Compiler.java
View file
Expand Up @@ -461,7 +461,7 @@ protected void reconcileOptionsWithGuards() {
/** Initializes the instance state needed for a compile job. */ /** Initializes the instance state needed for a compile job. */
public final <T1 extends SourceFile, T2 extends SourceFile> void init( public final <T1 extends SourceFile, T2 extends SourceFile> void init(
List<T1> externs, List<T2> sources, CompilerOptions options) { List<T1> externs, List<T2> sources, CompilerOptions options) {
JSModule module = new JSModule(JSModule.STRONG_MODULE_NAME); JSModule module = new JSModule(JSModule.SINGLETON_MODULE_NAME);
for (SourceFile source : sources) { for (SourceFile source : sources) {
module.add(new CompilerInput(source)); module.add(new CompilerInput(source));
} }
Expand All @@ -481,7 +481,6 @@ public <T extends SourceFile> void initModules(
initOptions(options); initOptions(options);


checkFirstModule(modules); checkFirstModule(modules);
modules = moveWeakSources(modules);
fillEmptyModules(modules); fillEmptyModules(modules);


this.externs = makeExternInputs(externs); this.externs = makeExternInputs(externs);
Expand Down Expand Up @@ -565,52 +564,6 @@ 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 * Empty modules get an empty "fill" file, so that we can move code into
* an empty module. * an empty module.
Expand All @@ -634,8 +587,9 @@ public static String joinPathParts(String... pathParts) {
/** Fill any empty modules with a place holder file. It makes any cross module motion easier. */ /** Fill any empty modules with a place holder file. It makes any cross module motion easier. */
private static void fillEmptyModules(Iterable<JSModule> modules) { private static void fillEmptyModules(Iterable<JSModule> modules) {
for (JSModule module : modules) { for (JSModule module : modules) {
if (!module.getName().equals(JSModule.WEAK_MODULE_NAME) && module.getInputs().isEmpty()) { if (module.getInputs().isEmpty()) {
module.add(SourceFile.fromCode(createFillFileName(module.getName()), "")); module.add(SourceFile.fromCode(
createFillFileName(module.getName()), ""));
} }
} }
} }
Expand Down Expand Up @@ -1467,8 +1421,10 @@ boolean addNewSourceAst(JsAst ast) {
CompilerInput newInput = new CompilerInput(ast); CompilerInput newInput = new CompilerInput(ast);


// TODO(tylerg): handle this for multiple modules at some point. // TODO(tylerg): handle this for multiple modules at some point.
JSModule firstModule = Iterables.getFirst(getModules(), null); Iterable<JSModule> modules = getModules();
if (firstModule.getName().equals(JSModule.STRONG_MODULE_NAME)) { if (Iterables.size(modules) == 1) {
// singleton module
JSModule firstModule = Iterables.getFirst(modules, null);
firstModule.add(newInput); firstModule.add(newInput);
} }


Expand Down
30 changes: 2 additions & 28 deletions src/com/google/javascript/jscomp/JSModule.java
View file
Expand Up @@ -40,16 +40,8 @@
* *
*/ */
public final class JSModule extends DependencyInfo.Base implements Serializable { public final class JSModule extends DependencyInfo.Base implements Serializable {
// The name of the artificial module containing all strong sources when there is no module spec. // The name of the artificial module containing all sources when no module spec is specified.
// If there is a module spec, strong sources go in their respective modules, and this module does public static final String SINGLETON_MODULE_NAME = "$singleton$";
// 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; private static final long serialVersionUID = 1;


Expand Down Expand Up @@ -230,16 +222,6 @@ public Set<JSModule> getThisAndAllDependencies() {
return deps; 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. * Gets this module's list of source code inputs.
* *
Expand Down Expand Up @@ -276,14 +258,6 @@ public boolean removeByName(String name) {
return found; 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). */ /** Returns the module name (primarily for debugging). */
@Override @Override
public String toString() { public String toString() {
Expand Down
18 changes: 1 addition & 17 deletions src/com/google/javascript/jscomp/JSModuleGraph.java
View file
Expand Up @@ -48,7 +48,6 @@
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.Set; import java.util.Set;
import javax.annotation.Nullable;


/** /**
* A {@link JSModule} dependency graph that assigns a depth to each module and can answer * A {@link JSModule} dependency graph that assigns a depth to each module and can answer
Expand Down Expand Up @@ -199,7 +198,7 @@ Iterable<CompilerInput> getAllInputs() {
int getInputCount() { int getInputCount() {
int count = 0; int count = 0;
for (JSModule module : modules) { for (JSModule module : modules) {
count += module.getInputCount(); count += module.getInputs().size();
} }
return count; return count;
} }
Expand All @@ -211,21 +210,6 @@ Iterable<JSModule> getAllModules() {
return Arrays.asList(modules); 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. * Gets all modules indexed by name.
*/ */
Expand Down
4 changes: 3 additions & 1 deletion src/com/google/javascript/jscomp/RecordFunctionInformation.java
View file
Expand Up @@ -71,12 +71,14 @@ public void visit(NodeTraversal t, Node n, Node parent) {


String compiledSource = compiler.toSource(n); String compiledSource = compiler.toSource(n);
JSModule module = t.getModule(); JSModule module = t.getModule();
String moduleName =
module.getName().equals(JSModule.SINGLETON_MODULE_NAME) ? "" : module.getName();
mapBuilder.addEntry( mapBuilder.addEntry(
FunctionInformationMap.Entry.newBuilder() FunctionInformationMap.Entry.newBuilder()
.setId(id) .setId(id)
.setSourceName(NodeUtil.getSourceName(n)) .setSourceName(NodeUtil.getSourceName(n))
.setLineNumber(n.getLineno()) .setLineNumber(n.getLineno())
.setModuleName(module.isSynthetic() ? "" : module.getName()) .setModuleName(moduleName)
.setSize(compiledSource.length()) .setSize(compiledSource.length())
.setName(functionNames.getFunctionName(n)) .setName(functionNames.getFunctionName(n))
.setCompiledSource(compiledSource) .setCompiledSource(compiledSource)
Expand Down
94 changes: 38 additions & 56 deletions test/com/google/javascript/jscomp/CommandLineRunnerTest.java
View file
Expand Up @@ -1350,23 +1350,19 @@ public void testChainModuleManifest() throws Exception {
lastCommandLineRunner.printModuleGraphManifestOrBundleTo( lastCommandLineRunner.printModuleGraphManifestOrBundleTo(
lastCompiler.getModuleGraph(), builder, true); lastCompiler.getModuleGraph(), builder, true);
assertThat(builder.toString()) assertThat(builder.toString())
.isEqualTo( .isEqualTo(Joiner.on('\n').join(
Joiner.on('\n') "{m0}",
.join( "i0.js",
"{m0}", "",
"i0.js", "{m1:m0}",
"", "i1.js",
"{m1:m0}", "",
"i1.js", "{m2:m1}",
"", "i2.js",
"{m2:m1}", "",
"i2.js", "{m3:m2}",
"", "i3.js",
"{m3:m2}", ""));
"i3.js",
"",
"{$weak$:m0,m1,m2,m3}",
""));
} }


public void testStarModuleManifest() throws Exception { public void testStarModuleManifest() throws Exception {
Expand All @@ -1378,23 +1374,19 @@ public void testStarModuleManifest() throws Exception {
lastCommandLineRunner.printModuleGraphManifestOrBundleTo( lastCommandLineRunner.printModuleGraphManifestOrBundleTo(
lastCompiler.getModuleGraph(), builder, true); lastCompiler.getModuleGraph(), builder, true);
assertThat(builder.toString()) assertThat(builder.toString())
.isEqualTo( .isEqualTo(Joiner.on('\n').join(
Joiner.on('\n') "{m0}",
.join( "i0.js",
"{m0}", "",
"i0.js", "{m1:m0}",
"", "i1.js",
"{m1:m0}", "",
"i1.js", "{m2:m0}",
"", "i2.js",
"{m2:m0}", "",
"i2.js", "{m3:m0}",
"", "i3.js",
"{m3:m0}", ""));
"i3.js",
"",
"{$weak$:m0,m1,m2,m3}",
""));
} }


public void testOutputModuleGraphJson() throws Exception { public void testOutputModuleGraphJson() throws Exception {
Expand Down Expand Up @@ -2122,17 +2114,12 @@ public void testJsonStreamAllowsAnyChunkName() {
fail("Unexpected exception " + e); fail("Unexpected exception " + e);
} }
String output = new String(outReader.toByteArray(), UTF_8); String output = new String(outReader.toByteArray(), UTF_8);
assertThat(output) assertThat(output).isEqualTo("[{\"src\":\"alert(\\\"foo\\\");\\n\","
.isEqualTo( + "\"path\":\"./foo/bar/baz.js\",\"source_map\":\"{\\n\\\"version\\\":3,"
"[{\"src\":\"alert(\\\"foo\\\");\\n\"," + "\\n\\\"file\\\":\\\"./foo/bar/baz.js\\\",\\n\\\"lineCount\\\":1,"
+ "\"path\":\"./foo/bar/baz.js\",\"source_map\":\"{\\n\\\"version\\\":3," + "\\n\\\"mappings\\\":\\\"AAAAA,KAAA,CAAM,KAAN;\\\","
+ "\\n\\\"file\\\":\\\"./foo/bar/baz.js\\\",\\n\\\"lineCount\\\":1," + "\\n\\\"sources\\\":[\\\"foo.js\\\"],"
+ "\\n\\\"mappings\\\":\\\"AAAAA,KAAA,CAAM,KAAN;\\\"," + "\\n\\\"names\\\":[\\\"alert\\\"]\\n}\\n\"}]");
+ "\\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\"}]");
} }


public void testOutputModuleNaming() { public void testOutputModuleNaming() {
Expand All @@ -2155,17 +2142,12 @@ public void testOutputModuleNaming() {
fail("Unexpected exception " + e); fail("Unexpected exception " + e);
} }
String output = new String(outReader.toByteArray(), UTF_8); String output = new String(outReader.toByteArray(), UTF_8);
assertThat(output) assertThat(output).isEqualTo("[{\"src\":\"alert(\\\"foo\\\");\\n\","
.isEqualTo( + "\"path\":\"./foo--bar.baz.js\",\"source_map\":\"{\\n\\\"version\\\":3,"
"[{\"src\":\"alert(\\\"foo\\\");\\n\"," + "\\n\\\"file\\\":\\\"./foo--bar.baz.js\\\",\\n\\\"lineCount\\\":1,"
+ "\"path\":\"./foo--bar.baz.js\",\"source_map\":\"{\\n\\\"version\\\":3," + "\\n\\\"mappings\\\":\\\"AAAAA,KAAA,CAAM,KAAN;\\\","
+ "\\n\\\"file\\\":\\\"./foo--bar.baz.js\\\",\\n\\\"lineCount\\\":1," + "\\n\\\"sources\\\":[\\\"foo.js\\\"],"
+ "\\n\\\"mappings\\\":\\\"AAAAA,KAAA,CAAM,KAAN;\\\"," + "\\n\\\"names\\\":[\\\"alert\\\"]\\n}\\n\"}]");
+ "\\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\"}]");
} }


public void testAssumeFunctionWrapper() { public void testAssumeFunctionWrapper() {
Expand Down

0 comments on commit c43eaff

Please sign in to comment.