From 6c7c763c3ad1c58439054590e11e19fce085a5c3 Mon Sep 17 00:00:00 2001 From: stalcup Date: Fri, 14 Jul 2017 13:10:55 -0700 Subject: [PATCH] Makes DefinitionUseSiteFinder incrementally updateable. Does not yet wrap it into the the persistent datastructure framework, that comes in the following CL. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=161996924 --- .../DeadPropertyAssignmentElimination.java | 12 +- .../jscomp/DefinitionUseSiteFinder.java | 102 +++++++---- .../javascript/jscomp/DefinitionsRemover.java | 24 ++- .../jscomp/J2clClinitPrunerPass.java | 6 +- .../jscomp/NameBasedDefinitionProvider.java | 169 +++++++++++------- .../javascript/jscomp/NodeTraversal.java | 16 +- .../jscomp/UnreachableCodeElimination.java | 6 +- .../jscomp/DefinitionUseSiteFinderTest.java | 82 ++++++++- .../javascript/jscomp/NodeTraversalTest.java | 8 +- 9 files changed, 298 insertions(+), 127 deletions(-) diff --git a/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java b/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java index 7351c054a0e..149d6592a7a 100644 --- a/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java +++ b/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java @@ -24,7 +24,7 @@ import com.google.common.collect.PeekingIterator; import com.google.common.collect.Sets; import com.google.javascript.jscomp.NodeTraversal.Callback; -import com.google.javascript.jscomp.NodeTraversal.FunctionCallback; +import com.google.javascript.jscomp.NodeTraversal.ChangeScopeRootCallback; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; import java.util.HashMap; @@ -88,7 +88,7 @@ public void process(Node externs, Node root) { NodeTraversal.traverseChangedFunctions(compiler, new FunctionVisitor(blacklistedPropNames)); } - private static class FunctionVisitor implements FunctionCallback { + private static class FunctionVisitor implements ChangeScopeRootCallback { /** * A set of properties names that are potentially unsafe to remove duplicate writes to. @@ -100,18 +100,18 @@ private static class FunctionVisitor implements FunctionCallback { } @Override - public void enterFunction(AbstractCompiler compiler, Node n) { - if (!n.isFunction()) { + public void enterChangeScopeRoot(AbstractCompiler compiler, Node root) { + if (!root.isFunction()) { return; } - Node body = NodeUtil.getFunctionBody(n); + Node body = NodeUtil.getFunctionBody(root); if (!body.hasChildren() || NodeUtil.containsFunction(body)) { return; } FindCandidateAssignmentTraversal traversal = - new FindCandidateAssignmentTraversal(blacklistedPropNames, NodeUtil.isConstructor(n)); + new FindCandidateAssignmentTraversal(blacklistedPropNames, NodeUtil.isConstructor(root)); NodeTraversal.traverseEs6(compiler, body, traversal); // Any candidate property assignment can have a write removed if that write is never read diff --git a/src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java b/src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java index 4aab5714feb..10798bd1917 100644 --- a/src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java +++ b/src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java @@ -20,40 +20,56 @@ import static com.google.common.base.Preconditions.checkState; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.HashMultimap; +import com.google.common.collect.Iterables; import com.google.common.collect.LinkedHashMultimap; import com.google.common.collect.Multimap; import com.google.javascript.jscomp.DefinitionsRemover.Definition; import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; import com.google.javascript.rhino.Node; import java.util.Collection; +import java.util.List; /** - * Built on top of the {@link NameBasedDefinitionProvider}, this class additionally collects the - * use sites for each definition. It is useful for constructing a full reference graph of the entire + * Built on top of the {@link NameBasedDefinitionProvider}, this class additionally collects the use + * sites for each definition. It is useful for constructing a full reference graph of the entire * ast. * */ // TODO(stalcup): track useSites in lhs of GET_ELEM nodes as well. public class DefinitionUseSiteFinder extends NameBasedDefinitionProvider { - private final Multimap nameUseSiteMultimap; + private static class NameAndUseSite { + final String name; + final UseSite useSite; + + NameAndUseSite(String name, UseSite useSite) { + this.name = name; + this.useSite = useSite; + } + } + + private final Multimap useSitesByName; + // Remember which UseSite instances are in which scopes, so that the knowledge about a changing + // scope can be rebuilt later. + private final Multimap useSitesByScopeNode; @VisibleForTesting - Multimap getNameUseSiteMultimap() { + Multimap getUseSitesByName() { // Defensive copy. - return LinkedHashMultimap.create(nameUseSiteMultimap); + return LinkedHashMultimap.create(useSitesByName); } public DefinitionUseSiteFinder(AbstractCompiler compiler) { super(compiler, false); - this.nameUseSiteMultimap = LinkedHashMultimap.create(); + this.useSitesByName = LinkedHashMultimap.create(); + this.useSitesByScopeNode = HashMultimap.create(); } @Override public void process(Node externs, Node source) { super.process(externs, source); - NodeTraversal.traverseEs6( - compiler, source, new UseSiteGatheringCallback()); + NodeTraversal.traverseEs6(compiler, source, new UseSiteGatheringCallback()); } /** @@ -64,9 +80,8 @@ public void process(Node externs, Node source) { * @return use site collection. */ public Collection getUseSites(Definition definition) { - checkState(hasProcessBeenRun, "The process was not run"); - String name = getSimplifiedName(definition.getLValue()); - return nameUseSiteMultimap.get(name); + checkState(hasProcessBeenRun, "Hasn't been initialized with process() yet."); + return useSitesByName.get(definition.getSimplifiedName()); } private class UseSiteGatheringCallback extends AbstractPostOrderCallback { @@ -85,9 +100,10 @@ public void visit(NodeTraversal traversal, Node node, Node parent) { String name = getSimplifiedName(first.getLValue()); checkNotNull(name); - nameUseSiteMultimap.put( - name, - new UseSite(node, traversal.getScope(), traversal.getModule())); + UseSite useSite = new UseSite(node, traversal.getScope(), traversal.getModule()); + useSitesByName.put(name, useSite); + useSitesByScopeNode.put( + NodeUtil.getEnclosingChangeScopeRoot(node), new NameAndUseSite(name, useSite)); } } @@ -129,8 +145,7 @@ boolean canModifyDefinition(Definition definition) { // mechanisms. Node nameNode = site.node; - Collection singleSiteDefinitions = - getDefinitionsReferencedAt(nameNode); + Collection singleSiteDefinitions = getDefinitionsReferencedAt(nameNode); if (singleSiteDefinitions.size() > 1) { return false; } @@ -142,9 +157,7 @@ boolean canModifyDefinition(Definition definition) { return true; } - /** - * @return Whether the definition is directly exported. - */ + /** @return Whether the definition is directly exported. */ private boolean isExported(Definition definition) { // Assume an exported method result is used. Node lValue = definition.getLValue(); @@ -167,32 +180,51 @@ private boolean isExported(Definition definition) { return codingConvention.isExported(partialName); } - /** - * Traverse a node and its children and remove any references to from - * the structures. - */ + @Override + public void rebuildScopeRoots(List changedScopeRoots, List deletedScopeRoots) { + super.rebuildScopeRoots(changedScopeRoots, deletedScopeRoots); + + for (Node scopeRoot : Iterables.concat(deletedScopeRoots, changedScopeRoots)) { + for (NameAndUseSite nameAndUseSite : useSitesByScopeNode.removeAll(scopeRoot)) { + useSitesByName.remove(nameAndUseSite.name, nameAndUseSite.useSite); + } + } + + NodeTraversal.traverseEs6ScopeRoots( + compiler, null, changedScopeRoots, new UseSiteGatheringCallback(), false); + } + + /** Traverse a node and its children and remove any references to from the structures. */ void removeReferences(Node node) { if (DefinitionsRemover.isDefinitionNode(node)) { - DefinitionSite defSite = definitionNodeByDefinitionSite.get(node); - if (defSite != null) { - Definition def = defSite.definition; - String name = getSimplifiedName(def.getLValue()); + Node definitionSiteNode = node; + DefinitionSite definitionSite = definitionSitesByDefinitionSiteNode.get(definitionSiteNode); + if (definitionSite != null) { + Definition definition = definitionSite.definition; + String name = definition.getSimplifiedName(); if (name != null) { - this.definitionNodeByDefinitionSite.remove(node); - this.nameDefinitionMultimap.remove(name, def); + Node definitionNode = definition.getLValue(); + definitionNodes.remove(definitionNode); + definitionsByName.remove(name, definition); + definitionSitesByDefinitionSiteNode.remove(definitionSiteNode); + Node scopeNode = NodeUtil.getEnclosingChangeScopeRoot(definitionSiteNode); + definitionSitesByScopeNode.remove(scopeNode, definitionSite); } } } else { - Node useSite = node; - if (useSite.isGetProp()) { - String propName = useSite.getLastChild().getString(); + Node useSiteNode = node; + if (useSiteNode.isGetProp()) { + String propName = useSiteNode.getLastChild().getString(); if (propName.equals("apply") || propName.equals("call")) { - useSite = useSite.getFirstChild(); + useSiteNode = useSiteNode.getFirstChild(); } } - String name = getSimplifiedName(useSite); + String name = getSimplifiedName(useSiteNode); if (name != null) { - this.nameUseSiteMultimap.remove(name, new UseSite(useSite, null, null)); + UseSite useSite = new UseSite(useSiteNode, null, null); + useSitesByName.remove(name, useSite); + useSitesByScopeNode.remove( + NodeUtil.getEnclosingChangeScopeRoot(useSiteNode), new NameAndUseSite(name, useSite)); } } diff --git a/src/com/google/javascript/jscomp/DefinitionsRemover.java b/src/com/google/javascript/jscomp/DefinitionsRemover.java index b6153ebae84..a4f17f79c97 100644 --- a/src/com/google/javascript/jscomp/DefinitionsRemover.java +++ b/src/com/google/javascript/jscomp/DefinitionsRemover.java @@ -130,9 +130,11 @@ static boolean isDefinitionNode(Node n) { abstract static class Definition { private final boolean isExtern; + private final String simplifiedName; - Definition(boolean isExtern) { + Definition(boolean isExtern, String simplifiedName) { this.isExtern = isExtern; + this.simplifiedName = simplifiedName; } /** @@ -154,6 +156,10 @@ public void remove(AbstractCompiler compiler) { */ protected abstract void performRemove(AbstractCompiler compiler); + public String getSimplifiedName() { + return simplifiedName; + } + /** * Variable or property name represented by this definition. * For example, in the case of assignments this method would @@ -194,7 +200,7 @@ abstract static class IncompleteDefinition extends Definition { private final Node lValue; IncompleteDefinition(Node lValue, boolean inExterns) { - super(inExterns); + super(inExterns, NameBasedDefinitionProvider.getSimplifiedName(lValue)); checkNotNull(lValue); Preconditions.checkArgument( @@ -273,7 +279,7 @@ abstract static class FunctionDefinition extends Definition { protected final Node function; FunctionDefinition(Node node, boolean inExterns) { - super(inExterns); + super(inExterns, NameBasedDefinitionProvider.getSimplifiedName(node.getFirstChild())); checkArgument(node.isFunction()); function = node; } @@ -357,7 +363,7 @@ abstract static class ClassDefinition extends Definition { protected final Node c; ClassDefinition(Node node, boolean inExterns) { - super(inExterns); + super(inExterns, NameBasedDefinitionProvider.getSimplifiedName(node.getFirstChild())); Preconditions.checkArgument(node.isClass()); c = node; } @@ -414,7 +420,7 @@ static final class AssignmentDefinition extends Definition { private final Node assignment; AssignmentDefinition(Node node, boolean inExterns) { - super(inExterns); + super(inExterns, NameBasedDefinitionProvider.getSimplifiedName(node.getFirstChild())); checkArgument(node.isAssign()); assignment = node; } @@ -470,7 +476,7 @@ static final class ObjectLiteralPropertyDefinition extends Definition { ObjectLiteralPropertyDefinition(Node lit, Node name, Node value, boolean isExtern) { - super(isExtern); + super(isExtern, NameBasedDefinitionProvider.getSimplifiedName(getLValue(name))); this.literal = lit; this.name = name; @@ -485,6 +491,10 @@ public void performRemove(AbstractCompiler compiler) { @Override public Node getLValue() { + return getLValue(name); + } + + private static Node getLValue(Node name) { // TODO(user) revisit: object literal definitions are an example // of definitions whose LHS doesn't correspond to a node that // exists in the AST. We will have to change the return type of @@ -516,7 +526,7 @@ public Node getRValue() { static final class VarDefinition extends Definition { private final Node name; VarDefinition(Node node, boolean inExterns) { - super(inExterns); + super(inExterns, NameBasedDefinitionProvider.getSimplifiedName(node)); checkArgument(NodeUtil.isVarDeclaration(node)); Preconditions.checkArgument(inExterns || node.hasChildren(), "VAR Declaration of %s must be assigned a value.", node.getString()); diff --git a/src/com/google/javascript/jscomp/J2clClinitPrunerPass.java b/src/com/google/javascript/jscomp/J2clClinitPrunerPass.java index 76085e0aa2f..9c88eeabaee 100644 --- a/src/com/google/javascript/jscomp/J2clClinitPrunerPass.java +++ b/src/com/google/javascript/jscomp/J2clClinitPrunerPass.java @@ -20,7 +20,7 @@ import com.google.common.base.Strings; import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; import com.google.javascript.jscomp.NodeTraversal.Callback; -import com.google.javascript.jscomp.NodeTraversal.FunctionCallback; +import com.google.javascript.jscomp.NodeTraversal.ChangeScopeRootCallback; import com.google.javascript.rhino.Node; import java.util.ArrayDeque; import java.util.Deque; @@ -76,10 +76,10 @@ private List getNonNestedParentScopeNodes() { } /** Removes redundant clinit calls inside method body if it is guaranteed to be called earlier. */ - private final class RedundantClinitPruner implements Callback, FunctionCallback { + private final class RedundantClinitPruner implements Callback, ChangeScopeRootCallback { @Override - public void enterFunction(AbstractCompiler compiler, Node fnRoot) { + public void enterChangeScopeRoot(AbstractCompiler compiler, Node root) { // Reset the clinit call tracking when starting over on a new scope. clinitsCalledAtBranch = new HierarchicalSet<>(null); stateStack.clear(); diff --git a/src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java b/src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java index 10436f0e9a5..2bb293dcfc1 100644 --- a/src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java +++ b/src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java @@ -20,18 +20,22 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import com.google.common.collect.HashMultimap; +import com.google.common.collect.Iterables; import com.google.common.collect.LinkedHashMultimap; +import com.google.common.collect.Lists; import com.google.common.collect.Multimap; import com.google.javascript.jscomp.DefinitionsRemover.Definition; import com.google.javascript.jscomp.DefinitionsRemover.ExternalNameOnlyDefinition; import com.google.javascript.jscomp.DefinitionsRemover.UnknownDefinition; import com.google.javascript.jscomp.NodeTraversal.Callback; +import com.google.javascript.jscomp.NodeTraversal.ChangeScopeRootCallback; import com.google.javascript.rhino.JSDocInfo; import com.google.javascript.rhino.Node; -import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import java.util.Set; @@ -44,19 +48,22 @@ * treated as unknowns. * *

This definition simply uses the variable name to determine a new definition site so - * potentially it could return multiple definition sites for a single variable. Although we could + * potentially it could return multiple definition sites for a single variable. Although we could * use the type system to make this more accurate, in practice after disambiguate properties has * run, names are unique enough that this works well enough to accept the performance gain. */ public class NameBasedDefinitionProvider implements DefinitionProvider, CompilerPass { - protected final Multimap nameDefinitionMultimap = LinkedHashMultimap.create(); - protected final Map definitionNodeByDefinitionSite = new LinkedHashMap<>(); - protected final Set definitionNodes = new HashSet<>(); + protected final AbstractCompiler compiler; - protected final boolean allowComplexFunctionDefs; - protected boolean hasProcessBeenRun = false; + protected final Multimap definitionsByName = LinkedHashMultimap.create(); + protected final Map definitionSitesByDefinitionSiteNode = + new LinkedHashMap<>(); + protected final Multimap definitionSitesByScopeNode = HashMultimap.create(); + protected final Set definitionNodes = new HashSet<>(); + protected final boolean allowComplexFunctionDefs; + protected boolean hasProcessBeenRun = false; public NameBasedDefinitionProvider(AbstractCompiler compiler, boolean allowComplexFunctionDefs) { this.compiler = compiler; @@ -66,6 +73,7 @@ public NameBasedDefinitionProvider(AbstractCompiler compiler, boolean allowCompl @Override public void process(Node externs, Node source) { checkState(!hasProcessBeenRun, "The definition provider is already initialized."); + this.hasProcessBeenRun = true; NodeTraversal.traverseEs6(compiler, externs, new DefinitionGatheringCallback(true)); @@ -74,6 +82,20 @@ public void process(Node externs, Node source) { NodeTraversal.traverseEs6(compiler, source, new DefinitionGatheringCallback(false)); } + public void rebuildScopeRoots(List changedScopeRoots, List deletedScopeRoots) { + for (Node scopeRoot : Iterables.concat(deletedScopeRoots, changedScopeRoots)) { + for (DefinitionSite definitionSite : definitionSitesByScopeNode.removeAll(scopeRoot)) { + Definition definition = definitionSite.definition; + definitionNodes.remove(definitionSite.node); + definitionsByName.remove(definition.getSimplifiedName(), definition); + definitionSitesByDefinitionSiteNode.remove(definitionSite.node); + } + } + + DefinitionGatheringCallback cb = new DefinitionGatheringCallback(); + NodeTraversal.traverseEs6ScopeRoots(compiler, null, changedScopeRoots, cb, cb, false); + } + /** @return Whether the node has a JSDoc that actually declares something. */ private boolean jsdocContainsDeclarations(Node node) { JSDocInfo info = node.getJSDocInfo(); @@ -88,7 +110,7 @@ private boolean jsdocContainsDeclarations(Node node) { * {@link PureFunctionIdentifier} and causing unkown side effects from propagating everywhere. * This should probably be solved in one of the following ways instead: * - *

a) Have a pass ealier in the compiler that goes in and removes these stub definitions. + *

a) Have a pass earlier in the compiler that goes in and removes these stub definitions. * *

b) Fix all extern files so that there are no untyped stubs mixed with typed ones and add a * restriction to the compiler to prevent this. @@ -97,21 +119,30 @@ private boolean jsdocContainsDeclarations(Node node) { * should not have to drop definitions itself. */ private void dropUntypedExterns() { - for (String externName : nameDefinitionMultimap.keySet()) { - for (Definition def : new ArrayList(nameDefinitionMultimap.get(externName))) { - if (def instanceof ExternalNameOnlyDefinition) { - Node node = def.getLValue(); - if (!jsdocContainsDeclarations(node)) { - for (Definition prevDef : nameDefinitionMultimap.get(externName)) { - if (prevDef != def && node.matchesQualifiedName(prevDef.getLValue())) { - nameDefinitionMultimap.remove(externName, def); - DefinitionSite site = definitionNodeByDefinitionSite.remove(def.getLValue()); - - // Since it's a stub we know its keyed by the name/getProp node. - checkNotNull(site); - break; - } - } + for (String name : definitionsByName.keySet()) { + for (Definition definition : Lists.newArrayList(definitionsByName.get(name))) { + if (!(definition instanceof ExternalNameOnlyDefinition)) { + continue; + } + Node definitionNode = definition.getLValue(); + if (jsdocContainsDeclarations(definitionNode)) { + continue; + } + + for (Definition previousDefinition : definitionsByName.get(name)) { + if (previousDefinition != definition + && definitionNode.matchesQualifiedName(previousDefinition.getLValue())) { + // *DON'T* remove from definitionNodes since it is desired to retain references to + // stub definitions. + definitionsByName.remove(name, definition); + DefinitionSite definitionSite = + definitionSitesByDefinitionSiteNode.remove(definitionNode); + Node scopeNode = NodeUtil.getEnclosingChangeScopeRoot(definitionNode); + definitionSitesByScopeNode.remove(scopeNode, definitionSite); + + // Since it's a stub we know its keyed by the name/getProp node. + checkNotNull(definitionSite); + break; } } } @@ -119,35 +150,44 @@ private void dropUntypedExterns() { } @Override - public Collection getDefinitionsReferencedAt(Node useSite) { - checkState(hasProcessBeenRun, "The process was not run"); - checkArgument(useSite.isGetProp() || useSite.isName()); - if (definitionNodes.contains(useSite)) { + public Collection getDefinitionsReferencedAt(Node useSiteNode) { + checkState(hasProcessBeenRun, "Hasn't been initialized with process() yet."); + checkArgument(useSiteNode.isGetProp() || useSiteNode.isName()); + + if (definitionNodes.contains(useSiteNode)) { return null; } - if (useSite.isGetProp()) { - String propName = useSite.getLastChild().getString(); + if (useSiteNode.isGetProp()) { + String propName = useSiteNode.getLastChild().getString(); if (propName.equals("apply") || propName.equals("call")) { - useSite = useSite.getFirstChild(); + useSiteNode = useSiteNode.getFirstChild(); } } - String name = getSimplifiedName(useSite); + String name = getSimplifiedName(useSiteNode); if (name != null) { - Collection defs = nameDefinitionMultimap.get(name); - return defs.isEmpty() ? null : defs; + Collection definitions = definitionsByName.get(name); + return definitions.isEmpty() ? null : definitions; } return null; } - private class DefinitionGatheringCallback implements Callback { - private final boolean inExterns; + private class DefinitionGatheringCallback implements Callback, ChangeScopeRootCallback { + + DefinitionGatheringCallback() {} DefinitionGatheringCallback(boolean inExterns) { this.inExterns = inExterns; } + @Override + public void enterChangeScopeRoot(AbstractCompiler compiler, Node root) { + this.inExterns = root.isFromExterns(); + } + + private boolean inExterns; + @Override public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { if (inExterns) { @@ -181,35 +221,35 @@ private void visitExterns(NodeTraversal traversal, Node node, Node parent) { } } - Definition def = DefinitionsRemover.getDefinition(node, true); - if (def != null) { - String name = getSimplifiedName(def.getLValue()); + Definition definition = DefinitionsRemover.getDefinition(node, true); + if (definition != null) { + String name = definition.getSimplifiedName(); if (name != null) { - Node rValue = def.getRValue(); + Node rValue = definition.getRValue(); if ((rValue != null) && !NodeUtil.isImmutableValue(rValue) && !rValue.isFunction()) { // Unhandled complex expression - Definition unknownDef = new UnknownDefinition(def.getLValue(), true); - def = unknownDef; + Definition unknownDefinition = new UnknownDefinition(definition.getLValue(), true); + definition = unknownDefinition; } - addDefinition(name, def, node, traversal); + addDefinition(name, definition, node, traversal); } } } private void visitCode(NodeTraversal traversal, Node node) { - Definition def = DefinitionsRemover.getDefinition(node, false); + Definition definition = DefinitionsRemover.getDefinition(node, false); - if (def != null) { - String name = getSimplifiedName(def.getLValue()); + if (definition != null) { + String name = definition.getSimplifiedName(); if (name != null) { - Node rValue = def.getRValue(); + Node rValue = definition.getRValue(); if (rValue != null && !NodeUtil.isImmutableValue(rValue) && !isKnownFunctionDefinition(rValue)) { // Unhandled complex expression - def = new UnknownDefinition(def.getLValue(), false); + definition = new UnknownDefinition(definition.getLValue(), false); } - addDefinition(name, def, node, traversal); + addDefinition(name, definition, node, traversal); } } } @@ -228,13 +268,22 @@ && isKnownFunctionDefinition(n.getSecondChild()) } } - private void addDefinition(String name, Definition def, Node node, NodeTraversal traversal) { - definitionNodes.add(def.getLValue()); - nameDefinitionMultimap.put(name, def); - definitionNodeByDefinitionSite.put( - node, + private void addDefinition( + String name, Definition definition, Node definitionSiteNode, NodeTraversal traversal) { + Node definitionNode = definition.getLValue(); + + definitionNodes.add(definitionNode); + definitionsByName.put(name, definition); + DefinitionSite definitionSite = new DefinitionSite( - node, def, traversal.getModule(), traversal.inGlobalScope(), def.isExtern())); + definitionSiteNode, + definition, + traversal.getModule(), + traversal.inGlobalScope(), + definition.isExtern()); + definitionSitesByDefinitionSiteNode.put(definitionSiteNode, definitionSite); + Node scopeNode = NodeUtil.getEnclosingChangeScopeRoot(definitionSiteNode); + definitionSitesByScopeNode.put(scopeNode, definitionSite); } /** @@ -245,7 +294,7 @@ private void addDefinition(String name, Definition def, Node node, NodeTraversal *

TODO(user) revisit. it would be helpful to at least use fully qualified names in the case of * namespaces. Might not matter as much if this pass runs after {@link CollapseProperties}. */ - protected static String getSimplifiedName(Node node) { + public static String getSimplifiedName(Node node) { if (node.isName()) { String name = node.getString(); if (name != null && !name.isEmpty()) { @@ -259,20 +308,20 @@ protected static String getSimplifiedName(Node node) { return null; } - /** * Returns the collection of definition sites found during traversal. * * @return definition site collection. */ + @Override public Collection getDefinitionSites() { - checkState(hasProcessBeenRun, "The process was not run"); - return definitionNodeByDefinitionSite.values(); + checkState(hasProcessBeenRun, "Hasn't been initialized with process() yet."); + return definitionSitesByDefinitionSiteNode.values(); } public DefinitionSite getDefinitionForFunction(Node function) { - checkState(hasProcessBeenRun, "The process was not run"); + checkState(hasProcessBeenRun, "Hasn't been initialized with process() yet."); checkState(function.isFunction()); - return definitionNodeByDefinitionSite.get(NodeUtil.getNameNode(function)); + return definitionSitesByDefinitionSiteNode.get(NodeUtil.getNameNode(function)); } } diff --git a/src/com/google/javascript/jscomp/NodeTraversal.java b/src/com/google/javascript/jscomp/NodeTraversal.java index 8511b9ab895..f02f584f77d 100644 --- a/src/com/google/javascript/jscomp/NodeTraversal.java +++ b/src/com/google/javascript/jscomp/NodeTraversal.java @@ -78,9 +78,9 @@ public class NodeTraversal { /** Possible callback for scope entry and exist **/ private ScopedCallback scopeCallback; - /** Callback for passes that iterate over a list of functions */ - public interface FunctionCallback { - void enterFunction(AbstractCompiler compiler, Node fnRoot); + /** Callback for passes that iterate over a list of change scope roots (FUNCTIONs and SCRIPTs) */ + public interface ChangeScopeRootCallback { + void enterChangeScopeRoot(AbstractCompiler compiler, Node root); } /** @@ -466,7 +466,7 @@ public static void traverseEs6ScopeRoots( Node root, List scopeNodes, final Callback cb, - final FunctionCallback fcb, + final ChangeScopeRootCallback changeCallback, final boolean traverseNested) { if (scopeNodes == null) { NodeTraversal.traverseEs6(compiler, root, cb); @@ -475,8 +475,8 @@ public static void traverseEs6ScopeRoots( new MemoizedScopeCreator(new Es6SyntacticScopeCreator(compiler)); for (final Node scopeNode : scopeNodes) { - if (fcb != null) { - fcb.enterFunction(compiler, scopeNode); + if (changeCallback != null) { + changeCallback.enterChangeScopeRoot(compiler, scopeNode); } Callback scb = @@ -624,14 +624,14 @@ public Node getCurrentNode() { * Compiler.reportChangeToEnclosingScope(Node n). */ public static void traverseChangedFunctions( - final AbstractCompiler compiler, final FunctionCallback callback) { + final AbstractCompiler compiler, final ChangeScopeRootCallback callback) { final Node jsRoot = compiler.getJsRoot(); NodeTraversal.traverseEs6(compiler, jsRoot, new AbstractPreOrderCallback() { @Override public final boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { if (NodeUtil.isChangeScopeRoot(n) && compiler.hasScopeChanged(n)) { - callback.enterFunction(compiler, n); + callback.enterChangeScopeRoot(compiler, n); } return true; } diff --git a/src/com/google/javascript/jscomp/UnreachableCodeElimination.java b/src/com/google/javascript/jscomp/UnreachableCodeElimination.java index d1469e5ec7f..2015ed1f739 100644 --- a/src/com/google/javascript/jscomp/UnreachableCodeElimination.java +++ b/src/com/google/javascript/jscomp/UnreachableCodeElimination.java @@ -20,7 +20,7 @@ import com.google.javascript.jscomp.ControlFlowGraph.Branch; import com.google.javascript.jscomp.NodeTraversal.AbstractShallowCallback; -import com.google.javascript.jscomp.NodeTraversal.FunctionCallback; +import com.google.javascript.jscomp.NodeTraversal.ChangeScopeRootCallback; import com.google.javascript.jscomp.graph.DiGraph.DiGraphEdge; import com.google.javascript.jscomp.graph.DiGraph.DiGraphNode; import com.google.javascript.jscomp.graph.GraphReachability; @@ -59,9 +59,9 @@ class UnreachableCodeElimination implements CompilerPass { @Override public void process(Node externs, Node toplevel) { - NodeTraversal.traverseChangedFunctions(compiler, new FunctionCallback() { + NodeTraversal.traverseChangedFunctions(compiler, new ChangeScopeRootCallback() { @Override - public void enterFunction(AbstractCompiler compiler, Node root) { + public void enterChangeScopeRoot(AbstractCompiler compiler, Node root) { // Computes the control flow graph. ControlFlowAnalysis cfa = new ControlFlowAnalysis(compiler, false, false); diff --git a/test/com/google/javascript/jscomp/DefinitionUseSiteFinderTest.java b/test/com/google/javascript/jscomp/DefinitionUseSiteFinderTest.java index 71df35a4d93..264b5a1ff91 100644 --- a/test/com/google/javascript/jscomp/DefinitionUseSiteFinderTest.java +++ b/test/com/google/javascript/jscomp/DefinitionUseSiteFinderTest.java @@ -16,11 +16,14 @@ package com.google.javascript.jscomp; +import static com.google.common.truth.Truth.assertThat; + import com.google.common.collect.ImmutableSet; import com.google.common.collect.Multiset; import com.google.common.collect.TreeMultiset; import com.google.javascript.jscomp.DefinitionsRemover.Definition; import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; +import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; import java.util.Collection; import java.util.Set; @@ -402,6 +405,81 @@ public void testDoubleNamedFunction() { "USE NAME f_d -> [FUNCTION]")); } + public void testGetChangesAndDeletions_changeDoesntOverrideDelete() { + Compiler compiler = new Compiler(); + DefinitionUseSiteFinder definitionsFinder = new DefinitionUseSiteFinder(compiler); + definitionsFinder.process(IR.root(), IR.root()); + + Node script = + compiler.parseSyntheticCode( + LINE_JOINER.join( + "function foo() {", + " foo.propOfFoo = 'asdf';", + "}", + "function bar() {", + " bar.propOfBar = 'asdf';", + "}")); + Node root = IR.root(script); + Node externs = IR.root(IR.script()); + IR.root(externs, root); // Create global root. + Node functionFoo = script.getFirstChild(); + Node functionBar = script.getSecondChild(); + + // Verify original baseline. + buildFound(definitionsFinder, found); + assertThat(found).isEmpty(); + + // Verify the fully processed state. + compiler.getChangedScopeNodesForPass("definitionsFinder"); + compiler.getDeletedScopeNodesForPass("definitionsFinder"); + definitionsFinder = new DefinitionUseSiteFinder(compiler); + definitionsFinder.process(externs, root); + buildFound(definitionsFinder, found); + assertThat(found) + .containsExactly( + "DEF NAME foo -> FUNCTION", + "DEF GETPROP foo.propOfFoo -> STRING", + "USE NAME foo -> [FUNCTION]", + "DEF NAME bar -> FUNCTION", + "DEF GETPROP bar.propOfBar -> STRING", + "USE NAME bar -> [FUNCTION]"); + + // Change nothing and re-verify state. + definitionsFinder.rebuildScopeRoots( + compiler.getChangedScopeNodesForPass("definitionsFinder"), + compiler.getDeletedScopeNodesForPass("definitionsFinder")); + buildFound(definitionsFinder, found); + assertThat(found) + .containsExactly( + "DEF NAME foo -> FUNCTION", + "DEF GETPROP foo.propOfFoo -> STRING", + "USE NAME foo -> [FUNCTION]", + "DEF NAME bar -> FUNCTION", + "DEF GETPROP bar.propOfBar -> STRING", + "USE NAME bar -> [FUNCTION]"); + + // Verify state after deleting function "foo". + compiler.reportFunctionDeleted(functionFoo); + definitionsFinder.rebuildScopeRoots( + compiler.getChangedScopeNodesForPass("definitionsFinder"), + compiler.getDeletedScopeNodesForPass("definitionsFinder")); + buildFound(definitionsFinder, found); + assertThat(found) + .containsExactly( + "DEF NAME bar -> FUNCTION", + "DEF GETPROP bar.propOfBar -> STRING", + "USE NAME bar -> [FUNCTION]"); + + // Verify state after changing the contents of function "bar" + functionBar.getLastChild().removeFirstChild(); + compiler.reportChangeToChangeScope(functionBar); + definitionsFinder.rebuildScopeRoots( + compiler.getChangedScopeNodesForPass("definitionsFinder"), + compiler.getDeletedScopeNodesForPass("definitionsFinder")); + buildFound(definitionsFinder, found); + assertThat(found).containsExactly("DEF NAME bar -> FUNCTION"); + } + void checkDefinitionsInExterns(String externs, Set expected) { checkDefinitions(externs, "", expected); } @@ -422,6 +500,8 @@ protected CompilerPass getProcessor(Compiler compiler) { } private static void buildFound(DefinitionUseSiteFinder definitionFinder, Set found) { + found.clear(); + for (DefinitionSite defSite : definitionFinder.getDefinitionSites()) { Node node = defSite.node; Definition definition = defSite.definition; @@ -446,7 +526,7 @@ private static void buildFound(DefinitionUseSiteFinder definitionFinder, Set defs = definitionFinder.getDefinitionsReferencedAt(node); diff --git a/test/com/google/javascript/jscomp/NodeTraversalTest.java b/test/com/google/javascript/jscomp/NodeTraversalTest.java index c137c01be8b..0e134f6b3b5 100644 --- a/test/com/google/javascript/jscomp/NodeTraversalTest.java +++ b/test/com/google/javascript/jscomp/NodeTraversalTest.java @@ -25,7 +25,7 @@ import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.jscomp.NodeTraversal.AbstractNodeTypePruningCallback; import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; -import com.google.javascript.jscomp.NodeTraversal.FunctionCallback; +import com.google.javascript.jscomp.NodeTraversal.ChangeScopeRootCallback; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; @@ -623,7 +623,7 @@ public void testTraverseEs6ScopeRoots_callsEnterFunction() { } private static final class EnterFunctionAccumulator extends AbstractPostOrderCallback - implements FunctionCallback { + implements ChangeScopeRootCallback { List enteredFunctions = new ArrayList<>(); @@ -631,8 +631,8 @@ private static final class EnterFunctionAccumulator extends AbstractPostOrderCal public void visit(NodeTraversal t, Node n, Node parent) {} @Override - public void enterFunction(AbstractCompiler compiler, Node fnRoot) { - enteredFunctions.add(fnRoot); + public void enterChangeScopeRoot(AbstractCompiler compiler, Node root) { + enteredFunctions.add(root); } }