Improve accuracy of locals tracking in PureFunctionIdentifier.

In future updates I plan to update this pass to be even more accurate.

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

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

@@ -4302,9 +4302,32 @@ static boolean evaluatesToLocalValue(Node value, Predicate<Node> locals) {
                || locals.apply(value);
       case FUNCTION:
       case REGEXP:
+      case EMPTY:
+        return true;
       case ARRAYLIT:
+        for (Node entry : value.children()) {
+          if (!evaluatesToLocalValue(entry, locals)) {
+            return false;
+          }
+        }
+        return true;
       case OBJECTLIT:
-        // Literals objects with non-literal children are allowed.
+        for (Node key : value.children()) {
+          Preconditions.checkState(
+              key.isGetterDef() || key.isSetterDef() || key.isStringKey() || key.isComputedProp(),
+              "Unexpected obj literal key:",
+              key);
+
+          if (key.isGetterDef() || key.isSetterDef()) {
+            continue;
+          }
+          if (key.isComputedProp() && !evaluatesToLocalValue(key.getSecondChild(), locals)) {
+            return false;
+          }
+          if (key.isStringKey() && !evaluatesToLocalValue(key.getFirstChild(), locals)) {
+            return false;
+          }
+        }
         return true;
       case DELPROP:
       case IN:

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

@@ -444,7 +444,7 @@ public boolean shouldTraverse(NodeTraversal traversal, Node node, Node parent) {
       if (node.isFunction()) {
         if (!functionSideEffectMap.containsKey(node)) {
           // This function was not part of a definition which is why it was not created by
-          // {@link buildGraph}. For example an anonymous function.
+          // {@link buildGraph}. For example, an anonymous function.
           FunctionInformation functionInfo = new FunctionInformation();
           functionSideEffectMap.put(node, functionInfo);
           functionInfo.graphNode = sideEffectGraph.createNode(functionInfo);
@@ -596,6 +596,8 @@ private boolean isVarDeclaredInScope(Var v, Scope scope) {
     private void visitAssignmentOrUnaryOperator(
         FunctionInformation sideEffectInfo, Scope scope, Node op, Node enclosingFunction) {
       Node lhs = op.getFirstChild();
+      Preconditions.checkState(
+          lhs.isName() || NodeUtil.isGet(lhs), "Unexpected LHS expression:", lhs.toStringTree());
       if (lhs.isName()) {
         Var var = scope.getVar(lhs.getString());
         if (isVarDeclaredInScope(var, scope)) {
@@ -613,32 +615,25 @@ private void visitAssignmentOrUnaryOperator(
         } else {
           sideEffectInfo.setTaintsGlobalState();
         }
-      } else if (NodeUtil.isGet(lhs)) {
+      } else if (NodeUtil.isGet(lhs)) { // a['elem'] or a.elem
         if (lhs.getFirstChild().isThis()) {
           sideEffectInfo.setTaintsThis();
         } else {
-          Var var = null;
           Node objectNode = lhs.getFirstChild();
           if (objectNode.isName()) {
-            var = scope.getVar(objectNode.getString());
-          }
-          if (isVarDeclaredInScope(var, scope)) {
-            // Maybe a local object modification.  We won't know for sure until
-            // we exit the scope and can validate the value of the local.
-            taintedVarsByFunction.put(enclosingFunction, var);
+            Var var = scope.getVar(objectNode.getString());
+            if (isVarDeclaredInScope(var, scope)) {
+              // Maybe a local object modification.  We won't know for sure until
+              // we exit the scope and can validate the value of the local.
+              taintedVarsByFunction.put(enclosingFunction, var);
+            } else {
+              sideEffectInfo.setTaintsGlobalState();
+            }
           } else {
+            // TODO(tdeegan): Perhaps handle multi level locals: local.prop.prop2++;
             sideEffectInfo.setTaintsGlobalState();
           }
         }
-      } else {
-        // TODO(johnlenz): track down what is inserting NULL on the LHS
-        // of an assign.
-
-        // The only valid LHS expressions are NAME, GETELEM, or GETPROP.
-        // throw new IllegalStateException(
-        //     "Unexpected LHS expression:" + lhs.toStringTree()
-        //    + ", parent: " + op.toStringTree() );
-        sideEffectInfo.setTaintsGlobalState();
       }
     }
 

test/com/google/javascript/jscomp/NodeUtilTest.java

@@ -1172,9 +1172,10 @@ public void testLocalValue1() throws Exception {
 
     // We can't know if new objects are local unless we know
     // that they don't alias themselves.
+    // TODO(tdeegan): Revisit this.
     assertFalse(testLocalValue("new x()"));
 
-    // property references are assume to be non-local
+    // property references are assumed to be non-local
     assertFalse(testLocalValue("(new x()).y"));
     assertFalse(testLocalValue("(new x())['y']"));
 
@@ -1190,9 +1191,17 @@ public void testLocalValue1() throws Exception {
     assertTrue(testLocalValue("[]"));
     assertTrue(testLocalValue("{}"));
 
-    // The contents of arrays and objects don't matter
-    assertTrue(testLocalValue("[x]"));
-    assertTrue(testLocalValue("{'a':x}"));
+    assertFalse(testLocalValue("[x]"));
+    assertFalse(testLocalValue("{'a':x}"));
+    assertTrue(testLocalValue("{'a': {'b': 2}}"));
+    assertFalse(testLocalValue("{'a': {'b': global}}"));
+    assertTrue(testLocalValue("{get someGetter() { return 1; }}"));
+    assertTrue(testLocalValue("{get someGetter() { return global; }}"));
+    assertTrue(testLocalValue("{set someSetter(value) {}}"));
+    assertTrue(testLocalValue("{[someComputedProperty]: {}}"));
+    assertFalse(testLocalValue("{[someComputedProperty]: global}"));
+    assertFalse(testLocalValue("{[someComputedProperty]: {'a':x}}"));
+    assertTrue(testLocalValue("{[someComputedProperty]: {'a':1}}"));
 
     // increment/decrement results in primitive number, the previous value is
     // always coersed to a number (even in the post.

test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java

@@ -713,11 +713,7 @@ public void testResultLocalitySimple() throws Exception {
         prefix + "return externObj.foo" + suffix, ImmutableList.<String>of());
   }
 
-  /**
-   * Note that this works because object literals are always seen as local according to {@link
-   * NodeUtil#evaluatesToLocalValue}
-   */
-  public void testReturnLocalityTaintLiteralWithGlobal() {
+  public void testReturnLocalityTaintObjectLiteralWithGlobal() {
     // return empty object literal.  This is completely local
     String source = LINE_JOINER.join(
         "function f() { return {} }",
@@ -726,11 +722,27 @@ public void testReturnLocalityTaintLiteralWithGlobal() {
     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.<String>of());
+  }
+
+  public void testReturnLocalityTaintArrayLiteralWithGlobal() {
+    String source =
+        LINE_JOINER.join(
+            "function f() { return []; }",
+            "f();",
+            "function g() { return [1, {}]; }",
+            "g();");
+    checkLocalityOfMarkedCalls(source, ImmutableList.of("f", "g"));
+    // 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"));
+            "function f() { return [2 ,global]; }",
+            "f();");
+    checkLocalityOfMarkedCalls(source, ImmutableList.<String>of());
   }
 
   public void testReturnLocalityMultipleDefinitionsSameName() {
@@ -944,14 +956,14 @@ public void testLocalizedSideEffects7() throws Exception {
   public void testLocalizedSideEffects8() throws Exception {
     // Returning a local object that has been modified
     // is not a global side-effect.
-    // TODO(johnlenz): Not yet. Propagate local object information.
-    String source = LINE_JOINER.join(
-        "/** @constructor A */ function A() {};",
-        "function f() {",
-        "  var a = new A; a.foo = 1; return a;",
-        "}",
-        "f()"
-    );
+    // TODO(tdeegan): Not yet. Propagate local object information.
+    String source =
+        LINE_JOINER.join(
+            "/** @constructor A */ function A() {};",
+            "function f() {",
+            "  var a = new A; a.foo = 1; return a;",
+            "}",
+            "f()");
     assertPureCallsMarked(source, ImmutableList.of("A"));
   }
 
@@ -984,20 +996,25 @@ public void testLocalizedSideEffects10() throws Exception {
   }
 
   public void testLocalizedSideEffects11() throws Exception {
+    // TODO(tdeegan): updateA is side effect free.
     // Calling a function of a local object that taints this.
-    String source = LINE_JOINER.join(
-        "/** @constructor */ function A() {}",
-        "A.prototype.update = function() { this.x = 1; };",
-        "/** @constructor */ function B() { ",
-        "  this.a_ = new A();",
-        "}",
-        "B.prototype.updateA = function() {",
-        "  var b = this.a_;",
-        "  b.update();",
-        "};",
-        "var x = new B();",
-        "x.updateA();"
-    );
+    String source =
+        LINE_JOINER.join(
+            "/** @constructor */",
+            "function A() {}",
+            "A.prototype.update = function() { this.x = 1; };",
+            "",
+            "/** @constructor */",
+            "function B() { ",
+            "  this.a_ = new A();",
+            "}",
+            "B.prototype.updateA = function() {",
+            "  var b = this.a_;",
+            "  b.update();",
+            "};",
+            "",
+            "var x = new B();",
+            "x.updateA();");
     assertPureCallsMarked(source, ImmutableList.of("A", "B"));
   }