From 9ed8af356c70ec0f2aeb95fc63bd86e8f36ccc07 Mon Sep 17 00:00:00 2001 From: johnlenz Date: Mon, 15 Aug 2016 11:30:29 -0700 Subject: [PATCH] Add support for multiple function definitions and specifically: something ? function() {} : function() {} This allows, goog.string.trim and similar functions to be considered side-effect free and as a result most goog.userAgent methods to be considered side-effect free and removable. This is mostly identical to the original except this version recurses properly to handle multiple levels of HOOK expressions and an additional unit test to cover the failing condition. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=130303575 --- .../google/javascript/jscomp/ChainCalls.java | 3 +- .../jscomp/DefinitionUseSiteFinder.java | 3 +- .../jscomp/MarkNoSideEffectCalls.java | 3 +- .../jscomp/NameBasedDefinitionProvider.java | 89 +++++++++--- .../jscomp/PureFunctionIdentifier.java | 132 ++++++++++++------ .../NameBasedDefinitionProviderTest.java | 3 +- .../jscomp/PureFunctionIdentifierTest.java | 19 ++- 7 files changed, 179 insertions(+), 73 deletions(-) diff --git a/src/com/google/javascript/jscomp/ChainCalls.java b/src/com/google/javascript/jscomp/ChainCalls.java index 3029b74d0aa..bf3696c2eea 100644 --- a/src/com/google/javascript/jscomp/ChainCalls.java +++ b/src/com/google/javascript/jscomp/ChainCalls.java @@ -21,6 +21,7 @@ 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; @@ -45,7 +46,7 @@ class ChainCalls implements CompilerPass { @Override public void process(Node externs, Node root) { - defFinder = new NameBasedDefinitionProvider(compiler); + defFinder = new NameBasedDefinitionProvider(compiler, false); defFinder.process(externs, root); NodeTraversal.traverseEs6(compiler, root, new GatherCallSites()); diff --git a/src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java b/src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java index b1f623287b5..d38d148803d 100644 --- a/src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java +++ b/src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java @@ -22,6 +22,7 @@ import com.google.javascript.jscomp.DefinitionsRemover.Definition; import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; import com.google.javascript.rhino.Node; + import java.util.Collection; /** @@ -35,7 +36,7 @@ public class DefinitionUseSiteFinder extends NameBasedDefinitionProvider { private final Multimap nameUseSiteMultimap; public DefinitionUseSiteFinder(AbstractCompiler compiler) { - super(compiler); + super(compiler, false); this.nameUseSiteMultimap = LinkedHashMultimap.create(); } diff --git a/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java b/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java index c1d8393e2d4..7a62aa8702a 100644 --- a/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java +++ b/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java @@ -21,6 +21,7 @@ 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; @@ -49,7 +50,7 @@ class MarkNoSideEffectCalls implements CompilerPass { @Override public void process(Node externs, Node root) { - NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler); + NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler, false); defFinder.process(externs, root); // Gather the list of function nodes that have @nosideeffects annotations. diff --git a/src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java b/src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java index b626bf3c0df..1e09d70fe9c 100644 --- a/src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java +++ b/src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java @@ -25,6 +25,7 @@ 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; @@ -47,11 +48,14 @@ public class NameBasedDefinitionProvider implements DefinitionProvider, Compiler protected final Multimap nameDefinitionMultimap = LinkedHashMultimap.create(); protected final Map definitionNodeByDefinitionSite = new LinkedHashMap<>(); protected final AbstractCompiler compiler; + protected final boolean allowComplexFunctionDefs; protected boolean hasProcessBeenRun = false; - public NameBasedDefinitionProvider(AbstractCompiler compiler) { + + public NameBasedDefinitionProvider(AbstractCompiler compiler, boolean allowComplexFunctionDefs) { this.compiler = compiler; + this.allowComplexFunctionDefs = allowComplexFunctionDefs; } @Override @@ -112,13 +116,21 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { @Override public void visit(NodeTraversal traversal, Node node, Node parent) { - if (inExterns && node.getJSDocInfo() != null) { + if (inExterns) { + visitExterns(traversal, node, parent); + } else { + visitCode(traversal, node); + } + } + + private void visitExterns(NodeTraversal traversal, Node node, Node parent) { + if (node.getJSDocInfo() != null) { for (Node typeRoot : node.getJSDocInfo().getTypeNodes()) { traversal.traverse(typeRoot); } } - Definition def = DefinitionsRemover.getDefinition(node, inExterns); + Definition def = DefinitionsRemover.getDefinition(node, true); if (def != null) { String name = getSimplifiedName(def.getLValue()); if (name != null) { @@ -126,27 +138,26 @@ public void visit(NodeTraversal traversal, Node node, Node parent) { if ((rValue != null) && !NodeUtil.isImmutableValue(rValue) && !rValue.isFunction()) { // Unhandled complex expression - Definition unknownDef = new UnknownDefinition(def.getLValue(), inExterns); + Definition unknownDef = new UnknownDefinition(def.getLValue(), true); 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); - } + + // 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); } } } @@ -156,11 +167,11 @@ public void visit(NodeTraversal traversal, Node node, Node parent) { definitionNodeByDefinitionSite.put( node, new DefinitionSite( - node, def, traversal.getModule(), traversal.inGlobalScope(), inExterns)); + node, def, traversal.getModule(), traversal.inGlobalScope(), true)); } } - if (inExterns && (parent != null) && parent.isExprResult()) { + if (parent != null && parent.isExprResult()) { String name = getSimplifiedName(node); if (name != null) { @@ -189,12 +200,46 @@ public void visit(NodeTraversal traversal, Node node, Node parent) { definitionNodeByDefinitionSite.put( node, new DefinitionSite( - node, definition, traversal.getModule(), traversal.inGlobalScope(), inExterns)); + node, definition, traversal.getModule(), traversal.inGlobalScope(), true)); + } + } + } + } + + private void visitCode(NodeTraversal traversal, Node node) { + Definition def = DefinitionsRemover.getDefinition(node, false); + if (def != null) { + String name = getSimplifiedName(def.getLValue()); + if (name != null) { + Node rValue = def.getRValue(); + if (rValue != null + && !NodeUtil.isImmutableValue(rValue) + && !isKnownFunctionDefinition(rValue)) { + // Unhandled complex expression + def = new UnknownDefinition(def.getLValue(), false); } + nameDefinitionMultimap.put(name, def); + definitionNodeByDefinitionSite.put( + node, + new DefinitionSite( + node, def, traversal.getModule(), traversal.inGlobalScope(), false)); } } } + boolean isKnownFunctionDefinition(Node n) { + switch (n.getToken()) { + case FUNCTION: + return true; + case HOOK: + return allowComplexFunctionDefs + && isKnownFunctionDefinition(n.getSecondChild()) + && isKnownFunctionDefinition(n.getLastChild()); + default: + return false; + } + } + /** @return Whether the node has a JSDoc that actually declares something. */ private boolean jsdocContainsDeclarations(Node node) { JSDocInfo info = node.getJSDocInfo(); diff --git a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java index 58e252de108..7c1b4f63d2f 100644 --- a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java +++ b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java @@ -35,6 +35,7 @@ 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; @@ -73,8 +74,7 @@ class PureFunctionIdentifier implements CompilerPass { private Node externs; private Node root; - public PureFunctionIdentifier(AbstractCompiler compiler, - DefinitionProvider definitionProvider) { + public PureFunctionIdentifier(AbstractCompiler compiler, DefinitionProvider definitionProvider) { this.compiler = compiler; this.definitionProvider = definitionProvider; this.functionSideEffectMap = new HashMap<>(); @@ -174,7 +174,7 @@ String getDebugReport() { * @param name Query node * @return non-empty definition list or null */ - private static Collection getCallableDefinitions( + private Collection getCallableDefinitions( DefinitionProvider definitionProvider, Node name) { if (name.isGetProp() || name.isName()) { List result = new ArrayList<>(); @@ -187,7 +187,7 @@ private static Collection getCallableDefinitions( for (Definition current : decls) { Node rValue = current.getRValue(); - if ((rValue != null) && rValue.isFunction()) { + if (rValue != null && isSupportedFunctionDefinition(rValue)) { result.add(current); } else { return null; @@ -203,10 +203,8 @@ private static Collection getCallableDefinitions( firstVal = name.getFirstChild(); } - Collection defs1 = getCallableDefinitions(definitionProvider, - firstVal); - Collection defs2 = getCallableDefinitions(definitionProvider, - firstVal.getNext()); + Collection defs1 = getCallableDefinitions(definitionProvider, firstVal); + Collection defs2 = getCallableDefinitions(definitionProvider, firstVal.getNext()); if (defs1 != null && defs2 != null) { defs1.addAll(defs2); return defs1; @@ -224,13 +222,24 @@ private static Collection getCallableDefinitions( // child of a call and thus the function expression // definition will never be an extern. return ImmutableList.of( - (Definition) - new DefinitionsRemover.FunctionExpressionDefinition(name, false)); + (Definition) new DefinitionsRemover.FunctionExpressionDefinition(name, false)); } else { return null; } } + boolean isSupportedFunctionDefinition(Node n) { + switch (n.getToken()) { + case FUNCTION: + return true; + case HOOK: + return isSupportedFunctionDefinition(n.getSecondChild()) + && isSupportedFunctionDefinition(n.getLastChild()); + default: + return false; + } + } + /** * Propagate side effect information by building a graph based on * call site information stored in FunctionInformation and the @@ -272,9 +281,7 @@ private void propagateSideEffects() { for (Definition def : defs) { Node defValue = def.getRValue(); - FunctionInformation dep = functionSideEffectMap.get(defValue); - Preconditions.checkNotNull(dep); - sideEffectGraph.connect(dep.graphNode, callSite, functionInfo.graphNode); + connectDefs(sideEffectGraph, defValue, callSite, functionInfo.graphNode); } } } @@ -291,6 +298,24 @@ private void propagateSideEffects() { } } + private void connectDefs( + LinkedDirectedGraph sideEffectGraph, Node def, Node callSite, + DiGraphNode graphNode) { + switch(def.getToken()) { + case FUNCTION: + FunctionInformation info = functionSideEffectMap.get(def); + Preconditions.checkNotNull(info); + sideEffectGraph.connect(info.graphNode, callSite, graphNode); + break; + case HOOK: + connectDefs(sideEffectGraph, def.getSecondChild(), callSite, graphNode); + connectDefs(sideEffectGraph, def.getLastChild(), callSite, graphNode); + break; + default: + throw new IllegalStateException("Unexpect definition node " + def); + } + } + /** * Set no side effect property at pure-function call sites. */ @@ -310,36 +335,7 @@ private void markPureFunctionCalls() { } else { flags.clearAllFlags(); for (Definition def : defs) { - FunctionInformation functionInfo = - functionSideEffectMap.get(def.getRValue()); - Preconditions.checkNotNull(functionInfo); - if (functionInfo.mutatesGlobalState()) { - flags.setMutatesGlobalState(); - } - - if (functionInfo.mutatesArguments()) { - flags.setMutatesArguments(); - } - - if (functionInfo.functionThrows()) { - flags.setThrows(); - } - - if (!callNode.isNew()) { - if (functionInfo.taintsThis()) { - // A FunctionInfo for "f" maps to both "f()" and "f.call()" nodes. - if (isCallOrApply(callNode)) { - flags.setMutatesArguments(); - } else { - flags.setMutatesThis(); - } - } - } - - if (functionInfo.taintsReturn()) { - flags.setReturnsTainted(); - } - + updateFlagsForDefs(callNode, def.getRValue(), flags); if (flags.areAllFlagsSet()) { break; } @@ -363,6 +359,52 @@ private void markPureFunctionCalls() { } } + private void updateFlagsForDefs(Node callNode, Node defNode, Node.SideEffectFlags flags) { + switch(defNode.getToken()) { + case FUNCTION: + updateFlagsForDef(callNode, defNode, flags); + break; + case HOOK: + updateFlagsForDefs(callNode, defNode.getSecondChild(), flags); + updateFlagsForDefs(callNode, defNode.getLastChild(), flags); + break; + default: + throw new IllegalStateException("Unexpect definition node " + defNode); + } + } + + private void updateFlagsForDef(Node callNode, Node defNode, Node.SideEffectFlags flags) { + FunctionInformation functionInfo = + functionSideEffectMap.get(defNode); + Preconditions.checkNotNull(functionInfo); + if (functionInfo.mutatesGlobalState()) { + flags.setMutatesGlobalState(); + } + + if (functionInfo.mutatesArguments()) { + flags.setMutatesArguments(); + } + + if (functionInfo.functionThrows()) { + flags.setThrows(); + } + + if (!callNode.isNew()) { + if (functionInfo.taintsThis()) { + // A FunctionInfo for "f" maps to both "f()" and "f.call()" nodes. + if (isCallOrApply(callNode)) { + flags.setMutatesArguments(); + } else { + flags.setMutatesThis(); + } + } + } + + if (functionInfo.taintsReturn()) { + flags.setReturnsTainted(); + } + } + private Collection getGoogCacheCallableDefinitions( DefinitionProvider definitionProvider, Cache cacheCall) { Preconditions.checkNotNull(cacheCall); @@ -810,7 +852,7 @@ private static boolean isCallOrApply(Node callSite) { * list of calls that appear in a function's body. */ private static class FunctionInformation { - private DiGraphNode graphNode; + private DiGraphNode graphNode; private List callsInFunctionBody = null; private Set blacklisted = null; private Set taintedLocals = null; @@ -1088,7 +1130,7 @@ static class Driver implements CompilerPass { @Override public void process(Node externs, Node root) { - NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler); + NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler, true); defFinder.process(externs, root); PureFunctionIdentifier pureFunctionIdentifier = diff --git a/test/com/google/javascript/jscomp/NameBasedDefinitionProviderTest.java b/test/com/google/javascript/jscomp/NameBasedDefinitionProviderTest.java index add286cf65f..eadac983656 100644 --- a/test/com/google/javascript/jscomp/NameBasedDefinitionProviderTest.java +++ b/test/com/google/javascript/jscomp/NameBasedDefinitionProviderTest.java @@ -22,6 +22,7 @@ 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.Set; import java.util.TreeSet; @@ -377,7 +378,7 @@ private class DefinitionEnumerator private final Compiler compiler; DefinitionEnumerator(Compiler compiler) { - this.passUnderTest = new NameBasedDefinitionProvider(compiler); + this.passUnderTest = new NameBasedDefinitionProvider(compiler, true); this.compiler = compiler; } diff --git a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java index cba036e83cc..9731443fa28 100644 --- a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java +++ b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java @@ -946,6 +946,7 @@ public void testHookOperator1() throws Exception { } public void testHookOperator2() throws Exception { + checkMarkedCalls("var f = true ? function(){} : externNsef2;\n" + "f()", ImmutableList.of()); @@ -960,7 +961,21 @@ public void testHookOperator3() throws Exception { public void testHookOperators4() throws Exception { checkMarkedCalls("var f = true ? function(){} : function(){};\n" + "f()", - ImmutableList.of()); + ImmutableList.of("f")); + } + + public void testHookOperators5() throws Exception { + checkMarkedCalls(LINE_JOINER.join( + "var f = String.prototype.trim ? function(str){return str} : function(){};", + "f()"), + ImmutableList.of("f")); + } + + public void testHookOperators6() throws Exception { + checkMarkedCalls(LINE_JOINER.join( + "var f = yyy ? function(str){return str} : xxx ? function() {} : function(){};", + "f()"), + ImmutableList.of("f")); } public void testThrow1() throws Exception { @@ -1399,7 +1414,7 @@ private class NoSideEffectCallEnumerator public void process(Node externs, Node root) { compiler.setHasRegExpGlobalReferences(regExpHaveSideEffects); compiler.getOptions().setUseTypesForOptimization(true); - NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler); + NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler, true); defFinder.process(externs, root); PureFunctionIdentifier passUnderTest = new PureFunctionIdentifier(compiler, defFinder);