Skip to content

Commit

Permalink
Don't allow a JSModule to be used in more than one JSModuleGraph.
Browse files Browse the repository at this point in the history
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=154083550
  • Loading branch information
brad4d authored and Tyler Breisacher committed Apr 25, 2017
1 parent d3a9496 commit 99c5321
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 19 deletions.
47 changes: 31 additions & 16 deletions src/com/google/javascript/jscomp/Compiler.java
Expand Up @@ -126,8 +126,6 @@ public class Compiler extends AbstractCompiler implements ErrorHandler, SourceFi
// The JS source modules // The JS source modules
private List<JSModule> modules; private List<JSModule> modules;


// The graph of the JS source modules. Must be null if there are less than
// 2 modules, because we use this as a signal for which passes to run.
private JSModuleGraph moduleGraph; private JSModuleGraph moduleGraph;


// The module loader for resolving paths into module URIs. // The module loader for resolving paths into module URIs.
Expand Down Expand Up @@ -536,18 +534,14 @@ public <T extends SourceFile> void initModules(
// Generate the module graph, and report any errors in the module // Generate the module graph, and report any errors in the module
// specification as errors. // specification as errors.
this.modules = modules; this.modules = modules;
if (modules.size() > 1) { try {
try { this.moduleGraph = new JSModuleGraph(modules);
this.moduleGraph = new JSModuleGraph(modules); } catch (JSModuleGraph.ModuleDependenceException e) {
} catch (JSModuleGraph.ModuleDependenceException e) { // problems with the module format. Report as an error. The
// problems with the module format. Report as an error. The // message gives all details.
// message gives all details. report(JSError.make(MODULE_DEPENDENCY_ERROR,
report(JSError.make(MODULE_DEPENDENCY_ERROR, e.getModule().getName(), e.getDependentModule().getName()));
e.getModule().getName(), e.getDependentModule().getName())); return;
return;
}
} else {
this.moduleGraph = null;
} }


this.inputs = getAllInputsFromModules(modules); this.inputs = getAllInputsFromModules(modules);
Expand All @@ -559,6 +553,16 @@ public <T extends SourceFile> void initModules(
initAST(); initAST();
} }


/**
* Exists only for some tests that want to reuse JSModules.
* @deprecated Fix those tests.
*/
@Deprecated
public void breakThisCompilerSoItsModulesCanBeReused() {
moduleGraph.breakThisGraphSoItsModulesCanBeReused();
moduleGraph = null;
}

/** /**
* Do any initialization that is dependent on the compiler options. * Do any initialization that is dependent on the compiler options.
*/ */
Expand Down Expand Up @@ -1342,17 +1346,28 @@ boolean addNewSourceAst(JsAst ast) {
return true; return true;
} }


/**
* The graph of the JS source modules.
*
* <p>Must return null if there are less than 2 modules,
* because we use this as a signal for which passes to run.
* TODO(bradfordcsmith): Just check for a single module instead of null.
*/
@Override @Override
JSModuleGraph getModuleGraph() { JSModuleGraph getModuleGraph() {
return moduleGraph; if (moduleGraph != null && modules.size() > 1) {
return moduleGraph;
} else {
return null;
}
} }


/** /**
* Gets a module graph. This will always return a module graph, even * Gets a module graph. This will always return a module graph, even
* in the degenerate case when there's only one module. * in the degenerate case when there's only one module.
*/ */
JSModuleGraph getDegenerateModuleGraph() { JSModuleGraph getDegenerateModuleGraph() {
return moduleGraph == null ? new JSModuleGraph(modules) : moduleGraph; return moduleGraph;
} }


@Override @Override
Expand Down
14 changes: 11 additions & 3 deletions src/com/google/javascript/jscomp/JSModule.java
Expand Up @@ -16,6 +16,8 @@


package com.google.javascript.jscomp; package com.google.javascript.jscomp;


import static com.google.common.base.Preconditions.checkArgument;

import com.google.common.base.Preconditions; import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -240,13 +242,18 @@ public String toString() {
} }


/** /**
* Removes any references to nodes of the AST. This method is needed to * Removes any references to nodes of the AST and resets fields used by JSModuleGraph.
* allow the ASTs to be garbage collected if the modules are kept around. *
* <p>This method is needed by some tests to allow modules to be reused and their ASTs garbage
* collected.
* @deprecated Fix tests to avoid reusing modules.
*/ */
public void clearAsts() { @Deprecated
void resetThisModuleSoItCanBeReused() {
for (CompilerInput input : inputs) { for (CompilerInput input : inputs) {
input.clearAst(); input.clearAst();
} }
depth = -1;
} }


/** /**
Expand All @@ -269,6 +276,7 @@ public void sortInputsByDeps(AbstractCompiler compiler) {
* @param dep the depth to set * @param dep the depth to set
*/ */
public void setDepth(int dep) { public void setDepth(int dep) {
checkArgument(dep >= 0, "invalid depth: %s", dep);
this.depth = dep; this.depth = dep;
} }


Expand Down
14 changes: 14 additions & 0 deletions src/com/google/javascript/jscomp/JSModuleGraph.java
Expand Up @@ -16,6 +16,8 @@


package com.google.javascript.jscomp; package com.google.javascript.jscomp;


import static com.google.common.base.Preconditions.checkState;

import com.google.common.annotations.GwtIncompatible; import com.google.common.annotations.GwtIncompatible;
import com.google.common.base.Preconditions; import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -88,6 +90,7 @@ public JSModuleGraph(List<JSModule> modulesInDepOrder) {
modulesByDepth = new ArrayList<>(); modulesByDepth = new ArrayList<>();


for (JSModule module : modulesInDepOrder) { for (JSModule module : modulesInDepOrder) {
checkState(module.getDepth() == -1, "Module already used in another graph: %s", module);
int depth = 0; int depth = 0;
for (JSModule dep : module.getDependencies()) { for (JSModule dep : module.getDependencies()) {
int depDepth = dep.getDepth(); int depDepth = dep.getDepth();
Expand All @@ -108,6 +111,17 @@ public JSModuleGraph(List<JSModule> modulesInDepOrder) {
} }
} }


/**
* This only exists as a temprorary workaround.
* @deprecated Fix the tests that use this.
*/
@Deprecated
public void breakThisGraphSoItsModulesCanBeReused() {
for (JSModule m : modules) {
m.resetThisModuleSoItCanBeReused();
}
}

/** /**
* Gets an iterable over all modules in dependency order. * Gets an iterable over all modules in dependency order.
*/ */
Expand Down

0 comments on commit 99c5321

Please sign in to comment.