From 99c53214a272d3b99e3a51f9aa6062884d274e82 Mon Sep 17 00:00:00 2001 From: bradfordcsmith Date: Mon, 24 Apr 2017 12:46:17 -0700 Subject: [PATCH] Don't allow a JSModule to be used in more than one JSModuleGraph. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=154083550 --- .../google/javascript/jscomp/Compiler.java | 47 ++++++++++++------- .../google/javascript/jscomp/JSModule.java | 14 ++++-- .../javascript/jscomp/JSModuleGraph.java | 14 ++++++ 3 files changed, 56 insertions(+), 19 deletions(-) diff --git a/src/com/google/javascript/jscomp/Compiler.java b/src/com/google/javascript/jscomp/Compiler.java index f5413bda27b..fb887966bbd 100644 --- a/src/com/google/javascript/jscomp/Compiler.java +++ b/src/com/google/javascript/jscomp/Compiler.java @@ -126,8 +126,6 @@ public class Compiler extends AbstractCompiler implements ErrorHandler, SourceFi // The JS source modules private List 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; // The module loader for resolving paths into module URIs. @@ -536,18 +534,14 @@ public void initModules( // Generate the module graph, and report any errors in the module // specification as errors. this.modules = modules; - if (modules.size() > 1) { - try { - this.moduleGraph = new JSModuleGraph(modules); - } catch (JSModuleGraph.ModuleDependenceException e) { - // problems with the module format. Report as an error. The - // message gives all details. - report(JSError.make(MODULE_DEPENDENCY_ERROR, - e.getModule().getName(), e.getDependentModule().getName())); - return; - } - } else { - this.moduleGraph = null; + try { + this.moduleGraph = new JSModuleGraph(modules); + } catch (JSModuleGraph.ModuleDependenceException e) { + // problems with the module format. Report as an error. The + // message gives all details. + report(JSError.make(MODULE_DEPENDENCY_ERROR, + e.getModule().getName(), e.getDependentModule().getName())); + return; } this.inputs = getAllInputsFromModules(modules); @@ -559,6 +553,16 @@ public void initModules( 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. */ @@ -1342,9 +1346,20 @@ boolean addNewSourceAst(JsAst ast) { return true; } + /** + * The graph of the JS source modules. + * + *

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 JSModuleGraph getModuleGraph() { - return moduleGraph; + if (moduleGraph != null && modules.size() > 1) { + return moduleGraph; + } else { + return null; + } } /** @@ -1352,7 +1367,7 @@ JSModuleGraph getModuleGraph() { * in the degenerate case when there's only one module. */ JSModuleGraph getDegenerateModuleGraph() { - return moduleGraph == null ? new JSModuleGraph(modules) : moduleGraph; + return moduleGraph; } @Override diff --git a/src/com/google/javascript/jscomp/JSModule.java b/src/com/google/javascript/jscomp/JSModule.java index 0fa39e7a33d..ff5160e6abd 100644 --- a/src/com/google/javascript/jscomp/JSModule.java +++ b/src/com/google/javascript/jscomp/JSModule.java @@ -16,6 +16,8 @@ package com.google.javascript.jscomp; +import static com.google.common.base.Preconditions.checkArgument; + import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -240,13 +242,18 @@ public String toString() { } /** - * Removes any references to nodes of the AST. This method is needed to - * allow the ASTs to be garbage collected if the modules are kept around. + * Removes any references to nodes of the AST and resets fields used by JSModuleGraph. + * + *

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) { input.clearAst(); } + depth = -1; } /** @@ -269,6 +276,7 @@ public void sortInputsByDeps(AbstractCompiler compiler) { * @param dep the depth to set */ public void setDepth(int dep) { + checkArgument(dep >= 0, "invalid depth: %s", dep); this.depth = dep; } diff --git a/src/com/google/javascript/jscomp/JSModuleGraph.java b/src/com/google/javascript/jscomp/JSModuleGraph.java index f65a914e5f6..06aaf235566 100644 --- a/src/com/google/javascript/jscomp/JSModuleGraph.java +++ b/src/com/google/javascript/jscomp/JSModuleGraph.java @@ -16,6 +16,8 @@ package com.google.javascript.jscomp; +import static com.google.common.base.Preconditions.checkState; + import com.google.common.annotations.GwtIncompatible; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -88,6 +90,7 @@ public JSModuleGraph(List modulesInDepOrder) { modulesByDepth = new ArrayList<>(); for (JSModule module : modulesInDepOrder) { + checkState(module.getDepth() == -1, "Module already used in another graph: %s", module); int depth = 0; for (JSModule dep : module.getDependencies()) { int depDepth = dep.getDepth(); @@ -108,6 +111,17 @@ public JSModuleGraph(List 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. */