From 012a6fdea5b92e3e289d68701ac6ac1339b0d756 Mon Sep 17 00:00:00 2001 From: sdh Date: Fri, 16 Mar 2018 12:59:52 -0700 Subject: [PATCH] Streamline special handling in Var.Arguments to also handle 'this' and 'super'. Adds an AbstractScope.ImplicitVar enum that includes the three special vars that are implicitly created in different scopes. The enum provides a method to know whether any given scope creates the var, and is integrated into the various Scope.getVar() methods to do the right thing. This is a step toward supporting ES6 block scopes in LinkedFlowScope, since we need to be more careful exactly which scope to declare qualified names on (there's no longer just the function scope, but the various block scopes in between). The general approach is to look for the root of the qualified name and this streamlines the case where the root is 'this' or 'super' to not need any special handling. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=189380979 --- .../javascript/jscomp/AbstractScope.java | 80 ++++++++++++++++--- .../google/javascript/jscomp/AbstractVar.java | 26 ++++-- src/com/google/javascript/jscomp/Scope.java | 4 +- .../javascript/jscomp/TypeInference.java | 7 +- .../google/javascript/jscomp/TypedScope.java | 21 ++++- .../javascript/jscomp/TypedScopeCreator.java | 6 +- .../google/javascript/jscomp/TypedVar.java | 37 --------- src/com/google/javascript/jscomp/Var.java | 42 ---------- .../jscomp/Es6SyntacticScopeCreatorTest.java | 52 ++++++++++++ 9 files changed, 169 insertions(+), 106 deletions(-) diff --git a/src/com/google/javascript/jscomp/AbstractScope.java b/src/com/google/javascript/jscomp/AbstractScope.java index 924af2e4008..e32b6e73ce2 100644 --- a/src/com/google/javascript/jscomp/AbstractScope.java +++ b/src/com/google/javascript/jscomp/AbstractScope.java @@ -24,6 +24,7 @@ import com.google.javascript.rhino.StaticScope; import java.io.Serializable; import java.util.Collections; +import java.util.EnumMap; import java.util.LinkedHashMap; import java.util.Map; @@ -53,8 +54,8 @@ abstract class AbstractScope, V extends AbstractVar> implements StaticScope, Serializable { private final Map vars = new LinkedHashMap<>(); + private final Map implicitVars = new EnumMap<>(ImplicitVar.class); private final Node rootNode; - private V arguments; AbstractScope(Node rootNode) { this.rootNode = rootNode; @@ -117,7 +118,7 @@ public final S getParentScope() { return getParent(); } - abstract V makeArgumentsVar(); + abstract V makeImplicitVar(ImplicitVar type); /** * Undeclares a variable, to be used when the compiler optimizes out @@ -157,6 +158,10 @@ public final V getOwnSlot(String name) { */ // Non-final for jsdev tests public V getVar(String name) { + ImplicitVar implicit = name != null ? ImplicitVar.of(name) : null; + if (implicit != null) { + return getImplicitVar(implicit, true); + } S scope = thisScope(); while (scope != null) { V var = scope.getOwnSlot(name); @@ -173,21 +178,30 @@ public V getVar(String name) { } /** - * Get a unique VAR object to represent "arguments" within this scope + * Get a unique Var object to represent "arguments" within this scope */ public final V getArgumentsVar() { - if (isGlobal() || isModuleScope()) { - return null; - } - - if (!isFunctionScope() || rootNode.isArrowFunction()) { - return getParent().getArgumentsVar(); - } + return getImplicitVar(ImplicitVar.ARGUMENTS, false); + } - if (arguments == null) { - arguments = makeArgumentsVar(); + private final V getImplicitVar(ImplicitVar var, boolean allowDeclaredVars) { + S scope = thisScope(); + while (scope != null) { + if (var.isMadeByScope(scope)) { + V result = ((AbstractScope) scope).implicitVars.get(var); + if (result == null) { + ((AbstractScope) scope).implicitVars.put(var, result = scope.makeImplicitVar(var)); + } + return result; + } + V result = allowDeclaredVars ? scope.getOwnSlot(var.name) : null; + if (result != null) { + return result; + } + // Recurse up the parent Scope + scope = scope.getParent(); } - return arguments; + return null; } /** @@ -372,4 +386,44 @@ void checkRootScope() { checkArgument( NodeUtil.createsScope(rootNode) || rootNode.isScript() || rootNode.isRoot(), rootNode); } + + /** + * The three implicit var types, which are defined implicitly (at least) in + * every vanilla function scope without actually being declared. + */ + enum ImplicitVar { + ARGUMENTS("arguments"), + SUPER("super"), + // TODO(sdh): Expand THIS.isMadeByScope to check super.isMadeByScope(scope) || scope.isGlobal() + // Currently this causes a number of problems (see b/74980936), but could eventually lead to + // better type information. We might also want to restrict this so that module-root scopes + // explicitly *don't* have access to the global this, though I think this is more than just + // returning false in isMadeByScope - rather, getVar() needs to stop checking and immediately + // return null. + THIS("this"); + + final String name; + + ImplicitVar(String name) { + this.name = name; + } + + /** Whether this kind of implicit variable is created/owned by the given scope. */ + boolean isMadeByScope(AbstractScope scope) { + return NodeUtil.isVanillaFunction(scope.getRootNode()); + } + + static ImplicitVar of(String name) { + switch (name) { + case "arguments": + return ARGUMENTS; + case "super": + return SUPER; + case "this": + return THIS; + default: + return null; + } + } + } } diff --git a/src/com/google/javascript/jscomp/AbstractVar.java b/src/com/google/javascript/jscomp/AbstractVar.java index 7f0939f2516..b544d82080e 100644 --- a/src/com/google/javascript/jscomp/AbstractVar.java +++ b/src/com/google/javascript/jscomp/AbstractVar.java @@ -16,6 +16,8 @@ package com.google.javascript.jscomp; +import static com.google.common.base.Preconditions.checkState; + import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.javascript.rhino.JSDocInfo; @@ -78,7 +80,7 @@ final CompilerInput getInput() { @Override public StaticSourceFile getSourceFile() { - return nameNode.getStaticSourceFile(); + return (nameNode != null ? nameNode : scope.getRootNode()).getStaticSourceFile(); } @Override @@ -100,7 +102,8 @@ public final Node getParentNode() { * that bleeds into the inner scope). */ public boolean isBleedingFunction() { - return NodeUtil.isFunctionExpression(getParentNode()); + Node parent = getParentNode(); + return parent != null && NodeUtil.isFunctionExpression(parent); } public final S getScope() { @@ -191,8 +194,17 @@ final boolean isImport() { return declarationType() == Token.IMPORT; } - public boolean isArguments() { - return false; + public final boolean isArguments() { + return Var.ARGUMENTS.equals(name) && scope.isFunctionScope(); + } + + public final boolean isThis() { + return "this".equals(name) && scope.isFunctionScope(); + } + + private boolean isImplicit() { + AbstractScope.ImplicitVar var = AbstractScope.ImplicitVar.of(name); + return var != null && var.isMadeByScope(scope); } private static final ImmutableSet DECLARATION_TYPES = Sets.immutableEnumSet( @@ -212,8 +224,10 @@ protected Token declarationType() { return current.getToken(); } } - throw new IllegalStateException("The nameNode for " + this + " must be a descendant" - + " of one of: " + DECLARATION_TYPES); + checkState( + isImplicit(), + "The nameNode for %s must be a descendant of one of: %s", this, DECLARATION_TYPES); + return null; } // This is safe because any concrete subclass of AbstractVar should be assignable to V. diff --git a/src/com/google/javascript/jscomp/Scope.java b/src/com/google/javascript/jscomp/Scope.java index 194c978c687..0b6d0d3b93c 100644 --- a/src/com/google/javascript/jscomp/Scope.java +++ b/src/com/google/javascript/jscomp/Scope.java @@ -66,8 +66,8 @@ Var declare(String name, Node nameNode, CompilerInput input) { } @Override - Var makeArgumentsVar() { - return Var.makeArgumentsVar(this); + Var makeImplicitVar(ImplicitVar var) { + return new Var(var.name, null /* nameNode */, this, -1 /* index */, null /* input */); } private static final class Simple extends Scope { diff --git a/src/com/google/javascript/jscomp/TypeInference.java b/src/com/google/javascript/jscomp/TypeInference.java index e44c5ef2b05..af4a528d035 100644 --- a/src/com/google/javascript/jscomp/TypeInference.java +++ b/src/com/google/javascript/jscomp/TypeInference.java @@ -622,8 +622,11 @@ private void updateScopeForTypeChange( String varName = left.getString(); TypedVar var = syntacticScope.getVar(varName); JSType varType = var == null ? null : var.getType(); - boolean isVarDeclaration = left.hasChildren() - && varType != null && !var.isTypeInferred(); + boolean isVarDeclaration = + left.hasChildren() + && varType != null + && !var.isTypeInferred() + && var.getNameNode() != null; boolean isTypelessConstDecl = isVarDeclaration diff --git a/src/com/google/javascript/jscomp/TypedScope.java b/src/com/google/javascript/jscomp/TypedScope.java index 4f1d851e8bf..c25a6f8e3a2 100644 --- a/src/com/google/javascript/jscomp/TypedScope.java +++ b/src/com/google/javascript/jscomp/TypedScope.java @@ -113,6 +113,8 @@ public TypedScope getParent() { public JSType getTypeOfThis() { if (isGlobal()) { return ObjectType.cast(getRootNode().getJSType()); + } else if (!getRootNode().isFunction()) { + return getClosestNonBlockScope().getTypeOfThis(); } checkState(getRootNode().isFunction()); @@ -138,8 +140,22 @@ TypedVar declare(String name, Node nameNode, } @Override - TypedVar makeArgumentsVar() { - return TypedVar.makeArguments(this); + TypedVar makeImplicitVar(ImplicitVar var) { + return new TypedVar(false, var.name, null, getImplicitVarType(var), this, -1, null); + } + + private JSType getImplicitVarType(ImplicitVar var) { + if (var == ImplicitVar.ARGUMENTS) { + // Look for an extern named "arguments" and use its type if available. + // TODO(sdh): consider looking for "Arguments" ctor rather than "arguments" var: this could + // allow deleting the variable, which doesn't really belong in externs in the first place. + TypedVar globalArgs = getGlobalScope().getVar(Var.ARGUMENTS); + return globalArgs != null && globalArgs.isExtern() + ? globalArgs.getType() + : null; + } + // TODO(sdh): get the superclass for super? + return getTypeOfThis(); } public Iterable getDeclarativelyUnboundVarsWithoutTypes() { @@ -171,7 +187,6 @@ void setTypeResolver(TypeResolver resolver) { this.typeResolver = resolver; } - @SuppressWarnings("unchecked") @Override public JSType getNamespaceOrTypedefType(String typeName) { StaticTypedSlot slot = getSlot(typeName); diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java index 2167d9df030..fb0c517fbaa 100644 --- a/src/com/google/javascript/jscomp/TypedScopeCreator.java +++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java @@ -1012,7 +1012,11 @@ private FunctionType createFunctionTypeFromNodes( ObjectType ownerType = null; if (ownerName != null) { ownerVar = scope.getVar(ownerName); - if (ownerVar != null) { + // TODO(sdh): Remove the !isThis() check. Historically scopes returned + // null for getVar("this"), but they now return a meaningful variable, + // which exposes a lot of type errors that need to be fixed before this + // restriction can be removed: b/74980936. + if (ownerVar != null && !ownerVar.isThis()) { ownerType = ObjectType.cast(ownerVar.getType()); } if (name != null) { diff --git a/src/com/google/javascript/jscomp/TypedVar.java b/src/com/google/javascript/jscomp/TypedVar.java index bae3d4af6aa..ee4bb87692b 100644 --- a/src/com/google/javascript/jscomp/TypedVar.java +++ b/src/com/google/javascript/jscomp/TypedVar.java @@ -18,8 +18,6 @@ import com.google.javascript.rhino.ErrorReporter; import com.google.javascript.rhino.Node; -import com.google.javascript.rhino.StaticSourceFile; -import com.google.javascript.rhino.Token; import com.google.javascript.rhino.jstype.JSType; import com.google.javascript.rhino.jstype.StaticTypedRef; import com.google.javascript.rhino.jstype.StaticTypedSlot; @@ -113,39 +111,4 @@ void markAssignedExactlyOnce() { boolean isMarkedAssignedExactlyOnce() { return markedAssignedExactlyOnce; } - - static TypedVar makeArguments(TypedScope scope) { - // Look for an extern named "arguments" and use its type if available. - // TODO(sdh): consider looking for "Arguments" ctor rather than "arguments" var: this could - // allow deleting the variable, which doesn't really belong in externs in the first place. - TypedVar globalArgs = scope.getGlobalScope().getVar(Var.ARGUMENTS); - JSType type = globalArgs != null && globalArgs.isExtern() ? globalArgs.getType() : null; - return new TypedArguments(type, scope); - } - - private static final class TypedArguments extends TypedVar { - TypedArguments(JSType type, TypedScope scope) { - super(false, Var.ARGUMENTS, null, type, scope, -1, null); - } - - @Override - public boolean isArguments() { - return true; - } - - @Override - public StaticSourceFile getSourceFile() { - return scope.getRootNode().getStaticSourceFile(); - } - - @Override - public boolean isBleedingFunction() { - return false; - } - - @Override - protected Token declarationType() { - return null; - } - } } diff --git a/src/com/google/javascript/jscomp/Var.java b/src/com/google/javascript/jscomp/Var.java index 3f74253fe85..00acd7535c4 100644 --- a/src/com/google/javascript/jscomp/Var.java +++ b/src/com/google/javascript/jscomp/Var.java @@ -19,8 +19,6 @@ import com.google.javascript.rhino.Node; import com.google.javascript.rhino.StaticRef; import com.google.javascript.rhino.StaticSlot; -import com.google.javascript.rhino.StaticSourceFile; -import com.google.javascript.rhino.Token; /** * Used by {@code Scope} to store information about variables. @@ -33,48 +31,8 @@ public class Var extends AbstractVar implements StaticSlot, StaticRe super(name, nameNode, scope, index, input); } - static Var makeArgumentsVar(Scope scope) { - return new Arguments(scope); - } - @Override public String toString() { return "Var " + name + " @ " + nameNode; } - - /** - * A special subclass of Var used to distinguish "arguments" in the current - * scope. - */ - private static class Arguments extends Var { - Arguments(Scope scope) { - super( - ARGUMENTS, // always arguments - null, // no declaration node - scope, - -1, // no variable index - null // input - ); - } - - @Override - public boolean isArguments() { - return true; - } - - @Override - public StaticSourceFile getSourceFile() { - return scope.getRootNode().getStaticSourceFile(); - } - - @Override - public boolean isBleedingFunction() { - return false; - } - - @Override - protected Token declarationType() { - return null; - } - } } diff --git a/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java b/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java index 6f551dd9325..a2f0faad16a 100644 --- a/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/Es6SyntacticScopeCreatorTest.java @@ -787,6 +787,58 @@ public void testArgumentsVariableInArrowFunction() { assertThat(innerFunctionScope.getArgumentsVar()).isSameAs(arguments); } + public void testTheThisVariable() { + String js = "function f() { if (true) { function g() {} } }"; + Node root = getRoot(js); + Scope global = scopeCreator.createScope(root, null); + + Node function = root.getFirstChild(); + checkState(function.isFunction(), function); + Scope fScope = scopeCreator.createScope(function, global); + assertFalse(fScope.isDeclared("this", true)); + Var thisVar = fScope.getVar("this"); + assertTrue(thisVar.isThis()); + + Node fBlock = NodeUtil.getFunctionBody(function); + Scope fBlockScope = scopeCreator.createScope(fBlock, fScope); + assertFalse(fBlockScope.isDeclared("this", true)); + assertThat(fBlockScope.getVar("this")).isSameAs(thisVar); + + Node ifBlock = fBlock.getFirstChild().getLastChild(); + Scope blockScope = scopeCreator.createScope(ifBlock, fBlockScope); + assertFalse(blockScope.isDeclared("this", true)); + 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)); + assertThat(gScope.getVar("this").getScope()).isSameAs(gScope); + } + + public void testTheThisVariableInArrowFunction() { + String js = "function outer() { var inner = () => this.x; }"; + Node root = getRoot(js); + Scope global = scopeCreator.createScope(root, null); + + Node outer = root.getFirstChild(); + checkState(outer.isFunction(), outer); + checkState(!outer.isArrowFunction(), outer); + Scope outerFunctionScope = scopeCreator.createScope(outer, global); + Var thisVar = outerFunctionScope.getVar("this"); + + Node outerBody = NodeUtil.getFunctionBody(outer); + Scope outerBodyScope = scopeCreator.createScope(outerBody, outerFunctionScope); + + Node inner = outerBody.getFirstChild() // VAR + .getFirstChild() // NAME + .getFirstChild(); // FUNCTION + checkState(inner.isFunction(), inner); + checkState(inner.isArrowFunction(), inner); + Scope innerFunctionScope = scopeCreator.createScope(inner, outerBodyScope); + assertThat(innerFunctionScope.getVar("this")).isSameAs(thisVar); + } + public void testIsFunctionBlockScoped() { String js = "if (true) { function f() {}; }"; Node root = getRoot(js);