From df94bcc677e0e59670c25aa0d9132188286fcea1 Mon Sep 17 00:00:00 2001 From: tdeegan Date: Tue, 26 Jul 2016 14:20:58 -0700 Subject: [PATCH] SimpleDefinitionFinder is *not* simple and does more than it needs to in some cases. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=128513771 --- .../javascript/jscomp/AbstractCompiler.java | 4 +- .../google/javascript/jscomp/CallGraph.java | 10 +- .../google/javascript/jscomp/ChainCalls.java | 5 +- .../google/javascript/jscomp/Compiler.java | 6 +- .../jscomp/DefinitionUseSiteFinder.java | 194 +++++++ .../jscomp/DevirtualizePrototypeMethods.java | 12 +- .../javascript/jscomp/InlineFunctions.java | 2 +- .../jscomp/MarkNoSideEffectCalls.java | 7 +- .../jscomp/NameBasedDefinitionProvider.java | 248 +++++++++ .../google/javascript/jscomp/NodeUtil.java | 40 ++ .../ObjectPropertyStringPostprocess.java | 2 +- .../ObjectPropertyStringPreprocess.java | 6 +- .../javascript/jscomp/OptimizeCalls.java | 6 +- .../javascript/jscomp/OptimizeParameters.java | 24 +- .../javascript/jscomp/OptimizeReturns.java | 10 +- .../jscomp/PureFunctionIdentifier.java | 5 +- .../javascript/jscomp/RemoveUnusedVars.java | 14 +- .../jscomp/SimpleDefinitionFinder.java | 480 ------------------ ...a => NameBasedDefinitionProviderTest.java} | 16 +- .../javascript/jscomp/OptimizeCallsTest.java | 22 +- .../jscomp/PeepholeRemoveDeadCodeTest.java | 4 +- .../jscomp/PureFunctionIdentifierTest.java | 3 +- .../jscomp/RemoveUnusedVarsTest.java | 2 +- 23 files changed, 560 insertions(+), 562 deletions(-) create mode 100644 src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java create mode 100644 src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java delete mode 100644 src/com/google/javascript/jscomp/SimpleDefinitionFinder.java rename test/com/google/javascript/jscomp/{SimpleDefinitionFinderTest.java => NameBasedDefinitionProviderTest.java} (96%) diff --git a/src/com/google/javascript/jscomp/AbstractCompiler.java b/src/com/google/javascript/jscomp/AbstractCompiler.java index 2c9bd640540..e53980f0826 100644 --- a/src/com/google/javascript/jscomp/AbstractCompiler.java +++ b/src/com/google/javascript/jscomp/AbstractCompiler.java @@ -186,9 +186,9 @@ public abstract class AbstractCompiler implements SourceExcerptProvider { * optimize-parameters, remove-unused-variables), to avoid having them * recompute it independently. */ - abstract SimpleDefinitionFinder getSimpleDefinitionFinder(); + abstract DefinitionUseSiteFinder getSimpleDefinitionFinder(); - abstract void setSimpleDefinitionFinder(SimpleDefinitionFinder defFinder); + abstract void setSimpleDefinitionFinder(DefinitionUseSiteFinder defFinder); /** * Parses code for injecting. diff --git a/src/com/google/javascript/jscomp/CallGraph.java b/src/com/google/javascript/jscomp/CallGraph.java index 32bd7e5d7e5..38229c3d673 100644 --- a/src/com/google/javascript/jscomp/CallGraph.java +++ b/src/com/google/javascript/jscomp/CallGraph.java @@ -138,12 +138,12 @@ public CallGraph(AbstractCompiler compiler) { public void process(Node externsRoot, Node jsRoot) { Preconditions.checkState(!alreadyRun); - SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler); - defFinder.process(externsRoot, jsRoot); + DefinitionUseSiteFinder definitionProvider = new DefinitionUseSiteFinder(compiler); + definitionProvider.process(externsRoot, jsRoot); - createFunctionsAndCallsites(jsRoot, defFinder); + createFunctionsAndCallsites(jsRoot, definitionProvider); - fillInFunctionInformation(defFinder); + fillInFunctionInformation(definitionProvider); alreadyRun = true; } @@ -338,7 +338,7 @@ private void connectCallsiteToTargets(Callsite callsite, *

We do this here, rather than when connecting the callgraph, to make sure that we have * correct information for all functions, rather than just functions that are actually called. */ - private void fillInFunctionInformation(SimpleDefinitionFinder finder) { + private void fillInFunctionInformation(DefinitionUseSiteFinder finder) { for (DefinitionSite definitionSite : finder.getDefinitionSites()) { Definition definition = definitionSite.definition; diff --git a/src/com/google/javascript/jscomp/ChainCalls.java b/src/com/google/javascript/jscomp/ChainCalls.java index 394557174e1..3029b74d0aa 100644 --- a/src/com/google/javascript/jscomp/ChainCalls.java +++ b/src/com/google/javascript/jscomp/ChainCalls.java @@ -21,7 +21,6 @@ import com.google.javascript.jscomp.NodeTraversal.ScopedCallback; import com.google.javascript.jscomp.graph.DiGraph.DiGraphEdge; import com.google.javascript.rhino.Node; - import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; @@ -37,7 +36,7 @@ class ChainCalls implements CompilerPass { private final Set badFunctionNodes = new HashSet<>(); private final Set goodFunctionNodes = new HashSet<>(); private final List callSites = new ArrayList<>(); - private SimpleDefinitionFinder defFinder; + private NameBasedDefinitionProvider defFinder; private GatherFunctions gatherFunctions = new GatherFunctions(); ChainCalls(AbstractCompiler compiler) { @@ -46,7 +45,7 @@ class ChainCalls implements CompilerPass { @Override public void process(Node externs, Node root) { - defFinder = new SimpleDefinitionFinder(compiler); + defFinder = new NameBasedDefinitionProvider(compiler); defFinder.process(externs, root); NodeTraversal.traverseEs6(compiler, root, new GatherCallSites()); diff --git a/src/com/google/javascript/jscomp/Compiler.java b/src/com/google/javascript/jscomp/Compiler.java index a11953bec4a..1dd8eed5ebd 100644 --- a/src/com/google/javascript/jscomp/Compiler.java +++ b/src/com/google/javascript/jscomp/Compiler.java @@ -210,7 +210,7 @@ public SourceFile apply(String filename) { public PerformanceTracker tracker; // Used by optimize-returns, optimize-parameters and remove-unused-variables - private SimpleDefinitionFinder defFinder = null; + private DefinitionUseSiteFinder defFinder = null; // For use by the new type inference private GlobalTypeInfo symbolTable; @@ -1346,12 +1346,12 @@ void setSymbolTable(CompilerPass symbolTable) { } @Override - SimpleDefinitionFinder getSimpleDefinitionFinder() { + DefinitionUseSiteFinder getSimpleDefinitionFinder() { return this.defFinder; } @Override - void setSimpleDefinitionFinder(SimpleDefinitionFinder defFinder) { + void setSimpleDefinitionFinder(DefinitionUseSiteFinder defFinder) { this.defFinder = defFinder; } diff --git a/src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java b/src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java new file mode 100644 index 00000000000..3d96a16cfb2 --- /dev/null +++ b/src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java @@ -0,0 +1,194 @@ +/* + * Copyright 2009 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.javascript.jscomp; + +import com.google.common.base.Preconditions; +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; + +/** + * 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. + * + */ +public class DefinitionUseSiteFinder extends NameBasedDefinitionProvider { + + + private final Multimap nameUseSiteMultimap; + + public DefinitionUseSiteFinder(AbstractCompiler compiler) { + super(compiler); + this.nameUseSiteMultimap = LinkedHashMultimap.create(); + } + + @Override + public void process(Node externs, Node source) { + if (hasProcessBeenRun) { + return; + } + super.process(externs, source); + NodeTraversal.traverseEs6( + compiler, source, new UseSiteGatheringCallback()); + } + + /** + * Returns a collection of use sites that may refer to provided definition. Returns an empty + * collection if the definition is not used anywhere. + * + * @param definition Definition of interest. + * @return use site collection. + */ + public Collection getUseSites(Definition definition) { + Preconditions.checkState(hasProcessBeenRun, "The process was not run"); + String name = getSimplifiedName(definition.getLValue()); + return nameUseSiteMultimap.get(name); + } + + private class UseSiteGatheringCallback extends AbstractPostOrderCallback { + @Override + public void visit(NodeTraversal traversal, Node node, Node parent) { + + Collection defs = getDefinitionsReferencedAt(node); + if (defs == null) { + return; + } + + Definition first = defs.iterator().next(); + + String name = getSimplifiedName(first.getLValue()); + Preconditions.checkNotNull(name); + nameUseSiteMultimap.put( + name, + new UseSite(node, traversal.getScope(), traversal.getModule())); + } + } + + /** + * @param use A use site to check. + * @return Whether the use is a call or new. + */ + static boolean isCallOrNewSite(UseSite use) { + Node call = use.node.getParent(); + if (call == null) { + // The node has been removed from the AST. + return false; + } + // We need to make sure we're dealing with a call to the function we're + // optimizing. If the the first child of the parent is not the site, this + // is a nested call and it's a call to another function. + return NodeUtil.isCallOrNew(call) && call.getFirstChild() == use.node; + } + + boolean canModifyDefinition(Definition definition) { + if (isExported(definition)) { + return false; + } + + // Don't modify unused definitions for two reasons: + // 1) It causes unnecessary churn + // 2) Other definitions might be used to reflect on this one using + // goog.reflect.object (the check for definitions with uses is below). + Collection useSites = getUseSites(definition); + if (useSites.isEmpty()) { + return false; + } + + for (UseSite site : useSites) { + // This catches the case where an object literal in goog.reflect.object + // and a prototype method have the same property name. + + // NOTE(nicksantos): Maps and trogedit both do this by different + // mechanisms. + + Node nameNode = site.node; + Collection singleSiteDefinitions = + getDefinitionsReferencedAt(nameNode); + if (singleSiteDefinitions.size() > 1) { + return false; + } + + Preconditions.checkState(!singleSiteDefinitions.isEmpty()); + Preconditions.checkState(singleSiteDefinitions.contains(definition)); + } + + return true; + } + + /** + * @return Whether the definition is directly exported. + */ + private boolean isExported(Definition definition) { + // Assume an exported method result is used. + Node lValue = definition.getLValue(); + if (lValue == null) { + return true; + } + + String partialName; + if (lValue.isGetProp()) { + partialName = lValue.getLastChild().getString(); + } else if (lValue.isName()) { + partialName = lValue.getString(); + } else { + // GETELEM is assumed to be an export or other expression are unknown + // uses. + return true; + } + + CodingConvention codingConvention = compiler.getCodingConvention(); + return codingConvention.isExported(partialName); + } + + /** + * Traverse a node and its children and remove any references to from + * the structures. + */ + void removeReferences(Node node) { + if (DefinitionsRemover.isDefinitionNode(node)) { + DefinitionSite defSite = definitionSiteMap.get(node); + if (defSite != null) { + Definition def = defSite.definition; + String name = getSimplifiedName(def.getLValue()); + if (name != null) { + this.definitionSiteMap.remove(node); + this.nameDefinitionMultimap.remove(name, node); + } + } + } else { + Node useSite = node; + if (useSite.isGetProp()) { + String propName = useSite.getLastChild().getString(); + if (propName.equals("apply") || propName.equals("call")) { + useSite = useSite.getFirstChild(); + } + } + String name = getSimplifiedName(useSite); + if (name != null) { + this.nameUseSiteMultimap.remove(name, new UseSite(useSite, null, null)); + } + } + + for (Node child : node.children()) { + removeReferences(child); + } + } +} diff --git a/src/com/google/javascript/jscomp/DevirtualizePrototypeMethods.java b/src/com/google/javascript/jscomp/DevirtualizePrototypeMethods.java index 56984e92c79..6b153895950 100644 --- a/src/com/google/javascript/jscomp/DevirtualizePrototypeMethods.java +++ b/src/com/google/javascript/jscomp/DevirtualizePrototypeMethods.java @@ -68,14 +68,14 @@ class DevirtualizePrototypeMethods @Override public void process(Node externs, Node root) { - SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler); + DefinitionUseSiteFinder defFinder = new DefinitionUseSiteFinder(compiler); defFinder.process(externs, root); process(externs, root, defFinder); } @Override public void process( - Node externs, Node root, SimpleDefinitionFinder definitions) { + Node externs, Node root, DefinitionUseSiteFinder definitions) { for (DefinitionSite defSite : definitions.getDefinitionSites()) { rewriteDefinitionIfEligible(defSite, definitions); } @@ -171,14 +171,14 @@ private static String getRewrittenMethodName(String originalMethodName) { * defined in the global scope exactly once. * * Definition and use site information is provided by the - * {@link SimpleDefinitionFinder} passed in as an argument. + * {@link DefinitionUseSiteFinder} passed in as an argument. * * @param defSite definition site to process. * @param defFinder structure that hold Node -> Definition and * Definition -> [UseSite] maps. */ private void rewriteDefinitionIfEligible(DefinitionSite defSite, - SimpleDefinitionFinder defFinder) { + DefinitionUseSiteFinder defFinder) { if (defSite.inExterns || !defSite.inGlobalScope || !isEligibleDefinition(defFinder, defSite)) { @@ -220,7 +220,7 @@ private void rewriteDefinitionIfEligible(DefinitionSite defSite, * choice at each call site. * - Definition must happen in a module loaded before the first use. */ - private boolean isEligibleDefinition(SimpleDefinitionFinder defFinder, + private boolean isEligibleDefinition(DefinitionUseSiteFinder defFinder, DefinitionSite definitionSite) { Definition definition = definitionSite.definition; @@ -305,7 +305,7 @@ private boolean isEligibleDefinition(SimpleDefinitionFinder defFinder, * After: * foo(o, a, b, c) */ - private void rewriteCallSites(SimpleDefinitionFinder defFinder, + private void rewriteCallSites(DefinitionUseSiteFinder defFinder, Definition definition, String newMethodName) { Collection useSites = defFinder.getUseSites(definition); diff --git a/src/com/google/javascript/jscomp/InlineFunctions.java b/src/com/google/javascript/jscomp/InlineFunctions.java index 2930d955b5e..d5a6c6935d5 100644 --- a/src/com/google/javascript/jscomp/InlineFunctions.java +++ b/src/com/google/javascript/jscomp/InlineFunctions.java @@ -590,7 +590,7 @@ private void checkNameUsage(Node n, Node parent) { if (parent.isNew()) { Node target = parent.getFirstChild(); if (target.isName() && target.getString().equals( - SimpleDefinitionFinder.EXTERN_OBJECT_PROPERTY_STRING)) { + NodeUtil.EXTERN_OBJECT_PROPERTY_STRING)) { // This method is going to be replaced so don't inline it anywhere. fs.setInline(false); } diff --git a/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java b/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java index 8839dea6b32..a9027332d89 100644 --- a/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java +++ b/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java @@ -21,7 +21,6 @@ import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; import com.google.javascript.rhino.JSDocInfo; import com.google.javascript.rhino.Node; - import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; @@ -50,7 +49,7 @@ class MarkNoSideEffectCalls implements CompilerPass { @Override public void process(Node externs, Node root) { - SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler); + NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler); defFinder.process(externs, root); // Gather the list of function nodes that have @nosideeffects annotations. @@ -158,9 +157,9 @@ public void visit(NodeTraversal traversal, Node node, Node parent) { * refer to function names that are known to have no side effects. */ private class SetNoSideEffectCallProperty extends AbstractPostOrderCallback { - private final SimpleDefinitionFinder defFinder; + private final NameBasedDefinitionProvider defFinder; - SetNoSideEffectCallProperty(SimpleDefinitionFinder defFinder) { + SetNoSideEffectCallProperty(NameBasedDefinitionProvider defFinder) { this.defFinder = defFinder; } diff --git a/src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java b/src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java new file mode 100644 index 00000000000..64eadf6f8a9 --- /dev/null +++ b/src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java @@ -0,0 +1,248 @@ +/* + * Copyright 2016 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.javascript.jscomp; + +import com.google.common.base.Preconditions; +import com.google.common.collect.LinkedHashMultimap; +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.rhino.JSDocInfo; +import com.google.javascript.rhino.Node; +import java.util.ArrayList; +import java.util.Collection; +import java.util.LinkedHashMap; +import java.util.Map; + +/** + * Simple name-based definition gatherer that implements {@link DefinitionProvider}. + * + *

It treats all variable writes as happening in the global scope and treats all objects as + * capable of having the same set of properties. The current implementation only handles definitions + * whose right hand side is an immutable value or function expression. All complex definitions are + * 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 + * 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 definitionSiteMap = new LinkedHashMap<>(); + protected final AbstractCompiler compiler; + + protected boolean hasProcessBeenRun = false; + + public NameBasedDefinitionProvider(AbstractCompiler compiler) { + this.compiler = compiler; + } + + @Override + public void process(Node externs, Node source) { + if (hasProcessBeenRun) { + return; + } + this.hasProcessBeenRun = true; + + NodeTraversal.traverseEs6(compiler, externs, new DefinitionGatheringCallback(true)); + NodeTraversal.traverseEs6(compiler, source, new DefinitionGatheringCallback(false)); + } + + @Override + public Collection getDefinitionsReferencedAt(Node useSite) { + if (definitionSiteMap.containsKey(useSite)) { + return null; + } + + Preconditions.checkState(hasProcessBeenRun, "The process was not run"); + if (useSite.isGetProp()) { + String propName = useSite.getLastChild().getString(); + if (propName.equals("apply") || propName.equals("call")) { + useSite = useSite.getFirstChild(); + } + } + + String name = getSimplifiedName(useSite); + if (name != null) { + Collection defs = nameDefinitionMultimap.get(name); + if (!defs.isEmpty()) { + return defs; + } else { + return null; + } + } else { + return null; + } + } + + private class DefinitionGatheringCallback implements Callback { + private final boolean inExterns; + + DefinitionGatheringCallback(boolean inExterns) { + this.inExterns = inExterns; + } + + @Override + public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { + if (inExterns) { + if (n.isFunction() && !n.getFirstChild().isName()) { + // No need to crawl functions in JSDoc + return false; + } + if (parent != null && parent.isFunction() && n != parent.getFirstChild()) { + // Arguments of external functions should not count as name + // definitions. They are placeholder names for documentation + // purposes only which are not reachable from anywhere. + return false; + } + } + return true; + } + + @Override + public void visit(NodeTraversal traversal, Node node, Node parent) { + if (inExterns && node.getJSDocInfo() != null) { + for (Node typeRoot : node.getJSDocInfo().getTypeNodes()) { + traversal.traverse(typeRoot); + } + } + + Definition def = DefinitionsRemover.getDefinition(node, inExterns); + if (def != null) { + String name = getSimplifiedName(def.getLValue()); + if (name != null) { + Node rValue = def.getRValue(); + if ((rValue != null) && !NodeUtil.isImmutableValue(rValue) && !rValue.isFunction()) { + + // Unhandled complex expression + Definition unknownDef = new UnknownDefinition(def.getLValue(), inExterns); + def = unknownDef; + } + + // TODO(johnlenz) : remove this stub dropping code if it becomes + // illegal to have untyped stubs in the externs definitions. + if (inExterns) { + // We need special handling of untyped externs stubs here: + // the stub should be dropped if the name is provided elsewhere. + + // If there is no qualified name for this, then there will be + // no stubs to remove. This will happen if node is an object + // literal key. + if (node.isQualifiedName()) { + for (Definition prevDef : new ArrayList<>(nameDefinitionMultimap.get(name))) { + if (prevDef instanceof ExternalNameOnlyDefinition + && !jsdocContainsDeclarations(node)) { + if (node.matchesQualifiedName(prevDef.getLValue())) { + // Drop this stub, there is a real definition. + nameDefinitionMultimap.remove(name, prevDef); + } + } + } + } + } + + nameDefinitionMultimap.put(name, def); + definitionSiteMap.put( + node, + new DefinitionSite( + node, def, traversal.getModule(), traversal.inGlobalScope(), inExterns)); + } + } + + if (inExterns && (parent != null) && parent.isExprResult()) { + String name = getSimplifiedName(node); + if (name != null) { + + // TODO(johnlenz) : remove this code if it becomes illegal to have + // stubs in the externs definitions. + + // We need special handling of untyped externs stubs here: + // the stub should be dropped if the name is provided elsewhere. + // We can't just drop the stub now as it needs to be used as the + // externs definition if no other definition is provided. + + boolean dropStub = false; + if (!jsdocContainsDeclarations(node) && node.isQualifiedName()) { + for (Definition prevDef : nameDefinitionMultimap.get(name)) { + if (node.matchesQualifiedName(prevDef.getLValue())) { + dropStub = true; + break; + } + } + } + + if (!dropStub) { + // Incomplete definition + Definition definition = new ExternalNameOnlyDefinition(node); + nameDefinitionMultimap.put(name, definition); + definitionSiteMap.put( + node, + new DefinitionSite( + node, definition, traversal.getModule(), traversal.inGlobalScope(), inExterns)); + } + } + } + } + + /** @return Whether the node has a JSDoc that actually declares something. */ + private boolean jsdocContainsDeclarations(Node node) { + JSDocInfo info = node.getJSDocInfo(); + return (info != null && info.containsDeclaration()); + } + } + + /** + * Extract a name from a node. In the case of GETPROP nodes, replace the namespace or object + * expression with "this" for simplicity and correctness at the expense of inefficiencies due to + * higher chances of name collisions. + * + *

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 "collapsing properties". + */ + protected static String getSimplifiedName(Node node) { + if (node.isName()) { + String name = node.getString(); + if (name != null && !name.isEmpty()) { + return name; + } else { + return null; + } + } else if (node.isGetProp()) { + return "this." + node.getLastChild().getString(); + } + return null; + } + + + /** + * Returns the collection of definition sites found during traversal. + * + * @return definition site collection. + */ + public Collection getDefinitionSites() { + return definitionSiteMap.values(); + } + + public DefinitionSite getDefinitionForFunction(Node function) { + Preconditions.checkState(hasProcessBeenRun, "The process was not run"); + Preconditions.checkState(function.isFunction()); + return definitionSiteMap.get(NodeUtil.getNameNode(function)); + } +} diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index 0a88df2d66d..314a82accec 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -54,6 +54,9 @@ * @author johnlenz@google.com (John Lenz) */ public final class NodeUtil { + + public static final String EXTERN_OBJECT_PROPERTY_STRING = + "JSCompiler_ObjectPropertyString"; static final long MAX_POSITIVE_INTEGER_NUMBER = 1L << 53; static final String JSC_PROPERTY_NAME_FN = "JSCompiler_renameProperty"; @@ -1637,6 +1640,43 @@ static boolean allResultsMatch(Node n, Predicate p) { } } + /** + * @return Whether the function is defined in a non-aliasing expression. + */ + static boolean isSimpleFunctionDeclaration(Node fn) { + Node parent = fn.getParent(); + Node grandparent = parent.getParent(); + + // Simple definition finder doesn't provide useful results in some + // cases, specifically: + // - functions with recursive definitions + // - functions defined in object literals + // - functions defined in array literals + // Here we defined a set of known function declaration that are 'ok'. + + // Some projects seem to actually define "JSCompiler_renameProperty" + // rather than simply having an extern definition. Don't mess with it. + Node nameNode = getNameNode(fn); + if (nameNode != null + && nameNode.isName()) { + String name = nameNode.getString(); + if (name.equals(JSC_PROPERTY_NAME_FN) + || name.equals(EXTERN_OBJECT_PROPERTY_STRING)) { + return false; + } + } + + // example: function a(){}; + if (isFunctionDeclaration(fn)) { + return true; + } + + // example: a = function(){}; + // example: var a = function(){}; + return fn.getFirstChild().getString().isEmpty() + && (isExprAssign(grandparent) || parent.isName()); + } + enum ValueType { UNDETERMINED, NULL, diff --git a/src/com/google/javascript/jscomp/ObjectPropertyStringPostprocess.java b/src/com/google/javascript/jscomp/ObjectPropertyStringPostprocess.java index 6268acabf73..d00f372295c 100644 --- a/src/com/google/javascript/jscomp/ObjectPropertyStringPostprocess.java +++ b/src/com/google/javascript/jscomp/ObjectPropertyStringPostprocess.java @@ -59,7 +59,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { Node objectName = n.getFirstChild(); if (!objectName.matchesQualifiedName( - SimpleDefinitionFinder.EXTERN_OBJECT_PROPERTY_STRING)) { + NodeUtil.EXTERN_OBJECT_PROPERTY_STRING)) { return; } diff --git a/src/com/google/javascript/jscomp/ObjectPropertyStringPreprocess.java b/src/com/google/javascript/jscomp/ObjectPropertyStringPreprocess.java index af44e61fdab..efadff75a84 100644 --- a/src/com/google/javascript/jscomp/ObjectPropertyStringPreprocess.java +++ b/src/com/google/javascript/jscomp/ObjectPropertyStringPreprocess.java @@ -63,7 +63,7 @@ final class ObjectPropertyStringPreprocess implements CompilerPass { public void process(Node externs, Node root) { addExternDeclaration(externs, IR.var( - IR.name(SimpleDefinitionFinder.EXTERN_OBJECT_PROPERTY_STRING))); + IR.name(NodeUtil.EXTERN_OBJECT_PROPERTY_STRING))); NodeTraversal.traverseEs6(compiler, root, new Callback()); } @@ -80,7 +80,7 @@ private class Callback extends AbstractPostOrderCallback { @Override public void visit(NodeTraversal t, Node n, Node parent) { if (n.matchesQualifiedName(OBJECT_PROPERTY_STRING)) { - Node newName = IR.name(SimpleDefinitionFinder.EXTERN_OBJECT_PROPERTY_STRING); + Node newName = IR.name(NodeUtil.EXTERN_OBJECT_PROPERTY_STRING); newName.useSourceInfoIfMissingFrom(n); parent.replaceChild(n, newName); compiler.reportCodeChange(); @@ -96,7 +96,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { Node objectName = n.getFirstChild(); - if (!objectName.matchesQualifiedName(SimpleDefinitionFinder.EXTERN_OBJECT_PROPERTY_STRING)) { + if (!objectName.matchesQualifiedName(NodeUtil.EXTERN_OBJECT_PROPERTY_STRING)) { return; } diff --git a/src/com/google/javascript/jscomp/OptimizeCalls.java b/src/com/google/javascript/jscomp/OptimizeCalls.java index cdb2f950c08..4a218103879 100644 --- a/src/com/google/javascript/jscomp/OptimizeCalls.java +++ b/src/com/google/javascript/jscomp/OptimizeCalls.java @@ -23,7 +23,7 @@ /** * A root pass that container for other passes that should run on - * with a single call graph (currently a SimpleDefinitionFinder). + * with a single call graph (currently a DefinitionUseSiteFinder). * Expected passes include: * - optimize parameters * - optimize returns @@ -45,13 +45,13 @@ OptimizeCalls addPass(CallGraphCompilerPass pass) { } interface CallGraphCompilerPass { - void process(Node externs, Node root, SimpleDefinitionFinder definitions); + void process(Node externs, Node root, DefinitionUseSiteFinder definitions); } @Override public void process(Node externs, Node root) { if (!passes.isEmpty()) { - SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler); + DefinitionUseSiteFinder defFinder = new DefinitionUseSiteFinder(compiler); compiler.setSimpleDefinitionFinder(defFinder); defFinder.process(externs, root); for (CallGraphCompilerPass pass : passes) { diff --git a/src/com/google/javascript/jscomp/OptimizeParameters.java b/src/com/google/javascript/jscomp/OptimizeParameters.java index d68367e44fd..3660c04e84d 100644 --- a/src/com/google/javascript/jscomp/OptimizeParameters.java +++ b/src/com/google/javascript/jscomp/OptimizeParameters.java @@ -55,14 +55,14 @@ class OptimizeParameters public void process(Node externs, Node root) { Preconditions.checkState( compiler.getLifeCycleStage() == LifeCycleStage.NORMALIZED); - SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler); + DefinitionUseSiteFinder defFinder = new DefinitionUseSiteFinder(compiler); defFinder.process(externs, root); process(externs, root, defFinder); } @Override public void process( - Node externs, Node root, SimpleDefinitionFinder definitions) { + Node externs, Node root, DefinitionUseSiteFinder definitions) { for (DefinitionSite defSite : definitions.getDefinitionSites()) { if (canChangeSignature(defSite, definitions)) { tryEliminateConstantArgs(defSite, definitions); @@ -82,7 +82,7 @@ public void process( * signature can be modified. */ private static boolean canChangeSignature( - DefinitionSite definitionSite, SimpleDefinitionFinder defFinder) { + DefinitionSite definitionSite, DefinitionUseSiteFinder defFinder) { Definition definition = definitionSite.definition; if (definitionSite.inExterns) { @@ -112,7 +112,7 @@ private static boolean canChangeSignature( // maps of functions use in for-in expressions, etc). // Be conservative, don't try to optimize any declaration that isn't as // simple function declaration or assignment. - if (!SimpleDefinitionFinder.isSimpleFunctionDeclaration(rValue)) { + if (!NodeUtil.isSimpleFunctionDeclaration(rValue)) { return false; } @@ -132,7 +132,7 @@ private static boolean canChangeSignature( // change the function signature, if all the aliases can't also be // changed. // TODO(johnlenz): Support .call signature changes. - if (!SimpleDefinitionFinder.isCallOrNewSite(site)) { + if (!DefinitionUseSiteFinder.isCallOrNewSite(site)) { return false; } @@ -157,7 +157,7 @@ private static boolean canChangeSignature( * Removes any optional parameters if no callers specifies it as an argument. */ private void tryEliminateOptionalArgs( - DefinitionSite defSite, SimpleDefinitionFinder defFinder) { + DefinitionSite defSite, DefinitionUseSiteFinder defFinder) { // Count the maximum number of arguments passed into this function all // all points of the program. int maxArgs = -1; @@ -165,7 +165,7 @@ private void tryEliminateOptionalArgs( Definition definition = defSite.definition; Collection useSites = defFinder.getUseSites(definition); for (UseSite site : useSites) { - Preconditions.checkState(SimpleDefinitionFinder.isCallOrNewSite(site)); + Preconditions.checkState(DefinitionUseSiteFinder.isCallOrNewSite(site)); Node call = site.node.getParent(); int numArgs = call.getChildCount() - 1; @@ -189,7 +189,7 @@ private void tryEliminateOptionalArgs( * foo(3); */ private void tryEliminateConstantArgs( - DefinitionSite defSite, SimpleDefinitionFinder defFinder) { + DefinitionSite defSite, DefinitionUseSiteFinder defFinder) { List parameters = new ArrayList<>(); boolean firstCall = true; @@ -199,7 +199,7 @@ private void tryEliminateConstantArgs( Collection useSites = defFinder.getUseSites(definition); boolean continueLooking = false; for (UseSite site : useSites) { - Preconditions.checkState(SimpleDefinitionFinder.isCallOrNewSite(site)); + Preconditions.checkState(DefinitionUseSiteFinder.isCallOrNewSite(site)); Node call = site.node.getParent(); Node cur = call.getFirstChild(); @@ -223,7 +223,7 @@ private void tryEliminateConstantArgs( // Remove the constant parameters in all the calls for (UseSite site : useSites) { - Preconditions.checkState(SimpleDefinitionFinder.isCallOrNewSite(site)); + Preconditions.checkState(DefinitionUseSiteFinder.isCallOrNewSite(site)); Node call = site.node.getParent(); optimizeCallSite(defFinder, parameters, call); @@ -400,7 +400,7 @@ private void optimizeFunctionDefinition(List parameters, } private void optimizeCallSite( - SimpleDefinitionFinder defFinder, List parameters, Node call) { + DefinitionUseSiteFinder defFinder, List parameters, Node call) { boolean mayMutateArgs = call.mayMutateArguments(); boolean mayMutateGlobalsOrThrow = call.mayMutateGlobalStateOrThrow(); for (int index = parameters.size() - 1; index >= 0; index--) { @@ -547,7 +547,7 @@ private static Node eliminateFunctionParamAt(Node function, int argIndex) { * @return The Node of the argument removed. */ private Node eliminateCallParamAt( - SimpleDefinitionFinder defFinder, Parameter p, Node call, int argIndex) { + DefinitionUseSiteFinder defFinder, Parameter p, Node call, int argIndex) { Preconditions.checkArgument( NodeUtil.isCallOrNew(call), "Node must be a call or new."); diff --git a/src/com/google/javascript/jscomp/OptimizeReturns.java b/src/com/google/javascript/jscomp/OptimizeReturns.java index 1373cba1476..86edb16b68c 100644 --- a/src/com/google/javascript/jscomp/OptimizeReturns.java +++ b/src/com/google/javascript/jscomp/OptimizeReturns.java @@ -48,14 +48,14 @@ class OptimizeReturns @Override @VisibleForTesting public void process(Node externs, Node root) { - SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler); + DefinitionUseSiteFinder defFinder = new DefinitionUseSiteFinder(compiler); defFinder.process(externs, root); process(externs, root, defFinder); } @Override public void process( - Node externs, Node root, SimpleDefinitionFinder definitions) { + Node externs, Node root, DefinitionUseSiteFinder definitions) { // Find all function nodes whose callers ignore the return values. List toOptimize = new ArrayList<>(); for (DefinitionSite defSite : definitions.getDefinitionSites()) { @@ -75,7 +75,7 @@ public void process( * - The definition is never accessed outside a function call context. */ private static boolean callResultsMaybeUsed( - SimpleDefinitionFinder defFinder, DefinitionSite definitionSite) { + DefinitionUseSiteFinder defFinder, DefinitionSite definitionSite) { Definition definition = definitionSite.definition; @@ -87,7 +87,7 @@ private static boolean callResultsMaybeUsed( // Be conservative, don't try to optimize any declaration that isn't as // simple function declaration or assignment. - if (!SimpleDefinitionFinder.isSimpleFunctionDeclaration(rValue)) { + if (!NodeUtil.isSimpleFunctionDeclaration(rValue)) { return true; } @@ -126,7 +126,7 @@ private static boolean callResultsMaybeUsed( * Useless return will be removed later by the peephole optimization passes. */ private void rewriteReturns( - final SimpleDefinitionFinder defFinder, Node fnNode) { + final DefinitionUseSiteFinder defFinder, Node fnNode) { Preconditions.checkState(fnNode.isFunction()); NodeUtil.visitPostOrder( fnNode.getLastChild(), diff --git a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java index 1fd7dade54d..00f75c98b52 100644 --- a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java +++ b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java @@ -35,7 +35,6 @@ import com.google.javascript.rhino.jstype.FunctionType; import com.google.javascript.rhino.jstype.JSType; import com.google.javascript.rhino.jstype.JSTypeNative; - import java.io.File; import java.io.IOException; import java.util.ArrayList; @@ -232,7 +231,7 @@ private static Collection getCallableDefinitions( } } else if (NodeUtil.isFunctionExpression(name)) { // The anonymous function reference is also the definition. - // TODO(user) Change SimpleDefinitionFinder so it is possible to query for + // TODO(user) Change DefinitionUseSiteFinder so it is possible to query for // function expressions by function node. // isExtern is false in the call to the constructor for the @@ -1221,7 +1220,7 @@ static class Driver implements CompilerPass { @Override public void process(Node externs, Node root) { - SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler); + NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler); defFinder.process(externs, root); PureFunctionIdentifier pureFunctionIdentifier = diff --git a/src/com/google/javascript/jscomp/RemoveUnusedVars.java b/src/com/google/javascript/jscomp/RemoveUnusedVars.java index e4fc30deb30..9affca697db 100644 --- a/src/com/google/javascript/jscomp/RemoveUnusedVars.java +++ b/src/com/google/javascript/jscomp/RemoveUnusedVars.java @@ -144,7 +144,7 @@ class RemoveUnusedVars @Override public void process(Node externs, Node root) { Preconditions.checkState(compiler.getLifeCycleStage().isNormalized()); - SimpleDefinitionFinder defFinder = compiler.getSimpleDefinitionFinder(); + DefinitionUseSiteFinder defFinder = compiler.getSimpleDefinitionFinder(); if (this.modifyCallSites) { // When RemoveUnusedVars is run after OptimizeCalls, this.modifyCallSites // is true. But if OptimizeCalls stops making changes, PhaseOptimizer @@ -171,7 +171,7 @@ public void process(Node externs, Node root) { @Override public void process( - Node externs, Node root, SimpleDefinitionFinder defFinder) { + Node externs, Node root, DefinitionUseSiteFinder defFinder) { if (modifyCallSites) { Preconditions.checkNotNull(defFinder); callSiteOptimizer = new CallSiteOptimizer(compiler, defFinder); @@ -433,13 +433,13 @@ private static Node getFunctionArgList(Node function) { private static class CallSiteOptimizer { private final AbstractCompiler compiler; - private final SimpleDefinitionFinder defFinder; + private final DefinitionUseSiteFinder defFinder; private final List toRemove = new ArrayList<>(); private final List toReplaceWithZero = new ArrayList<>(); CallSiteOptimizer( AbstractCompiler compiler, - SimpleDefinitionFinder defFinder) { + DefinitionUseSiteFinder defFinder) { this.compiler = compiler; this.defFinder = defFinder; } @@ -630,7 +630,7 @@ boolean canModifyCallers(Node function) { // Be conservative, don't try to optimize any declaration that isn't as // simple function declaration or assignment. - if (!SimpleDefinitionFinder.isSimpleFunctionDeclaration(function)) { + if (!NodeUtil.isSimpleFunctionDeclaration(function)) { return false; } @@ -642,7 +642,7 @@ boolean canModifyCallers(Node function) { * @return Whether the call site is suitable for modification */ private static boolean isModifiableCallSite(UseSite site) { - return SimpleDefinitionFinder.isCallOrNewSite(site) + return DefinitionUseSiteFinder.isCallOrNewSite(site) && !NodeUtil.isFunctionObjectApply(site.node.getParent()); } @@ -675,7 +675,7 @@ private boolean canChangeSignature(Node function) { } // Accessing the property directly prevents rewrite. - if (!SimpleDefinitionFinder.isCallOrNewSite(site)) { + if (!DefinitionUseSiteFinder.isCallOrNewSite(site)) { if (!(parent.isGetProp() && NodeUtil.isFunctionObjectCall(parent.getParent()))) { return false; diff --git a/src/com/google/javascript/jscomp/SimpleDefinitionFinder.java b/src/com/google/javascript/jscomp/SimpleDefinitionFinder.java deleted file mode 100644 index 9f84e3df8d9..00000000000 --- a/src/com/google/javascript/jscomp/SimpleDefinitionFinder.java +++ /dev/null @@ -1,480 +0,0 @@ -/* - * Copyright 2009 The Closure Compiler Authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.javascript.jscomp; - -import com.google.common.base.Preconditions; -import com.google.common.collect.LinkedHashMultimap; -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.AbstractPostOrderCallback; -import com.google.javascript.jscomp.NodeTraversal.Callback; -import com.google.javascript.rhino.JSDocInfo; -import com.google.javascript.rhino.Node; - -import java.util.ArrayList; -import java.util.Collection; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; - -/** - * Simple name-based definition gatherer that implements - * {@link DefinitionProvider}. - * - * It treats all variable writes as happening in the global scope and - * treats all objects as capable of having the same set of properties. - * The current implementation only handles definitions whose right - * hand side is an immutable value or function expression. All - * complex definitions are treated as unknowns. - * - */ -public class SimpleDefinitionFinder implements CompilerPass, DefinitionProvider { - public static final String EXTERN_OBJECT_PROPERTY_STRING = - "JSCompiler_ObjectPropertyString"; - - private final AbstractCompiler compiler; - private final Map definitionSiteMap; - private final Multimap nameDefinitionMultimap; - private final Multimap nameUseSiteMultimap; - // The same defFinder can be used by multiple passes, but its process method - // must be run only once - private boolean hasProcessBeenRun = false; - - public SimpleDefinitionFinder(AbstractCompiler compiler) { - this.compiler = compiler; - this.definitionSiteMap = new LinkedHashMap<>(); - this.nameDefinitionMultimap = LinkedHashMultimap.create(); - this.nameUseSiteMultimap = LinkedHashMultimap.create(); - } - - /** - * Returns the collection of definition sites found during traversal. - * - * @return definition site collection. - */ - public Collection getDefinitionSites() { - return definitionSiteMap.values(); - } - - private DefinitionSite getDefinitionAt(Node node) { - return definitionSiteMap.get(node); - } - - DefinitionSite getDefinitionForFunction(Node function) { - Preconditions.checkState(function.isFunction()); - return getDefinitionAt(getNameNodeFromFunctionNode(function)); - } - - @Override - public Collection getDefinitionsReferencedAt(Node useSite) { - if (definitionSiteMap.containsKey(useSite)) { - return null; - } - - if (useSite.isGetProp()) { - String propName = useSite.getLastChild().getString(); - if (propName.equals("apply") || propName.equals("call")) { - useSite = useSite.getFirstChild(); - } - } - - String name = getSimplifiedName(useSite); - if (name != null) { - Collection defs = nameDefinitionMultimap.get(name); - if (!defs.isEmpty()) { - return defs; - } else { - return null; - } - } else { - return null; - } - } - - @Override - public void process(Node externs, Node source) { - if (this.hasProcessBeenRun) { - return; - } - this.hasProcessBeenRun = true; - NodeTraversal.traverseEs6( - compiler, externs, new DefinitionGatheringCallback(true)); - NodeTraversal.traverseEs6( - compiler, source, new DefinitionGatheringCallback(false)); - NodeTraversal.traverseEs6( - compiler, source, new UseSiteGatheringCallback()); - } - - /** - * Returns a collection of use sites that may refer to provided - * definition. Returns an empty collection if the definition is not - * used anywhere. - * - * @param definition Definition of interest. - * @return use site collection. - */ - Collection getUseSites(Definition definition) { - String name = getSimplifiedName(definition.getLValue()); - return nameUseSiteMultimap.get(name); - } - - /** - * Extract a name from a node. In the case of GETPROP nodes, - * replace the namespace or object expression with "this" for - * simplicity and correctness at the expense of inefficiencies due - * to higher chances of name collisions. - * - * 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 "collapsing properties". - */ - private static String getSimplifiedName(Node node) { - if (node.isName()) { - String name = node.getString(); - if (name != null && !name.isEmpty()) { - return name; - } else { - return null; - } - } else if (node.isGetProp()) { - return "this." + node.getLastChild().getString(); - } - return null; - } - - private class DefinitionGatheringCallback implements Callback { - private boolean inExterns; - - DefinitionGatheringCallback(boolean inExterns) { - this.inExterns = inExterns; - } - - @Override - public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { - if (inExterns) { - if (n.isFunction() && !n.getFirstChild().isName()) { - // No need to crawl functions in JSDoc - return false; - } - if (parent != null - && parent.isFunction() && n != parent.getFirstChild()) { - // Arguments of external functions should not count as name - // definitions. They are placeholder names for documentation - // purposes only which are not reachable from anywhere. - return false; - } - } - return true; - } - - - @Override - public void visit(NodeTraversal traversal, Node node, Node parent) { - if (inExterns && node.getJSDocInfo() != null) { - for (Node typeRoot : node.getJSDocInfo().getTypeNodes()) { - traversal.traverse(typeRoot); - } - } - - Definition def = - DefinitionsRemover.getDefinition(node, inExterns); - if (def != null) { - String name = getSimplifiedName(def.getLValue()); - if (name != null) { - Node rValue = def.getRValue(); - if ((rValue != null) && - !NodeUtil.isImmutableValue(rValue) && - !rValue.isFunction()) { - - // Unhandled complex expression - Definition unknownDef = - new UnknownDefinition(def.getLValue(), inExterns); - def = unknownDef; - } - - // TODO(johnlenz) : remove this stub dropping code if it becomes - // illegal to have untyped stubs in the externs definitions. - if (inExterns) { - // We need special handling of untyped externs stubs here: - // the stub should be dropped if the name is provided elsewhere. - - List stubsToRemove = new ArrayList<>(); - - // If there is no qualified name for this, then there will be - // no stubs to remove. This will happen if node is an object - // literal key. - if (node.isQualifiedName()) { - for (Definition prevDef : nameDefinitionMultimap.get(name)) { - if (prevDef instanceof ExternalNameOnlyDefinition - && !jsdocContainsDeclarations(node)) { - if (node.matchesQualifiedName(prevDef.getLValue())) { - // Drop this stub, there is a real definition. - stubsToRemove.add(prevDef); - } - } - } - - for (Definition prevDef : stubsToRemove) { - nameDefinitionMultimap.remove(name, prevDef); - } - } - } - - nameDefinitionMultimap.put(name, def); - definitionSiteMap.put(node, - new DefinitionSite(node, - def, - traversal.getModule(), - traversal.inGlobalScope(), - inExterns)); - } - } - - if (inExterns && (parent != null) && parent.isExprResult()) { - String name = getSimplifiedName(node); - if (name != null) { - - // TODO(johnlenz) : remove this code if it becomes illegal to have - // stubs in the externs definitions. - - // We need special handling of untyped externs stubs here: - // the stub should be dropped if the name is provided elsewhere. - // We can't just drop the stub now as it needs to be used as the - // externs definition if no other definition is provided. - - boolean dropStub = false; - if (!jsdocContainsDeclarations(node) && node.isQualifiedName()) { - for (Definition prevDef : nameDefinitionMultimap.get(name)) { - if (node.matchesQualifiedName(prevDef.getLValue())) { - dropStub = true; - break; - } - } - } - - if (!dropStub) { - // Incomplete definition - Definition definition = new ExternalNameOnlyDefinition(node); - nameDefinitionMultimap.put(name, definition); - definitionSiteMap.put(node, - new DefinitionSite(node, - definition, - traversal.getModule(), - traversal.inGlobalScope(), - inExterns)); - } - } - } - } - - /** - * @return Whether the node has a JSDoc that actually declares something. - */ - private boolean jsdocContainsDeclarations(Node node) { - JSDocInfo info = node.getJSDocInfo(); - return (info != null && info.containsDeclaration()); - } - } - - private class UseSiteGatheringCallback extends AbstractPostOrderCallback { - @Override - public void visit(NodeTraversal traversal, Node node, Node parent) { - - Collection defs = getDefinitionsReferencedAt(node); - if (defs == null) { - return; - } - - Definition first = defs.iterator().next(); - - String name = getSimplifiedName(first.getLValue()); - Preconditions.checkNotNull(name); - nameUseSiteMultimap.put( - name, - new UseSite(node, traversal.getScope(), traversal.getModule())); - } - } - - /** - * @param use A use site to check. - * @return Whether the use is a call or new. - */ - static boolean isCallOrNewSite(UseSite use) { - Node call = use.node.getParent(); - if (call == null) { - // The node has been removed from the AST. - return false; - } - // We need to make sure we're dealing with a call to the function we're - // optimizing. If the the first child of the parent is not the site, this - // is a nested call and it's a call to another function. - return NodeUtil.isCallOrNew(call) && call.getFirstChild() == use.node; - } - - boolean canModifyDefinition(Definition definition) { - if (isExported(definition)) { - return false; - } - - // Don't modify unused definitions for two reasons: - // 1) It causes unnecessary churn - // 2) Other definitions might be used to reflect on this one using - // goog.reflect.object (the check for definitions with uses is below). - Collection useSites = getUseSites(definition); - if (useSites.isEmpty()) { - return false; - } - - for (UseSite site : useSites) { - // This catches the case where an object literal in goog.reflect.object - // and a prototype method have the same property name. - - // NOTE(nicksantos): Maps and trogedit both do this by different - // mechanisms. - - Node nameNode = site.node; - Collection singleSiteDefinitions = - getDefinitionsReferencedAt(nameNode); - if (singleSiteDefinitions.size() > 1) { - return false; - } - - Preconditions.checkState(!singleSiteDefinitions.isEmpty()); - Preconditions.checkState(singleSiteDefinitions.contains(definition)); - } - - return true; - } - - /** - * @return Whether the definition is directly exported. - */ - private boolean isExported(Definition definition) { - // Assume an exported method result is used. - Node lValue = definition.getLValue(); - if (lValue == null) { - return true; - } - - String partialName; - if (lValue.isGetProp()) { - partialName = lValue.getLastChild().getString(); - } else if (lValue.isName()) { - partialName = lValue.getString(); - } else { - // GETELEM is assumed to be an export or other expression are unknown - // uses. - return true; - } - - CodingConvention codingConvention = compiler.getCodingConvention(); - return codingConvention.isExported(partialName); - } - - /** - * @return Whether the function is defined in a non-aliasing expression. - */ - static boolean isSimpleFunctionDeclaration(Node fn) { - Node parent = fn.getParent(); - Node grandparent = parent.getParent(); - - // Simple definition finder doesn't provide useful results in some - // cases, specifically: - // - functions with recursive definitions - // - functions defined in object literals - // - functions defined in array literals - // Here we defined a set of known function declaration that are 'ok'. - - // Some projects seem to actually define "JSCompiler_renameProperty" - // rather than simply having an extern definition. Don't mess with it. - Node nameNode = SimpleDefinitionFinder.getNameNodeFromFunctionNode(fn); - if (nameNode != null - && nameNode.isName()) { - String name = nameNode.getString(); - if (name.equals(NodeUtil.JSC_PROPERTY_NAME_FN) - || name.equals(EXTERN_OBJECT_PROPERTY_STRING)) { - return false; - } - } - - // example: function a(){}; - if (NodeUtil.isFunctionDeclaration(fn)) { - return true; - } - - // example: a = function(){}; - // example: var a = function(){}; - return fn.getFirstChild().getString().isEmpty() - && (NodeUtil.isExprAssign(grandparent) || parent.isName()); - } - - /** - * @return the node defining the name for this function (if any). - */ - static Node getNameNodeFromFunctionNode(Node function) { - Preconditions.checkState(function.isFunction()); - if (NodeUtil.isFunctionDeclaration(function)) { - return function.getFirstChild(); - } else { - Node parent = function.getParent(); - if (NodeUtil.isVarDeclaration(parent)) { - return parent; - } else if (parent.isAssign()) { - return parent.getFirstChild(); - } else if (NodeUtil.isObjectLitKey(parent)) { - return parent; - } - } - return null; - } - - /** - * Traverse a node and its children and remove any references to from - * the structures. - */ - void removeReferences(Node node) { - if (DefinitionsRemover.isDefinitionNode(node)) { - DefinitionSite defSite = definitionSiteMap.get(node); - if (defSite != null) { - Definition def = defSite.definition; - String name = getSimplifiedName(def.getLValue()); - if (name != null) { - this.definitionSiteMap.remove(node); - this.nameDefinitionMultimap.remove(name, node); - } - } - } else { - Node useSite = node; - if (useSite.isGetProp()) { - String propName = useSite.getLastChild().getString(); - if (propName.equals("apply") || propName.equals("call")) { - useSite = useSite.getFirstChild(); - } - } - String name = getSimplifiedName(useSite); - if (name != null) { - this.nameUseSiteMultimap.remove(name, new UseSite(useSite, null, null)); - } - } - - for (Node child : node.children()) { - removeReferences(child); - } - } -} diff --git a/test/com/google/javascript/jscomp/SimpleDefinitionFinderTest.java b/test/com/google/javascript/jscomp/NameBasedDefinitionProviderTest.java similarity index 96% rename from test/com/google/javascript/jscomp/SimpleDefinitionFinderTest.java rename to test/com/google/javascript/jscomp/NameBasedDefinitionProviderTest.java index 7c6f24e60a1..1c253509498 100644 --- a/test/com/google/javascript/jscomp/SimpleDefinitionFinderTest.java +++ b/test/com/google/javascript/jscomp/NameBasedDefinitionProviderTest.java @@ -28,10 +28,10 @@ import java.util.TreeSet; /** - * Tests for {@link SimpleDefinitionFinder} + * Tests for {@link DefinitionUseSiteFinder} * */ -public final class SimpleDefinitionFinderTest extends CompilerTestCase { +public final class NameBasedDefinitionProviderTest extends CompilerTestCase { Set found = new TreeSet<>(); @Override @@ -366,19 +366,19 @@ void checkDefinitions(String externs, String source, Set expected) { @Override protected CompilerPass getProcessor(Compiler compiler) { - return new SimpleDefinitionEnumerator(compiler); + return new DefinitionEnumerator(compiler); } /** - * Run SimpleDefinitionFinder, then gather a list of definitions. + * Run DefinitionUseSiteFinder, then gather a list of definitions. */ - private class SimpleDefinitionEnumerator + private class DefinitionEnumerator extends AbstractPostOrderCallback implements CompilerPass { - private final SimpleDefinitionFinder passUnderTest; + private final NameBasedDefinitionProvider passUnderTest; private final Compiler compiler; - SimpleDefinitionEnumerator(Compiler compiler) { - this.passUnderTest = new SimpleDefinitionFinder(compiler); + DefinitionEnumerator(Compiler compiler) { + this.passUnderTest = new NameBasedDefinitionProvider(compiler); this.compiler = compiler; } diff --git a/test/com/google/javascript/jscomp/OptimizeCallsTest.java b/test/com/google/javascript/jscomp/OptimizeCallsTest.java index 3e88088e3eb..b4e4b4c44a2 100644 --- a/test/com/google/javascript/jscomp/OptimizeCallsTest.java +++ b/test/com/google/javascript/jscomp/OptimizeCallsTest.java @@ -35,26 +35,26 @@ protected CompilerPass getProcessor(final Compiler compiler) { @Override public void process(Node externs, Node root) { - new PureFunctionIdentifier(compiler, - new SimpleDefinitionFinder(compiler)).process(externs, root); + DefinitionUseSiteFinder definitionFinder = new DefinitionUseSiteFinder(compiler); + definitionFinder.process(externs, root); + new PureFunctionIdentifier(compiler, definitionFinder).process(externs, root); passes.process(externs, root); } }; } public void testRemovingReturnCallToFunctionWithUnusedParams() { - test("function foo() {var x; return x = bar(1)} foo(); function bar(x) {}", - "function foo() { bar(); return;} foo(); function bar() {}"); + test( + "function foo() {var x; return x = bar(1)} foo(); function bar(x) {}", + "function foo(){return}foo()"); } public void testNestingFunctionCallWithUnsedParams() { - test("function f1(x) { } function f2(x) { }" + - "function f3(x) { } function f4(x) { }" + - "f3(f1(f2()));", - "function f1() {f2()} function f2() { }" + - "function f3() {f1()} " + - "f3();" - ); + test( + "function f1(x) { } function f2(x) { }" + + "function f3(x) { } function f4(x) { }" + + "f3(f1(f2()));", + "function f3(){}f3()"); } public void testUnusedAssignOnFunctionWithUnusedParams() { diff --git a/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java b/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java index 8e0f95fb5dc..4c4fc763dcc 100644 --- a/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java +++ b/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java @@ -39,8 +39,8 @@ public CompilerPass getProcessor(final Compiler compiler) { return new CompilerPass() { @Override public void process(Node externs, Node root) { - SimpleDefinitionFinder definitionFinder = - new SimpleDefinitionFinder(compiler); + DefinitionUseSiteFinder definitionFinder = + new DefinitionUseSiteFinder(compiler); definitionFinder.process(externs, root); new PureFunctionIdentifier(compiler, definitionFinder) .process(externs, root); diff --git a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java index 4e76e64007f..bf8bb055c1c 100644 --- a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java +++ b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java @@ -24,7 +24,6 @@ import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; import com.google.javascript.rhino.Node; - import java.util.ArrayList; import java.util.List; @@ -1399,7 +1398,7 @@ private class NoSideEffectCallEnumerator @Override public void process(Node externs, Node root) { compiler.setHasRegExpGlobalReferences(regExpHaveSideEffects); - SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler); + NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler); defFinder.process(externs, root); PureFunctionIdentifier passUnderTest = new PureFunctionIdentifier(compiler, defFinder); diff --git a/test/com/google/javascript/jscomp/RemoveUnusedVarsTest.java b/test/com/google/javascript/jscomp/RemoveUnusedVarsTest.java index 68c9ad193bd..6e30c6f5f7f 100644 --- a/test/com/google/javascript/jscomp/RemoveUnusedVarsTest.java +++ b/test/com/google/javascript/jscomp/RemoveUnusedVarsTest.java @@ -39,7 +39,7 @@ public void setUp() { @Override protected CompilerPass getProcessor(final Compiler compiler) { if (this.modifyCallSites) { - SimpleDefinitionFinder defFinder = new SimpleDefinitionFinder(compiler); + DefinitionUseSiteFinder defFinder = new DefinitionUseSiteFinder(compiler); compiler.setSimpleDefinitionFinder(defFinder); } return new RemoveUnusedVars(