Skip to content

Commit

Permalink
Remove the Compiler inputs member variable.
Browse files Browse the repository at this point in the history 
The `inputs`, `modules` and `moduleGraph` compiler data structures overlap, which violates the SPOT rule and makes maintenance of the compiler's internal state more error-prone. This CL remedies part of the problem by removing `inputs` in favor of a `JSModuleGraph#getAllInputs` method, which returns an iterable over inputs in dependency order.

Note that it's already the case that `inputs` is in dependency order, since `initModules` and `rebuildInputsFromModules` (two of the three places where `inputs` is set, the third being `restoreState`) already initialize it in that manner. Thus, in most places where `inputs` occurs, we can simply replace it by a `moduleGraph.getAllInputs()` call. The exceptions are the methods that mutate the module graph: `hoistExterns` and `hoistNoCompileFiles`. In both cases, iteration must be over a copy of `getAllInputs()` to prevent concurrent modification.

Also note that, in `orderInputs`, passing `inputs` as an argument into `JSModuleGraph#manageDependencies` is redundant, since the graph itself contains that information.

Finally, a couple of places verify that the compiler has been initialized by a null check on `inputs`. These must be replaced by a null check on `moduleGraph`.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211408645
  • Loading branch information
@tjgq @lauraharker
tjgq authored and lauraharker committed Sep 4, 2018
1 parent 201741d commit 58ca0e1
Show file tree
Hide file tree
Showing 8 changed files with 106 additions and 132 deletions.
6 changes: 3 additions & 3 deletions src/com/google/javascript/jscomp/AbstractCompiler.java
View file
Expand Up @@ -83,10 +83,10 @@ public abstract class AbstractCompiler implements SourceExcerptProvider {
abstract JSModuleGraph getModuleGraph();

/**
* Gets the inputs in the order in which they are being processed.
* Only for use by {@code AbstractCompilerRunner}.
* Gets the inputs in the order in which they are being processed. Only for use by {@code
* AbstractCompilerRunner}.
*/
abstract List<CompilerInput> getInputsInOrder();
abstract Iterable<CompilerInput> getInputsInOrder();

/**
* Gets the total number of inputs.
Expand Down
138 changes: 56 additions & 82 deletions src/com/google/javascript/jscomp/Compiler.java
View file

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/ForwardingCompiler.java
View file
Expand Up @@ -85,7 +85,7 @@ public JSModuleGraph getModuleGraph() {
}

@Override
public List<CompilerInput> getInputsInOrder() {
public Iterable<CompilerInput> getInputsInOrder() {
return abstractCompiler.getInputsInOrder();
}

Expand Down
6 changes: 2 additions & 4 deletions src/com/google/javascript/jscomp/GlobalVarReferenceMap.java
View file
Expand Up @@ -44,10 +44,8 @@ class GlobalVarReferenceMap implements ReferenceMap {

private final Map<InputId, Integer> inputOrder;

/**
* @param inputs The ordered list of all inputs for the compiler.
*/
GlobalVarReferenceMap(List<CompilerInput> inputs, List<CompilerInput> externs) {
/** @param inputs The ordered list of all inputs for the compiler. */
GlobalVarReferenceMap(Iterable<CompilerInput> inputs, Iterable<CompilerInput> externs) {
inputOrder = new HashMap<>();
int ind = 0;
for (CompilerInput extern : externs) {
Expand Down
39 changes: 28 additions & 11 deletions src/com/google/javascript/jscomp/JSModuleGraph.java
View file
Expand Up @@ -186,6 +186,21 @@ public void breakThisGraphSoItsModulesCanBeReused() {
}
}

/** Gets an iterable over all input source files in dependency order. */
Iterable<CompilerInput> getAllInputs() {
return Iterables.concat(
Iterables.transform(Arrays.asList(modules), (module) -> module.getInputs()));
}

/** Gets the total number of input source files. */
int getInputCount() {
int count = 0;
for (JSModule module : modules) {
count += module.getInputs().size();
}
return count;
}

/**
* Gets an iterable over all modules in dependency order.
*/
Expand Down Expand Up @@ -398,20 +413,22 @@ private Set<JSModule> getTransitiveDeps(JSModule m) {
* Apply the dependency options to the list of sources, returning a new source list re-ordering
* and dropping files as necessary. This module graph will be updated to reflect the new list.
*
* @param inputs The original list of sources. Used to ensure that the sort is stable.
* @throws MissingProvideException if an entry point was not provided by any of the inputs.
* @see DependencyOptions for more info on how this works.
*/
public ImmutableList<CompilerInput> manageDependencies(
DependencyOptions depOptions, List<CompilerInput> inputs)
public ImmutableList<CompilerInput> manageDependencies(DependencyOptions depOptions)
throws MissingProvideException, MissingModuleException {

SortedDependencies<CompilerInput> sorter = new Es6SortedDependencies<>(inputs);
// Make a copy since we're going to mutate the graph below.
List<CompilerInput> originalInputs = ImmutableList.copyOf(getAllInputs());

SortedDependencies<CompilerInput> sorter = new Es6SortedDependencies<>(originalInputs);

Set<CompilerInput> entryPointInputs = createEntryPointInputs(depOptions, inputs, sorter);
Set<CompilerInput> entryPointInputs =
createEntryPointInputs(depOptions, getAllInputs(), sorter);

HashMap<String, CompilerInput> inputsByProvide = new HashMap<>();
for (CompilerInput input : inputs) {
for (CompilerInput input : originalInputs) {
for (String provide : input.getKnownProvides()) {
inputsByProvide.put(provide, input);
}
Expand All @@ -422,7 +439,7 @@ public ImmutableList<CompilerInput> manageDependencies(
// Dynamically imported files must be added to the module graph, but
// they should not be ordered ahead of the files that import them.
// We add them as entry points to ensure they get included.
for (CompilerInput input : inputs) {
for (CompilerInput input : originalInputs) {
for (String require : input.getDynamicRequires()) {
if (inputsByProvide.containsKey(require)) {
entryPointInputs.add(inputsByProvide.get(require));
Expand All @@ -432,7 +449,7 @@ public ImmutableList<CompilerInput> manageDependencies(

// The order of inputs, sorted independently of modules.
List<CompilerInput> absoluteOrder =
sorter.getDependenciesOf(inputs, depOptions.shouldSortDependencies());
sorter.getDependenciesOf(originalInputs, depOptions.shouldSortDependencies());

// Figure out which sources *must* be in each module.
ListMultimap<JSModule, CompilerInput> entryPointInputsPerModule =
Expand Down Expand Up @@ -461,7 +478,7 @@ public ImmutableList<CompilerInput> manageDependencies(
if (depOptions.shouldSortDependencies() && depOptions.shouldPruneDependencies()) {
transitiveClosure = new ArrayList<>();
// We need the ful set of dependencies for each module, so start with the full input set
Set<CompilerInput> inputsNotYetReached = new HashSet<>(inputs);
Set<CompilerInput> inputsNotYetReached = new HashSet<>(originalInputs);
for (CompilerInput entryPoint : entryPointInputsPerModule.get(module)) {
transitiveClosure.addAll(
getDepthFirstDependenciesOf(entryPoint, inputsNotYetReached, inputsByProvide));
Expand Down Expand Up @@ -545,7 +562,7 @@ private List<CompilerInput> getDepthFirstDependenciesOf(

private Set<CompilerInput> createEntryPointInputs(
DependencyOptions depOptions,
List<CompilerInput> inputs,
Iterable<CompilerInput> inputs,
SortedDependencies<CompilerInput> sorter)
throws MissingModuleException, MissingProvideException {
Set<CompilerInput> entryPointInputs = new LinkedHashSet<>();
Expand Down Expand Up @@ -588,7 +605,7 @@ private Set<CompilerInput> createEntryPointInputs(
entryPointInputs.add(entryPointInput);
}
} else {
entryPointInputs.addAll(inputs);
Iterables.addAll(entryPointInputs, inputs);
}
return entryPointInputs;
}
Expand Down
7 changes: 4 additions & 3 deletions src/com/google/javascript/jscomp/PrebuildAst.java
View file
Expand Up @@ -16,6 +16,7 @@

package com.google.javascript.jscomp;

import com.google.common.collect.Iterables;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
Expand All @@ -42,7 +43,7 @@ class PrebuildAst {
this.numParallelThreads = numParalleThreads;
}

void prebuild(List<CompilerInput> inputList) {
void prebuild(Iterable<CompilerInput> allInputs) {
ThreadFactory threadFactory = new ThreadFactory() {
@Override
public Thread newThread(Runnable r) {
Expand All @@ -60,9 +61,9 @@ public Thread newThread(Runnable r) {
new LinkedBlockingQueue<Runnable>(),
threadFactory);
ListeningExecutorService executorService = MoreExecutors.listeningDecorator(poolExecutor);
List<ListenableFuture<?>> futureList = new ArrayList<>(inputList.size());
List<ListenableFuture<?>> futureList = new ArrayList<>(Iterables.size(allInputs));
// TODO(moz): Support canceling all parsing on the first halting error
for (final CompilerInput input : inputList) {
for (final CompilerInput input : allInputs) {
futureList.add(executorService.submit(new Runnable() {
@Override
public void run() {
Expand Down
3 changes: 1 addition & 2 deletions src/com/google/javascript/jscomp/gwt/super/com/google/javascript/jscomp/PrebuildAst.java
View file
Expand Up @@ -16,7 +16,6 @@

package com.google.javascript.jscomp;

import java.util.List;

/** Gwt-compatible no-op version for {@code PrebuildAst}. */
// TODO(moz): Implement this using GWT's emulation of ListenableFuture and friends
Expand All @@ -25,5 +24,5 @@ class PrebuildAst {
PrebuildAst(AbstractCompiler compiler, int numParalleThreads) {
}

void prebuild(List<CompilerInput> inputList) {}
void prebuild(Iterable<CompilerInput> inputList) {}
}
37 changes: 11 additions & 26 deletions test/com/google/javascript/jscomp/JSModuleGraphTest.java
View file
Expand Up @@ -194,12 +194,12 @@ public void testGetTransitiveDepsDeepestFirst() {
}

public void testManageDependencies1() throws Exception {
List<CompilerInput> inputs = setUpManageDependenciesTest();
setUpManageDependenciesTest();
DependencyOptions depOptions = new DependencyOptions();
depOptions.setDependencySorting(true);
depOptions.setDependencyPruning(true);
depOptions.setEntryPoints(ImmutableList.<ModuleIdentifier>of());
List<CompilerInput> results = graph.manageDependencies(depOptions, inputs);
List<CompilerInput> results = graph.manageDependencies(depOptions);

assertInputs(A, "a1", "a3");
assertInputs(B, "a2", "b2");
Expand All @@ -210,12 +210,12 @@ public void testManageDependencies1() throws Exception {
}

public void testManageDependencies2() throws Exception {
List<CompilerInput> inputs = setUpManageDependenciesTest();
setUpManageDependenciesTest();
DependencyOptions depOptions = new DependencyOptions();
depOptions.setDependencySorting(true);
depOptions.setDependencyPruning(true);
depOptions.setEntryPoints(ImmutableList.of(ModuleIdentifier.forClosure("c2")));
List<CompilerInput> results = graph.manageDependencies(depOptions, inputs);
List<CompilerInput> results = graph.manageDependencies(depOptions);

assertInputs(A, "a1", "a3");
assertInputs(B, "a2", "b2");
Expand All @@ -227,13 +227,13 @@ public void testManageDependencies2() throws Exception {
}

public void testManageDependencies3Impl() throws Exception {
List<CompilerInput> inputs = setUpManageDependenciesTest();
setUpManageDependenciesTest();
DependencyOptions depOptions = new DependencyOptions();
depOptions.setDependencySorting(true);
depOptions.setDependencyPruning(true);
depOptions.setMoocherDropping(true);
depOptions.setEntryPoints(ImmutableList.of(ModuleIdentifier.forClosure("c2")));
List<CompilerInput> results = graph.manageDependencies(depOptions, inputs);
List<CompilerInput> results = graph.manageDependencies(depOptions);

// Everything gets pushed up into module c, because that's
// the only one that has entry points.
Expand All @@ -250,15 +250,7 @@ public void testManageDependencies4() throws Exception {
DependencyOptions depOptions = new DependencyOptions();
depOptions.setDependencySorting(true);

List<CompilerInput> inputs = new ArrayList<>();

// Add the inputs in a random order.
inputs.addAll(E.getInputs());
inputs.addAll(B.getInputs());
inputs.addAll(A.getInputs());
inputs.addAll(C.getInputs());

List<CompilerInput> results = graph.manageDependencies(depOptions, inputs);
List<CompilerInput> results = graph.manageDependencies(depOptions);

assertInputs(A, "a1", "a2", "a3");
assertInputs(B, "b1", "b2");
Expand Down Expand Up @@ -286,9 +278,7 @@ public void testManageDependencies5Impl() throws Exception {
DependencyOptions depOptions = new DependencyOptions();
depOptions.setDependencySorting(true);

List<CompilerInput> inputs = new ArrayList<>();
inputs.addAll(A.getInputs());
List<CompilerInput> results = graph.manageDependencies(depOptions, inputs);
List<CompilerInput> results = graph.manageDependencies(depOptions);

assertInputs(A, "base.js", "a1", "a2");

Expand All @@ -299,8 +289,7 @@ public void testNoFiles() throws Exception {
DependencyOptions depOptions = new DependencyOptions();
depOptions.setDependencySorting(true);

List<CompilerInput> inputs = new ArrayList<>();
List<CompilerInput> results = graph.manageDependencies(depOptions, inputs);
List<CompilerInput> results = graph.manageDependencies(depOptions);
assertThat(results).isEmpty();
}

Expand Down Expand Up @@ -378,9 +367,7 @@ public void testGoogBaseOrderedCorrectly() throws Exception {
input.setCompiler(compiler);
}

List<CompilerInput> inputs = new ArrayList<>();
inputs.addAll(A.getInputs());
List<CompilerInput> results = graph.manageDependencies(depOptions, inputs);
List<CompilerInput> results = graph.manageDependencies(depOptions);

assertInputs(A, "base.js", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a1");

Expand Down Expand Up @@ -437,9 +424,7 @@ public void testProperEs6ModuleOrdering() throws Exception {
input.setHasFullParseDependencyInfo(true);
}

List<CompilerInput> inputs = new ArrayList<>();
inputs.addAll(A.getInputs());
List<CompilerInput> results = graph.manageDependencies(depOptions, inputs);
List<CompilerInput> results = graph.manageDependencies(depOptions);

assertInputs(
A, "/b/c.js", "/b/b.js", "/b/a.js", "/important.js", "/a/b.js", "/a/a.js", "/entry.js");
Expand Down

0 comments on commit 58ca0e1

Please sign in to comment.