Some clean ups in PureFunctionIdentifier.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=128831817

src/com/google/javascript/jscomp/CallGraph.java

@@ -495,11 +495,15 @@ private DiGraph<Function, Callsite> constructDirectedGraph(boolean forward) {
    */
   private Collection<Definition> lookupDefinitionsForTargetsOfCall(
       Node callsite, DefinitionProvider definitionProvider) {
-    Preconditions.checkArgument(callsite.isCall()
-        || callsite.isNew());
+    Preconditions.checkArgument(
+        NodeUtil.isCallOrNew(callsite), "Expected CALL or NEW. Got:", callsite);
 
     Node targetExpression = callsite.getFirstChild();
 
+    if (!targetExpression.isName() && !targetExpression.isGetProp()) {
+      return null;
+    }
+
     Collection<Definition> definitions =
         definitionProvider.getDefinitionsReferencedAt(targetExpression);
 

src/com/google/javascript/jscomp/CheckJSDoc.java

@@ -55,6 +55,15 @@ final class CheckJSDoc extends AbstractPostOrderCallback implements HotSwapCompi
       "JSC_DEFAULT_PARAM_MUST_BE_MARKED_OPTIONAL",
       "Inline JSDoc on default parameters must be marked as optional");
 
+  public static final DiagnosticType INVALID_NO_SIDE_EFFECT_ANNOTATION =
+      DiagnosticType.error(
+          "JSC_INVALID_NO_SIDE_EFFECT_ANNOTATION",
+          "@nosideeffects may only appear in externs files.");
+
+  public static final DiagnosticType INVALID_MODIFIES_ANNOTATION =
+      DiagnosticType.error(
+          "JSC_INVALID_MODIFIES_ANNOTATION", "@modifies may only appear in externs files.");
+
   private final AbstractCompiler compiler;
 
   CheckJSDoc(AbstractCompiler compiler) {
@@ -453,8 +462,20 @@ private void validateDefaultValue(Node n, JSDocInfo info) {
    * Check that @nosideeeffects annotations are only present in externs.
    */
   private void validateNoSideEffects(Node n, JSDocInfo info) {
-    if (info != null && info.isNoSideEffects() && !n.isFromExterns()) {
-      reportMisplaced(n, "nosideeffects", "@nosideeffects is only supported in externs.");
+    // Cannot have @modifies or @nosideeffects in regular (non externs) js. Report errors.
+    if (info == null) {
+      return;
+    }
+
+    if (n.isFromExterns()) {
+      return;
+    }
+
+    if (info.hasSideEffectsArgumentsAnnotation() || info.modifiesThis()) {
+      report(n, INVALID_MODIFIES_ANNOTATION);
+    }
+    if (info.isNoSideEffects()) {
+      report(n, INVALID_NO_SIDE_EFFECT_ANNOTATION);
     }
   }
 }

src/com/google/javascript/jscomp/DefinitionUseSiteFinder.java

@@ -62,6 +62,9 @@ public Collection<UseSite> getUseSites(Definition definition) {
   private class UseSiteGatheringCallback extends AbstractPostOrderCallback {
     @Override
     public void visit(NodeTraversal traversal, Node node, Node parent) {
+      if (!node.isGetProp() && !node.isName()) {
+        return;
+      }
 
       Collection<Definition> defs = getDefinitionsReferencedAt(node);
       if (defs == null) {
@@ -160,12 +163,12 @@ private boolean isExported(Definition definition) {
    */
   void removeReferences(Node node) {
     if (DefinitionsRemover.isDefinitionNode(node)) {
-      DefinitionSite defSite = definitionSiteMap.get(node);
+      DefinitionSite defSite = definitionNodeByDefinitionSite.get(node);
       if (defSite != null) {
         Definition def = defSite.definition;
         String name = getSimplifiedName(def.getLValue());
         if (name != null) {
-          this.definitionSiteMap.remove(node);
+          this.definitionNodeByDefinitionSite.remove(node);
           this.nameDefinitionMultimap.remove(name, node);
         }
       }

src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java

@@ -111,10 +111,6 @@ private class GatherNoSideEffectFunctions extends AbstractPostOrderCallback {
 
     @Override
     public void visit(NodeTraversal traversal, Node node, Node parent) {
-      if (!inExterns && hasNoSideEffectsAnnotation(node)) {
-        traversal.report(node, PureFunctionIdentifier.INVALID_NO_SIDE_EFFECT_ANNOTATION);
-      }
-
       if (node.isGetProp()) {
         if (parent.isExprResult() &&
             hasNoSideEffectsAnnotation(node)) {
@@ -165,12 +161,16 @@ private class SetNoSideEffectCallProperty extends AbstractPostOrderCallback {
 
     @Override
     public void visit(NodeTraversal traversal, Node node, Node parent) {
-      if (!node.isCall() && !node.isNew()) {
+      if (!NodeUtil.isCallOrNew(node)) {
+        return;
+      }
+      Node nameNode = node.getFirstChild();
+      // This is the result of an anonymous function execution. function() {}();
+      if (!nameNode.isName() && !nameNode.isGetProp()) {
         return;
       }
 
-      Collection<Definition> definitions =
-          defFinder.getDefinitionsReferencedAt(node.getFirstChild());
+      Collection<Definition> definitions = defFinder.getDefinitionsReferencedAt(nameNode);
       if (definitions == null) {
         return;
       }

src/com/google/javascript/jscomp/NameBasedDefinitionProvider.java

@@ -45,7 +45,7 @@
  */
 public class NameBasedDefinitionProvider implements DefinitionProvider, CompilerPass {
   protected final Multimap<String, Definition> nameDefinitionMultimap = LinkedHashMultimap.create();
-  protected final Map<Node, DefinitionSite> definitionSiteMap = new LinkedHashMap<>();
+  protected final Map<Node, DefinitionSite> definitionNodeByDefinitionSite = new LinkedHashMap<>();
   protected final AbstractCompiler compiler;
 
   protected boolean hasProcessBeenRun = false;
@@ -66,7 +66,8 @@ public void process(Node externs, Node source) {
   @Override
   public Collection<Definition> getDefinitionsReferencedAt(Node useSite) {
     Preconditions.checkState(hasProcessBeenRun, "The process was not run");
-    if (definitionSiteMap.containsKey(useSite)) {
+    Preconditions.checkArgument(useSite.isGetProp() || useSite.isName());
+    if (definitionNodeByDefinitionSite.containsKey(useSite)) {
       return null;
     }
 
@@ -80,14 +81,9 @@ public Collection<Definition> getDefinitionsReferencedAt(Node useSite) {
     String name = getSimplifiedName(useSite);
     if (name != null) {
       Collection<Definition> defs = nameDefinitionMultimap.get(name);
-      if (!defs.isEmpty()) {
-        return defs;
-      } else {
-        return null;
-      }
-    } else {
-      return null;
+      return defs.isEmpty() ? null : defs;
     }
+    return null;
   }
 
   private class DefinitionGatheringCallback implements Callback {
@@ -157,7 +153,7 @@ public void visit(NodeTraversal traversal, Node node, Node parent) {
           }
 
           nameDefinitionMultimap.put(name, def);
-          definitionSiteMap.put(
+          definitionNodeByDefinitionSite.put(
               node,
               new DefinitionSite(
                   node, def, traversal.getModule(), traversal.inGlobalScope(), inExterns));
@@ -190,7 +186,7 @@ public void visit(NodeTraversal traversal, Node node, Node parent) {
             // Incomplete definition
             Definition definition = new ExternalNameOnlyDefinition(node);
             nameDefinitionMultimap.put(name, definition);
-            definitionSiteMap.put(
+            definitionNodeByDefinitionSite.put(
                 node,
                 new DefinitionSite(
                     node, definition, traversal.getModule(), traversal.inGlobalScope(), inExterns));
@@ -212,7 +208,7 @@ private boolean jsdocContainsDeclarations(Node node) {
    * higher chances of name collisions.
    *
    * <p>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".
+   * namespaces. Might not matter as much if this pass runs after {@link CollapseProperties}.
    */
   protected static String getSimplifiedName(Node node) {
     if (node.isName()) {
@@ -235,12 +231,13 @@ protected static String getSimplifiedName(Node node) {
    * @return definition site collection.
    */
   public Collection<DefinitionSite> getDefinitionSites() {
-    return definitionSiteMap.values();
+    Preconditions.checkState(hasProcessBeenRun, "The process was not run");
+    return definitionNodeByDefinitionSite.values();
   }
 
   public DefinitionSite getDefinitionForFunction(Node function) {
     Preconditions.checkState(hasProcessBeenRun, "The process was not run");
     Preconditions.checkState(function.isFunction());
-    return definitionSiteMap.get(NodeUtil.getNameNode(function));
+    return definitionNodeByDefinitionSite.get(NodeUtil.getNameNode(function));
   }
 }

src/com/google/javascript/jscomp/NodeUtil.java

@@ -35,7 +35,6 @@
 import com.google.javascript.rhino.dtoa.DToA;
 import com.google.javascript.rhino.jstype.JSType;
 import com.google.javascript.rhino.jstype.TernaryValue;
-
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -44,7 +43,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-
 import javax.annotation.Nullable;
 
 /**
@@ -1200,9 +1198,7 @@ && checkForStateChangeHelper(member.getFirstChild(), checkForNewObjects, compile
    * @param callNode - constructor call node
    */
   static boolean constructorCallHasSideEffects(Node callNode) {
-    if (!callNode.isNew()) {
-      throw new IllegalStateException("Expected NEW node, got " + callNode.getType());
-    }
+    Preconditions.checkArgument(callNode.isNew(), "Expected NEW node, got %s", callNode.getType());
 
     if (callNode.isNoSideEffectsCall()) {
       return false;