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

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());

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<String, UseSite> nameUseSiteMultimap;
 
   public DefinitionUseSiteFinder(AbstractCompiler compiler) {
-    super(compiler);
+    super(compiler, false);
     this.nameUseSiteMultimap = LinkedHashMultimap.create();
   }
 

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.

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<String, Definition> nameDefinitionMultimap = LinkedHashMultimap.create();
   protected final Map<Node, DefinitionSite> 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,41 +116,48 @@ 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) {
           Node rValue = def.getRValue();
           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();