From 90e2102c3c40ebb7910260040a95bde06b798f52 Mon Sep 17 00:00:00 2001 From: sdh Date: Thu, 5 Apr 2018 21:04:13 -0700 Subject: [PATCH] Finish block scope support in TypedScopeCreator. Now that all the groundwork has been laid, this change flips the scope creator to actually operate in block-scope mode, and updates the last few little bits that are required to support that. Note that the type checker still does not understand let/const at all, but at least it now has distinct scopes it can hang the different variables on. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=191841882 --- .../javascript/jscomp/AbstractScope.java | 31 ++++++- .../javascript/jscomp/NodeTraversal.java | 23 ++++-- .../google/javascript/jscomp/SymbolTable.java | 2 +- .../javascript/jscomp/TypeInference.java | 3 +- .../google/javascript/jscomp/TypedScope.java | 24 ++++-- .../javascript/jscomp/TypedScopeCreator.java | 81 ++++++++++++------- .../javascript/jscomp/TypeCheckTest.java | 24 +++++- .../javascript/jscomp/TypeInferenceTest.java | 15 +++- .../jscomp/TypedScopeCreatorTest.java | 55 ++++++------- 9 files changed, 171 insertions(+), 87 deletions(-) diff --git a/src/com/google/javascript/jscomp/AbstractScope.java b/src/com/google/javascript/jscomp/AbstractScope.java index 55e220bcdc2..712e28e11b5 100644 --- a/src/com/google/javascript/jscomp/AbstractScope.java +++ b/src/com/google/javascript/jscomp/AbstractScope.java @@ -65,7 +65,7 @@ abstract class AbstractScope, V extends AbstractVa private final Node rootNode; AbstractScope(Node rootNode) { - this.rootNode = rootNode; + this.rootNode = checkNotNull(rootNode); } /** The depth of the scope. The global scope has depth 0. */ @@ -143,6 +143,7 @@ final void undeclareInteral(V var) { } final void declareInternal(String name, V var) { + checkState(hasOwnSlot(name) || canDeclare(name), "Illegal shadow: %s", var.getNode()); vars.put(name, var); } @@ -229,6 +230,34 @@ public final boolean hasSlot(String name) { return false; } + /** + * Returns true if the name can be declared on this scope without causing illegal shadowing. + * Specifically, this is aware of the connection between function container scopes and function + * block scopes and returns false for redeclaring parameters on the block scope. + */ + final boolean canDeclare(String name) { + return !hasOwnSlot(name) + && (!isFunctionBlockScope() + || !getParent().hasOwnSlot(name) + || isBleedingFunctionName(name)); + } + + /** + * Returns true if the given name is a bleeding function name in this scope. Local variables in + * the function block are not allowed to shadow parameters, but they are allowed to shadow a + * bleeding function name. + */ + private boolean isBleedingFunctionName(String name) { + V var = getVar(name); + if (var == null) { + return false; + } + Node n = var.getScopeRoot(); + return n.isFunction() + && NodeUtil.isBleedingFunctionName(n.getFirstChild()) + && name.equals(n.getFirstChild().getString()); + } + /** * Return an iterable over all of the variables declared in this scope * (except the special 'arguments' variable). diff --git a/src/com/google/javascript/jscomp/NodeTraversal.java b/src/com/google/javascript/jscomp/NodeTraversal.java index 8348207af28..4132c6caca4 100644 --- a/src/com/google/javascript/jscomp/NodeTraversal.java +++ b/src/com/google/javascript/jscomp/NodeTraversal.java @@ -939,6 +939,16 @@ private void popScope(boolean quietly) { return scope; } + /** + * Instantiate some, but not necessarily all, scopes from stored roots. + * + *

NodeTraversal instantiates scopes lazily when getScope() or similar is called, by iterating + * over a stored list of not-yet-instantiated scopeRoots. When a not-yet-instantiated parent + * scope is requested, it doesn't make sense to instantiate all pending scopes. Instead, + * we count the number that are needed to ensure the requested parent is instantiated and call + * this function to instantiate only as many scopes as are needed, shifting their roots off the + * queue, and returning the deepest scope actually created. + */ private AbstractScope instantiateScopes(int count) { checkArgument(count <= scopeRoots.size()); AbstractScope scope = scopes.peek(); @@ -967,16 +977,13 @@ public Node getClosestHoistScopeRoot() { return scopes.peek().getClosestHoistScope().getRootNode(); } - public Node getClosestContainerScopeRoot() { - int roots = scopeRoots.size(); - for (int i = roots; i > 0; i--) { - Node rootNode = scopeRoots.get(i - 1); - if (!NodeUtil.createsBlockScope(rootNode)) { - return rootNode; + public AbstractScope getClosestContainerScope() { + for (int i = scopeRoots.size(); i > 0; i--) { + if (!NodeUtil.createsBlockScope(scopeRoots.get(i - 1))) { + return instantiateScopes(i); } } - - return scopes.peek().getClosestContainerScope().getRootNode(); + return scopes.peek().getClosestContainerScope(); } public AbstractScope getClosestHoistScope() { diff --git a/src/com/google/javascript/jscomp/SymbolTable.java b/src/com/google/javascript/jscomp/SymbolTable.java index 7ec31d846bc..ad4677c674e 100644 --- a/src/com/google/javascript/jscomp/SymbolTable.java +++ b/src/com/google/javascript/jscomp/SymbolTable.java @@ -1213,7 +1213,7 @@ private SymbolScope createScopeFrom(StaticScope otherScope) { // NOTE: Kythe is not set up to handle block scopes yet, so only create // SymbolScopes for container scope roots, giving a pre-ES6 view of the world. - while (!NodeUtil.isValidCfgRoot(otherScopeRoot)) { + while (NodeUtil.createsBlockScope(otherScopeRoot)) { otherScopeRoot = otherScopeRoot.getParent(); } diff --git a/src/com/google/javascript/jscomp/TypeInference.java b/src/com/google/javascript/jscomp/TypeInference.java index 086aa975646..06049d9e575 100644 --- a/src/com/google/javascript/jscomp/TypeInference.java +++ b/src/com/google/javascript/jscomp/TypeInference.java @@ -200,8 +200,7 @@ FlowScope flowThrough(Node n, FlowScope input) { return input; } - // TODO(sdh): Change to NodeUtil.getEnclosingScopeRoot(n) once we have block scopes. - Node root = NodeUtil.getEnclosingNode(n, TypeInference::createsContainerScope); + Node root = NodeUtil.getEnclosingScopeRoot(n); FlowScope output = input.createChildFlowScope(scopeCreator.createScope(root)); inferDeclarativelyUnboundVarsWithoutTypes(output); output = traverse(n, output); diff --git a/src/com/google/javascript/jscomp/TypedScope.java b/src/com/google/javascript/jscomp/TypedScope.java index b7517e9693c..0a76cb645a8 100644 --- a/src/com/google/javascript/jscomp/TypedScope.java +++ b/src/com/google/javascript/jscomp/TypedScope.java @@ -163,15 +163,23 @@ private JSType getImplicitVarType(ImplicitVar var) { return getTypeOfThis(); } + /** + * Returns the variables in this scope that have been declared with 'var' (or 'let') and not + * declared with a known type. These variables can safely be set to undefined (rather than + * unknown) at the start of type inference, and will be reset to the correct type when analyzing + * the first assignment to them. Parameters and externs are excluded because they are not + * initialized in the function body. + */ public Iterable getDeclarativelyUnboundVarsWithoutTypes() { - return Iterables.filter( - getVarIterable(), - var -> - // declaratively unbound vars without types - var.getParentNode() != null - && var.getType() == null - && var.getParentNode().isVar() - && !var.isExtern()); + return Iterables.filter(getVarIterable(), this::isDeclarativelyUnboundVarWithoutType); + } + + private boolean isDeclarativelyUnboundVarWithoutType(TypedVar var) { + return var.getParentNode() != null + && var.getType() == null + // TODO(sdh): should we include LET here as well? + && var.getParentNode().isVar() + && !var.isExtern(); } static interface TypeResolver { diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java index 5c876d319f9..d6eb6942ce8 100644 --- a/src/com/google/javascript/jscomp/TypedScopeCreator.java +++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java @@ -257,19 +257,12 @@ void removeScopesForScript(String scriptName) { memoized.keySet().removeIf(n -> scriptName.equals(NodeUtil.getSourceName(n))); } - /** Create a scope, looking up in the map for the parent scope. */ + /** Create a scope if it doesn't already exist, looking up in the map for the parent scope. */ TypedScope createScope(Node n) { - // NOTE(sdh): Ideally we could just look up the node in the map, - // but sometimes TypedScopeCreator does not create the scope in - // the first place (particularly for empty blocks). When these - // cases show up in the CFG, we need to do some extra legwork to - // ensure the scope exists. - // TODO(sdh): Use NodeUtil.getEnclosingScopeRoot(n.getParent()) once we're block-scoped. TypedScope s = memoized.get(n); return s != null ? s - : createScope( - n, createScope(NodeUtil.getEnclosingNode(n.getParent(), NodeUtil::isValidCfgRoot))); + : createScope(n, createScope(NodeUtil.getEnclosingScopeRoot(n.getParent()))); } /** @@ -575,18 +568,24 @@ public void resolveTypes() { @Override public final boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { inputId = t.getInputId(); - if (n.isFunction() || n.isScript()) { + if (n.isFunction() || n.isScript() || (n == currentScope.getRootNode() && inputId != null)) { checkNotNull(inputId); sourceName = NodeUtil.getSourceName(n); } - // We do want to traverse the name of a named function, but we don't - // want to traverse the arguments or body. + // The choice of whether to descend depends on the type of scope. Function scopes + // should traverse the outer FUNCTION node, and the NAME and PARAM_LIST nodes, but + // should not descend into the body. Non-function nodes need to at least look at + // every child node (parent == currentScope.root), but should not descend *into* + // any nodes that create scopes. The only exception to this is that we do want + // to inspect the first (NAME) child of a named function. boolean descend = - parent == null - || !parent.isFunction() - || n == parent.getFirstChild() - || parent == currentScope.getRootNode(); + currentScope.isFunctionScope() + ? !n.isNormalBlock() + : parent == null + || !NodeUtil.createsScope(parent) + || (parent.isFunction() && n == parent.getFirstChild()) + || parent == currentScope.getRootNode(); if (descend) { // Handle hoisted functions on pre-order traversal, so that they @@ -602,6 +601,24 @@ public final boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { } } + // Create any child block scopes "pre-order" as we see them. This is required because hoisted + // or qualified names defined in earlier blocks might be referred to later outside the block. + // This isn't a big deal in most cases since a NamedType will be created and resolved later, + // but if a NamedType is used for a superclass, we lose a lot of valuable checking. Recursing + // into child blocks immediately prevents this from being a problem. + if (NodeUtil.createsBlockScope(n) && n != currentScope.getRootNode()) { + // Only create immediately-nested scopes. We sometimes encounter nodes that create + // grandchild scopes, such as a BLOCK inside a FOR - in that case, don't create the + // scope here - it will be created while traversing its parent scope. + Node ancestor = parent; + while (ancestor != null && !NodeUtil.createsScope(ancestor)) { + ancestor = ancestor.getParent(); + } + if (ancestor != null && ancestor == currentScope.getRootNode()) { + createScope(n, currentScope); + } + } + return descend; } @@ -1265,9 +1282,8 @@ void defineSlot(Node n, Node parent, String variableName, // who declare "global" names in an anonymous namespace. TypedScope ownerScope = null; if (n.isGetProp()) { - ownerScope = getLValueRootScope(n); + ownerScope = scopeToDeclareIn; } else if (variableName.contains(".")) { - // TODO(sdh): can we pull this from 'n' instead of munging the string? TypedVar rootVar = currentScope.getVar(variableName.substring(0, variableName.indexOf('.'))); if (rootVar != null) { @@ -1305,7 +1321,7 @@ void defineSlot(Node n, Node parent, String variableName, // declared in closest scope? CompilerInput input = compiler.getInput(inputId); - if (scopeToDeclareIn.hasOwnSlot(variableName)) { + if (!scopeToDeclareIn.canDeclare(variableName)) { TypedVar oldVar = scopeToDeclareIn.getVar(variableName); newVar = validator.expectUndeclaredVariable( sourceName, input, n, parent, oldVar, variableName, type); @@ -2249,10 +2265,10 @@ void process(Node externs, Node root) { return; } - Node containerScopeRoot = t.getClosestContainerScopeRoot(); + Scope containerScope = (Scope) t.getClosestContainerScope(); if (n.isReturn() && n.getFirstChild() != null) { - functionsWithNonEmptyReturns.add(containerScopeRoot); + functionsWithNonEmptyReturns.add(containerScope.getRootNode()); } // Be careful of bleeding functions, which create variables @@ -2261,13 +2277,17 @@ void process(Node externs, Node root) { String name = n.getString(); Scope scope = t.getScope(); Var var = scope.getVar(name); + // TODO(sdh): consider checking hasSameHoistScope instead of container scope here and + // below. This will detect function(a) { a.foo = bar } as an escaped qualified name, + // which seems like the right thing to do (but could possibly break things?) + // Doing so will allow removing the warning on TypeCheckTest#testIssue1024b. if (var != null) { - Scope ownerScope = var.getScope().getClosestContainerScope(); + Scope ownerScope = var.getScope(); if (ownerScope.isLocal()) { - Node ownerScopeRoot = ownerScope.getRootNode(); - assignedVarNames.add(ScopedName.of(name, ownerScopeRoot)); - if (containerScopeRoot != ownerScopeRoot) { - escapedVarNames.add(ScopedName.of(name, ownerScopeRoot)); + ScopedName scopedName = ScopedName.of(name, ownerScope.getRootNode()); + assignedVarNames.add(scopedName); + if (!containerScope.hasSameContainerScope(ownerScope)) { + escapedVarNames.add(scopedName); } } } @@ -2276,10 +2296,9 @@ void process(Node externs, Node root) { Scope scope = t.getScope(); Var var = scope.getVar(name); if (var != null) { - Scope ownerScope = var.getScope().getClosestContainerScope(); - Node ownerScopeRoot = ownerScope.getRootNode(); - if (ownerScope.isLocal() && containerScopeRoot != ownerScopeRoot) { - escapedVarNames.add(ScopedName.of(n.getQualifiedName(), ownerScopeRoot)); + Scope ownerScope = var.getScope(); + if (ownerScope.isLocal() && !containerScope.hasSameContainerScope(ownerScope)) { + escapedVarNames.add(ScopedName.of(n.getQualifiedName(), ownerScope.getRootNode())); } } } @@ -2288,6 +2307,6 @@ void process(Node externs, Node root) { @Override public boolean hasBlockScope() { - return false; + return true; } } diff --git a/test/com/google/javascript/jscomp/TypeCheckTest.java b/test/com/google/javascript/jscomp/TypeCheckTest.java index c41a003e812..976ab64a621 100644 --- a/test/com/google/javascript/jscomp/TypeCheckTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckTest.java @@ -3146,6 +3146,28 @@ public void testDuplicateLocalVarDecl() { "required: number")); } + public void testDuplicateLocalVarDeclSuppressed() { + // We can't just leave this to VarCheck since otherwise if that warning is suppressed, we'll end + // up redeclaring it as undefined in the function block, which can cause spurious errors. + testTypes( + lines( + "/** @suppress {duplicate} */", + "function f(x) {", + " var x = x;", + " x.y = true;", + "}")); + } + + public void testShadowBleedingFunctionName() { + // This is allowed and creates a new binding inside the function shadowing the bled name. + testTypes( + lines( + "var f = function x() {", + " var x;", + " var /** undefined */ y = x;", + "};")); + } + public void testDuplicateInstanceMethod1() { // If there's no jsdoc on the methods, then we treat them like // any other inferred properties. @@ -19463,7 +19485,7 @@ public void testSuperclassDefinedInBlockOnNamespace() { "required: string")); } - public void testCovariantIThenable1() { + public void testCovariantIThenable1() { testTypes( lines( "/** @type {!IThenable} */ var x;", diff --git a/test/com/google/javascript/jscomp/TypeInferenceTest.java b/test/com/google/javascript/jscomp/TypeInferenceTest.java index 66e5b5738a6..4deeaab0e0b 100644 --- a/test/com/google/javascript/jscomp/TypeInferenceTest.java +++ b/test/com/google/javascript/jscomp/TypeInferenceTest.java @@ -37,6 +37,7 @@ import com.google.common.collect.ImmutableList; import com.google.javascript.jscomp.CodingConvention.AssertionFunctionSpec; import com.google.javascript.jscomp.DataFlowAnalysis.BranchedFlowState; +import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; import com.google.javascript.jscomp.type.FlowScope; import com.google.javascript.jscomp.type.ReverseAbstractInterpreter; import com.google.javascript.rhino.Node; @@ -116,8 +117,13 @@ private void parseAndRunTypeInference(String js) { Node n = root.getFirstFirstChild(); // Create the scope with the assumptions. TypedScopeCreator scopeCreator = new TypedScopeCreator(compiler); - TypedScope assumedScope = scopeCreator.createScope( - n, scopeCreator.createScope(root, null)); + new NodeTraversal(compiler, new AbstractPostOrderCallback() { + @Override + public void visit(NodeTraversal t, Node n, Node parent) { + t.getTypedScope(); + } + }, scopeCreator).traverse(root); + TypedScope assumedScope = scopeCreator.createScope(n); for (Map.Entry entry : assumptions.entrySet()) { assumedScope.declare(entry.getKey(), null, entry.getValue(), null, false); } @@ -134,7 +140,10 @@ private void parseAndRunTypeInference(String js) { // Get the scope of the implicit return. BranchedFlowState rtnState = cfg.getImplicitReturn().getAnnotation(); - returnScope = rtnState.getIn(); + // Reset the flow scope's syntactic scope to the function block, rather than the function node + // itself. This allows pulling out local vars from the function by name to verify their types. + returnScope = + rtnState.getIn().createChildFlowScope(scopeCreator.createScope(n.getLastChild())); } private JSType getType(String name) { diff --git a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java index bf87ad92f01..df06368872b 100644 --- a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java @@ -27,7 +27,6 @@ import com.google.common.base.Predicate; import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; -import com.google.javascript.jscomp.NodeTraversal.Callback; import com.google.javascript.rhino.InputId; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; @@ -51,6 +50,7 @@ public final class TypedScopeCreatorTest extends CompilerTestCase { private JSTypeRegistry registry; private TypedScope globalScope; private TypedScope lastLocalScope; + private TypedScope lastFunctionScope; @Override protected void setUp() throws Exception { @@ -63,14 +63,16 @@ protected int getNumRepetitions() { return 1; } - private final Callback callback = new AbstractPostOrderCallback() { + private class ScopeFinder extends AbstractPostOrderCallback { @Override public void visit(NodeTraversal t, Node n, Node parent) { TypedScope s = t.getTypedScope(); if (s.isGlobal()) { globalScope = s; - } else { + } else if (s.isBlockScope()) { lastLocalScope = s; + } else if (s.isFunctionScope()) { + lastFunctionScope = s; } } }; @@ -86,8 +88,7 @@ public void process(Node externs, Node root) { (new TypeInferencePass( compiler, compiler.getReverseAbstractInterpreter(), topScope, scopeCreator)).process(externs, root); - NodeTraversal t = new NodeTraversal(compiler, callback, scopeCreator); - t.traverseRoots(externs, root); + new NodeTraversal(compiler, new ScopeFinder(), scopeCreator).traverseRoots(externs, root); } }; } @@ -1269,13 +1270,11 @@ public void testTemplateType7() { "var result = goog.array.filter(arr," + " function(a,b,c) {var self=this;}, new Foo());"); - assertEquals("Foo", findNameType("self", lastLocalScope).toString()); - assertEquals("string", findNameType("a", lastLocalScope).toString()); - assertEquals("number", findNameType("b", lastLocalScope).toString()); - assertEquals("Array", - findNameType("c", lastLocalScope).toString()); - assertEquals("Array", - findNameType("result", globalScope).toString()); + assertEquals("Foo", findNameType("self", lastFunctionScope).toString()); + assertEquals("string", findNameType("a", lastFunctionScope).toString()); + assertEquals("number", findNameType("b", lastFunctionScope).toString()); + assertEquals("Array", findNameType("c", lastFunctionScope).toString()); + assertEquals("Array", findNameType("result", globalScope).toString()); } public void testTemplateType7b() { @@ -1305,13 +1304,11 @@ public void testTemplateType7b() { "var result = goog.array.filter(arr," + " function(a,b,c) {var self=this;}, new Foo());"); - assertEquals("Foo", findNameType("self", lastLocalScope).toString()); - assertEquals("string", findNameType("a", lastLocalScope).toString()); - assertEquals("number", findNameType("b", lastLocalScope).toString()); - assertEquals("Array", - findNameType("c", lastLocalScope).toString()); - assertEquals("Array", - findNameType("result", globalScope).toString()); + assertEquals("Foo", findNameType("self", lastFunctionScope).toString()); + assertEquals("string", findNameType("a", lastFunctionScope).toString()); + assertEquals("number", findNameType("b", lastFunctionScope).toString()); + assertEquals("Array", findNameType("c", lastFunctionScope).toString()); + assertEquals("Array", findNameType("result", globalScope).toString()); } public void testTemplateType7c() { @@ -1341,13 +1338,11 @@ public void testTemplateType7c() { "var result = goog.array.filter(arr," + " function(a,b,c) {var self=this;}, new Foo());"); - assertEquals("Foo", findNameType("self", lastLocalScope).toString()); - assertEquals("string", findNameType("a", lastLocalScope).toString()); - assertEquals("number", findNameType("b", lastLocalScope).toString()); - assertEquals("(Array|null)", - findNameType("c", lastLocalScope).toString()); - assertEquals("Array", - findNameType("result", globalScope).toString()); + assertEquals("Foo", findNameType("self", lastFunctionScope).toString()); + assertEquals("string", findNameType("a", lastFunctionScope).toString()); + assertEquals("number", findNameType("b", lastFunctionScope).toString()); + assertEquals("(Array|null)", findNameType("c", lastFunctionScope).toString()); + assertEquals("Array", findNameType("result", globalScope).toString()); } public void disable_testTemplateType8() { @@ -2072,17 +2067,13 @@ public void testBadIfaceInit2() throws Exception { public void testDeclaredCatchExpression1() { testSame( "try {} catch (e) {}"); - // Note: "e" actually belongs to a inner scope but we don't - // model catches as separate scopes currently. - assertNull(globalScope.getVar("e").getType()); + assertNull(lastLocalScope.getVar("e").getType()); } public void testDeclaredCatchExpression2() { testSame( "try {} catch (/** @type {string} */ e) {}"); - // Note: "e" actually belongs to a inner scope but we don't - // model catches as separate scopes currently. - assertEquals("string", globalScope.getVar("e").getType().toString()); + assertEquals("string", lastLocalScope.getVar("e").getType().toString()); } public void testGenerator1() {