From 940c41b3f030ea1e074fbddd4ad557362ab0811f Mon Sep 17 00:00:00 2001 From: bradfordcsmith Date: Fri, 10 May 2019 14:36:54 -0700 Subject: [PATCH] Remove NodeUtil.getPureBooleanValue() This also allows removal of the final call to mayHaveSideEffects() that doesn't pass in a compiler value. Caller of getImpureBooleanValue() or getLiteralBooleanValue() is responsible for checking for side effects. A followup change will merge those two methods into just getBooleanValue(). ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=247677906 --- .../jscomp/AbstractPeepholeOptimization.java | 19 +++ .../javascript/jscomp/MinimizeExitPoints.java | 2 +- .../google/javascript/jscomp/NodeUtil.java | 31 +---- .../jscomp/PeepholeFoldConstants.java | 6 +- .../jscomp/PeepholeMinimizeConditions.java | 10 +- .../jscomp/PeepholeRemoveDeadCode.java | 18 +-- .../javascript/jscomp/NodeUtilTest.java | 110 ++++++++---------- 7 files changed, 81 insertions(+), 115 deletions(-) diff --git a/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java b/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java index 7afdba00acd..5853ccb367f 100644 --- a/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java +++ b/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java @@ -20,6 +20,7 @@ import com.google.javascript.jscomp.parsing.parser.FeatureSet; import com.google.javascript.rhino.Node; +import com.google.javascript.rhino.jstype.TernaryValue; /** * An abstract class whose implementations run peephole optimizations: @@ -130,6 +131,24 @@ protected String getSideEffectFreeStringValue(Node n) { return value; } + /** + * Calculate the known boolean value for a node if possible and if it has no side effects. + * + *

Returns {@link TernaryValue#UNKNOWN} if the node has side effects or its value cannot be + * statically determined. + */ + protected TernaryValue getSideEffectFreeBooleanValue(Node n) { + TernaryValue value = NodeUtil.getLiteralBooleanValue(n); + // Calculating the boolean value, if any, is likely to be faster than calculating side effects, + // and there are only a very few cases where we can compute a boolean value, but there could + // also be side effects. e.g. `void doSomething()` has value `false`, regardless of the + // behavior of `doSomething()` + if (value != TernaryValue.UNKNOWN && astAnalyzer.mayHaveSideEffects(n)) { + value = TernaryValue.UNKNOWN; + } + return value; + } + /** * Returns true if the current node's type implies side effects. * diff --git a/src/com/google/javascript/jscomp/MinimizeExitPoints.java b/src/com/google/javascript/jscomp/MinimizeExitPoints.java index 769b279c82c..d93b96b01b4 100644 --- a/src/com/google/javascript/jscomp/MinimizeExitPoints.java +++ b/src/com/google/javascript/jscomp/MinimizeExitPoints.java @@ -51,7 +51,7 @@ Node optimizeSubtree(Node n) { tryMinimizeExits(NodeUtil.getLoopCodeBlock(n), Token.CONTINUE, null); Node cond = NodeUtil.getConditionExpression(n); - if (NodeUtil.getPureBooleanValue(cond) == TernaryValue.FALSE) { + if (getSideEffectFreeBooleanValue(cond) == TernaryValue.FALSE) { // Normally, we wouldn't be able to optimize BREAKs inside a loop // but as we know the condition will always be false, we can treat them // as we would a CONTINUE. diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index e04b73d4982..973e53aa790 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -85,8 +85,7 @@ private NodeUtil() {} * consisting of literals when possible. Use that method instead if you only want to find literal * values. * - *

This method does not consider whether the node may have side-effects. Use {@link - * #getPureBooleanValue(Node)} if you need to avoid side-effects. + *

This method does not consider whether the node may have side-effects. */ static TernaryValue getImpureBooleanValue(Node n) { // This switch consists of cases that are not supported by getLiteralBooleanValue(), @@ -141,8 +140,7 @@ static TernaryValue getImpureBooleanValue(Node n) { * * *

This method does not consider whether the literal might have side-effects when evaluated. - * This can be true for object literals, for example. Be sure to call {@link - * #getPureBooleanValue(Node)} if you need to care about side-effects. + * This can be true for object literals, for example. * *

Except for the cases listed above, this method also returns {@code TernaryValue.UNKNOWN} for * boolean expressions even when it is possible to determine their value. If you want to evaluate @@ -203,25 +201,6 @@ static TernaryValue getLiteralBooleanValue(Node n) { return TernaryValue.UNKNOWN; } - /** - * If {@code n} represents a literal value without side-effects that can be interpreted statically - * as a known boolean value, this method returns the corresponding {@code TernaryValue}. - * Otherwise, it returns {@code TernaryValue.UNKNOWN}. - * - *

Use {@link #getLiteralBooleanValue(Node)} if you don't care about side-effects. Use {@link - * #getImpureBooleanValue(Node)} if you don't care about side-effects and also want to compute - * values for non-literal expressions. - */ - static TernaryValue getPureBooleanValue(Node n) { - // Determining whether n is a literal boolean is likely to be faster than determining whether - // it has side-effects when n is a large subtree. - TernaryValue value = getLiteralBooleanValue(n); - if (value != TernaryValue.UNKNOWN && mayHaveSideEffects(n)) { - value = TernaryValue.UNKNOWN; - } - return value; - } - /** * Gets the value of a node as a String, or null if it cannot be converted. When it returns a * non-null String, this method effectively emulates the String() JavaScript cast @@ -1094,12 +1073,6 @@ static boolean mayEffectMutableState(Node n, AbstractCompiler compiler) { return checkForStateChangeHelper(n, true, compiler); } - // TODO(johnplaisted): All call sites should pass in the compiler. - @Deprecated - public static boolean mayHaveSideEffects(Node n) { - return mayHaveSideEffects(n, null); - } - /** * Returns true if the node which may have side effects when executed. * This version default to the "safe" assumptions when the compiler object is not diff --git a/src/com/google/javascript/jscomp/PeepholeFoldConstants.java b/src/com/google/javascript/jscomp/PeepholeFoldConstants.java index 9ec3b3c4841..daa8c78bae8 100644 --- a/src/com/google/javascript/jscomp/PeepholeFoldConstants.java +++ b/src/com/google/javascript/jscomp/PeepholeFoldConstants.java @@ -355,7 +355,7 @@ private Node tryFoldUnaryOperator(Node n) { return n; } - TernaryValue leftVal = NodeUtil.getPureBooleanValue(left); + TernaryValue leftVal = getSideEffectFreeBooleanValue(left); if (leftVal == TernaryValue.UNKNOWN) { return n; } @@ -1178,8 +1178,8 @@ private static TernaryValue tryStrictEqualityComparison( } case BOOLEAN: { - TernaryValue lv = NodeUtil.getPureBooleanValue(left); - TernaryValue rv = NodeUtil.getPureBooleanValue(right); + TernaryValue lv = peepholeOptimization.getSideEffectFreeBooleanValue(left); + TernaryValue rv = peepholeOptimization.getSideEffectFreeBooleanValue(right); return lv.and(rv).or(lv.not().and(rv.not())); } default: // Symbol and Object cannot be folded in the general case. diff --git a/src/com/google/javascript/jscomp/PeepholeMinimizeConditions.java b/src/com/google/javascript/jscomp/PeepholeMinimizeConditions.java index 079cad08546..049f99b3e8c 100644 --- a/src/com/google/javascript/jscomp/PeepholeMinimizeConditions.java +++ b/src/com/google/javascript/jscomp/PeepholeMinimizeConditions.java @@ -1033,8 +1033,8 @@ private Node performConditionSubstitutions(Node n) { // In the last two cases, code size may increase slightly (adding // some parens because the comma operator has a low precedence) but // the new AST is easier for other passes to handle. - TernaryValue rightVal = NodeUtil.getPureBooleanValue(right); - if (NodeUtil.getPureBooleanValue(right) != TernaryValue.UNKNOWN) { + TernaryValue rightVal = getSideEffectFreeBooleanValue(right); + if (getSideEffectFreeBooleanValue(right) != TernaryValue.UNKNOWN) { Token type = n.getToken(); Node replacement = null; boolean rval = rightVal.toBoolean(true); @@ -1083,8 +1083,8 @@ private Node performConditionSubstitutions(Node n) { // Only when x is NAME, hence x does not have side effects // x ? x : y --> x || y Node replacement = null; - TernaryValue trueNodeVal = NodeUtil.getPureBooleanValue(trueNode); - TernaryValue falseNodeVal = NodeUtil.getPureBooleanValue(falseNode); + TernaryValue trueNodeVal = getSideEffectFreeBooleanValue(trueNode); + TernaryValue falseNodeVal = getSideEffectFreeBooleanValue(falseNode); if (trueNodeVal == TernaryValue.TRUE && falseNodeVal == TernaryValue.FALSE) { // Remove useless conditionals, keep the condition condition.detach(); @@ -1120,7 +1120,7 @@ private Node performConditionSubstitutions(Node n) { default: // while(true) --> while(1) - TernaryValue nVal = NodeUtil.getPureBooleanValue(n); + TernaryValue nVal = getSideEffectFreeBooleanValue(n); if (nVal != TernaryValue.UNKNOWN) { boolean result = nVal.toBoolean(true); int equivalentResult = result ? 1 : 0; diff --git a/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java b/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java index 571511b86f4..d79c393aed4 100644 --- a/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java +++ b/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java @@ -1070,22 +1070,6 @@ private Node tryFoldHook(Node n) { return replacement; } - /** - * Removes WHILEs that always evaluate to false. - */ - Node tryFoldWhile(Node n) { - checkArgument(n.isWhile()); - Node cond = NodeUtil.getConditionExpression(n); - if (NodeUtil.getPureBooleanValue(cond) != TernaryValue.FALSE) { - return n; - } - NodeUtil.redeclareVarsInsideBranch(n); - reportChangeToEnclosingScope(n.getParent()); - NodeUtil.removeChild(n.getParent(), n); - - return null; - } - /** * Removes FORs that always evaluate to false. */ @@ -1265,7 +1249,7 @@ static boolean hasUnnamedBreakOrContinue(Node n) { * Remove always true loop conditions. */ private void tryFoldForCondition(Node forCondition) { - if (NodeUtil.getPureBooleanValue(forCondition) == TernaryValue.TRUE) { + if (getSideEffectFreeBooleanValue(forCondition) == TernaryValue.TRUE) { reportChangeToEnclosingScope(forCondition); forCondition.replaceWith(IR.empty()); } diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java index 159ff31575b..02787c5793c 100644 --- a/test/com/google/javascript/jscomp/NodeUtilTest.java +++ b/test/com/google/javascript/jscomp/NodeUtilTest.java @@ -198,97 +198,87 @@ public static final class BooleanValueTests { @Parameter(0) public String jsExpression; - /** Expected result of NodeUtil.getPureBooleanValue() */ - @Parameter(1) - public TernaryValue expectedPureResult; - /** Expected result of NodeUtil.getLiteralBooleanValue() */ - @Parameter(2) + @Parameter(1) public TernaryValue expectedLiteralResult; /** Expected result of NodeUtil.getImpureBooleanValue() */ - @Parameter(3) + @Parameter(2) public TernaryValue expectedImpureResult; - @Parameters(name = "\"{0}\" is pure ({1}) literal ({2}) impure ({3})") + @Parameters(name = "\"{0}\" is literal ({1}) impure ({2})") public static Iterable cases() { return ImmutableList.copyOf( new Object[][] { // truly literal, side-effect free values are always known - {"true", TernaryValue.TRUE, TernaryValue.TRUE, TernaryValue.TRUE}, - {"10", TernaryValue.TRUE, TernaryValue.TRUE, TernaryValue.TRUE}, - {"'0'", TernaryValue.TRUE, TernaryValue.TRUE, TernaryValue.TRUE}, - {"/a/", TernaryValue.TRUE, TernaryValue.TRUE, TernaryValue.TRUE}, - {"{}", TernaryValue.TRUE, TernaryValue.TRUE, TernaryValue.TRUE}, - {"[]", TernaryValue.TRUE, TernaryValue.TRUE, TernaryValue.TRUE}, - {"false", TernaryValue.FALSE, TernaryValue.FALSE, TernaryValue.FALSE}, - {"null", TernaryValue.FALSE, TernaryValue.FALSE, TernaryValue.FALSE}, - {"0", TernaryValue.FALSE, TernaryValue.FALSE, TernaryValue.FALSE}, - {"''", TernaryValue.FALSE, TernaryValue.FALSE, TernaryValue.FALSE}, - {"undefined", TernaryValue.FALSE, TernaryValue.FALSE, TernaryValue.FALSE}, + {"true", TernaryValue.TRUE, TernaryValue.TRUE}, + {"10", TernaryValue.TRUE, TernaryValue.TRUE}, + {"'0'", TernaryValue.TRUE, TernaryValue.TRUE}, + {"/a/", TernaryValue.TRUE, TernaryValue.TRUE}, + {"{}", TernaryValue.TRUE, TernaryValue.TRUE}, + {"[]", TernaryValue.TRUE, TernaryValue.TRUE}, + {"false", TernaryValue.FALSE, TernaryValue.FALSE}, + {"null", TernaryValue.FALSE, TernaryValue.FALSE}, + {"0", TernaryValue.FALSE, TernaryValue.FALSE}, + {"''", TernaryValue.FALSE, TernaryValue.FALSE}, + {"undefined", TernaryValue.FALSE, TernaryValue.FALSE}, // literals that have side-effects aren't pure - {"{a:foo()}", TernaryValue.UNKNOWN, TernaryValue.TRUE, TernaryValue.TRUE}, - {"[foo()]", TernaryValue.UNKNOWN, TernaryValue.TRUE, TernaryValue.TRUE}, + {"{a:foo()}", TernaryValue.TRUE, TernaryValue.TRUE}, + {"[foo()]", TernaryValue.TRUE, TernaryValue.TRUE}, // not really literals, but we pretend they are for our purposes - {"void 0", TernaryValue.FALSE, TernaryValue.FALSE, TernaryValue.FALSE}, + {"void 0", TernaryValue.FALSE, TernaryValue.FALSE}, // side-effect keeps this one from being pure - {"void foo()", TernaryValue.UNKNOWN, TernaryValue.FALSE, TernaryValue.FALSE}, - {"!true", TernaryValue.FALSE, TernaryValue.FALSE, TernaryValue.FALSE}, - {"!false", TernaryValue.TRUE, TernaryValue.TRUE, TernaryValue.TRUE}, - {"!''", TernaryValue.TRUE, TernaryValue.TRUE, TernaryValue.TRUE}, - {"class Klass {}", TernaryValue.TRUE, TernaryValue.TRUE, TernaryValue.TRUE}, - {"new Date()", TernaryValue.TRUE, TernaryValue.TRUE, TernaryValue.TRUE}, - {"b", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, - {"-'0.0'", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, + {"void foo()", TernaryValue.FALSE, TernaryValue.FALSE}, + {"!true", TernaryValue.FALSE, TernaryValue.FALSE}, + {"!false", TernaryValue.TRUE, TernaryValue.TRUE}, + {"!''", TernaryValue.TRUE, TernaryValue.TRUE}, + {"class Klass {}", TernaryValue.TRUE, TernaryValue.TRUE}, + {"new Date()", TernaryValue.TRUE, TernaryValue.TRUE}, + {"b", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, + {"-'0.0'", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, // template literals - {"``", TernaryValue.FALSE, TernaryValue.FALSE, TernaryValue.FALSE}, - {"`definiteLength`", TernaryValue.TRUE, TernaryValue.TRUE, TernaryValue.TRUE}, - {"`${some}str`", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, + {"``", TernaryValue.FALSE, TernaryValue.FALSE}, + {"`definiteLength`", TernaryValue.TRUE, TernaryValue.TRUE}, + {"`${some}str`", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, // non-literal expressions - {"a=true", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.TRUE}, - {"a=false", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.FALSE}, - {"a=(false,true)", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.TRUE}, - {"a=(true,false)", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.FALSE}, - {"a=(false || true)", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.TRUE}, - {"a=(true && false)", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.FALSE}, - {"a=!(true && false)", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.TRUE}, - {"a,true", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.TRUE}, - {"a,false", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.FALSE}, - {"true||false", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.TRUE}, - {"false||false", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.FALSE}, - {"true&&true", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.TRUE}, - {"true&&false", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.FALSE}, + {"a=true", TernaryValue.UNKNOWN, TernaryValue.TRUE}, + {"a=false", TernaryValue.UNKNOWN, TernaryValue.FALSE}, + {"a=(false,true)", TernaryValue.UNKNOWN, TernaryValue.TRUE}, + {"a=(true,false)", TernaryValue.UNKNOWN, TernaryValue.FALSE}, + {"a=(false || true)", TernaryValue.UNKNOWN, TernaryValue.TRUE}, + {"a=(true && false)", TernaryValue.UNKNOWN, TernaryValue.FALSE}, + {"a=!(true && false)", TernaryValue.UNKNOWN, TernaryValue.TRUE}, + {"a,true", TernaryValue.UNKNOWN, TernaryValue.TRUE}, + {"a,false", TernaryValue.UNKNOWN, TernaryValue.FALSE}, + {"true||false", TernaryValue.UNKNOWN, TernaryValue.TRUE}, + {"false||false", TernaryValue.UNKNOWN, TernaryValue.FALSE}, + {"true&&true", TernaryValue.UNKNOWN, TernaryValue.TRUE}, + {"true&&false", TernaryValue.UNKNOWN, TernaryValue.FALSE}, // Assignment ops other than ASSIGN are unknown. - {"a *= 2", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, + {"a *= 2", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, // Complex expressions that contain anything other then "=", ",", or "!" are // unknown. - {"2 + 2", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, + {"2 + 2", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, // assignment values are the RHS - {"a=1", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.TRUE}, - {"a=/a/", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.TRUE}, - {"a={}", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.TRUE}, + {"a=1", TernaryValue.UNKNOWN, TernaryValue.TRUE}, + {"a=/a/", TernaryValue.UNKNOWN, TernaryValue.TRUE}, + {"a={}", TernaryValue.UNKNOWN, TernaryValue.TRUE}, // hooks have impure boolean value if both cases have same impure boolean value - {"a?true:true", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.TRUE}, - {"a?false:false", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.FALSE}, - {"a?true:false", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, - {"a?true:foo()", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, + {"a?true:true", TernaryValue.UNKNOWN, TernaryValue.TRUE}, + {"a?false:false", TernaryValue.UNKNOWN, TernaryValue.FALSE}, + {"a?true:false", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, + {"a?true:foo()", TernaryValue.UNKNOWN, TernaryValue.UNKNOWN}, }); } - @Test - public void getPureBooleanValue() { - assertThat(NodeUtil.getPureBooleanValue(parseExpr(jsExpression))) - .isEqualTo(expectedPureResult); - } - @Test public void getLiteralBooleanValue() { assertThat(NodeUtil.getLiteralBooleanValue(parseExpr(jsExpression)))