Skip to content

Commit

Permalink
Handle block scoped vars and for-of loops in PureFunctionIdentifier
Browse files Browse the repository at this point in the history 
Fixes a bug that caused PureFunctionIdentifier to incorrectly determine
that methods were side-effect free, leading to removal of calls to them.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=201431241
  • Loading branch information
@brad4d
brad4d committed Jun 21, 2018
1 parent 6ac53b1 commit 9cfa692
Show file tree
Hide file tree
Showing 7 changed files with 429 additions and 62 deletions.
3 changes: 2 additions & 1 deletion src/com/google/javascript/jscomp/AbstractScope.java
View file
Expand Up @@ -429,7 +429,8 @@ S getCommonParent(S other) {
* the two scopes would be equivalent in a pre-ES2015-block-scopes view of the world.
*/
boolean hasSameContainerScope(S other) {
return getClosestContainerScope() == other.getClosestContainerScope();
// Do identity check first as a shortcut.
return this == other || getClosestContainerScope() == other.getClosestContainerScope();
}

/**
Expand Down
40 changes: 28 additions & 12 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -1498,7 +1498,7 @@ static boolean newHasLocalResult(Node n) {
*
* <p>This is a non-recursive version of the may have side effects
* check; used to check wherever the current node's type is one of
* the reason's why a subtree has side effects.
* the reasons why a subtree has side effects.
*/
static boolean nodeTypeMayHaveSideEffects(Node n) {
return nodeTypeMayHaveSideEffects(n, null);
Expand All @@ -1516,6 +1516,8 @@ static boolean nodeTypeMayHaveSideEffects(Node n, AbstractCompiler compiler) {
case YIELD:
case THROW:
case AWAIT:
case FOR_IN: // assigns to a loop LHS
case FOR_OF: // assigns to a loop LHS
return true;
case CALL:
return NodeUtil.functionCallHasSideEffects(n, compiler);
Expand Down Expand Up @@ -4236,6 +4238,14 @@ private static void getLhsNodesHelper(Node n, List<Node> lhsNodes) {
return;
case EMPTY:
return;
case FOR_IN:
case FOR_OF:
// Enhanced for loops assign to variables in their first child
// e.g.
// for (some.prop in someObj) {...
// for ({a, b} of someIterable) {...
getLhsNodesHelper(n.getFirstChild(), lhsNodes);
return;
default:
if (isAssignmentOp(n)) {
getLhsNodesHelper(n.getFirstChild(), lhsNodes);
Expand All @@ -4245,19 +4255,25 @@ private static void getLhsNodesHelper(Node n, List<Node> lhsNodes) {
}
}

/** Retrieves lhs nodes declared in the current declaration or ASSIGN statement. */
public static List<Node> findLhsNodesInNode(Node declNode) {
/**
* Retrieves lhs nodes declared or assigned in a given assigning parent node.
*
* <p>An assigning parent node is one that assigns a value to one or more LHS nodes.
*/
public static List<Node> findLhsNodesInNode(Node assigningParent) {
checkArgument(
isNameDeclaration(declNode)
|| declNode.isParamList()
|| isAssignmentOp(declNode)
|| declNode.isCatch()
|| declNode.isDestructuringLhs()
|| declNode.isDefaultValue()
|| declNode.isImport(),
declNode);
isNameDeclaration(assigningParent)
|| assigningParent.isParamList()
|| isAssignmentOp(assigningParent)
|| assigningParent.isCatch()
|| assigningParent.isDestructuringLhs()
|| assigningParent.isDefaultValue()
|| assigningParent.isImport()
// enhanced for loops assign to loop variables
|| isEnhancedFor(assigningParent),
assigningParent);
ArrayList<Node> lhsNodes = new ArrayList<>();
getLhsNodesHelper(declNode, lhsNodes);
getLhsNodesHelper(assigningParent, lhsNodes);
return lhsNodes;
}

Expand Down
146 changes: 97 additions & 49 deletions src/com/google/javascript/jscomp/PureFunctionIdentifier.java
View file
Expand Up @@ -49,6 +49,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.function.Predicate;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -435,6 +436,14 @@ private void markPureFunctionCalls() {
}
}

private static final Predicate<Node> RHS_IS_ALWAYS_LOCAL = lhs -> true;
private static final Predicate<Node> RHS_IS_NEVER_LOCAL = lhs -> false;
private static final Predicate<Node> FIND_RHS_AND_CHECK_FOR_LOCAL_VALUE = lhs -> {
Node rhs = NodeUtil.getRValueOfLValue(lhs);
return rhs == null || NodeUtil.evaluatesToLocalValue(rhs);
};


/**
* Gather list of functions, functions with @nosideeffects annotations, call sites, and functions
* that may mutate variables not defined in the local scope.
Expand Down Expand Up @@ -478,11 +487,12 @@ public void visit(NodeTraversal traversal, Node node, Node parent) {
allFunctionCalls.add(node);
}

// TODO: This may be more expensive than necessary.
Node enclosingFunction = traversal.getEnclosingFunction();
if (enclosingFunction == null) {
Scope containerScope = traversal.getScope().getClosestContainerScope();
if (!containerScope.isFunctionScope()) {
// We only need to look at nodes in function scopes.
return;
}
Node enclosingFunction = containerScope.getRootNode();

for (FunctionInformation sideEffectInfo : functionSideEffectMap.get(enclosingFunction)) {
checkNotNull(sideEffectInfo);
Expand All @@ -495,9 +505,64 @@ public void updateSideEffectsForNode(
NodeTraversal traversal,
Node node,
Node enclosingFunction) {
if (NodeUtil.isAssignmentOp(node) || node.isInc() || node.isDelProp() || node.isDec()) {
visitAssignmentOrUnaryOperator(
sideEffectInfo, traversal.getScope(), node, enclosingFunction);
if (node.isAssign()) {
// e.g.
// lhs = rhs;
// ({x, y} = object);
visitLhsNodes(
sideEffectInfo,
traversal.getScope(),
enclosingFunction,
NodeUtil.findLhsNodesInNode(node),
FIND_RHS_AND_CHECK_FOR_LOCAL_VALUE);
} else if (NodeUtil.isCompoundAssignmentOp(node)) {
// e.g.
// x += 3;
visitLhsNodes(
sideEffectInfo,
traversal.getScope(),
enclosingFunction,
ImmutableList.of(node.getFirstChild()),
// The update assignments (e.g. `+=) always assign primitive, and therefore local,
// values.
RHS_IS_ALWAYS_LOCAL);
} else if (node.isInc() || node.isDelProp() || node.isDec()) {
// e.g.
// x++;
visitLhsNodes(
sideEffectInfo,
traversal.getScope(),
enclosingFunction,
ImmutableList.of(node.getOnlyChild()),
// The value assigned by a unary op is always local.
RHS_IS_ALWAYS_LOCAL);
} else if (node.isForOf()) {
// e.g.
// for (const {prop1, prop2} of iterable) {...}
// for ({prop1: x.p1, prop2: x.p2} of iterable) {...}
//
// TODO(bradfordcsmith): Possibly we should try to determine whether the iteration itself
// could have side effects.
visitLhsNodes(
sideEffectInfo,
traversal.getScope(),
enclosingFunction,
NodeUtil.findLhsNodesInNode(node),
// The RHS of a for-of must always be an iterable, making it a container, so we can't
// consider its contents to be local
RHS_IS_NEVER_LOCAL);
} else if (node.isForIn()) {
// e.g.
// for (prop in obj) {...}
// Also this, though not very useful or readable.
// for ([char1, char2, ...x.rest] in obj) {...}
visitLhsNodes(
sideEffectInfo,
traversal.getScope(),
enclosingFunction,
NodeUtil.findLhsNodesInNode(node),
// A for-in always assigns a string, which is a local value by definition.
RHS_IS_ALWAYS_LOCAL);
} else if (NodeUtil.isCallOrNew(node)) {
visitCall(sideEffectInfo, node);
} else if (node.isName()) {
Expand Down Expand Up @@ -537,14 +602,12 @@ public void enterScope(NodeTraversal t) {

@Override
public void exitScope(NodeTraversal t) {
if (!t.getScope().isFunctionBlockScope() && !t.getScope().isFunctionScope()) {
return;
}

Node function = NodeUtil.getEnclosingFunction(t.getScopeRoot());
if (function == null) {
Scope closestContainerScope = t.getScope().getClosestContainerScope();
if (!closestContainerScope.isFunctionScope()) {
// Only functions and the scopes within them are of interest to us.
return;
}
Node function = closestContainerScope.getRootNode();

// Handle deferred local variable modifications:
for (FunctionInformation sideEffectInfo : functionSideEffectMap.get(function)) {
Expand Down Expand Up @@ -589,34 +652,28 @@ public void exitScope(NodeTraversal t) {
}
}

private boolean isVarDeclaredInScope(@Nullable Var v, Scope scope) {
if (v == null) {
return false;
}
if (v.scope == scope) {
return true;
}
Node declarationRoot = NodeUtil.getEnclosingFunction(v.scope.getRootNode());
Node scopeRoot = NodeUtil.getEnclosingFunction(scope.getRootNode());
return declarationRoot == scopeRoot;
private boolean isVarDeclaredInSameContainerScope(@Nullable Var v, Scope scope) {
return v != null && v.scope.hasSameContainerScope(scope);
}

/**
* Record information about the side effects caused by an assignment or mutating unary operator.
* Record information about the side effects caused by assigning a value to a given LHS.
*
* <p>If the operation modifies this or taints global state, mark the enclosing function as
* having those side effects.
*
* @param op operation being performed.
* @param sideEffectInfo Function side effect record to be updated
* @param scope variable scope in which the variable assignment occurs
* @param enclosingFunction FUNCTION node for the enclosing function
* @param lhsNodes LHS nodes that are all assigned values by a given parent node
* @param hasLocalRhs Predicate indicating whether a given LHS is being assigned a local value
*/
private void visitAssignmentOrUnaryOperator(
FunctionInformation sideEffectInfo, Scope scope, Node op, Node enclosingFunction) {
Iterable<Node> lhsNodes;
if (isIncDec(op) || op.isDelProp()) {
lhsNodes = ImmutableList.of(op.getOnlyChild());
} else {
lhsNodes = NodeUtil.findLhsNodesInNode(op);
}
private void visitLhsNodes(
FunctionInformation sideEffectInfo,
Scope scope,
Node enclosingFunction,
Iterable<Node> lhsNodes,
Predicate<Node> hasLocalRhs) {
for (Node lhs : lhsNodes) {
if (NodeUtil.isGet(lhs)) {
if (lhs.getFirstChild().isThis()) {
Expand All @@ -625,30 +682,26 @@ private void visitAssignmentOrUnaryOperator(
Node objectNode = lhs.getFirstChild();
if (objectNode.isName()) {
Var var = scope.getVar(objectNode.getString());
if (isVarDeclaredInScope(var, scope)) {
if (isVarDeclaredInSameContainerScope(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++;
// Don't track multi level locals: local.prop.prop2++;
sideEffectInfo.setTaintsGlobalState();
}
}
} else {
checkState(lhs.isName(), lhs);
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.
// parameter values depend on the caller.

// 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(lhs);
if (rhs != null && op.isAssign() && !NodeUtil.evaluatesToLocalValue(rhs)) {
if (isVarDeclaredInSameContainerScope(var, scope)) {
if (!hasLocalRhs.test(lhs)) {
// Assigned value is not guaranteed to be a local value,
// so if we see any property assignments on this variable,
// they could be tainting a non-local value.
blacklistedVarsByFunction.put(enclosingFunction, var);
}
} else {
Expand Down Expand Up @@ -685,11 +738,6 @@ private void visitCall(FunctionInformation sideEffectInfo, Node node) {
}
}

private static boolean isIncDec(Node n) {
Token type = n.getToken();
return (type == Token.INC || type == Token.DEC);
}

private static boolean isCallOrApply(Node callSite) {
return NodeUtil.isFunctionObjectCall(callSite) || NodeUtil.isFunctionObjectApply(callSite);
}
Expand Down
18 changes: 18 additions & 0 deletions src/com/google/javascript/jscomp/testing/NodeSubject.java
View file
Expand Up @@ -18,6 +18,7 @@
import static com.google.common.truth.Truth.assertAbout;
import static org.junit.Assert.assertEquals;

import com.google.common.truth.Fact;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.Subject;
import com.google.javascript.rhino.Node;
Expand Down Expand Up @@ -55,6 +56,23 @@ public void hasType(Token type) {
assertEquals(message, type, actual().getToken());
}

public void isName(String name) {
Node node = actual();
if (!node.isName()) {
failWithActual(Fact.simpleFact("expected to be a NAME node"));
}
if (!node.getString().equals(name)) {
failWithActual("expected name to be", name);
}
}

public void matchesQualifiedName(String qname) {
Node node = actual();
if (!node.matchesQualifiedName(qname)) {
failWithActual("expected qualified name", qname);
}
}

public void hasCharno(int charno) {
assertEquals(charno, actual().getCharno());
}
Expand Down
27 changes: 27 additions & 0 deletions test/com/google/javascript/jscomp/IntegrationTest.java
View file
Expand Up @@ -49,6 +49,33 @@ public final class IntegrationTest extends IntegrationTestCase {
private static final String CLOSURE_COMPILED =
"var COMPILED = true; var goog$exportSymbol = function() {};";

public void testForOfDoesNotFoolSideEffectDetection() {
CompilerOptions options = createCompilerOptions();
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
options.setLanguageIn(LanguageMode.ECMASCRIPT_NEXT);
options.setLanguageOut(LanguageMode.ECMASCRIPT_NEXT);
// we don't want to see injected library code in the output
useNoninjectingCompiler = true;
testSame(
options,
lines(
"class C {",
" constructor(elements) {",
" this.elements = elements;",
" this.m1();", // this call should not be removed, because it has side effects
" }",
// m1() must be considered to have side effects because it taints a non-local object
// through basically this.elements[i].sompProp.
" m1() {",
" for (const element of this.elements) {",
" element.someProp = 1;",
" }",
" }",
"}",
"new C([]);",
""));
}

public void testIssue2822() {
CompilerOptions options = createCompilerOptions();
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
Expand Down

0 comments on commit 9cfa692

Please sign in to comment.