Skip to content

Commit

Permalink
Simplify CrossModuleReferenceCollector: part 2
Browse files Browse the repository at this point in the history 
Remove Behavior callback.

This required adding a global variable names map and using that instead of
Behavior for testing. This map will also be used when CrossModuleCodeMotion is
rewritten.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=156129669
  • Loading branch information
@brad4d
brad4d authored and Tyler Breisacher committed May 16, 2017
1 parent b9a049b commit f1cc33d
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 126 deletions.
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/CrossModuleCodeMotion.java
View file
Expand Up @@ -301,7 +301,7 @@ private void processRead(Reference ref, NamedInfo info) {

private void collectReferences(Node root) {
CrossModuleReferenceCollector collector = new CrossModuleReferenceCollector(
compiler, CrossModuleReferenceCollector.DO_NOTHING_BEHAVIOR,
compiler,
new Es6SyntacticScopeCreator(compiler));
collector.process(root);

Expand Down
58 changes: 18 additions & 40 deletions src/com/google/javascript/jscomp/CrossModuleReferenceCollector.java
View file
Expand Up @@ -19,19 +19,25 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.javascript.jscomp.NodeTraversal.ScopedCallback;
import com.google.javascript.rhino.Node;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

/** Collects global variable references for use by {@link CrossModuleCodeMotion}. */
public final class CrossModuleReferenceCollector implements ScopedCallback, CompilerPass {

/** Maps global variable name to the corresponding {@link Var} object. */
private final Map<String, Var> varsByName = new HashMap<>();

/**
* Maps a given variable to a collection of references to that name. Note that
* Var objects are not stable across multiple traversals (unlike scope root or
Expand All @@ -45,11 +51,6 @@ public final class CrossModuleReferenceCollector implements ScopedCallback, Comp
*/
private List<BasicBlock> blockStack = new ArrayList<>();

/**
* Source of behavior at various points in the traversal.
*/
private final Behavior behavior;

private final ScopeCreator scopeCreator;

/**
Expand All @@ -68,10 +69,8 @@ public final class CrossModuleReferenceCollector implements ScopedCallback, Comp
/**
* Constructor initializes block stack.
*/
CrossModuleReferenceCollector(AbstractCompiler compiler, Behavior behavior,
ScopeCreator creator) {
CrossModuleReferenceCollector(AbstractCompiler compiler, ScopeCreator creator) {
this.compiler = compiler;
this.behavior = behavior;
this.scopeCreator = creator;
}

Expand Down Expand Up @@ -113,18 +112,28 @@ ReferenceCollection getReferences(Var v) {
return referenceMap.get(v);
}

ImmutableMap<String, Var> getGlobalVariableNamesMap() {
return ImmutableMap.copyOf(varsByName);
}

/**
* For each node, update the block stack and reference collection
* as appropriate.
*/
@Override
public void visit(NodeTraversal t, Node n, Node parent) {
if (n.isName() || (n.isStringKey() && !n.hasChildren())) {
Var v = t.getScope().getVar(n.getString());
String varName = n.getString();
Var v = t.getScope().getVar(varName);

if (v != null) {
// Only global, non-exported names can be moved
if (v.isGlobal() && !compiler.getCodingConvention().isExported(v.getName())) {
if (varsByName.containsKey(varName)) {
checkState(Objects.equals(varsByName.get(varName), v));
} else {
varsByName.put(varName, v);
}
addReference(v, new Reference(n, t, peek(blockStack)));
}

Expand Down Expand Up @@ -205,7 +214,6 @@ public void exitScope(NodeTraversal t) {
if (t.getScope().isHoistScope()) {
pop(blockStack);
}
behavior.afterExitScope(t, new ReferenceMapWrapper(referenceMap));
}

/**
Expand Down Expand Up @@ -305,34 +313,4 @@ private void addReference(Var v, Reference reference) {
// Add this particular reference
referenceInfo.add(reference);
}

private static class ReferenceMapWrapper implements ReferenceMap {
private final Map<Var, ReferenceCollection> referenceMap;

public ReferenceMapWrapper(Map<Var, ReferenceCollection> referenceMap) {
this.referenceMap = referenceMap;
}

@Override
public ReferenceCollection getReferences(Var var) {
return referenceMap.get(var);
}
}

/**
* Way for callers to add specific behavior during traversal that
* utilizes the built-up reference information.
*/
public interface Behavior {
/**
* Called after we finish with a scope.
*/
void afterExitScope(NodeTraversal t, ReferenceMap referenceMap);
}

static final Behavior DO_NOTHING_BEHAVIOR = new Behavior() {
@Override
public void afterExitScope(NodeTraversal t, ReferenceMap referenceMap) {}
};

}
152 changes: 67 additions & 85 deletions test/com/google/javascript/jscomp/CrossModuleReferenceCollectorTest.java
View file
Expand Up @@ -20,16 +20,15 @@
import static com.google.javascript.jscomp.CompilerOptions.LanguageMode.ECMASCRIPT_NEXT;
import static com.google.javascript.jscomp.testing.NodeSubject.assertNode;

import com.google.javascript.jscomp.CrossModuleReferenceCollector.Behavior;
import com.google.common.collect.ImmutableMap;
import com.google.javascript.rhino.Token;

public final class CrossModuleReferenceCollectorTest extends CompilerTestCase {
private Behavior behavior;
private CrossModuleReferenceCollector testedCollector;

@Override
public void setUp() {
setLanguage(ECMASCRIPT_NEXT, ECMASCRIPT_NEXT);
behavior = null;
}

@Override
Expand All @@ -44,119 +43,102 @@ protected int getNumRepetitions() {
@Override
protected CompilerPass getProcessor(final Compiler compiler) {
ScopeCreator scopeCreator = new Es6SyntacticScopeCreator(compiler);
return new CrossModuleReferenceCollector(
testedCollector = new CrossModuleReferenceCollector(
compiler,
this.behavior,
scopeCreator);
}

private void testBehavior(String js, Behavior behavior) {
this.behavior = behavior;
testSame(js);
return testedCollector;
}

public void testVarInBlock() {
testBehavior(
LINE_JOINER.join(
testSame(LINE_JOINER.join(
" if (true) {",
" var y = x;",
" y;",
" y;",
" }"),
new Behavior() {
@Override
public void afterExitScope(NodeTraversal t, ReferenceMap rm) {
if (t.getScope().isGlobal()) {
ReferenceCollection y = rm.getReferences(t.getScope().getVar("y"));
assertThat(y.isAssignedOnceInLifetime()).isTrue();
assertThat(y.isWellDefined()).isTrue();
}
}
});
" }"));
ImmutableMap<String, Var> globalVariableNamesMap = testedCollector.getGlobalVariableNamesMap();
assertThat(globalVariableNamesMap).containsKey("y");
Var yVar = globalVariableNamesMap.get("y");
ReferenceCollection yRefs = testedCollector.getReferences(yVar);
assertThat(yRefs.isAssignedOnceInLifetime()).isTrue();
assertThat(yRefs.isWellDefined()).isTrue();
}

public void testVarInLoopNotAssignedOnlyOnceInLifetime() {
Behavior behavior =
new Behavior() {
@Override
public void afterExitScope(NodeTraversal t, ReferenceMap rm) {
if (t.getScope().isGlobal()) {
ReferenceCollection x = rm.getReferences(t.getScope().getVar("x"));
assertThat(x.isAssignedOnceInLifetime()).isFalse();
}
}
};
testBehavior("var x; while (true) { x = 0; }", behavior);
testBehavior("let x; while (true) { x = 0; }", behavior);
testSame("var x; while (true) { x = 0; }");
ImmutableMap<String, Var> globalVariableNamesMap = testedCollector.getGlobalVariableNamesMap();
Var xVar = globalVariableNamesMap.get("x");
assertThat(globalVariableNamesMap).containsKey("x");
ReferenceCollection xRefs = testedCollector.getReferences(xVar);
assertThat(xRefs.isAssignedOnceInLifetime()).isFalse();

testSame("let x; while (true) { x = 0; }");
globalVariableNamesMap = testedCollector.getGlobalVariableNamesMap();
xVar = globalVariableNamesMap.get("x");
assertThat(globalVariableNamesMap).containsKey("x");
xRefs = testedCollector.getReferences(xVar);
assertThat(xRefs.isAssignedOnceInLifetime()).isFalse();
}

/**
* Although there is only one assignment to x in the code, it's in a function which could be
* called multiple times, so {@code isAssignedOnceInLifetime()} returns false.
*/
public void testVarInFunctionNotAssignedOnlyOnceInLifetime() {
Behavior behavior =
new Behavior() {
@Override
public void afterExitScope(NodeTraversal t, ReferenceMap rm) {
if (t.getScope().isGlobal()) {
ReferenceCollection x = rm.getReferences(t.getScope().getVar("x"));
assertThat(x.isAssignedOnceInLifetime()).isFalse();
}
}
};
testBehavior("var x; function f() { x = 0; }", behavior);
testBehavior("let x; function f() { x = 0; }", behavior);
testSame("var x; function f() { x = 0; }");
ImmutableMap<String, Var> globalVariableNamesMap = testedCollector.getGlobalVariableNamesMap();
Var xVar = globalVariableNamesMap.get("x");
assertThat(globalVariableNamesMap).containsKey("x");
ReferenceCollection xRefs = testedCollector.getReferences(xVar);
assertThat(xRefs.isAssignedOnceInLifetime()).isFalse();

testSame("let x; function f() { x = 0; }");
globalVariableNamesMap = testedCollector.getGlobalVariableNamesMap();
xVar = globalVariableNamesMap.get("x");
assertThat(globalVariableNamesMap).containsKey("x");
xRefs = testedCollector.getReferences(xVar);
assertThat(xRefs.isAssignedOnceInLifetime()).isFalse();
}

public void testVarAssignedOnceInLifetime1() {
Behavior behavior =
new Behavior() {
@Override
public void afterExitScope(NodeTraversal t, ReferenceMap rm) {
if (t.getScope().isGlobal()) {
ReferenceCollection x = rm.getReferences(t.getScope().getVar("x"));
assertThat(x.isAssignedOnceInLifetime()).isTrue();
}
}
};
testBehavior("var x = 0;", behavior);
testBehavior("let x = 0;", behavior);
testSame("var x = 0;");
ImmutableMap<String, Var> globalVariableNamesMap = testedCollector.getGlobalVariableNamesMap();
Var xVar = globalVariableNamesMap.get("x");
assertThat(globalVariableNamesMap).containsKey("x");
ReferenceCollection xRefs = testedCollector.getReferences(xVar);
assertThat(xRefs.isAssignedOnceInLifetime()).isTrue();

testSame("let x = 0;");
globalVariableNamesMap = testedCollector.getGlobalVariableNamesMap();
xVar = globalVariableNamesMap.get("x");
assertThat(globalVariableNamesMap).containsKey("x");
xRefs = testedCollector.getReferences(xVar);
assertThat(xRefs.isAssignedOnceInLifetime()).isTrue();
}

public void testVarAssignedOnceInLifetime2() {
testBehavior(
"{ var x = 0; }",
new Behavior() {
@Override
public void afterExitScope(NodeTraversal t, ReferenceMap rm) {
if (t.getScope().isGlobal()) {
ReferenceCollection x = rm.getReferences(t.getScope().getVar("x"));
assertThat(x.isAssignedOnceInLifetime()).isTrue();
}
}
});
testSame("{ var x = 0; }");
ImmutableMap<String, Var> globalVariableNamesMap = testedCollector.getGlobalVariableNamesMap();
Var xVar = globalVariableNamesMap.get("x");
assertThat(globalVariableNamesMap).containsKey("x");
ReferenceCollection xRefs = testedCollector.getReferences(xVar);
assertThat(xRefs.isAssignedOnceInLifetime()).isTrue();
}

public void testBasicBlocks() {
testBehavior(
LINE_JOINER.join(
testSame(LINE_JOINER.join(
"var x = 0;",
"switch (x) {",
" case 0:",
" x;",
"}"),
new Behavior() {
@Override
public void afterExitScope(NodeTraversal t, ReferenceMap rm) {
if (t.getScope().isGlobal()) {
ReferenceCollection x = rm.getReferences(t.getScope().getVar("x"));
assertThat(x.references).hasSize(3);
assertNode(x.references.get(0).getBasicBlock().getRoot()).hasType(Token.ROOT);
assertNode(x.references.get(1).getBasicBlock().getRoot()).hasType(Token.ROOT);
assertNode(x.references.get(2).getBasicBlock().getRoot()).hasType(Token.CASE);
}
}
});
"}"));
ImmutableMap<String, Var> globalVariableNamesMap = testedCollector.getGlobalVariableNamesMap();
Var xVar = globalVariableNamesMap.get("x");
assertThat(globalVariableNamesMap).containsKey("x");
ReferenceCollection xRefs = testedCollector.getReferences(xVar);
assertThat(xRefs.references).hasSize(3);
assertNode(xRefs.references.get(0).getBasicBlock().getRoot()).hasType(Token.ROOT);
assertNode(xRefs.references.get(1).getBasicBlock().getRoot()).hasType(Token.ROOT);
assertNode(xRefs.references.get(2).getBasicBlock().getRoot()).hasType(Token.CASE);
}
}

0 comments on commit f1cc33d

Please sign in to comment.