From 2a3c809db5d0cdc07e1d84195ec413f7d0a354e9 Mon Sep 17 00:00:00 2001 From: nickreid Date: Mon, 18 Mar 2019 14:09:58 -0700 Subject: [PATCH] Updates `PeepholeRemoveDeadCode` to preserve side-effectful SPREADs. 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 --- .../jscomp/PeepholeRemoveDeadCode.java | 269 +++++++++++------- .../jscomp/PeepholeRemoveDeadCodeTest.java | 48 +++- 2 files changed, 201 insertions(+), 116 deletions(-) diff --git a/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java b/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java index 80d6281bf59..b308d292984 100644 --- a/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java +++ b/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java @@ -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; /** @@ -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: @@ -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. + * + *

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 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}. + * + *

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. + * + *

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 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. + * + *

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()); + } } /** @@ -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 diff --git a/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java b/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java index 9cf350b7b4c..3e9245d0c20 100644 --- a/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java +++ b/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java @@ -18,7 +18,6 @@ import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.rhino.Node; -import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -56,16 +55,8 @@ public void process(Node externs, Node root) { }; } - @Override - @Before - public void setUp() throws Exception { - super.setUp(); - setAcceptedLanguage(LanguageMode.ECMASCRIPT_2017); - } - @Override protected int getNumRepetitions() { - // Reduce this to 2 if we get better expression evaluators. return 2; } @@ -1187,23 +1178,39 @@ public void testRemoveFromLabel2() { } @Test - public void testCall1() { + public void testCall() { + // We use a function with no side-effects, otherwise the entire invocation would be preserved. test("Math.sin(0);", ""); + test("1 + Math.sin(0);", ""); + test("Math.sin(...c)", "([...c])"); } @Test - public void testCall2() { - test("1 + Math.sin(0);", ""); + public void testCall_containingSpread() { + // We use a function with no side-effects, otherwise the entire invocation would be preserved. + test("Math.sin(...c)", "([...c])"); + test("Math.sin(4, ...c, a)", "([...c])"); + test("Math.sin(foo(), ...c, bar())", "(foo(), [...c], bar())"); + test("Math.sin(...a, b, ...c)", "([...a], [...c])"); + test("Math.sin(...b, ...c)", "([...b], [...c])"); } @Test - public void testNew1() { + public void testNew() { + // We use a function with no side-effects, otherwise the entire invocation would be preserved. test("new Date;", ""); + test("1 + new Date;", ""); + test("new Date(...c)", "([...c])"); } @Test - public void testNew2() { - test("1 + new Date;", ""); + public void testNew_containingSpread() { + // We use a function with no side-effects, otherwise the entire invocation would be preserved. + test("new Date(...c)", "([...c])"); + test("new Date(4, ...c, a)", "([...c])"); + test("new Date(foo(), ...c, bar())", "(foo(), [...c], bar())"); + test("new Date(...a, b, ...c)", "([...a], [...c])"); + test("new Date(...b, ...c)", "([...b], [...c])"); } @Test @@ -1239,6 +1246,8 @@ public void testObjectLiteral() { test("({a:1})", ""); test("({a:foo()})", "foo()"); test("({'a':foo()})", "foo()"); + test("({...a})", ""); + test("({...foo()})", "foo()"); } @Test @@ -1249,6 +1258,15 @@ public void testArrayLiteral() { test("([foo()])", "foo()"); } + @Test + public void testArrayLiteral_containingSpread() { + testSame("([...c])"); + test("([4, ...c, a])", "([...c])"); + test("([foo(), ...c, bar()])", "(foo(), [...c], bar())"); + test("([...a, b, ...c])", "([...a], [...c])"); + testSame("([...b, ...c])"); // It would also be fine if the spreads were split apart. + } + @Test public void testAwait() { testSame("async function f() { await something(); }");