Skip to content

Commit

Permalink
Updates PeepholeRemoveDeadCode to preserve side-effectful SPREADs.
Browse files Browse the repository at this point in the history 
Since SPREAD is not an expression, this update parents them under ARRAYLITs so they can be freely relocated. ARRAYLITs containing such SPREADs are considered to have side-effects, despite the fact that ARRAYLITs themselves do not.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=239057715
  • Loading branch information
@nreid260 @tjgq
nreid260 authored and tjgq committed Mar 19, 2019
1 parent 658b5cf commit 2a3c809
Show file tree
Hide file tree
Showing 2 changed files with 201 additions and 116 deletions.
269 changes: 168 additions & 101 deletions src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java
View file
Expand Up @@ -25,6 +25,7 @@
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token;
import com.google.javascript.rhino.jstype.TernaryValue;
import java.util.ArrayDeque;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -81,8 +82,7 @@ Node optimizeSubtree(Node subtree) {
case BLOCK:
return tryOptimizeBlock(subtree);
case EXPR_RESULT:
subtree = tryFoldExpr(subtree);
return subtree;
return tryFoldExpr(subtree);
case HOOK:
return tryFoldHook(subtree);
case SWITCH:
Expand Down Expand Up @@ -277,132 +277,191 @@ private Node tryFoldExpr(Node subtree) {
}

/**
* General cascading unused operation node removal.
* @param n The root of the expression to simplify.
* @return The replacement node, or null if the node was is not useful.
* Replaces {@code expression} with an expression that contains only side-effects of the original.
*
* <p>This replacement is made under the assumption that the result of {@code expression} is
* unused and therefore it is correct to eliminate non-side-effectful nodes.
*
* @return The replacement expression, or {@code null} if there were no side-effects to preserve.
*/
private Node trySimplifyUnusedResult(Node n) {
return trySimplifyUnusedResult(n, true);
@Nullable
private Node trySimplifyUnusedResult(Node expression) {
ArrayDeque<Node> sideEffectRoots = new ArrayDeque<>();
boolean atFixedPoint = trySimplifyUnusedResultInternal(expression, sideEffectRoots);

if (atFixedPoint) {
// `expression` is in a form that cannot be further optimized.
return expression;
} else if (sideEffectRoots.isEmpty()) {
deleteNode(expression);
return null;
} else if (sideEffectRoots.peekFirst() == expression) {
// Expression was a conditional that was transformed. There can't be any other side-effects,
// but we also can't detach the transformed root.
checkState(sideEffectRoots.size() == 1, sideEffectRoots);
reportChangeToEnclosingScope(expression);
return expression;
} else {
Node sideEffects = asDetachedExpression(sideEffectRoots.pollFirst());

// Assemble a tree of comma expressions for all the side-effects. The tree must execute the
// side-effects in FIFO order with respect to the queue. It must also be left leaning to match
// the parser's preferred strucutre.
while (!sideEffectRoots.isEmpty()) {
Node next = asDetachedExpression(sideEffectRoots.pollFirst());
sideEffects = IR.comma(sideEffects, next).srcref(next);
}

expression.getParent().addChildBefore(sideEffects, expression);
deleteNode(expression);
return sideEffects;
}
}

/**
* General cascading unused operation node removal.
* @param n The root of the expression to simplify.
* @param removeUnused If true, the node is removed from the AST if
* it is not useful, otherwise it replaced with an EMPTY node.
* @return The replacement node, or null if the node was is not useful.
* Collects any potentially side-effectful subtrees within {@code tree} into {@code
* sideEffectRoots}.
*
* <p>When a node is determined to have side-effects its descendants are not explored. This method
* assumes the entire subtree of such a node must be preserved. As a corollary, the contents of
* {@code sideEffectRoots} are a forest.
*
* <p>This operation generally does not mutate {@code tree}; however, exceptions are made for
* expressions that alter control-flow. Such expression will be pruned of their side-effectless
* branches. Even in this case, {@code tree} is never detached.
*
* @param sideEffectRoots The roots of subtrees determined to have side-effects, in execution
* order.
* @return {@code true} iff there is no code to be removed from within {@code tree}; it is already
* at a fixed point for code removal.
*/
private Node trySimplifyUnusedResult(Node n, boolean removeUnused) {
Node result = n;

// Simplify the results of conditional expressions
switch (n.getToken()) {
private boolean trySimplifyUnusedResultInternal(Node tree, ArrayDeque<Node> sideEffectRoots) {
// Special cases for conditional expressions that may be using results.
switch (tree.getToken()) {
case HOOK:
Node trueNode = trySimplifyUnusedResult(n.getSecondChild());
Node falseNode = trySimplifyUnusedResult(n.getLastChild());
// If one or more of the conditional children were removed,
// transform the HOOK to an equivalent operation:
// Try to remove one or more of the conditional children and transform the HOOK to an
// equivalent operation. Remember that if either value branch still exists, the result of
// the predicate expression is being used, and so cannot be removed.
// x() ? foo() : 1 --> x() && foo()
// x() ? 1 : foo() --> x() || foo()
// x() ? 1 : 1 --> x()
// x ? 1 : 1 --> null

Node trueNode = trySimplifyUnusedResult(tree.getSecondChild());
Node falseNode = trySimplifyUnusedResult(tree.getLastChild());
if (trueNode == null && falseNode != null) {
n.setToken(Token.OR);
checkState(n.hasTwoChildren(), n);
checkState(tree.hasTwoChildren(), tree);

tree.setToken(Token.OR);
sideEffectRoots.addLast(tree);
return false; // The node type was changed.
} else if (trueNode != null && falseNode == null) {
n.setToken(Token.AND);
checkState(n.hasTwoChildren(), n);
checkState(tree.hasTwoChildren(), tree);

tree.setToken(Token.AND);
sideEffectRoots.addLast(tree);
return false; // The node type was changed.
} else if (trueNode == null && falseNode == null) {
result = trySimplifyUnusedResult(n.getFirstChild());
// Don't bother adding true and false branch children to make the AST valid; this HOOK is
// going to be deleted. We just need to collect any side-effects from the predicate
// expression.
trySimplifyUnusedResultInternal(tree.getOnlyChild(), sideEffectRoots);
return false; // This HOOK must be cleaned up.
} else {
// The structure didn't change.
result = n;
sideEffectRoots.addLast(tree);
return hasFixedPointParent(tree);
}
break;

case AND:
case OR:
// Try to remove the second operand from a AND or OR operations:
// Try to remove the second operand from a AND or OR operations. Remember that if the second
// child still exists, the result of the first expression is being used, and so cannot be
// removed.
// x() || f --> x()
// x() && f --> x()
Node conditionalResultNode = trySimplifyUnusedResult(n.getLastChild());

Node conditionalResultNode = trySimplifyUnusedResult(tree.getLastChild());
if (conditionalResultNode == null) {
checkState(n.hasOneChild(), n);
// The conditionally executed code was removed, so
// replace the AND/OR with its LHS or remove it if it isn't useful.
result = trySimplifyUnusedResult(n.getFirstChild());
// Don't bother adding a second child to make the AST valid; this op is going to be
// deleted. We just need to collect any side-effects from the predicate first child.
trySimplifyUnusedResultInternal(tree.getOnlyChild(), sideEffectRoots);
return false; // This op must be cleaned up.
} else {
sideEffectRoots.addLast(tree);
return hasFixedPointParent(tree);
}
break;

case FUNCTION:
// A function expression isn't useful if it isn't used, remove it and
// don't bother to look at its children.
result = null;
break;
case COMMA:
// We rewrite other operations as COMMA expressions (which will later
// get split into individual EXPR_RESULT statement, if possible), so
// we special case COMMA (we don't want to rewrite COMMAs as new COMMAs
// nodes.
Node left = trySimplifyUnusedResult(n.getFirstChild());
Node right = trySimplifyUnusedResult(n.getLastChild());
if (left == null && right == null) {
result = null;
} else if (left == null) {
result = right;
} else if (right == null){
result = left;
} else {
// The structure didn't change.
result = n;
// Functions that aren't being invoked are dead. If they were invoked we'd see the CALL
// before arriving here. We don't want to look at any children since they'll never execute.
return false;

default:
// This is the meat of this function. It covers the general case of nodes which are unused
if (nodeTypeMayHaveSideEffects(tree)) {
sideEffectRoots.addLast(tree);
return hasFixedPointParent(tree);
} else if (!tree.hasChildren()) {
return false; // A node must have children or side-effects to be at fixed-point.
}

boolean atFixedPoint = hasFixedPointParent(tree);
for (Node child = tree.getFirstChild(); child != null; child = child.getNext()) {
atFixedPoint &= trySimplifyUnusedResultInternal(child, sideEffectRoots);
}
return atFixedPoint;
}
}

/**
* Returns a expression executing {@code expr} which is legal in any expression context.
*
* @param expr An attached expression
* @return A detached expression
*/
private static Node asDetachedExpression(Node expr) {
switch (expr.getToken()) {
case SPREAD:
expr = IR.arraylit(expr.detach()).srcref(expr);
break;
default:
if (!nodeTypeMayHaveSideEffects(n)
// TODO(b/123649765): Actually suppport SPREAD as being side-effectful
|| n.isSpread()) {
// This is the meat of this function. The node itself doesn't generate
// any side-effects but preserve any side-effects in the children.
Node resultList = null;
for (Node next, c = n.getFirstChild(); c != null; c = next) {
next = c.getNext();
c = trySimplifyUnusedResult(c);
if (c != null) {
c.detach();
if (resultList == null) {
// The first side-effect can be used stand-alone.
resultList = c;
} else {
// Leave the side-effects in-place, simplifying it to a COMMA
// expression.
resultList = IR.comma(resultList, c).srcref(c);
}
}
}
result = resultList;
}
break;
}

// Fix up the AST, replace or remove the an unused node (if requested).
if (n != result) {
Node parent = n.getParent();
if (result == null) {
if (removeUnused) {
parent.removeChild(n);
markFunctionsDeleted(n);
} else {
result = IR.empty().srcref(n);
parent.replaceChild(n, result);
}
} else {
// A new COMMA expression may not have an existing parent.
if (result.getParent() != null) {
result.detach();
}
n.replaceWith(result);
}
reportChangeToEnclosingScope(parent);
if (expr.getParent() != null) {
expr.detach();
}

return result;
checkState(IR.mayBeExpression(expr), expr);
return expr;
}

/**
* Returns {@code true} iff {@code expr} is parented such that it is valid in a fixed-point
* representation of an unused expression tree.
*
* <p>A fixed-point representation is one in which no futher nodes should be changed or removed
* when removing unused code. This method assumes that the expression tree in question is unused,
* so only side-effects are relevant.
*/
private static boolean hasFixedPointParent(Node expr) {
// Most kinds of nodes shouldn't be branches in the fixed-point tree of an unused
// expression. Those listed below are the only valid kinds.
switch (expr.getParent().getToken()) {
case AND:
case COMMA:
case HOOK:
case OR:
return true;
case ARRAYLIT:
// Make a special allowance for SPREADs so they remain in a legal context. Iterable SPREADs
// with other parent types are not fixed-point because ARRAYLIT is the tersest legal parent
// and is known to be side-effect free.
return expr.isSpread();
default:
// Statments are always fixed-point parents. All other expressions are not.
return NodeUtil.isStatement(expr.getParent());
}
}

/**
Expand Down Expand Up @@ -1037,11 +1096,19 @@ Node tryFoldFor(Node n) {
Node increment = cond.getNext();

if (!init.isEmpty() && !NodeUtil.isNameDeclaration(init)) {
init = trySimplifyUnusedResult(init, false);
init = trySimplifyUnusedResult(init);
if (init == null) {
init = IR.empty().srcref(n);
n.addChildToFront(init);
}
}

if (!increment.isEmpty()) {
increment = trySimplifyUnusedResult(increment, false);
increment = trySimplifyUnusedResult(increment);
if (increment == null) {
increment = IR.empty().srcref(n);
n.addChildAfter(increment, cond);
}
}

// There is an initializer skip it
Expand Down

0 comments on commit 2a3c809

Please sign in to comment.