Skip to content

Commit

Permalink
Roll memoization directly into TypedScopeCreator.
Browse files Browse the repository at this point in the history
This is a prerequisite for handling block scoping correctly: to ensure names are defined in the correct order, TypedScopeCreator needs to create nested block scopes when they are encountered in the createScope traversal, rather than afterwards.  This is required for, e.g. `if (true) { class Base {} } class Sub extends Base {}`.  TypedScopeCreator can now create these scopes and be sure they are correctly memoized.  This is straightforward since every usage of TypedScopeCreator was always wrapped in MemoizedTypedScopeCreator.  The upshot is that MemoizedTypedScopeCreator can now be deleted.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=187245381
  • Loading branch information
shicks authored and Tyler Breisacher committed Feb 28, 2018
1 parent 0547c42 commit 3c31832
Show file tree
Hide file tree
Showing 11 changed files with 122 additions and 218 deletions.
8 changes: 4 additions & 4 deletions src/com/google/javascript/jscomp/CleanupPasses.java
Expand Up @@ -97,10 +97,10 @@ protected FeatureSet featureSet() {

/**
* A CleanupPass implementation that will remove stored scopes from the
* MemoizedTypedScopeCreator of the compiler instance for a the hot swapped script.
* TypedScopeCreator of the compiler instance for a the hot swapped script.
* <p>
* This pass will also clear out Source Nodes of Function Types declared on
* Vars tracked by MemoizedTypedScopeCreator
* Vars tracked by TypedScopeCreator
*/
static class MemoizedScopeCleanupPass implements HotSwapCompilerPass {

Expand All @@ -113,8 +113,8 @@ public MemoizedScopeCleanupPass(AbstractCompiler compiler) {
@Override
public void hotSwapScript(Node scriptRoot, Node originalRoot) {
ScopeCreator creator = compiler.getTypedScopeCreator();
if (creator instanceof MemoizedTypedScopeCreator) {
MemoizedTypedScopeCreator scopeCreator = (MemoizedTypedScopeCreator) creator;
if (creator instanceof TypedScopeCreator) {
TypedScopeCreator scopeCreator = (TypedScopeCreator) creator;
String newSrc = scriptRoot.getSourceFileName();
for (TypedVar var : scopeCreator.getAllSymbols()) {
TypeI type = var.getType();
Expand Down
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/Compiler.java
Expand Up @@ -1581,7 +1581,7 @@ MostRecentTypechecker getMostRecentTypechecker() {

@Override
// Only used by jsdev
public MemoizedTypedScopeCreator getTypedScopeCreator() {
public TypedScopeCreator getTypedScopeCreator() {
return getPassConfig().getTypedScopeCreator();
}

Expand All @@ -1607,7 +1607,7 @@ DefaultPassConfig ensureDefaultPassConfig() {
public SymbolTable buildKnownSymbolTable() {
SymbolTable symbolTable = new SymbolTable(this, getTypeRegistry());

MemoizedTypedScopeCreator typedScopeCreator = getTypedScopeCreator();
TypedScopeCreator typedScopeCreator = getTypedScopeCreator();
if (typedScopeCreator != null) {
symbolTable.addScopes(typedScopeCreator.getAllMemoizedScopes());
symbolTable.addSymbolsFrom(typedScopeCreator);
Expand Down
121 changes: 0 additions & 121 deletions src/com/google/javascript/jscomp/MemoizedTypedScopeCreator.java

This file was deleted.

24 changes: 6 additions & 18 deletions src/com/google/javascript/jscomp/PassConfig.java
Expand Up @@ -35,17 +35,7 @@ public abstract class PassConfig {
// Used by the subclasses.
protected final CompilerOptions options;

/**
* A memoized version of scopeCreator. It must be memoized so that
* we can make two separate passes over the AST, one for inferring types
* and one for checking types.
*/
private MemoizedTypedScopeCreator typedScopeCreator;

/**
* This is the scope creator that {@code TypedScopeCreator} delegates to.
*/
private TypedScopeCreator internalScopeCreator;
private TypedScopeCreator typedScopeCreator;

/** The global typed scope. */
TypedScope topScope = null;
Expand All @@ -61,13 +51,11 @@ public PassConfig(CompilerOptions options) {
* @param root The root of the AST.
*/
void regenerateGlobalTypedScope(AbstractCompiler compiler, Node root) {
internalScopeCreator = new TypedScopeCreator(compiler);
typedScopeCreator = new MemoizedTypedScopeCreator(internalScopeCreator);
typedScopeCreator = new TypedScopeCreator(compiler);
topScope = typedScopeCreator.createScope(root, null);
}

void clearTypedScope() {
internalScopeCreator = null;
typedScopeCreator = null;
topScope = null;
}
Expand All @@ -80,14 +68,14 @@ void clearTypedScope() {
* @param scriptRoot The root of the AST used to generate global scope.
*/
void patchGlobalTypedScope(AbstractCompiler compiler, Node scriptRoot) {
checkNotNull(internalScopeCreator);
internalScopeCreator.patchGlobalScope(topScope, scriptRoot);
checkNotNull(typedScopeCreator);
typedScopeCreator.patchGlobalScope(topScope, scriptRoot);
}

/**
* Gets the scope creator for typed scopes.
*/
MemoizedTypedScopeCreator getTypedScopeCreator() {
TypedScopeCreator getTypedScopeCreator() {
return typedScopeCreator;
}

Expand Down Expand Up @@ -266,7 +254,7 @@ protected List<PassFactory> getWhitespaceOnlyPasses() {
return delegate.getTranspileOnlyPasses();
}

@Override MemoizedTypedScopeCreator getTypedScopeCreator() {
@Override TypedScopeCreator getTypedScopeCreator() {
return delegate.getTypedScopeCreator();
}

Expand Down
6 changes: 3 additions & 3 deletions src/com/google/javascript/jscomp/TypeCheck.java
Expand Up @@ -339,7 +339,7 @@ public final class TypeCheck implements NodeTraversal.Callback, CompilerPass {
private final JSTypeRegistry typeRegistry;
private TypedScope topScope;

private MemoizedTypedScopeCreator scopeCreator;
private TypedScopeCreator scopeCreator;

private final boolean reportUnknownTypes;
private SubtypingMode subtypingMode = SubtypingMode.NORMAL;
Expand Down Expand Up @@ -372,7 +372,7 @@ public TypeCheck(
ReverseAbstractInterpreter reverseInterpreter,
JSTypeRegistry typeRegistry,
TypedScope topScope,
MemoizedTypedScopeCreator scopeCreator) {
TypedScopeCreator scopeCreator) {
this.compiler = compiler;
this.validator = compiler.getTypeValidator();
this.reverseInterpreter = reverseInterpreter;
Expand Down Expand Up @@ -431,7 +431,7 @@ public TypedScope processForTesting(Node externsRoot, Node jsRoot) {
checkState(jsRoot.getParent() != null);
Node externsAndJsRoot = jsRoot.getParent();

scopeCreator = new MemoizedTypedScopeCreator(new TypedScopeCreator(compiler));
scopeCreator = new TypedScopeCreator(compiler);
topScope = scopeCreator.createScope(externsAndJsRoot, null);

TypeInferencePass inference = new TypeInferencePass(compiler,
Expand Down
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/TypeInferencePass.java
Expand Up @@ -38,12 +38,12 @@ class TypeInferencePass implements CompilerPass {
private final AbstractCompiler compiler;
private final ReverseAbstractInterpreter reverseInterpreter;
private final TypedScope topScope;
private final MemoizedTypedScopeCreator scopeCreator;
private final TypedScopeCreator scopeCreator;
private final Map<String, AssertionFunctionSpec> assertionFunctionsMap;

TypeInferencePass(AbstractCompiler compiler,
ReverseAbstractInterpreter reverseInterpreter,
TypedScope topScope, MemoizedTypedScopeCreator scopeCreator) {
TypedScope topScope, TypedScopeCreator scopeCreator) {
this.compiler = compiler;
this.reverseInterpreter = reverseInterpreter;
this.topScope = topScope;
Expand Down
72 changes: 71 additions & 1 deletion src/com/google/javascript/jscomp/TypedScopeCreator.java
Expand Up @@ -51,6 +51,9 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Multiset;
import com.google.javascript.jscomp.CodingConvention.DelegateRelationship;
import com.google.javascript.jscomp.CodingConvention.ObjectLiteralCast;
Expand All @@ -64,6 +67,7 @@
import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.NominalTypeBuilder;
import com.google.javascript.rhino.StaticSymbolTable;
import com.google.javascript.rhino.Token;
import com.google.javascript.rhino.jstype.EnumType;
import com.google.javascript.rhino.jstype.FunctionParamBuilder;
Expand All @@ -78,6 +82,7 @@
import com.google.javascript.rhino.jstype.TemplateTypeMap;
import com.google.javascript.rhino.jstype.TemplateTypeMapReplacer;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
Expand All @@ -98,7 +103,7 @@
*
* @author nicksantos@google.com (Nick Santos)
*/
final class TypedScopeCreator implements ScopeCreator {
final class TypedScopeCreator implements ScopeCreator, StaticSymbolTable<TypedVar, TypedVar> {
/**
* A suffix for naming delegate proxies differently from their base.
*/
Expand Down Expand Up @@ -157,6 +162,7 @@ final class TypedScopeCreator implements ScopeCreator {
private final JSTypeRegistry typeRegistry;
private final List<FunctionType> delegateProxyCtors = new ArrayList<>();
private final Map<String, String> delegateCallingConventions = new HashMap<>();
private final Map<Node, TypedScope> memoized = new LinkedHashMap<>();
private final boolean runsAfterNTI;

// Simple properties inferred about functions.
Expand Down Expand Up @@ -208,6 +214,57 @@ private void report(JSError error) {
}
}

@Override
public Iterable<TypedVar> getReferences(TypedVar var) {
return ImmutableList.of(var);
}

@Override
public TypedScope getScope(TypedVar var) {
return var.scope;
}

@Override
public Iterable<TypedVar> getAllSymbols() {
List<TypedVar> vars = new ArrayList<>();
for (TypedScope s : memoized.values()) {
Iterables.addAll(vars, s.getAllSymbols());
}
return vars;
}

Collection<TypedScope> getAllMemoizedScopes() {
// Return scopes in reverse order of creation so that IIFEs will
// come before the global scope.
return Lists.reverse(ImmutableList.copyOf(memoized.values()));
}

/**
* Removes all scopes with root nodes from a given script file.
*
* @param scriptName the name of the script file to remove nodes for.
*/
void removeScopesForScript(String scriptName) {
for (Node scopeRoot : ImmutableSet.copyOf(memoized.keySet())) {
if (scriptName.equals(scopeRoot.getSourceFileName())) {
memoized.remove(scopeRoot);
}
}
}

/** Create a scope, 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.
TypedScope s = memoized.get(n);
return s != null
? s
: createScope(n, createScope(NodeUtil.getEnclosingScopeRoot(n.getParent())));
}

/**
* Creates a scope with all types declared. Declares newly discovered types
* and type properties in the type registry.
Expand All @@ -216,10 +273,23 @@ private void report(JSError error) {
public TypedScope createScope(Node root, AbstractScope<?, ?> parent) {
checkArgument(parent == null || parent instanceof TypedScope);
TypedScope typedParent = (TypedScope) parent;

TypedScope scope = memoized.get(root);
if (scope != null) {
checkState(typedParent == scope.getParent());
} else {
scope = createScopeInternal(root, typedParent);
memoized.put(root, scope);
}
return scope;
}

private TypedScope createScopeInternal(Node root, TypedScope typedParent) {
// Constructing the global scope is very different than constructing
// inner scopes, because only global scopes can contain named classes that
// show up in the type registry.
TypedScope newScope = null;

AbstractScopeBuilder scopeBuilder = null;
if (typedParent == null) {
JSType globalThis =
Expand Down

0 comments on commit 3c31832

Please sign in to comment.