Query the representative nodes for side effect information instead of…

… iterating over all the possible definitions. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=132724037
google · Sep 9, 2016 · ee7493e · ee7493e
1 parent ed6b9d6
commit ee7493e
Show file tree

Hide file tree

Showing 2 changed files with 92 additions and 86 deletions.
diff --git a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java
@@ -75,13 +75,19 @@ class PureFunctionIdentifier implements CompilerPass {
   // List of all function call sites; used to iterate in markPureFunctionCalls.
   private final List<Node> allFunctionCalls;
 
+  /**
+   * Map of function names to side effect gathering representative nodes. See {@link
+   * PureFunctionIdentifier#generateRepresentativeNodes(LinkedDirectedGraph)}
+   */
+  private Map<String, FunctionInformation> representativeNodesByName;
+
   // Externs and ast tree root, for use in getDebugReport.  These two
   // fields are null until process is called.
   private Node externs;
   private Node root;
 
   public PureFunctionIdentifier(AbstractCompiler compiler, DefinitionProvider definitionProvider) {
-    this.compiler = compiler;
+    this.compiler = Preconditions.checkNotNull(compiler);
     this.definitionProvider = definitionProvider;
     this.functionSideEffectMap = new LinkedHashMap<>();
     this.allFunctionCalls = new ArrayList<>();
@@ -361,7 +367,7 @@ private void propagateSideEffects() {
       functionInfo.graphNode = sideEffectGraph.createNode(functionInfo);
     }
 
-    Map<String, FunctionInformation> reps = generateRepresentativeNodes(sideEffectGraph);
+    representativeNodesByName = generateRepresentativeNodes(sideEffectGraph);
 
     // add connections to called functions and side effect root.
     for (FunctionInformation functionInfo : functionSideEffectMap.values()) {
@@ -371,7 +377,7 @@ private void propagateSideEffects() {
 
       for (Node callSite : functionInfo.getCallsInFunctionBody()) {
         List<FunctionInformation> possibleSideEffects =
-            getSideEffectsForCall(callSite, definitionProvider, reps);
+            getSideEffectsForCall(callSite, definitionProvider, representativeNodesByName);
         if (possibleSideEffects == null) {
           functionInfo.setTaintsGlobalState();
           break;
@@ -469,6 +475,9 @@ private Map<String, FunctionInformation> generateRepresentativeNodes(
             if (definition.mutatesArguments()) {
               representativeNode.setTaintsArguments();
             }
+            if (definition.taintsReturn()) {
+              representativeNode.setTaintsReturn();
+            }
             Preconditions.checkNotNull(definition);
 
             sideEffectGraph.connect(
@@ -484,26 +493,50 @@ private Map<String, FunctionInformation> generateRepresentativeNodes(
   /** Set no side effect property at pure-function call sites. */
   private void markPureFunctionCalls() {
     for (Node callNode : allFunctionCalls) {
-      Collection<Definition> defs = getFunctionDefinitions(definitionProvider, callNode);
+      List<FunctionInformation> possibleSideEffects =
+          getSideEffectsForCall(callNode, definitionProvider, representativeNodesByName);
       // Default to side effects, non-local results
       Node.SideEffectFlags flags = new Node.SideEffectFlags();
-      if (defs == null) {
+      if (possibleSideEffects == null) {
         flags.setMutatesGlobalState();
         flags.setThrows();
         flags.setReturnsTainted();
       } else {
         flags.clearAllFlags();
-        for (Definition def : defs) {
+
-          updateFlagsForDefs(callNode, def.getRValue(), flags);
+        for (FunctionInformation functionInfo : possibleSideEffects) {
-          if (flags.areAllFlagsSet()) {
+          Preconditions.checkNotNull(functionInfo);
-            break;
+          if (functionInfo.mutatesGlobalState()) {
+            flags.setMutatesGlobalState();
+          }
+
+          if (functionInfo.mutatesArguments()) {
+            flags.setMutatesArguments();
+          }
+
+          if (functionInfo.functionThrows()) {
+            flags.setThrows();
+          }
+
+          if (callNode.isCall()) {
+            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();
           }
         }
       }
 
       // Handle special cases (Math, RegExp)
       if (callNode.isCall()) {
-        Preconditions.checkState(compiler != null);
         if (!NodeUtil.functionCallHasSideEffects(callNode, compiler)) {
           flags.clearSideEffectFlags();
         }
@@ -518,51 +551,6 @@ 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 functionNode, Node.SideEffectFlags flags) {
-    FunctionInformation functionInfo = functionSideEffectMap.get(functionNode);
-    Preconditions.checkNotNull(functionInfo);
-    if (functionInfo.mutatesGlobalState()) {
-      flags.setMutatesGlobalState();
-    }
-
-    if (functionInfo.mutatesArguments()) {
-      flags.setMutatesArguments();
-    }
-
-    if (functionInfo.functionThrows()) {
-      flags.setThrows();
-    }
-
-    if (callNode.isCall()) {
-      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();
-    }
-  }
-
   /**
    * Gather list of functions, functions with @nosideeffects annotations, call sites, and functions
    * that may mutate variables not defined in the local scope.

diff --git a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java
@@ -590,53 +590,70 @@ public void testNoSideEffectsSimple3() throws Exception {
   public void testResultLocalitySimple() throws Exception {
     String prefix = "var g; function f(){";
     String suffix = "} f()";
-    List<String> expected = ImmutableList.of("f");
+    final List<String> fReturnsLocal = ImmutableList.of("f");
+    final List<String> fReturnsNonLocal = ImmutableList.<String>of();
 
     // no return
-    checkLocalityOfMarkedCalls(
+    checkLocalityOfMarkedCalls(prefix + "" + suffix, fReturnsLocal);
-        prefix + "" + suffix, expected);
     // simple return expressions
-    checkLocalityOfMarkedCalls(
+    checkLocalityOfMarkedCalls(prefix + "return 1" + suffix, fReturnsLocal);
-        prefix + "return 1" + suffix, expected);
+    checkLocalityOfMarkedCalls(prefix + "return 1 + 2" + suffix, fReturnsLocal);
-    checkLocalityOfMarkedCalls(
-        prefix + "return 1 + 2" + suffix, expected);
 
     // global result
-    checkLocalityOfMarkedCalls(
+    checkLocalityOfMarkedCalls(prefix + "return g" + suffix, fReturnsNonLocal);
-        prefix + "return g" + suffix, NO_PURE_CALLS);
 
     // multiple returns
-    checkLocalityOfMarkedCalls(
+    checkLocalityOfMarkedCalls(prefix + "return 1; return 2" + suffix, fReturnsLocal);
-        prefix + "return 1; return 2" + suffix, expected);
+    checkLocalityOfMarkedCalls(prefix + "return 1; return g" + suffix, fReturnsNonLocal);
-    checkLocalityOfMarkedCalls(
-        prefix + "return 1; return g" + suffix, NO_PURE_CALLS);
 
-    // local var, not yet.
+    // local var, not yet. Note we do not handle locals properly here.
-    checkLocalityOfMarkedCalls(
+    checkLocalityOfMarkedCalls(prefix + "var a = 1; return a" + suffix, fReturnsNonLocal);
-        prefix + "var a = 1; return a" + suffix, NO_PURE_CALLS);
 
-    // mutate local var, not yet.
+    // mutate local var, not yet. Note we do not handle locals properly here.
-    checkLocalityOfMarkedCalls(
+    checkLocalityOfMarkedCalls(prefix + "var a = 1; a = 2; return a" + suffix, fReturnsNonLocal);
-        prefix + "var a = 1; a = 2; return a" + suffix, NO_PURE_CALLS);
+    checkLocalityOfMarkedCalls(prefix + "var a = 1; a = 2; return a + 1" + suffix, fReturnsLocal);
-    checkLocalityOfMarkedCalls(
-        prefix + "var a = 1; a = 2; return a + 1" + suffix, expected);
 
     // read from obj literal
+    checkLocalityOfMarkedCalls(prefix + "return {foo : 1}.foo" + suffix, fReturnsNonLocal);
     checkLocalityOfMarkedCalls(
-        prefix + "return {foo : 1}.foo" + suffix,
+        prefix + "var a = {foo : 1}; return a.foo" + suffix, fReturnsNonLocal);
-        NO_PURE_CALLS);
-    checkLocalityOfMarkedCalls(
-        prefix + "var a = {foo : 1}; return a.foo" + suffix,
-        NO_PURE_CALLS);
 
     // read from extern
-    checkLocalityOfMarkedCalls(
+    checkLocalityOfMarkedCalls(prefix + "return externObj" + suffix, NO_PURE_CALLS);
-        prefix + "return externObj" + suffix, NO_PURE_CALLS);
     checkLocalityOfMarkedCalls(
         "function inner(x) { x.foo = 3; }" /* to suppress missing property */ +
         prefix + "return externObj.foo" + suffix, NO_PURE_CALLS);
   }
 
+  /**
+   * Note that this works because object literals are always seen as local according to {@link
+   * NodeUtil#evaluatesToLocalValue}
+   */
+  public void testReturnLocalityTaintLiteralWithGlobal() {
+    // return empty object literal.  This is completely local
+    String source = LINE_JOINER.join(
+        "function f() { return {} }",
+        "f();"
+    );
+    checkLocalityOfMarkedCalls(source, ImmutableList.of("f"));
+    // return obj literal with global taint.
+    source = LINE_JOINER.join(
+            "var global = new Object();",
+            "function f() { return {'asdf': global} }",
+            "f();"
+        );
+    checkLocalityOfMarkedCalls(source, ImmutableList.of("f"));
+  }
+
+  public void testReturnLocalityMultipleDefinitionsSameName() {
+    String source = LINE_JOINER.join(
+            "var global = new Object();",
+            "A.func = function() {return global}", // return global (taintsReturn)
+            "B.func = function() {return 1; }", // returns local
+            "C.func();");
+    checkLocalityOfMarkedCalls(source, ImmutableList.<String>of());
+  }
+
   public void testExternCalls() throws Exception {
     String prefix = "function f(){";
     String suffix = "} f()";
@@ -1636,12 +1653,13 @@ public void process(Node externs, Node root) {
       compiler.getOptions().setUseTypesForOptimization(true);
       NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler, true);
       defFinder.process(externs, root);
-      PureFunctionIdentifier passUnderTest =
+
+      PureFunctionIdentifier pureFunctionIdentifier =
           new PureFunctionIdentifier(compiler, defFinder);
-      passUnderTest.process(externs, root);
+      pureFunctionIdentifier.process(externs, root);
 
       // Ensure that debug report computation doesn't crash.
-      passUnderTest.getDebugReport();
+      pureFunctionIdentifier.getDebugReport();
 
       NodeTraversal.traverseEs6(compiler, externs, this);
       NodeTraversal.traverseEs6(compiler, root, this);