diff --git a/src/com/google/javascript/jscomp/AbstractScope.java b/src/com/google/javascript/jscomp/AbstractScope.java index b9b11df1bca..55e220bcdc2 100644 --- a/src/com/google/javascript/jscomp/AbstractScope.java +++ b/src/com/google/javascript/jscomp/AbstractScope.java @@ -34,15 +34,16 @@ * scope. * *

ES 2015 introduces new scoping rules, which adds some complexity to this class. In particular, - * scopes fall into two mutually exclusive categories: block and container. Block + * scopes fall into two mutually exclusive categories: block and container. Block * 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 - * comprise function scopes, global scopes, and module scopes, and (aside from modules, which - * didn't exist in ES5) corresponds to the ES5 scope rules. This corresponds roughly to one - * container scope per CFG root (but not exactly, due to SCRIPT-level CFGs). + * blocks are omitted. These scopes did not exist at all prior to ES 2015. Container scopes comprise + * function scopes, global scopes, and module scopes, and (aside from modules, which didn't exist in + * ES5) corresponds to the ES5 scope rules. This corresponds roughly to one container scope per CFG + * root (but not exactly, due to SCRIPT-level CFGs). * *

All container scopes, plus the outermost block scope within a function (i.e. the function - * block scope) are considered hoist scopes. Hoist scopes are relevant because "var" + * block scope) are considered hoist scopes. 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 * apply to the specific scope in which they occur. * @@ -380,10 +381,29 @@ void checkRootScope() { 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 - * the same function, or else both in the global hoist scope). This is equivalent to asking - * whether the two scopes would be equivalent in a pre-ES2015-block-scopes view of the world. + * Whether {@code this} and the {@code other} scope have the same container scope (i.e. are in the + * same function, or else both in the global hoist scope). This is equivalent to asking whether + * the two scopes would be equivalent in a pre-ES2015-block-scopes view of the world. */ boolean hasSameContainerScope(S other) { return getClosestContainerScope() == other.getClosestContainerScope(); diff --git a/src/com/google/javascript/jscomp/LinkedFlowScope.java b/src/com/google/javascript/jscomp/LinkedFlowScope.java index 0813fd82205..3974940c811 100644 --- a/src/com/google/javascript/jscomp/LinkedFlowScope.java +++ b/src/com/google/javascript/jscomp/LinkedFlowScope.java @@ -39,12 +39,16 @@ * @author nicksantos@google.com (Nick Santos) */ class LinkedFlowScope implements FlowScope { + // The closest flow scope cache. private final FlatFlowScopeCache cache; // The parent flow scope. 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. private int depth; @@ -62,48 +66,43 @@ class LinkedFlowScope implements FlowScope { 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 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.lastSlot = null; this.depth = 0; this.parent = cache.linkedEquivalent; + this.syntacticScope = syntacticScope; } - /** - * Creates a child flow scope with a single parent. - */ - private LinkedFlowScope(LinkedFlowScope directParent) { + /** Creates a child flow scope with a single parent. */ + private LinkedFlowScope(LinkedFlowScope directParent, TypedScope syntacticScope) { this.cache = directParent.cache; this.lastSlot = directParent.lastSlot; this.depth = directParent.depth + 1; this.parent = directParent; - } - - /** Gets the function scope for this flow scope. */ - private TypedScope getFunctionScope() { - return cache.functionScope; + this.syntacticScope = syntacticScope; } /** Whether this flows from a bottom scope. */ private boolean flowsFromBottom() { - return getFunctionScope().isBottom(); + return cache.functionScope.isBottom(); } /** * Creates an entry lattice for the flow. */ public static LinkedFlowScope createEntryLattice(TypedScope scope) { - return new LinkedFlowScope(new FlatFlowScopeCache(scope)); + return new LinkedFlowScope(new FlatFlowScopeCache(scope), scope); } @Override public void inferSlotType(String symbol, JSType type) { checkState(!frozen); - ScopedName var = getVarFromFunctionScope(symbol); + ScopedName var = getVarFromSyntacticScope(symbol); lastSlot = new LinkedFlowSlot(var, type, lastSlot); depth++; cache.dirtySymbols.add(var); @@ -112,14 +111,26 @@ public void inferSlotType(String symbol, JSType type) { @Override public void inferQualifiedSlot(Node node, String symbol, JSType bottomType, JSType inferredType, boolean declared) { - TypedScope functionScope = getFunctionScope(); - if (functionScope.isGlobal()) { + if (cache.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; } - - TypedVar v = functionScope.getVar(symbol); - if (v == null && !functionScope.isBottom()) { - v = functionScope.declare(symbol, node, bottomType, null, !declared); + TypedVar v = syntacticScope.getVar(symbol); + if (v == null && !cache.functionScope.isBottom()) { + // NOTE(sdh): Qualified names are declared on scopes lazily via this method. + // 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; @@ -149,7 +160,7 @@ public JSType getTypeOfThis() { @Override public Node getRootNode() { - return getFunctionScope().getRootNode(); + return syntacticScope.getRootNode(); } @Override @@ -162,7 +173,7 @@ public StaticTypedScope getParentScope() { */ @Override public StaticTypedSlot getSlot(String name) { - return getSlot(getVarFromFunctionScope(name)); + return getSlot(getVarFromSyntacticScope(name)); } private StaticTypedSlot getSlot(ScopedName var) { @@ -174,7 +185,7 @@ private StaticTypedSlot getSlot(ScopedName 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) { @@ -187,14 +198,14 @@ private static String getRootOfQualifiedName(String name) { // 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 // name's root, but not registered on the scope. - private ScopedName getVarFromFunctionScope(String name) { - TypedVar v = getFunctionScope().getVar(name); + private ScopedName getVarFromSyntacticScope(String name) { + TypedVar v = syntacticScope.getVar(name); if (v != null) { return v; } - TypedVar rootVar = getFunctionScope().getVar(getRootOfQualifiedName(name)); + TypedVar rootVar = syntacticScope.getVar(getRootOfQualifiedName(name)); 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()); } @@ -205,16 +216,22 @@ public StaticTypedSlot getOwnSlot(String name) { @Override public FlowScope createChildFlowScope() { + return createChildFlowScope(syntacticScope); + } + + @Override + public FlowScope createChildFlowScope(StaticTypedScope scope) { frozen = true; + TypedScope typedScope = (TypedScope) scope; if (depth > MAX_DEPTH) { if (flattened == null) { flattened = new FlatFlowScopeCache(this); } - return new LinkedFlowScope(flattened); + return new LinkedFlowScope(flattened, typedScope); } - return new LinkedFlowScope(this); + return new LinkedFlowScope(this, typedScope); } /** @@ -257,10 +274,21 @@ public StaticTypedSlot findUniqueRefinedSlot(FlowScope blindScope) { // of short circuiting AND and OR operators). @Override public LinkedFlowScope optimize() { - LinkedFlowScope current; - for (current = this; - current.parent != null && current.lastSlot == current.parent.lastSlot; - current = current.parent) {} + LinkedFlowScope current = this; + // NOTE(sdh): This function does not take syntacticScope into account. + // This means that an optimized scope cannot be used to look up names + // 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; } @@ -281,7 +309,7 @@ private boolean optimizesToSameScope(LinkedFlowScope that) { @Override public TypedScope getDeclarationScope() { - return this.getFunctionScope(); + return this.syntacticScope; } /** Join the two FlowScopes. */ @@ -299,8 +327,33 @@ public FlowScope apply(FlowScope a, FlowScope b) { } // 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. - 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 @@ -308,6 +361,7 @@ public boolean equals(Object other) { if (!(other instanceof LinkedFlowScope)) { return false; } + LinkedFlowScope that = (LinkedFlowScope) other; if (this.optimizesToSameScope(that)) { return true; @@ -321,7 +375,7 @@ public boolean equals(Object other) { // they're equal--this just means that data flow analysis will have // to propagate the entry lattice a little bit further than it // really needs to. Everything will still come out ok. - if (this.getFunctionScope() != that.getFunctionScope()) { + if (this.cache.functionScope != that.cache.functionScope) { return false; } @@ -356,10 +410,8 @@ public boolean equals(Object other) { private static boolean diffSlots(StaticTypedSlot slotA, StaticTypedSlot slotB) { boolean aIsNull = slotA == null || slotA.getType() == null; boolean bIsNull = slotB == null || slotB.getType() == null; - if (aIsNull && bIsNull) { - return false; - } else if (aIsNull ^ bIsNull) { - return true; + if (aIsNull || bIsNull) { + return aIsNull != bIsNull; } // Both slots and types must be non-null. @@ -493,8 +545,10 @@ private static class FlatFlowScopeCache { // Always prefer the "real" function scope to the faked-out // bottom scope. - this.functionScope = joinedScopeA.flowsFromBottom() - ? joinedScopeB.getFunctionScope() : joinedScopeA.getFunctionScope(); + this.functionScope = + joinedScopeA.flowsFromBottom() + ? joinedScopeB.cache.functionScope + : joinedScopeA.cache.functionScope; Map slotsA = joinedScopeA.allFlowSlots(); Map slotsB = joinedScopeB.allFlowSlots(); @@ -518,7 +572,7 @@ private static class FlatFlowScopeCache { LinkedFlowSlot slotB = slotsB.get(var); JSType joinedType = 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(); if (fnSlotType == null) { // Case #1 -- already inserted. @@ -527,7 +581,7 @@ private static class FlatFlowScopeCache { joinedType = slotA.getType().getLeastSupertype(fnSlotType); } } 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(); if (fnSlotType == null || fnSlotType == slotB.getType()) { // Case #2 @@ -541,7 +595,7 @@ private static class FlatFlowScopeCache { 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)); } } diff --git a/src/com/google/javascript/jscomp/TypeInference.java b/src/com/google/javascript/jscomp/TypeInference.java index 8c5476b5761..97515acac12 100644 --- a/src/com/google/javascript/jscomp/TypeInference.java +++ b/src/com/google/javascript/jscomp/TypeInference.java @@ -57,6 +57,7 @@ import com.google.javascript.rhino.jstype.UnionType; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; @@ -87,6 +88,9 @@ class TypeInference private final TypedScopeCreator scopeCreator; private final Map assertionFunctionsMap; + // Scopes that have had their unbound untyped vars inferred as undefined. + private final Set inferredUnboundVars = new HashSet<>(); + // For convenience private final ObjectType unknownType; @@ -107,19 +111,27 @@ class TypeInference this.scopeCreator = scopeCreator; this.assertionFunctionsMap = assertionFunctionsMap; + inferDeclarativelyUnboundVarsWithoutTypes(functionScope); + + this.bottomScope = + LinkedFlowScope.createEntryLattice( + TypedScope.createLatticeBottom(syntacticScope.getRootNode())); + } + + private void inferDeclarativelyUnboundVarsWithoutTypes(FlowScope flow) { + TypedScope scope = (TypedScope) flow.getDeclarationScope(); + if (!inferredUnboundVars.add(scope)) { + return; + } // For each local variable declared with the VAR keyword, the entry // type is VOID. - for (TypedVar var : syntacticScope.getDeclarativelyUnboundVarsWithoutTypes()) { + for (TypedVar var : scope.getDeclarativelyUnboundVarsWithoutTypes()) { if (isUnflowable(var)) { continue; } - this.functionScope.inferSlotType( - var.getName(), getNativeType(VOID_TYPE)); + flow.inferSlotType(var.getName(), getNativeType(VOID_TYPE)); } - - this.bottomScope = LinkedFlowScope.createEntryLattice( - TypedScope.createLatticeBottom(functionScope.getRootNode())); } /** @@ -189,7 +201,9 @@ FlowScope flowThrough(Node n, FlowScope input) { } // TODO(sdh): Change to NodeUtil.getEnclosingScopeRoot(n) once we have block scopes. - FlowScope output = input.createChildFlowScope(); + Node root = NodeUtil.getEnclosingNode(n, TypeInference::createsContainerScope); + FlowScope output = input.createChildFlowScope(scopeCreator.createScope(root)); + inferDeclarativelyUnboundVarsWithoutTypes(output); output = traverse(n, output); return output; } @@ -716,9 +730,7 @@ private void updateScopeForTypeChange( } } - /** - * Defines a property if the property has not been defined yet. - */ + /** Defines a property if the property has not been defined yet. */ private void ensurePropertyDefined(Node getprop, JSType rightType, FlowScope scope) { String propName = getprop.getLastChild().getString(); Node obj = getprop.getFirstChild(); @@ -789,6 +801,7 @@ private void ensurePropertyDefined(Node getprop, JSType rightType, FlowScope sco /** * Declares a property on its owner, if necessary. + * * @return True if a property was declared. */ private boolean ensurePropertyDeclaredHelper( @@ -1073,29 +1086,24 @@ private FlowScope narrowScope(FlowScope scope, Node node, JSType narrowed) { } /** - * We only do forward type inference. We do not do full backwards - * type inference. + * We only do forward type inference. We do not do full backwards type inference. * - * In other words, if we have, - * + *

In other words, if we have, * var x = f(); * g(x); - * - * a forward type-inference engine would try to figure out the type - * of "x" from the return type of "f". A backwards type-inference engine - * would try to figure out the type of "x" from the parameter type of "g". + * a forward type-inference engine would try to figure out the type of "x" from the return + * type of "f". A backwards type-inference engine would try to figure out the type of "x" from the + * parameter type of "g". * - * However, there are a few special syntactic forms where we do some - * some half-assed backwards type-inference, because programmers - * expect it in this day and age. To take an example from Java, - * + *

However, there are a few special syntactic forms where we do some some half-assed backwards + * type-inference, because programmers expect it in this day and age. To take an example from + * Java, * List x = Lists.newArrayList(); - * - * The Java compiler will be able to infer the generic type of the List - * returned by newArrayList(). + * The Java compiler will be able to infer the generic type of the List returned by + * newArrayList(). * - * In much the same way, we do some special-case backwards inference for - * JS. Those cases are enumerated here. + *

In much the same way, we do some special-case backwards inference for JS. Those cases are + * enumerated here. */ private void backwardsInferenceFromCallSite(Node n, FunctionType fnType, FlowScope scope) { boolean updatedFnType = inferTemplatedTypesForCall(n, fnType, scope); @@ -1417,10 +1425,7 @@ private Map buildTypeVariables( return typeVars; } - /** - * This function will evaluate the type transformations associated to the - * template types - */ + /** This function will evaluate the type transformations associated to the template types */ private Map evaluateTypeTransformations( ImmutableList templateTypes, Map inferredTypes, @@ -1453,10 +1458,9 @@ private Map evaluateTypeTransformations( } /** - * For functions that use template types, specialize the function type for - * the call target based on the call-site specific arguments. - * Specifically, this enables inference to set the type of any function - * literal parameters based on these inferred types. + * For functions that use template types, specialize the function type for the call target based + * on the call-site specific arguments. Specifically, this enables inference to set the type of + * any function literal parameters based on these inferred types. */ private boolean inferTemplatedTypesForCall(Node n, FunctionType fnType, FlowScope scope) { ImmutableList keys = fnType.getTemplateTypeMap().getTemplateKeys(); diff --git a/src/com/google/javascript/jscomp/VariableReferenceCheck.java b/src/com/google/javascript/jscomp/VariableReferenceCheck.java index fb991ae5b76..501f970ff34 100644 --- a/src/com/google/javascript/jscomp/VariableReferenceCheck.java +++ b/src/com/google/javascript/jscomp/VariableReferenceCheck.java @@ -182,11 +182,9 @@ private void checkDefaultParam( compiler, param.getParentNode().getSecondChild(), /** - * Do a shallow check since cases like: {@code - * function f(y = () => x, x = 5) { return y(); } - * } is legal. We are going to miss cases like: {@code - * function f(y = (() => x)(), x = 5) { return y(); } - * } but this should be rare. + * Do a shallow check since cases like: {@code function f(y = () => x, x = 5) { return + * y(); } } is legal. We are going to miss cases like: {@code function f(y = (() => x)(), + * x = 5) { return y(); } } but this should be rare. */ new AbstractShallowCallback() { @Override diff --git a/src/com/google/javascript/jscomp/type/FlowScope.java b/src/com/google/javascript/jscomp/type/FlowScope.java index d6ca0576c96..34b8806f653 100644 --- a/src/com/google/javascript/jscomp/type/FlowScope.java +++ b/src/com/google/javascript/jscomp/type/FlowScope.java @@ -38,6 +38,12 @@ public interface FlowScope extends StaticTypedScope, LatticeElement { */ FlowScope createChildFlowScope(); + /** + * Creates a child flow scope with the given syntactic scope, which may be required to be a + * specific subclass, such as TypedScope. + */ + FlowScope createChildFlowScope(StaticTypedScope scope); + /** * Defines the type of a symbol at this point in the flow. * @throws IllegalArgumentException If no slot for this symbol exists.