Skip to content

Commit

Permalink
In PureFunctionIdentifier, make sure assignments and assignment opera…
Browse files Browse the repository at this point in the history 
…tions like += and *= are handled correctly.

To do this, expand NodeUtil.findLhsNodesInNode to understand all assignment operations.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=175889990
  • Loading branch information
tbreisacher committed Nov 16, 2017
1 parent acacc78 commit 5650838
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 31 deletions.
11 changes: 8 additions & 3 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -4041,7 +4041,6 @@ private static void getLhsNodesHelper(Node n, List<Node> lhsNodes) {
case DEFAULT_VALUE:
case CATCH:
case REST:
case ASSIGN:
getLhsNodesHelper(n.getFirstChild(), lhsNodes);
return;
case IMPORT_SPEC:
Expand All @@ -4065,8 +4064,14 @@ private static void getLhsNodesHelper(Node n, List<Node> lhsNodes) {
// Not valid in declarations but may appear in assignments.
lhsNodes.add(n);
return;
case EMPTY:
return;
default:
Preconditions.checkState(n.isEmpty(), "Invalid node in lhs: %s", n);
if (isAssignmentOp(n)) {
getLhsNodesHelper(n.getFirstChild(), lhsNodes);
} else {
throw new IllegalStateException("Invalid node in lhs: " + n);
}
}
}

Expand All @@ -4075,7 +4080,7 @@ static List<Node> findLhsNodesInNode(Node declNode) {
checkArgument(
isNameDeclaration(declNode)
|| declNode.isParamList()
|| declNode.isAssign()
|| isAssignmentOp(declNode)
|| declNode.isCatch()
|| declNode.isDestructuringLhs()
|| declNode.isDefaultValue()
Expand Down
49 changes: 24 additions & 25 deletions src/com/google/javascript/jscomp/PureFunctionIdentifier.java
View file
Expand Up @@ -607,35 +607,34 @@ private boolean isVarDeclaredInScope(Var v, Scope scope) {
*/
private void visitAssignmentOrUnaryOperator(
FunctionInformation sideEffectInfo, Scope scope, Node op, Node enclosingFunction) {
Node lhs = op.getFirstChild();
if (NodeUtil.isGet(lhs)) { // a['elem'] or a.elem
if (lhs.getFirstChild().isThis()) {
sideEffectInfo.setTaintsThis();
} else {
Node objectNode = lhs.getFirstChild();
if (objectNode.isName()) {
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);
Iterable<Node> lhsNodes;
if (isIncDec(op) || op.isDelProp()) {
lhsNodes = ImmutableList.of(op.getOnlyChild());
} else {
lhsNodes = NodeUtil.findLhsNodesInNode(op);
}
for (Node lhs : lhsNodes) {
if (NodeUtil.isGet(lhs)) {
if (lhs.getFirstChild().isThis()) {
sideEffectInfo.setTaintsThis();
} else {
Node objectNode = lhs.getFirstChild();
if (objectNode.isName()) {
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(tdeegan): Perhaps handle multi level locals: local.prop.prop2++;
sideEffectInfo.setTaintsGlobalState();
}
}
} else {
Iterable<Node> names;
if (lhs.isName()) {
names = ImmutableList.of(lhs);
} else {
names = NodeUtil.findLhsNodesInNode(op);
}
for (Node name : names) {
Var var = scope.getVar(name.getString());
Var var = scope.getVar(lhs.getString());
if (isVarDeclaredInScope(var, scope)) {
// Assignment to local, if the value isn't a safe local value,
// a literal or new object creation, add it to the local blacklist.
Expand All @@ -644,7 +643,7 @@ private void visitAssignmentOrUnaryOperator(
// Note: other ops result in the name or prop being assigned a local
// value (x++ results in a number, for instance)
checkState(NodeUtil.isAssignmentOp(op) || isIncDec(op) || op.isDelProp());
Node rhs = NodeUtil.getRValueOfLValue(name);
Node rhs = NodeUtil.getRValueOfLValue(lhs);
if (rhs != null && op.isAssign() && !NodeUtil.evaluatesToLocalValue(rhs)) {
blacklistedVarsByFunction.put(enclosingFunction, var);
}
Expand Down
15 changes: 12 additions & 3 deletions test/com/google/javascript/jscomp/NodeUtilTest.java
View file
Expand Up @@ -2870,6 +2870,13 @@ public void testFindLhsNodesInNode() {
assertThat(findLhsNodesInNode("[y, this.z] = rhs;")).hasSize(2);
assertThat(findLhsNodesInNode("[x[y]] = rhs;")).hasSize(1);
assertThat(findLhsNodesInNode("[x.y.z] = rhs;")).hasSize(1);

assertThat(findLhsNodesInNode("x += 1;")).hasSize(1);
assertThat(findLhsNodesInNode("x.y += 1;")).hasSize(1);
assertThat(findLhsNodesInNode("x -= 1;")).hasSize(1);
assertThat(findLhsNodesInNode("x.y -= 1;")).hasSize(1);
assertThat(findLhsNodesInNode("x *= 2;")).hasSize(1);
assertThat(findLhsNodesInNode("x.y *= 2;")).hasSize(1);
}

public void testIsConstructor() {
Expand Down Expand Up @@ -3068,16 +3075,18 @@ private static Node getClassNode(String js) {

/**
* @param js JavaScript node to be passed to {@code NodeUtil.findLhsNodesInNode}. Must be either
* an EXPR_RESULT containing an ASSIGN, in which case the ASSIGN will be passed to
* {@code NodeUtil.findLhsNodesInNode}, or a VAR, LET, or CONST statement.
* an EXPR_RESULT containing an assignment operation (e.g. =, +=, /=, etc)
* in which case the assignment node will be passed to
* {@code NodeUtil.findLhsNodesInNode}, or a VAR, LET, or CONST statement, in which case the
* declaration statement will be passed.
*/
private static Iterable<Node> findLhsNodesInNode(String js) {
Node root = parse(js);
checkState(root.isScript(), root);
root = root.getOnlyChild();
if (root.isExprResult()) {
root = root.getOnlyChild();
checkState(root.isAssign(), root);
checkState(NodeUtil.isAssignmentOp(root), root);
}
return NodeUtil.findLhsNodesInNode(root);
}
Expand Down
56 changes: 56 additions & 0 deletions test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java
View file
Expand Up @@ -1072,6 +1072,62 @@ public void testLocalizedSideEffects17() {
assertPureCallsMarked(source, ImmutableList.of("f"));
}

public void testLocalizedSideEffects18() {
this.mode = TypeInferenceMode.NEITHER;
String source = lines(
"function SomeCtor() { [this.x, this.y] = getCoordinates(); }",
"new SomeCtor()");
assertNoPureCalls(source);
}

public void testLocalizedSideEffects19() {
this.mode = TypeInferenceMode.NEITHER;
String source = lines(
"function SomeCtor() { [this.x, this.y] = [0, 1]; }",
"new SomeCtor()");
assertPureCallsMarked(source, ImmutableList.of("SomeCtor"));
}

public void testLocalizedSideEffects20() {
this.mode = TypeInferenceMode.NEITHER;
String source = lines(
"function SomeCtor() { this.x += 1; }",
"new SomeCtor()");
assertPureCallsMarked(source, ImmutableList.of("SomeCtor"));
}

public void testLocalizedSideEffects21() {
this.mode = TypeInferenceMode.NEITHER;
String source = lines(
"function f(values) { var x = {}; [x.y, x.z] = values; }",
"f()");
assertPureCallsMarked(source, ImmutableList.of("f"));
}

public void testLocalizedSideEffects22() {
this.mode = TypeInferenceMode.NEITHER;
String source = lines(
"var x = {}; function f(values) { [x.y, x.z] = values; }",
"f()");
assertNoPureCalls(source);
}

public void testLocalizedSideEffects23() {
this.mode = TypeInferenceMode.NEITHER;
String source = lines(
"function f(values) { var x = {}; [x.y, x.z = defaultNoSideEffects] = values; }",
"f()");
assertPureCallsMarked(source, ImmutableList.of("f"));
}

public void testLocalizedSideEffects24() {
this.mode = TypeInferenceMode.NEITHER;
String source = lines(
"function f(values) { var x = {}; [x.y, x.z = defaultWithSideEffects()] = values; }",
"f()");
assertNoPureCalls(source);
}

public void testUnaryOperators1() throws Exception {
String source = lines(
"function f() {var x = 1; x++}",
Expand Down

0 comments on commit 5650838

Please sign in to comment.