diff --git a/src/com/google/javascript/jscomp/AbstractScope.java b/src/com/google/javascript/jscomp/AbstractScope.java index e948ca937cc..b9b11df1bca 100644 --- a/src/com/google/javascript/jscomp/AbstractScope.java +++ b/src/com/google/javascript/jscomp/AbstractScope.java @@ -33,21 +33,27 @@ * points back to its parent scope. A Scope contains information about variables defined in that * scope. * - *

ES6 introduces new scoping rules, which adds some complexity to this class. In particular, - * scopes fall into four mutually exclusive categories based on their root node: block, function, - * module, or global. Function, module, and global scopes are collectively referred to as "non-block - * scopes". We also define a scope as a "hoist scope" if it is a non-block scope *or* it is the - * outermost block scope within a function (i.e. a "function block scope"). Hoist scopes are - * important because "var" declarations are hoisted to the closest hoist scope, as opposed to ES6 - * "let" and "const" which are not hoisted, but instead added directly to whatever scope they're - * declared in. + *

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 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). * - *

Finally, a caution about function scopes and function block scopes: the language does not - * permit any shadowing to occur between them (with the exception of bleeding function names), so in - * many situations these scopes are treated as a single scope. Under block scoping, only function - * parameters (and optionally, bleeding function names) are declared in the function scope. It is - * kept as a separate scope so that default parameter initializers may be evaluated in a separate - * scope from the function body. + *

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" + * 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. + * + *

Note that every function actually has two distinct hoist scopes: a container scope on the + * FUNCTION node, and a block-scope on the top-level BLOCK in the function (the "function block"). + * Local variables are declared on the function block, while parameters and optionally the function + * name (if it bleeds, i.e. from a named function expression) are declared on the container scope. + * This is required so that default parameter initializers can refer to names from outside the + * function that could possibly be shadowed in the function block. But these scopes are not fully + * independent of one another, since the language does not allow a top-level local variable to + * shadow a parameter name - so in some situations these scopes must be treated as a single scope. * * @see NodeTraversal */ @@ -145,6 +151,9 @@ final void clearVarsInternal() { @Override public final V getSlot(String name) { + // TODO(sdh): This behavior is inconsistent with getOwnSlot, hasSlot, and hasOwnSlot + // when it comes to implicit vars. The other three methods all exclude implicits, + // but this one returns them. It would be good to clean this up one way or the other. return getVar(name); } @@ -202,35 +211,21 @@ private final V getImplicitVar(ImplicitVar var, boolean allowDeclaredVars) { return null; } - /** - * Use only when in a function block scope and want to tell if a name is either at the top of the - * function block scope or the function parameter scope. This obviously only applies to ES6 block - * scopes. - */ - public final boolean isDeclaredInFunctionBlockOrParameter(String name) { - // In ES6, we create a separate "function parameter scope" above the function block scope to - // handle default parameters. Since nothing in the function block scope is allowed to shadow - // the variables in the function scope, we treat the two scopes as one in this method. - checkState(isFunctionBlockScope()); - return isDeclared(name, false) || (getParent().isDeclared(name, false)); + /** Returns true if a variable is declared in this scope, with no recursion. */ + public final boolean hasOwnSlot(String name) { + return vars.containsKey(name); } - /** - * Returns true if a variable is declared. - */ - public final boolean isDeclared(String name, boolean recurse) { + /** Returns true if a variable is declared in this or any parent scope. */ + public final boolean hasSlot(String name) { S scope = thisScope(); - while (true) { - if (scope.getOwnSlot(name) != null) { + while (scope != null) { + if (scope.hasOwnSlot(name)) { return true; } - - if (scope.getParent() != null && recurse) { - scope = scope.getParent(); - continue; - } - return false; + scope = scope.getParent(); } + return false; } /** @@ -333,11 +328,11 @@ final boolean isHoistScope() { /** * If a var were declared in this scope, return the scope it would be hoisted to. * - * For function scopes, we return back the scope itself, since even though there is no way - * to declare a var inside function parameters, it would make even less sense to say that - * such declarations would be "hoisted" somewhere else. + *

For function scopes, we return back the scope itself, since even though there is no way to + * declare a var inside function parameters, it would make even less sense to say that such + * declarations would be "hoisted" somewhere else. */ - public final S getClosestHoistScope() { + final S getClosestHoistScope() { S current = thisScope(); while (current != null) { if (current.isHoistScope()) { @@ -349,10 +344,10 @@ public final S getClosestHoistScope() { } /** - * Returns the closest non-block scope. This is equivalent to what the current scope would have - * been for non-block-scope creators, and is thus useful for migrating code to use block scopes. + * Returns the closest container scope. This is equivalent to what the current scope would have + * been for non-ES6 scope creators, and is thus useful for migrating code to use block scopes. */ - public final S getClosestNonBlockScope() { + public final S getClosestContainerScope() { S scope = getClosestHoistScope(); if (scope.isBlockScope()) { scope = scope.getParent(); @@ -385,6 +380,15 @@ void checkRootScope() { NodeUtil.createsScope(rootNode) || rootNode.isScript() || rootNode.isRoot(), rootNode); } + /** + * 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(); + } + /** * The three implicit var types, which are defined implicitly (at least) in * every vanilla function scope without actually being declared. diff --git a/src/com/google/javascript/jscomp/CoalesceVariableNames.java b/src/com/google/javascript/jscomp/CoalesceVariableNames.java index 45729888203..f2e0158dddf 100644 --- a/src/com/google/javascript/jscomp/CoalesceVariableNames.java +++ b/src/com/google/javascript/jscomp/CoalesceVariableNames.java @@ -227,7 +227,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { pseudoName = Joiner.on("_").join(allMergedNames); - while (t.getScope().isDeclared(pseudoName, true)) { + while (t.getScope().hasSlot(pseudoName)) { pseudoName += "$"; } diff --git a/src/com/google/javascript/jscomp/Es6RenameReferences.java b/src/com/google/javascript/jscomp/Es6RenameReferences.java index 1b99730246a..38a07e50574 100644 --- a/src/com/google/javascript/jscomp/Es6RenameReferences.java +++ b/src/com/google/javascript/jscomp/Es6RenameReferences.java @@ -64,7 +64,7 @@ private void renameReference(NodeTraversal t, Node n) { n.setString(newName); t.reportCodeChange(); return; - } else if (current.isDeclared(oldName, false)) { + } else if (current.hasOwnSlot(oldName)) { return; } else { current = current.getParent(); diff --git a/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java b/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java index 12a577ff65d..6839a9a6a0e 100644 --- a/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java +++ b/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java @@ -96,10 +96,10 @@ && inLoop(n)) { Scope hoistScope = scope.getClosestHoistScope(); if (scope != hoistScope) { String newName = oldName; - if (hoistScope.isDeclared(oldName, true) || undeclaredNames.contains(oldName)) { + if (hoistScope.hasSlot(oldName) || undeclaredNames.contains(oldName)) { do { newName = oldName + "$" + compiler.getUniqueNameIdSupplier().get(); - } while (hoistScope.isDeclared(newName, true)); + } while (hoistScope.hasSlot(newName)); nameNode.setString(newName); compiler.reportChangeToEnclosingScope(nameNode); Node scopeRoot = scope.getRootNode(); @@ -218,7 +218,7 @@ private void rewriteDeclsToVars() { private class CollectUndeclaredNames extends AbstractPostOrderCallback { @Override public void visit(NodeTraversal t, Node n, Node parent) { - if (n.isName() && !t.getScope().isDeclared(n.getString(), true)) { + if (n.isName() && !t.getScope().hasSlot(n.getString())) { undeclaredNames.add(n.getString()); } } diff --git a/src/com/google/javascript/jscomp/Es6TypedToEs6Converter.java b/src/com/google/javascript/jscomp/Es6TypedToEs6Converter.java index bcd99316aef..9d917d8f796 100644 --- a/src/com/google/javascript/jscomp/Es6TypedToEs6Converter.java +++ b/src/com/google/javascript/jscomp/Es6TypedToEs6Converter.java @@ -509,7 +509,7 @@ private void maybeVisitColonType(NodeTraversal t, Node n, Node jsDocNode) { private void visitTypeAlias(NodeTraversal t, Node n, Node parent) { String alias = n.getString(); - if (t.getScope().isDeclared(alias, true)) { + if (t.getScope().hasSlot(alias)) { compiler.report( JSError.make(n, TYPE_ALIAS_ALREADY_DECLARED, alias)); } diff --git a/src/com/google/javascript/jscomp/FlowSensitiveInlineVariables.java b/src/com/google/javascript/jscomp/FlowSensitiveInlineVariables.java index f53483438b9..1458f6b0633 100644 --- a/src/com/google/javascript/jscomp/FlowSensitiveInlineVariables.java +++ b/src/com/google/javascript/jscomp/FlowSensitiveInlineVariables.java @@ -633,7 +633,7 @@ public boolean apply(Node input) { public boolean apply(Node input) { if (input.isName()) { String name = input.getString(); - if (!name.isEmpty() && !usageScope.isDeclared(name, true)) { + if (!name.isEmpty() && !usageScope.hasSlot(name)) { return true; // unsafe to inline. } } diff --git a/src/com/google/javascript/jscomp/LiveVariablesAnalysis.java b/src/com/google/javascript/jscomp/LiveVariablesAnalysis.java index 2c9b49477a5..da83ce15b6c 100644 --- a/src/com/google/javascript/jscomp/LiveVariablesAnalysis.java +++ b/src/com/google/javascript/jscomp/LiveVariablesAnalysis.java @@ -368,11 +368,11 @@ private void addToSetIfLocal(Node node, BitSet set) { // ES6 separates the scope but if the variable is declared in the param it should be local // to the function body. if (localScope.isFunctionBlockScope()) { - local = localScope.isDeclaredInFunctionBlockOrParameter(name); + local = isDeclaredInFunctionBlockOrParameter(localScope, name); } else if (localScope == jsScope && jsScopeChild != null) { - local = jsScopeChild.isDeclaredInFunctionBlockOrParameter(name); + local = isDeclaredInFunctionBlockOrParameter(jsScopeChild, name); } else { - local = localScope.isDeclared(name, false); + local = localScope.hasOwnSlot(name); } if (!local) { @@ -384,6 +384,14 @@ private void addToSetIfLocal(Node node, BitSet set) { } } + private static boolean isDeclaredInFunctionBlockOrParameter(Scope scope, String name) { + // In ES6, we create a separate container scope above the function block scope to handle + // default parameters. Since nothing in the function block scope is allowed to shadow + // the variables in the function scope, we treat the two scopes as one in this method. + checkState(scope.isFunctionBlockScope()); + return scope.hasOwnSlot(name) || scope.getParent().hasOwnSlot(name); + } + /** * Give up computing liveness of formal parameter by putting all the parameter names in the * escaped set. @@ -402,12 +410,12 @@ void markAllParametersEscaped() { private boolean isArgumentsName(Node n) { boolean childDeclared; if (jsScopeChild != null) { - childDeclared = jsScopeChild.isDeclared(ARGUMENT_ARRAY_ALIAS, false); + childDeclared = jsScopeChild.hasOwnSlot(ARGUMENT_ARRAY_ALIAS); } else { childDeclared = true; } return n.isName() && n.getString().equals(ARGUMENT_ARRAY_ALIAS) - && (!jsScope.isDeclared(ARGUMENT_ARRAY_ALIAS, false) || !childDeclared); + && (!jsScope.hasOwnSlot(ARGUMENT_ARRAY_ALIAS) || !childDeclared); } } diff --git a/src/com/google/javascript/jscomp/NodeTraversal.java b/src/com/google/javascript/jscomp/NodeTraversal.java index 74016c085da..8348207af28 100644 --- a/src/com/google/javascript/jscomp/NodeTraversal.java +++ b/src/com/google/javascript/jscomp/NodeTraversal.java @@ -967,7 +967,7 @@ public Node getClosestHoistScopeRoot() { return scopes.peek().getClosestHoistScope().getRootNode(); } - public Node getClosestNonBlockScopeRoot() { + public Node getClosestContainerScopeRoot() { int roots = scopeRoots.size(); for (int i = roots; i > 0; i--) { Node rootNode = scopeRoots.get(i - 1); @@ -976,7 +976,7 @@ public Node getClosestNonBlockScopeRoot() { } } - return scopes.peek().getClosestNonBlockScope().getRootNode(); + return scopes.peek().getClosestContainerScope().getRootNode(); } public AbstractScope getClosestHoistScope() { diff --git a/src/com/google/javascript/jscomp/RemoveUnusedCode.java b/src/com/google/javascript/jscomp/RemoveUnusedCode.java index a059b73f156..04e560d4b73 100644 --- a/src/com/google/javascript/jscomp/RemoveUnusedCode.java +++ b/src/com/google/javascript/jscomp/RemoveUnusedCode.java @@ -235,7 +235,7 @@ private void traverseAndRemoveUnusedReferences(Node root) { // Create scope from parent of root node, which also has externs as a child, so we'll // have extern definitions in scope. Scope scope = scopeCreator.createScope(root.getParent(), null); - if (!scope.isDeclared(NodeUtil.JSC_PROPERTY_NAME_FN, /* recurse */ true)) { + if (!scope.hasSlot(NodeUtil.JSC_PROPERTY_NAME_FN)) { // TODO(b/70730762): Passes that add references to this should ensure it is declared. // NOTE: null input makes this an extern var. scope.declare( diff --git a/src/com/google/javascript/jscomp/ShadowVariables.java b/src/com/google/javascript/jscomp/ShadowVariables.java index 7799cd147c2..5437526c390 100644 --- a/src/com/google/javascript/jscomp/ShadowVariables.java +++ b/src/com/google/javascript/jscomp/ShadowVariables.java @@ -247,7 +247,7 @@ private Assignment findBestShadow(Scope curScope, Var var) { for (Assignment assignment : varsByFrequency) { if (assignment.isLocal) { if (!scopeUpRefMap.containsEntry(curScope.getRootNode(), assignment.oldName)) { - if (curScope.isDeclared(assignment.oldName, true)) { + if (curScope.hasSlot(assignment.oldName)) { // Don't shadow if the scopes are the same eg.: // function f() { var a = 1; { var a = 2; } } // Unsafe Var toShadow = curScope.getVar(assignment.oldName); diff --git a/src/com/google/javascript/jscomp/SymbolTable.java b/src/com/google/javascript/jscomp/SymbolTable.java index 6e1b3dde98b..7ec31d846bc 100644 --- a/src/com/google/javascript/jscomp/SymbolTable.java +++ b/src/com/google/javascript/jscomp/SymbolTable.java @@ -1212,7 +1212,7 @@ private SymbolScope createScopeFrom(StaticScope otherScope) { Node otherScopeRoot = otherScope.getRootNode(); // NOTE: Kythe is not set up to handle block scopes yet, so only create - // SymbolScopes for CFG root nodes, giving a pre-ES6 view of the world. + // SymbolScopes for container scope roots, giving a pre-ES6 view of the world. while (!NodeUtil.isValidCfgRoot(otherScopeRoot)) { otherScopeRoot = otherScopeRoot.getParent(); } diff --git a/src/com/google/javascript/jscomp/SyntacticScopeCreator.java b/src/com/google/javascript/jscomp/SyntacticScopeCreator.java index e48e541d7de..5e49863cf70 100644 --- a/src/com/google/javascript/jscomp/SyntacticScopeCreator.java +++ b/src/com/google/javascript/jscomp/SyntacticScopeCreator.java @@ -186,7 +186,7 @@ private void declareVar(Node n) { CompilerInput input = compiler.getInput(inputId); String name = n.getString(); - if (!scope.isDeclared(name, false) && !(scope.isLocal() && name.equals(Var.ARGUMENTS))) { + if (!scope.hasOwnSlot(name) && (!scope.isLocal() || !name.equals(Var.ARGUMENTS))) { if (isTyped) { ((TypedScope) scope).declare(name, n, null, input); } else { diff --git a/src/com/google/javascript/jscomp/TransformAMDToCJSModule.java b/src/com/google/javascript/jscomp/TransformAMDToCJSModule.java index 607eb505659..125a4ff59f4 100644 --- a/src/com/google/javascript/jscomp/TransformAMDToCJSModule.java +++ b/src/com/google/javascript/jscomp/TransformAMDToCJSModule.java @@ -181,11 +181,10 @@ private void handleRequire(NodeTraversal t, Node defineNode, Node script, String aliasName = aliasNode != null ? aliasNode.getString() : null; Scope globalScope = t.getScope(); - if (aliasName != null && - globalScope.isDeclared(aliasName, true)) { + if (aliasName != null && globalScope.hasSlot(aliasName)) { while (true) { String renamed = aliasName + VAR_RENAME_SUFFIX + renameIndex; - if (!globalScope.isDeclared(renamed, true)) { + if (!globalScope.hasSlot(renamed)) { NodeTraversal.traverseEs6(compiler, callback, new RenameCallback(aliasName, renamed)); aliasName = renamed; diff --git a/src/com/google/javascript/jscomp/TypeCheck.java b/src/com/google/javascript/jscomp/TypeCheck.java index a756063eb76..744e90b909b 100644 --- a/src/com/google/javascript/jscomp/TypeCheck.java +++ b/src/com/google/javascript/jscomp/TypeCheck.java @@ -483,17 +483,15 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { case FUNCTION: // normal type checking final TypedScope outerScope = t.getTypedScope(); - final String functionPrivateName = n.getFirstChild().getString(); - if (functionPrivateName != null - && functionPrivateName.length() > 0 - && outerScope.isDeclared(functionPrivateName, false) + final TypedVar var = outerScope.getVar(n.getFirstChild().getString()); + if (var != null + && var.getScope().hasSameContainerScope(outerScope) // Ideally, we would want to check whether the type in the scope // differs from the type being defined, but then the extern // redeclarations of built-in types generates spurious warnings. - && !(outerScope.getVar(functionPrivateName).getType() instanceof FunctionType) - && !TypeValidator.hasDuplicateDeclarationSuppression( - compiler, outerScope.getVar(functionPrivateName).getNameNode())) { - report(t, n, FUNCTION_MASKS_VARIABLE, functionPrivateName); + && !(var.getType() instanceof FunctionType) + && !TypeValidator.hasDuplicateDeclarationSuppression(compiler, var.getNameNode())) { + report(t, n, FUNCTION_MASKS_VARIABLE, var.getName()); } // TODO(user): Only traverse the function's body. The function's diff --git a/src/com/google/javascript/jscomp/TypeInference.java b/src/com/google/javascript/jscomp/TypeInference.java index fa7e0b26cab..a449f677879 100644 --- a/src/com/google/javascript/jscomp/TypeInference.java +++ b/src/com/google/javascript/jscomp/TypeInference.java @@ -83,7 +83,7 @@ class TypeInference private final ReverseAbstractInterpreter reverseInterpreter; private final FlowScope functionScope; private final FlowScope bottomScope; - private final TypedScope cfgRootScope; + private final TypedScope containerScope; private final TypedScopeCreator scopeCreator; private final Map assertionFunctionsMap; @@ -104,7 +104,7 @@ class TypeInference this.unknownType = registry.getNativeObjectType(UNKNOWN_TYPE); this.currentScope = syntacticScope; - this.cfgRootScope = syntacticScope; + this.containerScope = syntacticScope; inferArguments(syntacticScope); this.functionScope = LinkedFlowScope.createEntryLattice(syntacticScope); @@ -193,14 +193,12 @@ FlowScope flowThrough(Node n, FlowScope input) { } // TODO(sdh): Change to NodeUtil.getEnclosingScopeRoot(n) once we have block scopes. - this.currentScope = - scopeCreator.createScope(NodeUtil.getEnclosingNode(n, TypeInference::createsNonBlockScope)); FlowScope output = input.createChildFlowScope(); output = traverse(n, output); return output; } - private static boolean createsNonBlockScope(Node n) { + private static boolean createsContainerScope(Node n) { return NodeUtil.createsScope(n) && !NodeUtil.createsBlockScope(n); } @@ -732,7 +730,7 @@ private void ensurePropertyDefined(Node getprop, JSType rightType) { ObjectType objectType = ObjectType.cast( nodeType.restrictByNotNullOrUndefined()); boolean propCreationInConstructor = - obj.isThis() && getJSType(cfgRootScope.getRootNode()).isConstructor(); + obj.isThis() && getJSType(containerScope.getRootNode()).isConstructor(); if (objectType == null) { registry.registerPropertyOnType(propName, nodeType); @@ -749,7 +747,7 @@ private void ensurePropertyDefined(Node getprop, JSType rightType) { // top level and the constructor Foo is defined in the same file. boolean staticPropCreation = false; Node maybeAssignStm = getprop.getGrandparent(); - if (cfgRootScope.isGlobal() && NodeUtil.isPrototypePropertyDeclaration(maybeAssignStm)) { + if (containerScope.isGlobal() && NodeUtil.isPrototypePropertyDeclaration(maybeAssignStm)) { String propCreationFilename = maybeAssignStm.getSourceFileName(); Node ctor = objectType.getOwnerFunction().getSource(); if (ctor != null && ctor.getSourceFileName().equals(propCreationFilename)) { @@ -853,7 +851,9 @@ private FlowScope traverseName(Node n, FlowScope scope) { // In this case, we would infer the first reference to t as // type {number}, even though it's undefined. TypedVar maybeOuterVar = - isInferred && cfgRootScope.isLocal() ? cfgRootScope.getParent().getVar(varName) : null; + isInferred && containerScope.isLocal() + ? containerScope.getParent().getVar(varName) + : null; boolean nonLocalInferredSlot = var.equals(maybeOuterVar) && !maybeOuterVar.isMarkedAssignedExactlyOnce(); @@ -1850,7 +1850,7 @@ private boolean isUnflowable(TypedVar v) { && v.isLocal() && v.isMarkedEscaped() // It's OK to flow a variable in the scope where it's escaped. - && v.getScope().getClosestNonBlockScope() == cfgRootScope; + && v.getScope().getClosestContainerScope() == containerScope; } /** diff --git a/src/com/google/javascript/jscomp/TypeInferencePass.java b/src/com/google/javascript/jscomp/TypeInferencePass.java index b0dc4473b32..d44995cfb8c 100644 --- a/src/com/google/javascript/jscomp/TypeInferencePass.java +++ b/src/com/google/javascript/jscomp/TypeInferencePass.java @@ -141,7 +141,7 @@ public void enterScope(NodeTraversal t) { // This ensures that incremental compilation only touches the root // that's been swapped out. TypedScope scope = t.getTypedScope(); - if (!scope.isBlockScope()) { // ignore non-cfg-root scopes. + if (!scope.isBlockScope()) { // ignore scopes that don't have their own CFGs. inferScope(t.getCurrentNode(), scope); } // Resolve any new type names found during the inference. diff --git a/src/com/google/javascript/jscomp/TypedScope.java b/src/com/google/javascript/jscomp/TypedScope.java index 16bd6f4ef98..b7517e9693c 100644 --- a/src/com/google/javascript/jscomp/TypedScope.java +++ b/src/com/google/javascript/jscomp/TypedScope.java @@ -114,7 +114,7 @@ public JSType getTypeOfThis() { if (isGlobal()) { return ObjectType.cast(getRootNode().getJSType()); } else if (!getRootNode().isFunction()) { - return getClosestNonBlockScope().getTypeOfThis(); + return getClosestContainerScope().getTypeOfThis(); } checkState(getRootNode().isFunction()); diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java index cb924efab00..d5d3947394f 100644 --- a/src/com/google/javascript/jscomp/TypedScopeCreator.java +++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java @@ -1258,7 +1258,7 @@ void defineSlot(Node n, Node parent, String variableName, // don't try to declare in the global scope if there's // already a symbol there with this name. - if (!globalScope.isDeclared(variableName, false)) { + if (!globalScope.hasOwnSlot(variableName)) { scopeToDeclareIn = scope.getGlobalScope(); } } @@ -1269,7 +1269,7 @@ void defineSlot(Node n, Node parent, String variableName, // declared in closest scope? CompilerInput input = compiler.getInput(inputId); - if (scopeToDeclareIn.isDeclared(variableName, false)) { + if (scopeToDeclareIn.hasOwnSlot(variableName)) { TypedVar oldVar = scopeToDeclareIn.getVar(variableName); newVar = validator.expectUndeclaredVariable( sourceName, input, n, parent, oldVar, variableName, type); @@ -1842,7 +1842,7 @@ private boolean isQualifiedNameInferred( if (inferred && rhsValue != null && rhsValue.isFunction()) { if (info != null) { return false; - } else if (!scope.isDeclared(qName, false) && n.isUnscopedQualifiedName()) { + } else if (!scope.hasOwnSlot(qName) && n.isUnscopedQualifiedName()) { // Check if this is in a conditional block. // Functions assigned in conditional blocks are inferred. @@ -1908,7 +1908,7 @@ void resolveStubDeclarations() { String ownerName = stub.ownerName; boolean isExtern = stub.isExtern; - if (scope.isDeclared(qName, false)) { + if (scope.hasOwnSlot(qName)) { continue; } @@ -2206,10 +2206,10 @@ void process(Node externs, Node root) { return; } - Node nonBlockScopeRoot = t.getClosestNonBlockScopeRoot(); + Node containerScopeRoot = t.getClosestContainerScopeRoot(); if (n.isReturn() && n.getFirstChild() != null) { - functionsWithNonEmptyReturns.add(nonBlockScopeRoot); + functionsWithNonEmptyReturns.add(containerScopeRoot); } // Be careful of bleeding functions, which create variables @@ -2219,11 +2219,11 @@ void process(Node externs, Node root) { Scope scope = t.getScope(); Var var = scope.getVar(name); if (var != null) { - Scope ownerScope = var.getScope().getClosestNonBlockScope(); + Scope ownerScope = var.getScope().getClosestContainerScope(); if (ownerScope.isLocal()) { Node ownerScopeRoot = ownerScope.getRootNode(); assignedVarNames.add(ScopedName.of(name, ownerScopeRoot)); - if (nonBlockScopeRoot != ownerScopeRoot) { + if (containerScopeRoot != ownerScopeRoot) { escapedVarNames.add(ScopedName.of(name, ownerScopeRoot)); } } @@ -2233,9 +2233,9 @@ void process(Node externs, Node root) { Scope scope = t.getScope(); Var var = scope.getVar(name); if (var != null) { - Scope ownerScope = var.getScope().getClosestNonBlockScope(); + Scope ownerScope = var.getScope().getClosestContainerScope(); Node ownerScopeRoot = ownerScope.getRootNode(); - if (ownerScope.isLocal() && nonBlockScopeRoot != ownerScopeRoot) { + if (ownerScope.isLocal() && containerScopeRoot != ownerScopeRoot) { escapedVarNames.add(ScopedName.of(n.getQualifiedName(), ownerScopeRoot)); } } diff --git a/src/com/google/javascript/jscomp/VariableReferenceCheck.java b/src/com/google/javascript/jscomp/VariableReferenceCheck.java index fce1762ee06..fb991ae5b76 100644 --- a/src/com/google/javascript/jscomp/VariableReferenceCheck.java +++ b/src/com/google/javascript/jscomp/VariableReferenceCheck.java @@ -182,11 +182,11 @@ private void checkDefaultParam( compiler, param.getParentNode().getSecondChild(), /** - * Do a shallow check since cases like: + * 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: + * } is legal. We are going to miss cases like: {@code * function f(y = (() => x)(), x = 5) { return y(); } - * but this should be rare. + * } but this should be rare. */ new AbstractShallowCallback() { @Override @@ -195,7 +195,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { return; } String refName = n.getString(); - if (varsInFunctionBody.contains(refName) && !scope.isDeclared(refName, true)) { + if (varsInFunctionBody.contains(refName) && !scope.hasSlot(refName)) { compiler.report(JSError.make(n, EARLY_REFERENCE_ERROR, refName)); } } diff --git a/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java b/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java index 6cabec6f37d..da180ccc56d 100644 --- a/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java @@ -729,7 +729,6 @@ public void testFunctionArgument() { Scope fBlockScope = scopeCreator.createScope(functionBlock, functionScope); assertScope(fBlockScope).declares("x").on(functionScope); - assertTrue(fBlockScope.isDeclaredInFunctionBlockOrParameter("x")); assertScope(fBlockScope).doesNotDeclare("y"); Node ifBlock = functionBlock.getLastChild().getLastChild(); @@ -793,24 +792,24 @@ public void testTheThisVariable() { Node function = root.getFirstChild(); checkState(function.isFunction(), function); Scope fScope = scopeCreator.createScope(function, global); - assertFalse(fScope.isDeclared("this", true)); + assertFalse(fScope.hasSlot("this")); Var thisVar = fScope.getVar("this"); assertTrue(thisVar.isThis()); Node fBlock = NodeUtil.getFunctionBody(function); Scope fBlockScope = scopeCreator.createScope(fBlock, fScope); - assertFalse(fBlockScope.isDeclared("this", true)); + assertScope(fBlockScope).doesNotDeclare("this"); assertThat(fBlockScope.getVar("this")).isSameAs(thisVar); Node ifBlock = fBlock.getFirstChild().getLastChild(); Scope blockScope = scopeCreator.createScope(ifBlock, fBlockScope); - assertFalse(blockScope.isDeclared("this", true)); + assertScope(blockScope).doesNotDeclare("this"); assertThat(blockScope.getVar("this")).isSameAs(thisVar); assertThat(blockScope.getVar("this").getScope()).isSameAs(fScope); Node gFunction = ifBlock.getFirstChild(); Scope gScope = scopeCreator.createScope(gFunction, blockScope); - assertFalse(gScope.isDeclared("this", true)); + assertScope(gScope).doesNotDeclare("this"); assertThat(gScope.getVar("this").getScope()).isSameAs(gScope); } @@ -1008,7 +1007,6 @@ public void testSimpleFunctionParam() { Node fBlock = NodeUtil.getFunctionBody(fNode); Scope fBlockScope = scopeCreator.createScope(fBlock, fScope); assertScope(fBlockScope).declares("x").on(fScope); - assertTrue(fBlockScope.isDeclaredInFunctionBlockOrParameter("x")); } public void testOnlyOneDeclaration() { @@ -1022,7 +1020,6 @@ public void testOnlyOneDeclaration() { Node fBlock = fNode.getLastChild(); Scope fBlockScope = scopeCreator.createScope(fBlock, fScope); assertScope(fBlockScope).declares("x").on(fScope); - assertTrue(fBlockScope.isDeclaredInFunctionBlockOrParameter("x")); Node ifBlock = fBlock.getFirstChild().getLastChild(); Scope ifBlockScope = scopeCreator.createScope(ifBlock, fBlockScope); diff --git a/test/com/google/javascript/jscomp/IncrementalScopeCreatorTest.java b/test/com/google/javascript/jscomp/IncrementalScopeCreatorTest.java index 81bc9471033..665d507d943 100644 --- a/test/com/google/javascript/jscomp/IncrementalScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/IncrementalScopeCreatorTest.java @@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertThat; +import static com.google.javascript.jscomp.ScopeSubject.assertScope; import com.google.common.base.Predicate; import com.google.common.base.Predicates; @@ -83,11 +84,11 @@ public void testParialGlobalScopeRefresh() throws Exception { // recreated, so we need to inspect a Var in the scope to see if it has been freshed or not. Var inside = globalFunction.getVar("inside"); - assertTrue(globalScope.isDeclared("a", true)); - assertTrue(globalScope.isDeclared("b", true)); - assertTrue(globalScope.isDeclared("x", true)); - assertTrue(globalScope.isDeclared("ext", true)); - assertFalse(globalScope.isDeclared("nonexistant", true)); + assertScope(globalScope).declares("a"); + assertScope(globalScope).declares("b"); + assertScope(globalScope).declares("x"); + assertScope(globalScope).declares("ext"); + assertScope(globalScope).doesNotDeclare("nonexistant"); // Make a change that affects the global scope (and report it) removeFirstDecl(compiler, compiler.getRoot(), "a"); @@ -98,11 +99,11 @@ public void testParialGlobalScopeRefresh() throws Exception { assertSame(globalFunction, creator.createScope(fnFoo, globalScope)); assertSame(inside, globalFunction.getVar("inside")); - assertTrue(globalScope2.isDeclared("a", true)); // still declared, scope creator is frozen - assertTrue(globalScope2.isDeclared("b", true)); - assertTrue(globalScope2.isDeclared("x", true)); - assertTrue(globalScope2.isDeclared("ext", true)); - assertFalse(globalScope2.isDeclared("nonexistant", true)); + assertScope(globalScope2).declares("a"); // still declared, scope creator is frozen + assertScope(globalScope2).declares("b"); + assertScope(globalScope2).declares("x"); + assertScope(globalScope2).declares("ext"); + assertScope(globalScope2).doesNotDeclare("nonexistant"); // Allow the scopes to be updated by calling "thaw" and "freeze" @@ -116,11 +117,11 @@ public void testParialGlobalScopeRefresh() throws Exception { assertSame(globalFunction, creator.createScope(fnFoo, globalScope)); assertSame(inside, globalFunction.getVar("inside")); - assertFalse(globalScope3.isDeclared("a", true)); // no declared, scope creator has refreshed - assertTrue(globalScope3.isDeclared("b", true)); - assertTrue(globalScope3.isDeclared("x", true)); - assertTrue(globalScope3.isDeclared("ext", true)); - assertFalse(globalScope3.isDeclared("nonexistant", true)); + assertScope(globalScope3).doesNotDeclare("a"); // no declared, scope creator has refreshed + assertScope(globalScope3).declares("b"); + assertScope(globalScope3).declares("x"); + assertScope(globalScope3).declares("ext"); + assertScope(globalScope3).doesNotDeclare("nonexistant"); IncrementalScopeCreator.getInstance(compiler).thaw(); } @@ -144,11 +145,11 @@ public void testPartialGlobalScopeRefreshWithMove() throws Exception { Scope globalScope = creator.createScope(root, null); - assertTrue(globalScope.isDeclared("a", false)); - assertTrue(globalScope.isDeclared("b", false)); - assertTrue(globalScope.isDeclared("x", false)); - assertTrue(globalScope.isDeclared("y", false)); - assertFalse(globalScope.isDeclared("nonexistant", false)); + assertScope(globalScope).declares("a").directly(); + assertScope(globalScope).declares("b").directly(); + assertScope(globalScope).declares("x").directly(); + assertScope(globalScope).declares("y").directly(); + assertScope(globalScope).doesNotDeclare("nonexistant"); Node script1 = checkNotNull(NodeUtil.getEnclosingScript(findDecl(root, "a"))); Node script2 = checkNotNull(NodeUtil.getEnclosingScript(findDecl(root, "x"))); @@ -171,11 +172,11 @@ public void testPartialGlobalScopeRefreshWithMove() throws Exception { globalScope = creator.createScope(root, null); - assertTrue(globalScope.isDeclared("a", false)); - assertTrue(globalScope.isDeclared("b", false)); - assertTrue(globalScope.isDeclared("x", false)); - assertTrue(globalScope.isDeclared("y", false)); - assertFalse(globalScope.isDeclared("nonexistant", false)); + assertScope(globalScope).declares("a").directly(); + assertScope(globalScope).declares("b").directly(); + assertScope(globalScope).declares("x").directly(); + assertScope(globalScope).declares("y").directly(); + assertScope(globalScope).doesNotDeclare("nonexistant"); compiler.reportChangeToChangeScope(script1); // invalidate the original scope. @@ -185,11 +186,11 @@ public void testPartialGlobalScopeRefreshWithMove() throws Exception { globalScope = creator.createScope(root, null); - assertTrue(globalScope.isDeclared("a", false)); - assertTrue(globalScope.isDeclared("b", false)); - assertTrue(globalScope.isDeclared("x", false)); - assertTrue(globalScope.isDeclared("y", false)); - assertFalse(globalScope.isDeclared("nonexistant", false)); + assertScope(globalScope).declares("a").directly(); + assertScope(globalScope).declares("b").directly(); + assertScope(globalScope).declares("x").directly(); + assertScope(globalScope).declares("y").directly(); + assertScope(globalScope).doesNotDeclare("nonexistant"); creator.thaw(); } @@ -209,8 +210,8 @@ public void testRefreshedGlobalScopeWithRedeclaration() throws Exception { Scope globalScope = creator.createScope(root, null); - assertTrue(globalScope.isDeclared("a", true)); - assertTrue(globalScope.isDeclared("b", true)); + assertScope(globalScope).declares("a"); + assertScope(globalScope).declares("b"); removeFirstDecl(compiler, compiler.getRoot(), "a"); // leaves the second declaration removeFirstDecl(compiler, compiler.getRoot(), "b"); @@ -224,8 +225,8 @@ public void testRefreshedGlobalScopeWithRedeclaration() throws Exception { Scope globalScope2 = creator.createScope(compiler.getRoot(), null); assertSame(globalScope, globalScope2); - assertTrue(globalScope2.isDeclared("a", true)); // still declared in second file - assertFalse(globalScope2.isDeclared("b", true)); + assertScope(globalScope2).declares("a"); // still declared in second file + assertScope(globalScope2).doesNotDeclare("b"); IncrementalScopeCreator.getInstance(compiler).thaw(); } diff --git a/test/com/google/javascript/jscomp/ScopeSubject.java b/test/com/google/javascript/jscomp/ScopeSubject.java index 459c96555c1..3b29b28402d 100644 --- a/test/com/google/javascript/jscomp/ScopeSubject.java +++ b/test/com/google/javascript/jscomp/ScopeSubject.java @@ -15,6 +15,7 @@ */ package com.google.javascript.jscomp; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertAbout; @@ -33,7 +34,7 @@ * ... * assertScope(scope).declares("somevar"); * assertScope(scope).declares("otherVar").directly(); - * assertScope(scope).declares("yetAnotherVar").onClosestNonBlockScope(); + * assertScope(scope).declares("yetAnotherVar").onClosestContainerScope(); * */ public final class ScopeSubject extends Subject> { @@ -47,14 +48,14 @@ private ScopeSubject(FailureMetadata failureMetadata, AbstractScope scope) } public void doesNotDeclare(String name) { - AbstractVar var = getSubject().getVar(name); + AbstractVar var = getVar(name); if (var != null) { failWithBadResults("does not declare", name, "declared", var); } } public DeclarationSubject declares(String name) { - AbstractVar var = getSubject().getVar(name); + AbstractVar var = getVar(name); if (var == null) { ImmutableList> declared = ImmutableList.copyOf(getSubject().getAllAccessibleVariables()); @@ -72,6 +73,10 @@ public DeclarationSubject declares(String name) { return new DeclarationSubject(var); } + private AbstractVar getVar(String name) { + return getSubject().hasSlot(name) ? checkNotNull(getSubject().getVar(name)) : null; + } + public final class DeclarationSubject { private final AbstractVar var; @@ -108,9 +113,9 @@ public void on(AbstractScope scope) { expectScope("on", scope, scope); } - /** Expects the declared variable to be declared on the closest non-block scope. */ - public void onClosestNonBlockScope() { - expectScope("on", "the closest non-block scope", getSubject().getClosestNonBlockScope()); + /** Expects the declared variable to be declared on the closest container scope. */ + public void onClosestContainerScope() { + expectScope("on", "the closest container scope", getSubject().getClosestContainerScope()); } /** Expects the declared variable to be declared on the closest hoist scope. */ diff --git a/test/com/google/javascript/jscomp/SyntacticScopeCreatorTest.java b/test/com/google/javascript/jscomp/SyntacticScopeCreatorTest.java index 4fd24bc64fd..5be457446c2 100644 --- a/test/com/google/javascript/jscomp/SyntacticScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/SyntacticScopeCreatorTest.java @@ -16,6 +16,8 @@ package com.google.javascript.jscomp; +import static com.google.javascript.jscomp.ScopeSubject.assertScope; + import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; import junit.framework.TestCase; @@ -57,16 +59,16 @@ public void testFunctionScope() { "[function bar2() { var y; }];" + "if (true) { function z() {} }"); - assertTrue(scope.isDeclared("foo", false)); - assertTrue(scope.isDeclared("x", false)); - assertTrue(scope.isDeclared("z", false)); + assertScope(scope).declares("foo").directly(); + assertScope(scope).declares("x").directly(); + assertScope(scope).declares("z").directly(); // The following should not be declared in this scope - assertFalse(scope.isDeclared("a1", false)); - assertFalse(scope.isDeclared("bar", false)); - assertFalse(scope.isDeclared("bar2", false)); - assertFalse(scope.isDeclared("y", false)); - assertFalse(scope.isDeclared("", false)); + assertScope(scope).doesNotDeclare("a1"); + assertScope(scope).doesNotDeclare("bar"); + assertScope(scope).doesNotDeclare("bar2"); + assertScope(scope).doesNotDeclare("y"); + assertScope(scope).doesNotDeclare(""); } public void testNestedFunctionScope() { @@ -75,12 +77,12 @@ public void testNestedFunctionScope() { Node fNode = root.getFirstChild(); Scope outerFScope = (Scope) scopeCreator.createScope(fNode, globalScope); - assertTrue(outerFScope.isDeclared("x", false)); + assertScope(outerFScope).declares("x").directly(); Node innerFNode = fNode.getLastChild().getFirstChild(); Scope innerFScope = (Scope) scopeCreator.createScope(innerFNode, outerFScope); - assertFalse(innerFScope.isDeclared("x", false)); - assertTrue(innerFScope.isDeclared("y", false)); + assertScope(innerFScope).declares("x").onSomeParent(); + assertScope(innerFScope).declares("y").directly(); } public void testScopeRootNode() { @@ -93,16 +95,16 @@ public void testScopeRootNode() { assertEquals(Token.FUNCTION, fooNode.getToken()); Scope fooScope = (Scope) scopeCreator.createScope(fooNode, globalScope); assertEquals(fooNode, fooScope.getRootNode()); - assertTrue(fooScope.isDeclared("x", false)); + assertScope(fooScope).declares("x").directly(); } public void testFunctionExpressionInForLoopInitializer() { Node root = getRoot("for (function foo() {};;) {}"); Scope globalScope = (Scope) scopeCreator.createScope(root, null); - assertFalse(globalScope.isDeclared("foo", false)); + assertScope(globalScope).doesNotDeclare("foo"); Node fNode = root.getFirstFirstChild(); Scope fScope = (Scope) scopeCreator.createScope(fNode, globalScope); - assertTrue(fScope.isDeclared("foo", false)); + assertScope(fScope).declares("foo").directly(); } } diff --git a/test/com/google/javascript/jscomp/TypeCheckTest.java b/test/com/google/javascript/jscomp/TypeCheckTest.java index 81de7512905..d4ab1be68f7 100644 --- a/test/com/google/javascript/jscomp/TypeCheckTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckTest.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertThat; +import static com.google.javascript.jscomp.ScopeSubject.assertScope; import static com.google.javascript.jscomp.TypeCheck.INSTANTIATE_ABSTRACT_CLASS; import static com.google.javascript.jscomp.TypeCheck.STRICT_INEXISTENT_PROPERTY; import static com.google.javascript.jscomp.parsing.JsDocInfoParser.BAD_TYPE_WIKI_LINK; @@ -2150,8 +2151,8 @@ public void testScoping10() { TypeCheckResult p = parseAndTypeCheckWithScope("var a = function b(){};"); // a declared, b is not - assertTrue(p.scope.isDeclared("a", false)); - assertFalse(p.scope.isDeclared("b", false)); + assertScope(p.scope).declares("a"); + assertScope(p.scope).doesNotDeclare("b"); // checking that a has the correct assigned type assertEquals("function(): undefined", diff --git a/test/com/google/javascript/jscomp/VarCheckTest.java b/test/com/google/javascript/jscomp/VarCheckTest.java index 85e0bdf00d7..9aeb87491ea 100644 --- a/test/com/google/javascript/jscomp/VarCheckTest.java +++ b/test/com/google/javascript/jscomp/VarCheckTest.java @@ -17,7 +17,7 @@ package com.google.javascript.jscomp; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.javascript.jscomp.ScopeSubject.assertScope; import static com.google.javascript.jscomp.VarCheck.BLOCK_SCOPED_DECL_MULTIPLY_DECLARED_ERROR; import static com.google.javascript.jscomp.VarCheck.VAR_MULTIPLY_DECLARED_ERROR; @@ -778,9 +778,7 @@ public void process(Node externs, Node root) { @Override public void visit(NodeTraversal t, Node n, Node parent) { if (n.isName() && !parent.isFunction() && !parent.isLabel()) { - assertWithMessage("Variable %s should have been declared", n) - .that(t.getScope().isDeclared(n.getString(), true)) - .isTrue(); + assertScope(t.getScope()).declares(n.getString()); } } },