From 0f952c4eae4279293673e993a6d915868450d4be Mon Sep 17 00:00:00 2001 From: tjgq Date: Fri, 30 Nov 2018 18:32:57 -0800 Subject: [PATCH] Check more carefully whether a weak module already exists and test that the weak module invariants are enforced. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=223606496 --- .../google/javascript/jscomp/Compiler.java | 90 ++++++++++------- .../javascript/jscomp/CompilerTest.java | 98 +++++++++++++++++++ 2 files changed, 153 insertions(+), 35 deletions(-) diff --git a/src/com/google/javascript/jscomp/Compiler.java b/src/com/google/javascript/jscomp/Compiler.java index 2b2cd4e092e..1661ad384ca 100644 --- a/src/com/google/javascript/jscomp/Compiler.java +++ b/src/com/google/javascript/jscomp/Compiler.java @@ -567,49 +567,69 @@ private void checkFirstModule(List modules) { } } - /** Moves all weak sources into a separate weak module that depends on every other module. */ - private List moveWeakSources(List modules) { - // Collect weak sources. - List weakInputs = new ArrayList<>(); + /** + * Returns a new module list where all weak sources have been moved into a separate weak module + * that depends on every other module. + * + *

The weak module may already exist when calling the compiler through the Java API, e.g. when + * restoring state for a multi-stage compilation. In this case, this method asserts that the weak + * module contains exactly the weak sources and depends on every other module. + * + * @throws RuntimeException if the weak module already exists but it does not respect the weak + * module invariants. + */ + private static List moveWeakSources(List modules) { + // Try to find an existing weak module, and collect weak sources belonging to strong modules. + JSModule weakModule = null; + List weakInputsToMove = 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++; + weakModule = module; + } else { + for (int i = 0; i < module.getInputCount(); ) { + CompilerInput input = module.getInput(i); + if (input.getSourceFile().isWeak()) { + module.remove(input); + weakInputsToMove.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); + if (weakModule != null) { + // The weak module already exists. Check the invariants. + if (!weakInputsToMove.isEmpty()) { + throw new RuntimeException( + "A weak module already exists but weak sources were found in other modules."); + } + for (CompilerInput input : weakModule.getInputs()) { + if (!input.getSourceFile().isWeak()) { + throw new RuntimeException( + "A weak module already exists but strong sources were found in it."); + } + } + ImmutableSet deps = ImmutableSet.copyOf(weakModule.getDependencies()); + for (JSModule module : modules) { + if (!module.equals(weakModule) && !deps.contains(module)) { + throw new RuntimeException( + "A weak module already exists but it does not depend on every other module."); + } + } + } else { + // The weak module does not exist yet. Create it and move the weak sources into it. + weakModule = new JSModule(JSModule.WEAK_MODULE_NAME); + for (JSModule module : modules) { + weakModule.addDependency(module); + } + for (CompilerInput input : weakInputsToMove) { + weakModule.add(input); + } + // Make a copy in case the original list is immutable. + modules = ImmutableList.builder().addAll(modules).add(weakModule).build(); } - // Make a copy in case the original list is immutable. - modules = ImmutableList.builder().addAll(modules).add(weakModule).build(); - return modules; } diff --git a/test/com/google/javascript/jscomp/CompilerTest.java b/test/com/google/javascript/jscomp/CompilerTest.java index 97054c33f85..97e04699fab 100644 --- a/test/com/google/javascript/jscomp/CompilerTest.java +++ b/test/com/google/javascript/jscomp/CompilerTest.java @@ -19,6 +19,7 @@ import static com.google.common.truth.Truth.assertWithMessage; import static com.google.javascript.jscomp.CompilerTestCase.lines; import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.fail; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; @@ -2077,4 +2078,101 @@ public void testWeakSourcesEntryPoint() throws Exception { assertThat(compiler.toSource()) .isEqualTo("var module$exports$strong={};function module$contents$strong_f(x){alert(x)};"); } + + @Test + public void testPreexistingWeakModule() throws Exception { + JSModule strong = new JSModule("m"); + strong.add(SourceFile.fromCode("strong.js", "goog.provide('a');", SourceKind.STRONG)); + JSModule weak = new JSModule(JSModule.WEAK_MODULE_NAME); + weak.add(SourceFile.fromCode("weak.js", "goog.provide('b');", SourceKind.WEAK)); + weak.addDependency(strong); + + CompilerOptions options = new CompilerOptions(); + options.setEmitUseStrict(false); + options.setClosurePass(true); + + Compiler compiler = new Compiler(); + + compiler.initModules(ImmutableList.of(), ImmutableList.of(strong, weak), options); + + compiler.parse(); + compiler.check(); + compiler.performOptimizations(); + + assertThat(compiler.getModuleGraph().getModuleCount()).isEqualTo(2); + assertThat(Iterables.get(compiler.getModuleGraph().getAllModules(), 0).getName()) + .isEqualTo("m"); + assertThat(Iterables.get(compiler.getModuleGraph().getAllModules(), 1).getName()) + .isEqualTo(JSModule.WEAK_MODULE_NAME); + + assertThat(compiler.toSource()).isEqualTo("var a={};"); + } + + @Test + public void testPreexistingWeakModuleWithAdditionalStrongSources() throws Exception { + JSModule strong = new JSModule("m"); + strong.add(SourceFile.fromCode("strong.js", "goog.provide('a');", SourceKind.STRONG)); + JSModule weak = new JSModule(JSModule.WEAK_MODULE_NAME); + weak.add(SourceFile.fromCode("weak.js", "goog.provide('b');", SourceKind.WEAK)); + weak.add( + SourceFile.fromCode( + "weak_but_actually_strong.js", "goog.provide('c');", SourceKind.STRONG)); + weak.addDependency(strong); + + CompilerOptions options = new CompilerOptions(); + Compiler compiler = new Compiler(); + + try { + compiler.initModules(ImmutableList.of(), ImmutableList.of(strong, weak), options); + fail(); + } catch (RuntimeException e) { + assertThat(e) + .hasMessageThat() + .isEqualTo("A weak module already exists but strong sources were found in it."); + } + } + + @Test + public void testPreexistingWeakModuleWithMissingWeakSources() throws Exception { + JSModule strong = new JSModule("m"); + strong.add(SourceFile.fromCode("strong.js", "goog.provide('a');", SourceKind.STRONG)); + strong.add( + SourceFile.fromCode("strong_but_actually_weak.js", "goog.provide('b');", SourceKind.WEAK)); + JSModule weak = new JSModule(JSModule.WEAK_MODULE_NAME); + weak.add(SourceFile.fromCode("weak.js", "goog.provide('c');", SourceKind.WEAK)); + weak.addDependency(strong); + + CompilerOptions options = new CompilerOptions(); + Compiler compiler = new Compiler(); + + try { + compiler.initModules(ImmutableList.of(), ImmutableList.of(strong, weak), options); + fail(); + } catch (RuntimeException e) { + + assertThat(e) + .hasMessageThat() + .isEqualTo("A weak module already exists but weak sources were found in other modules."); + } + } + + @Test + public void testPreexistingWeakModuleWithIncorrectDependencies() throws Exception { + JSModule m1 = new JSModule("m1"); + JSModule m2 = new JSModule("m2"); + JSModule weak = new JSModule(JSModule.WEAK_MODULE_NAME); + weak.addDependency(m1); + + CompilerOptions options = new CompilerOptions(); + Compiler compiler = new Compiler(); + + try { + compiler.initModules(ImmutableList.of(), ImmutableList.of(m1, m2, weak), options); + fail(); + } catch (RuntimeException e) { + assertThat(e) + .hasMessageThat() + .isEqualTo("A weak module already exists but it does not depend on every other module."); + } + } }