Skip to content

Commit

Permalink
Restructure LinkedFlowScope and TypeInference to prepare for block-sc…
Browse files Browse the repository at this point in the history 
…ope support.

* LinkedFlowScope gets a new syntacticScope field, which determines the block scope for name lookups and is populated (and read) by TypeInference upon flowing into a node, via a new parameter to FlowScope#createChildFlowScope.

* TypeInference now has to deal with multiple scopes, so some special handling for "declaratively unbound vars" has been factored out.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=191409809
  • Loading branch information
@shicks @lauraharker
shicks authored and lauraharker committed Apr 4, 2018
1 parent b2837ba commit 5a68bc0
Show file tree
Hide file tree
Showing 5 changed files with 177 additions and 95 deletions.
38 changes: 29 additions & 9 deletions src/com/google/javascript/jscomp/AbstractScope.java
View file
Expand Up @@ -34,15 +34,16 @@
* scope. * scope.
* *
* <p>ES 2015 introduces new scoping rules, which adds some complexity to this class. In particular, * <p>ES 2015 introduces new scoping rules, which adds some complexity to this class. In particular,
* scopes fall into two mutually exclusive categories: <i>block</i> and <i>container</i>. Block * scopes fall into two mutually exclusive categories: <i>block</i> and <i>container</i>. Block
* scopes are all scopes whose roots are blocks, as well as any control structures whose optional * scopes are all scopes whose roots are blocks, as well as any control structures whose optional
* blocks are omitted. These scopes did not exist at all prior to ES 2015. Container scopes * blocks are omitted. These scopes did not exist at all prior to ES 2015. Container scopes comprise
* comprise function scopes, global scopes, and module scopes, and (aside from modules, which * function scopes, global scopes, and module scopes, and (aside from modules, which didn't exist in
* didn't exist in ES5) corresponds to the ES5 scope rules. This corresponds roughly to one * ES5) corresponds to the ES5 scope rules. This corresponds roughly to one container scope per CFG
* container scope per CFG root (but not exactly, due to SCRIPT-level CFGs). * root (but not exactly, due to SCRIPT-level CFGs).
* *
* <p>All container scopes, plus the outermost block scope within a function (i.e. the <i>function * <p>All container scopes, plus the outermost block scope within a function (i.e. the <i>function
* block scope</i>) are considered <i>hoist scopes</i>. Hoist scopes are relevant because "var" * block scope</i>) are considered <i>hoist scopes</i>. All functions thus have two hoist scopes:
* the function scope and the function block scope. Hoist scopes are relevant because "var"
* declarations are hoisted to the closest hoist scope, as opposed to "let" and "const" which always * declarations are hoisted to the closest hoist scope, as opposed to "let" and "const" which always
* apply to the specific scope in which they occur. * apply to the specific scope in which they occur.
* *
Expand Down Expand Up @@ -380,10 +381,29 @@ void checkRootScope() {
NodeUtil.createsScope(rootNode) || rootNode.isScript() || rootNode.isRoot(), rootNode); NodeUtil.createsScope(rootNode) || rootNode.isScript() || rootNode.isRoot(), rootNode);
} }


/** Returns the nearest common parent between two scopes. */
S getCommonParent(S other) {
S left = thisScope();
S right = other;
while (left != null && right != null && left != right) {
int leftDepth = left.getDepth();
int rightDepth = right.getDepth();
if (leftDepth >= rightDepth) {
left = left.getParent();
}
if (leftDepth <= rightDepth) {
right = right.getParent();
}
}

checkState(left != null && left == right);
return left;
}

/** /**
* Whether {@code this} and the {@code other} scope have the same container scope (i.e. are in * Whether {@code this} and the {@code other} scope have the same container scope (i.e. are in the
* the same function, or else both in the global hoist scope). This is equivalent to asking * same function, or else both in the global hoist scope). This is equivalent to asking whether
* whether the two scopes would be equivalent in a pre-ES2015-block-scopes view of the world. * the two scopes would be equivalent in a pre-ES2015-block-scopes view of the world.
*/ */
boolean hasSameContainerScope(S other) { boolean hasSameContainerScope(S other) {
return getClosestContainerScope() == other.getClosestContainerScope(); return getClosestContainerScope() == other.getClosestContainerScope();
Expand Down
146 changes: 100 additions & 46 deletions src/com/google/javascript/jscomp/LinkedFlowScope.java
View file
Expand Up @@ -39,12 +39,16 @@
* @author nicksantos@google.com (Nick Santos) * @author nicksantos@google.com (Nick Santos)
*/ */
class LinkedFlowScope implements FlowScope { class LinkedFlowScope implements FlowScope {

// The closest flow scope cache. // The closest flow scope cache.
private final FlatFlowScopeCache cache; private final FlatFlowScopeCache cache;


// The parent flow scope. // The parent flow scope.
private final LinkedFlowScope parent; private final LinkedFlowScope parent;


// The TypedScope for the block that this flow scope is defined for.
private final TypedScope syntacticScope;

// The distance between this flow scope and the closest flat flow scope. // The distance between this flow scope and the closest flat flow scope.
private int depth; private int depth;


Expand All @@ -62,48 +66,43 @@ class LinkedFlowScope implements FlowScope {
private LinkedFlowSlot lastSlot; private LinkedFlowSlot lastSlot;


/** /**
* Creates a flow scope without a direct parent. This can happen in three cases: (1) the "bottom" * Creates a flow scope without a direct parent. This can happen in three cases: (1) the "bottom"
* scope for a CFG root, (2) a direct child of a parent at the maximum depth, or (3) a joined * scope for a CFG root, (2) a direct child of a parent at the maximum depth, or (3) a joined
* scope with more than one direct parent. The parent is non-null only in the second case. * scope with more than one direct parent. The parent is non-null only in the second case.
*/ */
private LinkedFlowScope(FlatFlowScopeCache cache) { private LinkedFlowScope(FlatFlowScopeCache cache, TypedScope syntacticScope) {
this.cache = cache; this.cache = cache;
this.lastSlot = null; this.lastSlot = null;
this.depth = 0; this.depth = 0;
this.parent = cache.linkedEquivalent; this.parent = cache.linkedEquivalent;
this.syntacticScope = syntacticScope;
} }


/** /** Creates a child flow scope with a single parent. */
* Creates a child flow scope with a single parent. private LinkedFlowScope(LinkedFlowScope directParent, TypedScope syntacticScope) {
*/
private LinkedFlowScope(LinkedFlowScope directParent) {
this.cache = directParent.cache; this.cache = directParent.cache;
this.lastSlot = directParent.lastSlot; this.lastSlot = directParent.lastSlot;
this.depth = directParent.depth + 1; this.depth = directParent.depth + 1;
this.parent = directParent; this.parent = directParent;
} this.syntacticScope = syntacticScope;

/** Gets the function scope for this flow scope. */
private TypedScope getFunctionScope() {
return cache.functionScope;
} }


/** Whether this flows from a bottom scope. */ /** Whether this flows from a bottom scope. */
private boolean flowsFromBottom() { private boolean flowsFromBottom() {
return getFunctionScope().isBottom(); return cache.functionScope.isBottom();
} }


/** /**
* Creates an entry lattice for the flow. * Creates an entry lattice for the flow.
*/ */
public static LinkedFlowScope createEntryLattice(TypedScope scope) { public static LinkedFlowScope createEntryLattice(TypedScope scope) {
return new LinkedFlowScope(new FlatFlowScopeCache(scope)); return new LinkedFlowScope(new FlatFlowScopeCache(scope), scope);
} }


@Override @Override
public void inferSlotType(String symbol, JSType type) { public void inferSlotType(String symbol, JSType type) {
checkState(!frozen); checkState(!frozen);
ScopedName var = getVarFromFunctionScope(symbol); ScopedName var = getVarFromSyntacticScope(symbol);
lastSlot = new LinkedFlowSlot(var, type, lastSlot); lastSlot = new LinkedFlowSlot(var, type, lastSlot);
depth++; depth++;
cache.dirtySymbols.add(var); cache.dirtySymbols.add(var);
Expand All @@ -112,14 +111,26 @@ public void inferSlotType(String symbol, JSType type) {
@Override @Override
public void inferQualifiedSlot(Node node, String symbol, JSType bottomType, public void inferQualifiedSlot(Node node, String symbol, JSType bottomType,
JSType inferredType, boolean declared) { JSType inferredType, boolean declared) {
TypedScope functionScope = getFunctionScope(); if (cache.functionScope.isGlobal()) {
if (functionScope.isGlobal()) { // Do not infer qualified names on the global scope. Ideally these would be
// added to the scope by TypedScopeCreator, but if they are not, adding them
// here causes scaling problems (large projects can have tens of thousands of
// undeclared qualified names in the global scope) with no real benefit.
return; return;
} }

TypedVar v = syntacticScope.getVar(symbol);
TypedVar v = functionScope.getVar(symbol); if (v == null && !cache.functionScope.isBottom()) {
if (v == null && !functionScope.isBottom()) { // NOTE(sdh): Qualified names are declared on scopes lazily via this method.
v = functionScope.declare(symbol, node, bottomType, null, !declared); // The difficulty is that it's not always clear which scope they need to be
// defined on. In particular, syntacticScope is wrong because it is often a
// nested block scope that is ignored when branches are joined; functionScope
// is also wrong because it could lead to ambiguity if the same root name is
// declared in multiple different blocks. Instead, the qualified name is declared
// on the scope that owns the root, when possible.
TypedVar rootVar = syntacticScope.getVar(getRootOfQualifiedName(symbol));
TypedScope rootScope =
rootVar != null ? rootVar.getScope() : syntacticScope.getClosestHoistScope();
v = rootScope.declare(symbol, node, bottomType, null, !declared);
} }


JSType declaredType = v != null ? v.getType() : null; JSType declaredType = v != null ? v.getType() : null;
Expand Down Expand Up @@ -149,7 +160,7 @@ public JSType getTypeOfThis() {


@Override @Override
public Node getRootNode() { public Node getRootNode() {
return getFunctionScope().getRootNode(); return syntacticScope.getRootNode();
} }


@Override @Override
Expand All @@ -162,7 +173,7 @@ public StaticTypedScope<JSType> getParentScope() {
*/ */
@Override @Override
public StaticTypedSlot<JSType> getSlot(String name) { public StaticTypedSlot<JSType> getSlot(String name) {
return getSlot(getVarFromFunctionScope(name)); return getSlot(getVarFromSyntacticScope(name));
} }


private StaticTypedSlot<JSType> getSlot(ScopedName var) { private StaticTypedSlot<JSType> getSlot(ScopedName var) {
Expand All @@ -174,7 +185,7 @@ private StaticTypedSlot<JSType> getSlot(ScopedName var) {
} }
} }
LinkedFlowSlot slot = cache.symbols.get(var); LinkedFlowSlot slot = cache.symbols.get(var);
return slot != null ? slot : getFunctionScope().getSlot(var.getName()); return slot != null ? slot : syntacticScope.getSlot(var.getName());
} }


private static String getRootOfQualifiedName(String name) { private static String getRootOfQualifiedName(String name) {
Expand All @@ -187,14 +198,14 @@ private static String getRootOfQualifiedName(String name) {
// for qualified names, though some unit tests fail to declare simple names as // for qualified names, though some unit tests fail to declare simple names as
// well), a simple ScopedName will be created, using the scope of the qualified // well), a simple ScopedName will be created, using the scope of the qualified
// name's root, but not registered on the scope. // name's root, but not registered on the scope.
private ScopedName getVarFromFunctionScope(String name) { private ScopedName getVarFromSyntacticScope(String name) {
TypedVar v = getFunctionScope().getVar(name); TypedVar v = syntacticScope.getVar(name);
if (v != null) { if (v != null) {
return v; return v;
} }
TypedVar rootVar = getFunctionScope().getVar(getRootOfQualifiedName(name)); TypedVar rootVar = syntacticScope.getVar(getRootOfQualifiedName(name));
TypedScope rootScope = rootVar != null ? rootVar.getScope() : null; TypedScope rootScope = rootVar != null ? rootVar.getScope() : null;
rootScope = rootScope != null ? rootScope : getFunctionScope().getGlobalScope(); rootScope = rootScope != null ? rootScope : cache.functionScope;
return ScopedName.of(name, rootScope.getRootNode()); return ScopedName.of(name, rootScope.getRootNode());
} }


Expand All @@ -205,16 +216,22 @@ public StaticTypedSlot<JSType> getOwnSlot(String name) {


@Override @Override
public FlowScope createChildFlowScope() { public FlowScope createChildFlowScope() {
return createChildFlowScope(syntacticScope);
}

@Override
public FlowScope createChildFlowScope(StaticTypedScope<JSType> scope) {
frozen = true; frozen = true;


TypedScope typedScope = (TypedScope) scope;
if (depth > MAX_DEPTH) { if (depth > MAX_DEPTH) {
if (flattened == null) { if (flattened == null) {
flattened = new FlatFlowScopeCache(this); flattened = new FlatFlowScopeCache(this);
} }
return new LinkedFlowScope(flattened); return new LinkedFlowScope(flattened, typedScope);
} }


return new LinkedFlowScope(this); return new LinkedFlowScope(this, typedScope);
} }


/** /**
Expand Down Expand Up @@ -257,10 +274,21 @@ public StaticTypedSlot<JSType> findUniqueRefinedSlot(FlowScope blindScope) {
// of short circuiting AND and OR operators). // of short circuiting AND and OR operators).
@Override @Override
public LinkedFlowScope optimize() { public LinkedFlowScope optimize() {
LinkedFlowScope current; LinkedFlowScope current = this;
for (current = this; // NOTE(sdh): This function does not take syntacticScope into account.
current.parent != null && current.lastSlot == current.parent.lastSlot; // This means that an optimized scope cannot be used to look up names
current = current.parent) {} // by string without first creating a child in the correct block. This
// is not a problem, since this is only used for (a) determining whether
// to join two scopes, (b) determining whether two scopes are equal, or
// (c) optimizing away unnecessary children generated by flowing through
// an expression. In (a) and (b) the result is only inspected locally and
// not escaped. In (c) the result is fed directly into further joins and
// will always have a block scope reassigned before flowing into another
// node. In all cases, it's therefore safe to ignore block scope changes
// when optimizing.
while (current.parent != null && current.lastSlot == current.parent.lastSlot) {
current = current.parent;
}
return current; return current;
} }


Expand All @@ -281,7 +309,7 @@ private boolean optimizesToSameScope(LinkedFlowScope that) {


@Override @Override
public TypedScope getDeclarationScope() { public TypedScope getDeclarationScope() {
return this.getFunctionScope(); return this.syntacticScope;
} }


/** Join the two FlowScopes. */ /** Join the two FlowScopes. */
Expand All @@ -299,15 +327,41 @@ public FlowScope apply(FlowScope a, FlowScope b) {
} }
// TODO(sdh): Consider reusing the input cache if both inputs are identical. // TODO(sdh): Consider reusing the input cache if both inputs are identical.
// We can evaluate how often this happens to see whather this would be a win. // We can evaluate how often this happens to see whather this would be a win.
return new LinkedFlowScope(new FlatFlowScopeCache(linkedA, linkedB)); FlatFlowScopeCache cache = new FlatFlowScopeCache(linkedA, linkedB);

// NOTE: it would be nice to put 'null' as the syntactic scope if they're not
// equal, but this is not currently feasible. For joins that occur within a
// single CFG node's flow, it's irrelevant, but for joins between separate
// CFG nodes, there is *one* place where the syntactic scope is actually used:
// when joining more than two scopes, the first two scopes are joined, and
// then the join result is joined with the third. When joining, we look up
// the types (and existence) of vars in one scope in the other; so when a var
// from the third scope (say, a local) is missing from the join result, it
// looks through the syntactic scope before realizing this. A quick fix
// might be to just check that the scope is non-null before trying to join;
// a better long-term fix would be to improve how we do joins to avoid
// excessive map entry creation: find a common ancestor, etc. One
// interesting consequence of the current approach is that we may end up
// adding irrelevant block-local variables to the joined scope unnecessarily.
return new LinkedFlowScope(cache, getCommonParentDeclarationScope(linkedA, linkedB));
}
}

static TypedScope getCommonParentDeclarationScope(LinkedFlowScope left, LinkedFlowScope right) {
if (left.flowsFromBottom()) {
return right.syntacticScope;
} else if (right.flowsFromBottom()) {
return left.syntacticScope;
} }
return left.syntacticScope.getCommonParent(right.syntacticScope);
} }


@Override @Override
public boolean equals(Object other) { public boolean equals(Object other) {
if (!(other instanceof LinkedFlowScope)) { if (!(other instanceof LinkedFlowScope)) {
return false; return false;
} }

LinkedFlowScope that = (LinkedFlowScope) other; LinkedFlowScope that = (LinkedFlowScope) other;
if (this.optimizesToSameScope(that)) { if (this.optimizesToSameScope(that)) {
return true; return true;
Expand All @@ -321,7 +375,7 @@ public boolean equals(Object other) {
// they're equal--this just means that data flow analysis will have // they're equal--this just means that data flow analysis will have
// to propagate the entry lattice a little bit further than it // to propagate the entry lattice a little bit further than it
// really needs to. Everything will still come out ok. // really needs to. Everything will still come out ok.
if (this.getFunctionScope() != that.getFunctionScope()) { if (this.cache.functionScope != that.cache.functionScope) {
return false; return false;
} }


Expand Down Expand Up @@ -356,10 +410,8 @@ public boolean equals(Object other) {
private static boolean diffSlots(StaticTypedSlot<JSType> slotA, StaticTypedSlot<JSType> slotB) { private static boolean diffSlots(StaticTypedSlot<JSType> slotA, StaticTypedSlot<JSType> slotB) {
boolean aIsNull = slotA == null || slotA.getType() == null; boolean aIsNull = slotA == null || slotA.getType() == null;
boolean bIsNull = slotB == null || slotB.getType() == null; boolean bIsNull = slotB == null || slotB.getType() == null;
if (aIsNull && bIsNull) { if (aIsNull || bIsNull) {
return false; return aIsNull != bIsNull;
} else if (aIsNull ^ bIsNull) {
return true;
} }


// Both slots and types must be non-null. // Both slots and types must be non-null.
Expand Down Expand Up @@ -493,8 +545,10 @@ private static class FlatFlowScopeCache {


// Always prefer the "real" function scope to the faked-out // Always prefer the "real" function scope to the faked-out
// bottom scope. // bottom scope.
this.functionScope = joinedScopeA.flowsFromBottom() this.functionScope =
? joinedScopeB.getFunctionScope() : joinedScopeA.getFunctionScope(); joinedScopeA.flowsFromBottom()
? joinedScopeB.cache.functionScope
: joinedScopeA.cache.functionScope;


Map<ScopedName, LinkedFlowSlot> slotsA = joinedScopeA.allFlowSlots(); Map<ScopedName, LinkedFlowSlot> slotsA = joinedScopeA.allFlowSlots();
Map<ScopedName, LinkedFlowSlot> slotsB = joinedScopeB.allFlowSlots(); Map<ScopedName, LinkedFlowSlot> slotsB = joinedScopeB.allFlowSlots();
Expand All @@ -518,7 +572,7 @@ private static class FlatFlowScopeCache {
LinkedFlowSlot slotB = slotsB.get(var); LinkedFlowSlot slotB = slotsB.get(var);
JSType joinedType = null; JSType joinedType = null;
if (slotB == null || slotB.getType() == null) { if (slotB == null || slotB.getType() == null) {
TypedVar fnSlot = joinedScopeB.getFunctionScope().getVar(var.getName()); TypedVar fnSlot = joinedScopeB.syntacticScope.getSlot(var.getName());
JSType fnSlotType = fnSlot == null ? null : fnSlot.getType(); JSType fnSlotType = fnSlot == null ? null : fnSlot.getType();
if (fnSlotType == null) { if (fnSlotType == null) {
// Case #1 -- already inserted. // Case #1 -- already inserted.
Expand All @@ -527,7 +581,7 @@ private static class FlatFlowScopeCache {
joinedType = slotA.getType().getLeastSupertype(fnSlotType); joinedType = slotA.getType().getLeastSupertype(fnSlotType);
} }
} else if (slotA == null || slotA.getType() == null) { } else if (slotA == null || slotA.getType() == null) {
TypedVar fnSlot = joinedScopeA.getFunctionScope().getVar(var.getName()); TypedVar fnSlot = joinedScopeA.syntacticScope.getSlot(var.getName());
JSType fnSlotType = fnSlot == null ? null : fnSlot.getType(); JSType fnSlotType = fnSlot == null ? null : fnSlot.getType();
if (fnSlotType == null || fnSlotType == slotB.getType()) { if (fnSlotType == null || fnSlotType == slotB.getType()) {
// Case #2 // Case #2
Expand All @@ -541,7 +595,7 @@ private static class FlatFlowScopeCache {
joinedType = slotA.getType().getLeastSupertype(slotB.getType()); joinedType = slotA.getType().getLeastSupertype(slotB.getType());
} }


if (joinedType != null) { if (joinedType != null && (slotA == null || joinedType != slotA.getType())) {
symbols.put(var, new LinkedFlowSlot(var, joinedType, null)); symbols.put(var, new LinkedFlowSlot(var, joinedType, null));
} }
} }
Expand Down

0 comments on commit 5a68bc0

Please sign in to comment.