diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index 74a738dd088..4f010bc9674 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -4302,9 +4302,32 @@ static boolean evaluatesToLocalValue(Node value, Predicate 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: diff --git a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java index 755d26291c1..5471129b715 100644 --- a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java +++ b/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(); } } diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java index 1f77d1305ee..3ca23c79ef7 100644 --- a/test/com/google/javascript/jscomp/NodeUtilTest.java +++ b/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. diff --git a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java index 225e8dd72f0..9bf1c5ddd18 100644 --- a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java +++ b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java @@ -713,11 +713,7 @@ public void testResultLocalitySimple() throws Exception { prefix + "return externObj.foo" + suffix, ImmutableList.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.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.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")); }