Skip to content

Commit

Permalink
Rollback of "Improve accuracy of locals tracking in PureFunctionIdent…
Browse files Browse the repository at this point in the history 
…ifier."

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=152642698
  • Loading branch information
@brad4d
brad4d committed Apr 10, 2017
1 parent 08bd900 commit ecd500a
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 97 deletions.
25 changes: 1 addition & 24 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -4317,32 +4317,9 @@ static boolean evaluatesToLocalValue(Node value, Predicate<Node> locals) {
|| locals.apply(value); || locals.apply(value);
case FUNCTION: case FUNCTION:
case REGEXP: case REGEXP:
case EMPTY:
return true;
case ARRAYLIT: case ARRAYLIT:
for (Node entry : value.children()) {
if (!evaluatesToLocalValue(entry, locals)) {
return false;
}
}
return true;
case OBJECTLIT: case OBJECTLIT:
for (Node key : value.children()) { // Literals objects with non-literal children are allowed.
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; return true;
case DELPROP: case DELPROP:
case IN: case IN:
Expand Down
31 changes: 18 additions & 13 deletions src/com/google/javascript/jscomp/PureFunctionIdentifier.java
View file
Expand Up @@ -444,7 +444,7 @@ public boolean shouldTraverse(NodeTraversal traversal, Node node, Node parent) {
if (node.isFunction()) { if (node.isFunction()) {
if (!functionSideEffectMap.containsKey(node)) { if (!functionSideEffectMap.containsKey(node)) {
// This function was not part of a definition which is why it was not created by // 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(); FunctionInformation functionInfo = new FunctionInformation();
functionSideEffectMap.put(node, functionInfo); functionSideEffectMap.put(node, functionInfo);
functionInfo.graphNode = sideEffectGraph.createNode(functionInfo); functionInfo.graphNode = sideEffectGraph.createNode(functionInfo);
Expand Down Expand Up @@ -596,8 +596,6 @@ private boolean isVarDeclaredInScope(Var v, Scope scope) {
private void visitAssignmentOrUnaryOperator( private void visitAssignmentOrUnaryOperator(
FunctionInformation sideEffectInfo, Scope scope, Node op, Node enclosingFunction) { FunctionInformation sideEffectInfo, Scope scope, Node op, Node enclosingFunction) {
Node lhs = op.getFirstChild(); Node lhs = op.getFirstChild();
Preconditions.checkState(
lhs.isName() || NodeUtil.isGet(lhs), "Unexpected LHS expression:", lhs.toStringTree());
if (lhs.isName()) { if (lhs.isName()) {
Var var = scope.getVar(lhs.getString()); Var var = scope.getVar(lhs.getString());
if (isVarDeclaredInScope(var, scope)) { if (isVarDeclaredInScope(var, scope)) {
Expand All @@ -615,25 +613,32 @@ private void visitAssignmentOrUnaryOperator(
} else { } else {
sideEffectInfo.setTaintsGlobalState(); sideEffectInfo.setTaintsGlobalState();
} }
} else if (NodeUtil.isGet(lhs)) { // a['elem'] or a.elem } else if (NodeUtil.isGet(lhs)) {
if (lhs.getFirstChild().isThis()) { if (lhs.getFirstChild().isThis()) {
sideEffectInfo.setTaintsThis(); sideEffectInfo.setTaintsThis();
} else { } else {
Var var = null;
Node objectNode = lhs.getFirstChild(); Node objectNode = lhs.getFirstChild();
if (objectNode.isName()) { if (objectNode.isName()) {
Var var = scope.getVar(objectNode.getString()); var = scope.getVar(objectNode.getString());
if (isVarDeclaredInScope(var, scope)) { }
// Maybe a local object modification. We won't know for sure until if (isVarDeclaredInScope(var, scope)) {
// we exit the scope and can validate the value of the local. // Maybe a local object modification. We won't know for sure until
taintedVarsByFunction.put(enclosingFunction, var); // we exit the scope and can validate the value of the local.
} else { taintedVarsByFunction.put(enclosingFunction, var);
sideEffectInfo.setTaintsGlobalState();
}
} else { } else {
// TODO(tdeegan): Perhaps handle multi level locals: local.prop.prop2++;
sideEffectInfo.setTaintsGlobalState(); 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();
} }
} }


Expand Down
17 changes: 4 additions & 13 deletions test/com/google/javascript/jscomp/NodeUtilTest.java
View file
Expand Up @@ -1172,10 +1172,9 @@ public void testLocalValue1() throws Exception {


// We can't know if new objects are local unless we know // We can't know if new objects are local unless we know
// that they don't alias themselves. // that they don't alias themselves.
// TODO(tdeegan): Revisit this.
assertFalse(testLocalValue("new x()")); assertFalse(testLocalValue("new x()"));


// property references are assumed to be non-local // property references are assume to be non-local
assertFalse(testLocalValue("(new x()).y")); assertFalse(testLocalValue("(new x()).y"));
assertFalse(testLocalValue("(new x())['y']")); assertFalse(testLocalValue("(new x())['y']"));


Expand All @@ -1191,17 +1190,9 @@ public void testLocalValue1() throws Exception {
assertTrue(testLocalValue("[]")); assertTrue(testLocalValue("[]"));
assertTrue(testLocalValue("{}")); assertTrue(testLocalValue("{}"));


assertFalse(testLocalValue("[x]")); // The contents of arrays and objects don't matter
assertFalse(testLocalValue("{'a':x}")); assertTrue(testLocalValue("[x]"));
assertTrue(testLocalValue("{'a': {'b': 2}}")); assertTrue(testLocalValue("{'a':x}"));
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 // increment/decrement results in primitive number, the previous value is
// always coersed to a number (even in the post. // always coersed to a number (even in the post.
Expand Down
77 changes: 30 additions & 47 deletions test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java
View file
Expand Up @@ -713,7 +713,11 @@ public void testResultLocalitySimple() throws Exception {
prefix + "return externObj.foo" + suffix, ImmutableList.<String>of()); prefix + "return externObj.foo" + suffix, ImmutableList.<String>of());
} }


public void testReturnLocalityTaintObjectLiteralWithGlobal() { /**
* 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 // return empty object literal. This is completely local
String source = LINE_JOINER.join( String source = LINE_JOINER.join(
"function f() { return {} }", "function f() { return {} }",
Expand All @@ -722,27 +726,11 @@ public void testReturnLocalityTaintObjectLiteralWithGlobal() {
checkLocalityOfMarkedCalls(source, ImmutableList.of("f")); checkLocalityOfMarkedCalls(source, ImmutableList.of("f"));
// return obj literal with global taint. // return obj literal with global taint.
source = LINE_JOINER.join( 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();", "var global = new Object();",
"function f() { return [2 ,global]; }", "function f() { return {'asdf': global} }",
"f();"); "f();"
checkLocalityOfMarkedCalls(source, ImmutableList.<String>of()); );
checkLocalityOfMarkedCalls(source, ImmutableList.of("f"));
} }


public void testReturnLocalityMultipleDefinitionsSameName() { public void testReturnLocalityMultipleDefinitionsSameName() {
Expand Down Expand Up @@ -956,14 +944,14 @@ public void testLocalizedSideEffects7() throws Exception {
public void testLocalizedSideEffects8() throws Exception { public void testLocalizedSideEffects8() throws Exception {
// Returning a local object that has been modified // Returning a local object that has been modified
// is not a global side-effect. // is not a global side-effect.
// TODO(tdeegan): Not yet. Propagate local object information. // TODO(johnlenz): Not yet. Propagate local object information.
String source = String source = LINE_JOINER.join(
LINE_JOINER.join( "/** @constructor A */ function A() {};",
"/** @constructor A */ function A() {};", "function f() {",
"function f() {", " var a = new A; a.foo = 1; return a;",
" var a = new A; a.foo = 1; return a;", "}",
"}", "f()"
"f()"); );
assertPureCallsMarked(source, ImmutableList.of("A")); assertPureCallsMarked(source, ImmutableList.of("A"));
} }


Expand Down Expand Up @@ -996,25 +984,20 @@ public void testLocalizedSideEffects10() throws Exception {
} }


public void testLocalizedSideEffects11() throws Exception { public void testLocalizedSideEffects11() throws Exception {
// TODO(tdeegan): updateA is side effect free.
// Calling a function of a local object that taints this. // Calling a function of a local object that taints this.
String source = String source = LINE_JOINER.join(
LINE_JOINER.join( "/** @constructor */ function A() {}",
"/** @constructor */", "A.prototype.update = function() { this.x = 1; };",
"function A() {}", "/** @constructor */ function B() { ",
"A.prototype.update = function() { this.x = 1; };", " this.a_ = new A();",
"", "}",
"/** @constructor */", "B.prototype.updateA = function() {",
"function B() { ", " var b = this.a_;",
" this.a_ = new A();", " b.update();",
"}", "};",
"B.prototype.updateA = function() {", "var x = new B();",
" var b = this.a_;", "x.updateA();"
" b.update();", );
"};",
"",
"var x = new B();",
"x.updateA();");
assertPureCallsMarked(source, ImmutableList.of("A", "B")); assertPureCallsMarked(source, ImmutableList.of("A", "B"));
} }


Expand Down

0 comments on commit ecd500a

Please sign in to comment.