Skip to content

Commit

Permalink
Distinguish the three kinds of disagreement between the initial stron…
Browse files Browse the repository at this point in the history 
…g/weak marking of files and entry point based pruning.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=246384471
  • Loading branch information
@tjgq @brad4d
tjgq authored and brad4d committed May 3, 2019
1 parent 9317356 commit 30e067e
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 44 deletions.
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/Compiler.java
View file
Expand Up @@ -1778,7 +1778,7 @@ void orderInputs() {
}

try {
moduleGraph.manageDependencies(options.getDependencyOptions());
moduleGraph.manageDependencies(this, options.getDependencyOptions());
staleInputs = true;
} catch (MissingProvideException e) {
report(JSError.make(
Expand Down
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/DependencyOptions.java
View file
Expand Up @@ -103,8 +103,8 @@ public boolean shouldPrune() {
/**
* Returns whether moochers should be dropped.
*
* <p>A moocher is a file that does not goog.provide a namespace and is not a goog.module, ES
* module or CommonJS module.
* <p>A moocher is a strong file that does not goog.provide a namespace and is not a goog.module,
* ES module or CommonJS module. Weak files are never considered moochers.
*
* <p>If true, moochers should not be considered implicit entry points.
*/
Expand Down
48 changes: 41 additions & 7 deletions src/com/google/javascript/jscomp/JSModuleGraph.java
View file
Expand Up @@ -59,6 +59,21 @@
*/
public final class JSModuleGraph implements Serializable {

static final DiagnosticType WEAK_FILE_REACHABLE_FROM_ENTRY_POINT_ERROR =
DiagnosticType.error(
"JSC_WEAK_FILE_REACHABLE_FROM_ENTRY_POINT_ERROR",
"File strongly reachable from an entry point must not be weak: {0}");

static final DiagnosticType EXPLICIT_WEAK_ENTRY_POINT_ERROR =
DiagnosticType.error(
"JSC_EXPLICIT_WEAK_ENTRY_POINT_ERROR",
"Explicit entry point input must not be weak: {0}");

static final DiagnosticType IMPLICIT_WEAK_ENTRY_POINT_ERROR =
DiagnosticType.warning(
"JSC_IMPLICIT_WEAK_ENTRY_POINT_ERROR",
"Implicit entry point input should not be weak: {0}");

private final JSModule[] modules;

/**
Expand Down Expand Up @@ -501,7 +516,8 @@ private static void moveMarkedWeakSources(JSModule weakModule, Iterable<Compiler
*
* @throws MissingProvideException if an entry point was not provided by any of the inputs.
*/
public ImmutableList<CompilerInput> manageDependencies(DependencyOptions dependencyOptions)
public ImmutableList<CompilerInput> manageDependencies(
AbstractCompiler compiler, DependencyOptions dependencyOptions)
throws MissingProvideException, MissingModuleException {

// Make a copy since we're going to mutate the graph below.
Expand All @@ -510,7 +526,7 @@ public ImmutableList<CompilerInput> manageDependencies(DependencyOptions depende
SortedDependencies<CompilerInput> sorter = new Es6SortedDependencies<>(originalInputs);

Set<CompilerInput> entryPointInputs =
createEntryPointInputs(dependencyOptions, getAllInputs(), sorter);
createEntryPointInputs(compiler, dependencyOptions, getAllInputs(), sorter);

HashMap<String, CompilerInput> inputsByProvide = new HashMap<>();
for (CompilerInput input : originalInputs) {
Expand Down Expand Up @@ -582,9 +598,12 @@ public ImmutableList<CompilerInput> manageDependencies(DependencyOptions depende
entryPointInputsPerModule.get(module), dependencyOptions.shouldSort());
}
for (CompilerInput input : transitiveClosure) {
if (dependencyOptions.shouldPrune() && input.getSourceFile().isWeak()) {
throw new IllegalStateException(
"A file that is reachable via an entry point cannot be marked as weak.");
if (dependencyOptions.shouldPrune()
&& input.getSourceFile().isWeak()
&& !entryPointInputs.contains(input)) {
compiler.report(
JSError.make(
WEAK_FILE_REACHABLE_FROM_ENTRY_POINT_ERROR, input.getSourceFile().getName()));
}
JSModule oldModule = input.getModule();
if (oldModule == null) {
Expand Down Expand Up @@ -668,6 +687,7 @@ private List<CompilerInput> getDepthFirstDependenciesOf(
}

private Set<CompilerInput> createEntryPointInputs(
AbstractCompiler compiler,
DependencyOptions dependencyOptions,
Iterable<CompilerInput> inputs,
SortedDependencies<CompilerInput> sorter)
Expand All @@ -683,7 +703,15 @@ private Set<CompilerInput> createEntryPointInputs(
}

if (!dependencyOptions.shouldDropMoochers()) {
entryPointInputs.addAll(sorter.getInputsWithoutProvides());
for (CompilerInput entryPointInput : sorter.getInputsWithoutProvides()) {
if (entryPointInput.getSourceFile().isWeak()) {
compiler.report(
JSError.make(
IMPLICIT_WEAK_ENTRY_POINT_ERROR, entryPointInput.getSourceFile().getName()));
} else {
entryPointInputs.add(entryPointInput);
}
}
}

for (ModuleIdentifier entryPoint : dependencyOptions.getEntryPoints()) {
Expand All @@ -709,7 +737,13 @@ private Set<CompilerInput> createEntryPointInputs(
throw new MissingProvideException(entryPoint.getName(), e);
}

entryPointInputs.add(entryPointInput);
if (entryPointInput.getSourceFile().isWeak()) {
compiler.report(
JSError.make(
EXPLICIT_WEAK_ENTRY_POINT_ERROR, entryPointInput.getSourceFile().getName()));
} else {
entryPointInputs.add(entryPointInput);
}
}
} else {
Iterables.addAll(entryPointInputs, inputs);
Expand Down
75 changes: 51 additions & 24 deletions test/com/google/javascript/jscomp/CompilerTest.java
View file
Expand Up @@ -15,10 +15,12 @@
*/
package com.google.javascript.jscomp;

import static com.google.common.collect.Iterables.getOnlyElement;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.javascript.jscomp.CompilerTestCase.lines;
import static com.google.javascript.jscomp.testing.JSCompCorrespondences.DIAGNOSTIC_EQUALITY;
import static com.google.javascript.jscomp.testing.JSErrorSubject.assertError;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.Assert.fail;

Expand Down Expand Up @@ -2538,7 +2540,7 @@ public void testTransitiveImplicitWeakSourcesWithEntryPoint() throws Exception {
}

@Test
public void testWeakAsEntryPointIsError() throws Exception {
public void testExplicitWeakEntryPointIsError() throws Exception {
SourceFile extern = SourceFile.fromCode("extern.js", "");
SourceFile weakEntry =
SourceFile.fromCode(
Expand All @@ -2557,16 +2559,45 @@ public void testWeakAsEntryPointIsError() throws Exception {
Compiler compiler = new Compiler();

compiler.init(ImmutableList.of(extern), ImmutableList.of(weakEntry), options);
compiler.parse();

try {
compiler.parse();
fail();
} catch (IllegalStateException e) {
// expected
assertThat(e)
.hasMessageThat()
.isEqualTo("A file that is reachable via an entry point cannot be marked as weak.");
}
assertThat(compiler.getWarnings()).isEmpty();
assertThat(compiler.getErrors()).hasSize(1);
assertError(getOnlyElement(compiler.getErrors()))
.hasMessage("Explicit entry point input must not be weak: weakEntry.js");
}

@Test
public void testImplicitWeakEntryPointIsWarning() throws Exception {
SourceFile extern = SourceFile.fromCode("extern.js", "/** @externs */ function alert(x) {}");
SourceFile weakMoocher =
SourceFile.fromCode(
"weakMoocher.js",
lines(
"const {T} = goog.require('weakByAssociation');",
"/** @param {!T} x */ function f(x) { alert(x); }"),
SourceKind.WEAK);
SourceFile weakByAssociation =
SourceFile.fromCode(
"weakByAssociation.js",
lines(
"goog.module('weakByAssociation');",
"/** @typedef {number|string} */ exports.T;",
"sideeffect();"));

CompilerOptions options = new CompilerOptions();
options.setDependencyOptions(DependencyOptions.pruneLegacyForEntryPoints(ImmutableList.of()));

Compiler compiler = new Compiler();

compiler.init(
ImmutableList.of(extern), ImmutableList.of(weakMoocher, weakByAssociation), options);
compiler.parse();

assertThat(compiler.getErrors()).isEmpty();
assertThat(compiler.getWarnings()).hasSize(1);
assertError(getOnlyElement(compiler.getWarnings()))
.hasMessage("Implicit entry point input should not be weak: weakMoocher.js");
}

@Test
Expand All @@ -2577,14 +2608,14 @@ public void testWeakStronglyReachableIsError() throws Exception {
"strong.js",
lines(
"goog.module('strong');",
"const T = goog.require('weakEntry');",
"const T = goog.require('weak');",
"/** @param {!T} x */ function f(x) { alert(x); }"),
SourceKind.STRONG);
SourceFile weakEntry =
SourceFile weak =
SourceFile.fromCode(
"weakEntry.js",
"weak.js",
lines(
"goog.module('weakEntry');",
"goog.module('weak');",
"/** @typedef {number|string} */ exports.T;",
"sideEffect();"),
SourceKind.WEAK);
Expand All @@ -2596,16 +2627,12 @@ public void testWeakStronglyReachableIsError() throws Exception {

Compiler compiler = new Compiler();

compiler.init(ImmutableList.of(extern), ImmutableList.of(strong, weakEntry), options);
compiler.init(ImmutableList.of(extern), ImmutableList.of(strong, weak), options);
compiler.parse();

try {
compiler.parse();
fail();
} catch (IllegalStateException e) {
// expected
assertThat(e)
.hasMessageThat()
.isEqualTo("A file that is reachable via an entry point cannot be marked as weak.");
}
assertThat(compiler.getWarnings()).isEmpty();
assertThat(compiler.getErrors()).hasSize(1);
assertError(getOnlyElement(compiler.getErrors()))
.hasMessage("File strongly reachable from an entry point must not be weak: weak.js");
}
}
25 changes: 15 additions & 10 deletions test/com/google/javascript/jscomp/JSModuleGraphTest.java
View file
Expand Up @@ -342,7 +342,7 @@ public void testManageDependenciesLooseWithoutEntryPoint() throws Exception {
makeGraph();
setUpManageDependenciesTest();
DependencyOptions depOptions = DependencyOptions.pruneLegacyForEntryPoints(ImmutableList.of());
List<CompilerInput> results = graph.manageDependencies(depOptions);
List<CompilerInput> results = graph.manageDependencies(compiler, depOptions);

assertInputs(moduleA, "a1", "a3");
assertInputs(moduleB, "a2", "b2");
Expand All @@ -361,7 +361,7 @@ public void testManageDependenciesLooseWithEntryPoint() throws Exception {
DependencyOptions depOptions =
DependencyOptions.pruneLegacyForEntryPoints(
ImmutableList.of(ModuleIdentifier.forClosure("c2")));
List<CompilerInput> results = graph.manageDependencies(depOptions);
List<CompilerInput> results = graph.manageDependencies(compiler, depOptions);

assertInputs(moduleA, "a1", "a3");
assertInputs(moduleB, "a2", "b2");
Expand All @@ -379,7 +379,7 @@ public void testManageDependenciesStrictWithEntryPoint() throws Exception {
setUpManageDependenciesTest();
DependencyOptions depOptions =
DependencyOptions.pruneForEntryPoints(ImmutableList.of(ModuleIdentifier.forClosure("c2")));
List<CompilerInput> results = graph.manageDependencies(depOptions);
List<CompilerInput> results = graph.manageDependencies(compiler, depOptions);

// Everything gets pushed up into module c, because that's
// the only one that has entry points.
Expand All @@ -396,7 +396,7 @@ public void testManageDependenciesSortOnly() throws Exception {
makeDeps();
makeGraph();
setUpManageDependenciesTest();
List<CompilerInput> results = graph.manageDependencies(DependencyOptions.sortOnly());
List<CompilerInput> results = graph.manageDependencies(compiler, DependencyOptions.sortOnly());

assertInputs(moduleA, "a1", "a2", "a3");
assertInputs(moduleB, "b1", "b2");
Expand All @@ -423,7 +423,7 @@ public void testManageDependenciesSortOnlyImpl() throws Exception {
input.setCompiler(compiler);
}

List<CompilerInput> results = graph.manageDependencies(DependencyOptions.sortOnly());
List<CompilerInput> results = graph.manageDependencies(compiler, DependencyOptions.sortOnly());

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

Expand All @@ -434,7 +434,7 @@ public void testManageDependenciesSortOnlyImpl() throws Exception {
public void testNoFiles() throws Exception {
makeDeps();
makeGraph();
List<CompilerInput> results = graph.manageDependencies(DependencyOptions.sortOnly());
List<CompilerInput> results = graph.manageDependencies(compiler, DependencyOptions.sortOnly());
assertThat(results).isEmpty();
}

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

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

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

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

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

assertInputs(
moduleA,
Expand Down Expand Up @@ -641,7 +641,7 @@ public void testMoveMarkedWeakSourcesDuringManageDepsSortOnly() throws Exception
}

makeGraph();
graph.manageDependencies(DependencyOptions.sortOnly());
graph.manageDependencies(compiler, DependencyOptions.sortOnly());

assertThat(
getWeakModule().getInputs().stream()
Expand Down Expand Up @@ -673,6 +673,7 @@ public void testIgnoreMarkedWeakSourcesDuringManageDepsPrune() throws Exception

makeGraph();
graph.manageDependencies(
compiler,
DependencyOptions.pruneForEntryPoints(
ImmutableList.of(
ModuleIdentifier.forFile("strong1"), ModuleIdentifier.forFile("strong2"))));
Expand Down Expand Up @@ -715,6 +716,7 @@ public void testIgnoreDepsOfMarkedWeakSourcesDuringManageDepsPrune() throws Exce

makeGraph();
graph.manageDependencies(
compiler,
DependencyOptions.pruneForEntryPoints(
ImmutableList.of(
ModuleIdentifier.forFile("strong1"), ModuleIdentifier.forFile("strong2"))));
Expand Down Expand Up @@ -747,6 +749,7 @@ public void testMoveImplicitWeakSourcesFromMoocherDuringManageDepsLegacyPrune()

makeGraph();
graph.manageDependencies(
compiler,
DependencyOptions.pruneLegacyForEntryPoints(
ImmutableList.of(ModuleIdentifier.forFile("strong"))));

Expand Down Expand Up @@ -779,7 +782,7 @@ public void testImplicitWeakSourcesNotMovedDuringManageDepsSortOnly() throws Exc
}

makeGraph();
graph.manageDependencies(DependencyOptions.sortOnly());
graph.manageDependencies(compiler, DependencyOptions.sortOnly());

assertThat(getWeakModule().getInputs()).isEmpty();
assertThat(
Expand Down Expand Up @@ -807,6 +810,7 @@ public void testImplicitWeakSourcesMovedDuringManageDepsPrune() throws Exception

makeGraph();
graph.manageDependencies(
compiler,
DependencyOptions.pruneForEntryPoints(
ImmutableList.of(
ModuleIdentifier.forFile("strong1"), ModuleIdentifier.forFile("strong2"))));
Expand Down Expand Up @@ -847,6 +851,7 @@ public void testTransitiveWeakSources() throws Exception {

makeGraph();
graph.manageDependencies(
compiler,
DependencyOptions.pruneForEntryPoints(
ImmutableList.of(ModuleIdentifier.forFile("strong1"))));

Expand Down

0 comments on commit 30e067e

Please sign in to comment.