diff --git a/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java b/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java index c66478c937b..7afdba00acd 100644 --- a/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java +++ b/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java @@ -95,6 +95,41 @@ protected boolean mayHaveSideEffects(Node n) { return astAnalyzer.mayHaveSideEffects(n); } + /** + * Returns the number value of the node if it has one and it cannot have side effects. + * + *

Returns {@code null} otherwise. + */ + protected Double getSideEffectFreeNumberValue(Node n) { + Double value = NodeUtil.getNumberValue(n); + // Calculating the number 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 number value, but there could + // also be side effects. e.g. `void doSomething()` has value NaN, regardless of the behavior + // of `doSomething()` + if (value != null && astAnalyzer.mayHaveSideEffects(n)) { + value = null; + } + return value; + } + + /** + * Gets the value of a node as a String, or {@code null} if it cannot be converted. + * + *

This method effectively emulates the String() JavaScript cast function when + * possible and the node has no side effects. Otherwise, it returns {@code null}. + */ + protected String getSideEffectFreeStringValue(Node n) { + String value = NodeUtil.getStringValue(n); + // Calculating the string 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 string value, but there could + // also be side effects. e.g. `void doSomething()` has value 'undefined', regardless of the + // behavior of `doSomething()` + if (value != null && astAnalyzer.mayHaveSideEffects(n)) { + value = null; + } + return value; + } + /** * Returns true if the current node's type implies side effects. * diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index e72bcea00ab..33b17885116 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -346,11 +346,7 @@ static Double getNumberValue(Node n) { return n.getDouble(); case VOID: - if (mayHaveSideEffects(n.getFirstChild())) { - return null; - } else { - return Double.NaN; - } + return Double.NaN; case NAME: // Check for known constants diff --git a/src/com/google/javascript/jscomp/PeepholeCollectPropertyAssignments.java b/src/com/google/javascript/jscomp/PeepholeCollectPropertyAssignments.java index 8da146ff06c..5d8bf1005e9 100644 --- a/src/com/google/javascript/jscomp/PeepholeCollectPropertyAssignments.java +++ b/src/com/google/javascript/jscomp/PeepholeCollectPropertyAssignments.java @@ -235,7 +235,7 @@ private boolean collectObjectProperty(Node objectLiteral, Node propertyCandidate String propertyName; if (property.isNumber()) { - propertyName = NodeUtil.getStringValue(property); + propertyName = getSideEffectFreeStringValue(property); } else { propertyName = property.getString(); } diff --git a/src/com/google/javascript/jscomp/PeepholeFoldConstants.java b/src/com/google/javascript/jscomp/PeepholeFoldConstants.java index 97bea07416f..b80e5b2dbed 100644 --- a/src/com/google/javascript/jscomp/PeepholeFoldConstants.java +++ b/src/com/google/javascript/jscomp/PeepholeFoldConstants.java @@ -269,8 +269,8 @@ private void tryConvertToNumber(Node n) { break; } - Double result = NodeUtil.getNumberValue(n); - if (result == null) { + Double result = getSideEffectFreeNumberValue(n); + if (result == null || mayHaveSideEffects(n)) { return; } @@ -656,7 +656,9 @@ private Node tryFoldChildAddString(Node n, Node left, Node right) { // foo() + 2 + 'a' because we don't know what foo() will return, and // therefore we don't know if left is a string concat, or a numeric add. if (lr.isString()) { - String leftString = NodeUtil.getStringValue(lr); + String leftString = lr.getString(); + // Note that we don't have to call getSideEffectFreeStringValue() here because + // the enclosing logic guarantees that `right` is a literal value. String rightString = NodeUtil.getStringValue(right); if (leftString != null && rightString != null) { left.removeChild(ll); @@ -679,8 +681,10 @@ private Node tryFoldChildAddString(Node n, Node left, Node right) { // foo() + 2 + 'a' because we don't know what foo() will return, and // therefore we don't know if left is a string concat, or a numeric add. if (rl.isString()) { + // Note that we don't have to call getSideEffectFreeStringValue() here because + // the enclosing logic guarantees that `left` is a literal value. String leftString = NodeUtil.getStringValue(left); - String rightString = NodeUtil.getStringValue(rl); + String rightString = rl.getString(); if (leftString != null && rightString != null) { right.removeChild(rr); String result = leftString + rightString; @@ -702,8 +706,8 @@ private Node tryFoldAddConstantString(Node n, Node left, Node right) { if (left.isString() || right.isString() || left.isArrayLit() || right.isArrayLit()) { // Add strings. - String leftString = NodeUtil.getStringValue(left); - String rightString = NodeUtil.getStringValue(right); + String leftString = getSideEffectFreeStringValue(left); + String rightString = getSideEffectFreeStringValue(right); if (leftString != null && rightString != null) { Node newStringNode = IR.string(leftString + rightString); n.replaceWith(newStringNode); @@ -746,12 +750,16 @@ private Node performArithmeticOp(Token opType, Node left, Node right) { // TODO(johnlenz): Handle NaN with unknown value. BIT ops convert NaN // to zero so this is a little awkward here. - Double lValObj = NodeUtil.getNumberValue(left); - Double rValObj = NodeUtil.getNumberValue(right); + Double lValObj = getSideEffectFreeNumberValue(left); + Double rValObj = getSideEffectFreeNumberValue(right); // at least one of the two operands must have a value and both must be numeric if ((lValObj == null && rValObj == null) || !isNumeric(left) || !isNumeric(right)) { return null; } + // Cannot replace if either has side effects. + if (mayHaveSideEffects(left) || mayHaveSideEffects(right)) { + return null; + } // handle the operations that have algebraic identities, since we can simplify the tree without // actually knowing the value statically. switch (opType) { @@ -883,7 +891,7 @@ private Node tryFoldLeftChildOp(Node n, Node left, Node right) { // Use getNumberValue to handle constants like "NaN" and "Infinity" // other values are converted to numbers elsewhere. - Double rightValObj = NodeUtil.getNumberValue(right); + Double rightValObj = getSideEffectFreeNumberValue(right); if (rightValObj != null && left.getToken() == opType) { checkState(left.hasTwoChildren()); @@ -1026,15 +1034,18 @@ private Node tryFoldComparison(Node n, Node left, Node right) { } /** http://www.ecma-international.org/ecma-262/6.0/#sec-abstract-relational-comparison */ - private static TernaryValue tryAbstractRelationalComparison(Node left, Node right, + private static TernaryValue tryAbstractRelationalComparison( + AbstractPeepholeOptimization peepholeOptimization, + Node left, + Node right, boolean willNegate) { // First, try to evaluate based on the general type. ValueType leftValueType = NodeUtil.getKnownValueType(left); ValueType rightValueType = NodeUtil.getKnownValueType(right); if (leftValueType != ValueType.UNDETERMINED && rightValueType != ValueType.UNDETERMINED) { if (leftValueType == ValueType.STRING && rightValueType == ValueType.STRING) { - String lv = NodeUtil.getStringValue(left); - String rv = NodeUtil.getStringValue(right); + String lv = peepholeOptimization.getSideEffectFreeStringValue(left); + String rv = peepholeOptimization.getSideEffectFreeStringValue(right); if (lv != null && rv != null) { // In JS, browsers parse \v differently. So do not compare strings if one contains \v. if (lv.indexOf('\u000B') != -1 || rv.indexOf('\u000B') != -1) { @@ -1042,8 +1053,10 @@ private static TernaryValue tryAbstractRelationalComparison(Node left, Node righ } else { return TernaryValue.forBoolean(lv.compareTo(rv) < 0); } - } else if (left.isTypeOf() && right.isTypeOf() - && left.getFirstChild().isName() && right.getFirstChild().isName() + } else if (left.isTypeOf() + && right.isTypeOf() + && left.getFirstChild().isName() + && right.getFirstChild().isName() && left.getFirstChild().getString().equals(right.getFirstChild().getString())) { // Special case: `typeof a < typeof a` is always false. return TernaryValue.FALSE; @@ -1051,8 +1064,8 @@ private static TernaryValue tryAbstractRelationalComparison(Node left, Node righ } } // Then, try to evaluate based on the value of the node. Try comparing as numbers. - Double lv = NodeUtil.getNumberValue(left); - Double rv = NodeUtil.getNumberValue(right); + Double lv = peepholeOptimization.getSideEffectFreeNumberValue(left); + Double rv = peepholeOptimization.getSideEffectFreeNumberValue(right); if (lv == null || rv == null) { // Special case: `x < x` is always false. // @@ -1073,14 +1086,15 @@ private static TernaryValue tryAbstractRelationalComparison(Node left, Node righ } /** http://www.ecma-international.org/ecma-262/6.0/#sec-abstract-equality-comparison */ - private static TernaryValue tryAbstractEqualityComparison(Node left, Node right) { + private static TernaryValue tryAbstractEqualityComparison( + AbstractPeepholeOptimization peepholeOptimization, Node left, Node right) { // Evaluate based on the general type. ValueType leftValueType = NodeUtil.getKnownValueType(left); ValueType rightValueType = NodeUtil.getKnownValueType(right); if (leftValueType != ValueType.UNDETERMINED && rightValueType != ValueType.UNDETERMINED) { // Delegate to strict equality comparison for values of the same type. if (leftValueType == rightValueType) { - return tryStrictEqualityComparison(left, right); + return tryStrictEqualityComparison(peepholeOptimization, left, right); } if ((leftValueType == ValueType.NULL && rightValueType == ValueType.VOID) || (leftValueType == ValueType.VOID && rightValueType == ValueType.NULL)) { @@ -1088,17 +1102,17 @@ private static TernaryValue tryAbstractEqualityComparison(Node left, Node right) } if ((leftValueType == ValueType.NUMBER && rightValueType == ValueType.STRING) || rightValueType == ValueType.BOOLEAN) { - Double rv = NodeUtil.getNumberValue(right); + Double rv = peepholeOptimization.getSideEffectFreeNumberValue(right); return rv == null ? TernaryValue.UNKNOWN - : tryAbstractEqualityComparison(left, IR.number(rv)); + : tryAbstractEqualityComparison(peepholeOptimization, left, IR.number(rv)); } if ((leftValueType == ValueType.STRING && rightValueType == ValueType.NUMBER) || leftValueType == ValueType.BOOLEAN) { - Double lv = NodeUtil.getNumberValue(left); + Double lv = peepholeOptimization.getSideEffectFreeNumberValue(left); return lv == null ? TernaryValue.UNKNOWN - : tryAbstractEqualityComparison(IR.number(lv), right); + : tryAbstractEqualityComparison(peepholeOptimization, IR.number(lv), right); } if ((leftValueType == ValueType.STRING || leftValueType == ValueType.NUMBER) && rightValueType == ValueType.OBJECT) { @@ -1115,7 +1129,8 @@ private static TernaryValue tryAbstractEqualityComparison(Node left, Node right) } /** http://www.ecma-international.org/ecma-262/6.0/#sec-strict-equality-comparison */ - private static TernaryValue tryStrictEqualityComparison(Node left, Node right) { + private static TernaryValue tryStrictEqualityComparison( + AbstractPeepholeOptimization peepholeOptimization, Node left, Node right) { // First, try to evaluate based on the general type. ValueType leftValueType = NodeUtil.getKnownValueType(left); ValueType rightValueType = NodeUtil.getKnownValueType(right); @@ -1128,44 +1143,49 @@ private static TernaryValue tryStrictEqualityComparison(Node left, Node right) { case VOID: case NULL: return TernaryValue.TRUE; - case NUMBER: { - if (NodeUtil.isNaN(left)) { - return TernaryValue.FALSE; - } - if (NodeUtil.isNaN(right)) { - return TernaryValue.FALSE; - } - Double lv = NodeUtil.getNumberValue(left); - Double rv = NodeUtil.getNumberValue(right); - if (lv != null && rv != null) { - return TernaryValue.forBoolean(lv.doubleValue() == rv.doubleValue()); + case NUMBER: + { + if (NodeUtil.isNaN(left)) { + return TernaryValue.FALSE; + } + if (NodeUtil.isNaN(right)) { + return TernaryValue.FALSE; + } + Double lv = peepholeOptimization.getSideEffectFreeNumberValue(left); + Double rv = peepholeOptimization.getSideEffectFreeNumberValue(right); + if (lv != null && rv != null) { + return TernaryValue.forBoolean(lv.doubleValue() == rv.doubleValue()); + } + break; } - break; - } - case STRING: { - String lv = NodeUtil.getStringValue(left); - String rv = NodeUtil.getStringValue(right); - if (lv != null && rv != null) { - // In JS, browsers parse \v differently. So do not consider strings - // equal if one contains \v. - if (lv.indexOf('\u000B') != -1 || rv.indexOf('\u000B') != -1) { - return TernaryValue.UNKNOWN; - } else { - return lv.equals(rv) ? TernaryValue.TRUE : TernaryValue.FALSE; + case STRING: + { + String lv = peepholeOptimization.getSideEffectFreeStringValue(left); + String rv = peepholeOptimization.getSideEffectFreeStringValue(right); + if (lv != null && rv != null) { + // In JS, browsers parse \v differently. So do not consider strings + // equal if one contains \v. + if (lv.indexOf('\u000B') != -1 || rv.indexOf('\u000B') != -1) { + return TernaryValue.UNKNOWN; + } else { + return lv.equals(rv) ? TernaryValue.TRUE : TernaryValue.FALSE; + } + } else if (left.isTypeOf() + && right.isTypeOf() + && left.getFirstChild().isName() + && right.getFirstChild().isName() + && left.getFirstChild().getString().equals(right.getFirstChild().getString())) { + // Special case, typeof a == typeof a is always true. + return TernaryValue.TRUE; } - } else if (left.isTypeOf() && right.isTypeOf() - && left.getFirstChild().isName() && right.getFirstChild().isName() - && left.getFirstChild().getString().equals(right.getFirstChild().getString())) { - // Special case, typeof a == typeof a is always true. - return TernaryValue.TRUE; + break; + } + case BOOLEAN: + { + TernaryValue lv = NodeUtil.getPureBooleanValue(left); + TernaryValue rv = NodeUtil.getPureBooleanValue(right); + return lv.and(rv).or(lv.not().and(rv.not())); } - break; - } - case BOOLEAN: { - TernaryValue lv = NodeUtil.getPureBooleanValue(left); - TernaryValue rv = NodeUtil.getPureBooleanValue(right); - return lv.and(rv).or(lv.not().and(rv.not())); - } default: // Symbol and Object cannot be folded in the general case. return TernaryValue.UNKNOWN; } @@ -1189,21 +1209,21 @@ static TernaryValue evaluateComparison( switch (op) { case EQ: - return tryAbstractEqualityComparison(left, right); + return tryAbstractEqualityComparison(peepholeOptimization, left, right); case NE: - return tryAbstractEqualityComparison(left, right).not(); + return tryAbstractEqualityComparison(peepholeOptimization, left, right).not(); case SHEQ: - return tryStrictEqualityComparison(left, right); + return tryStrictEqualityComparison(peepholeOptimization, left, right); case SHNE: - return tryStrictEqualityComparison(left, right).not(); + return tryStrictEqualityComparison(peepholeOptimization, left, right).not(); case LT: - return tryAbstractRelationalComparison(left, right, false); + return tryAbstractRelationalComparison(peepholeOptimization, left, right, false); case GT: - return tryAbstractRelationalComparison(right, left, false); + return tryAbstractRelationalComparison(peepholeOptimization, right, left, false); case LE: - return tryAbstractRelationalComparison(right, left, true).not(); + return tryAbstractRelationalComparison(peepholeOptimization, right, left, true).not(); case GE: - return tryAbstractRelationalComparison(left, right, true).not(); + return tryAbstractRelationalComparison(peepholeOptimization, left, right, true).not(); default: break; } @@ -1269,11 +1289,7 @@ private Node tryFoldInForcedStringContext(Node n) { if (value == null) { stringValue = ""; } else { - if (!NodeUtil.isImmutableValue(value)) { - return n; - } - - stringValue = NodeUtil.getStringValue(value); + stringValue = getSideEffectFreeStringValue(value); } if (stringValue == null) { diff --git a/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java b/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java index 582830cbe18..6373d5fa494 100644 --- a/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java +++ b/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java @@ -91,7 +91,7 @@ private strictfp Node tryFoldKnownMathMethods(Node subtree, Node callTarget) { // first collect the arguments, if they are all numbers then we proceed List args = ImmutableList.of(); for (Node arg = callTarget.getNext(); arg != null; arg = arg.getNext()) { - Double d = NodeUtil.getNumberValue(arg); + Double d = getSideEffectFreeNumberValue(arg); if (d != null) { if (args.isEmpty()) { // lazily allocate, most calls will not be optimizable @@ -248,10 +248,10 @@ private Node tryFoldKnownStringMethods(Node subtree, Node callTarget) { || (stringNode.getJSType() != null && stringNode.getJSType().isStringValueType()))) { if (subtree.hasXChildren(3)) { - Double maybeStart = NodeUtil.getNumberValue(firstArg); + Double maybeStart = getSideEffectFreeNumberValue(firstArg); if (maybeStart != null) { int start = maybeStart.intValue(); - Double maybeLengthOrEnd = NodeUtil.getNumberValue(firstArg.getNext()); + Double maybeLengthOrEnd = getSideEffectFreeNumberValue(firstArg.getNext()); if (maybeLengthOrEnd != null) { switch (functionNameString) { case "substr": @@ -408,7 +408,7 @@ private Node tryFoldParseNumber( String stringVal = null; Double checkVal; if (firstArg.isNumber()) { - checkVal = NodeUtil.getNumberValue(firstArg); + checkVal = getSideEffectFreeNumberValue(firstArg); if (!(radix == 0 || radix == 10) && isParseInt) { //Convert a numeric first argument to a different base stringVal = String.valueOf(checkVal.intValue()); @@ -428,7 +428,7 @@ private Node tryFoldParseNumber( return numericNode; } } else { - stringVal = NodeUtil.getStringValue(firstArg); + stringVal = getSideEffectFreeStringValue(firstArg); if (stringVal == null) { return n; } @@ -510,10 +510,10 @@ private Node tryFoldStringIndexOf( checkArgument(n.isCall()); checkArgument(lstringNode.isString()); - String lstring = NodeUtil.getStringValue(lstringNode); + String lstring = lstringNode.getString(); boolean isIndexOf = functionName.equals("indexOf"); Node secondArg = firstArg.getNext(); - String searchValue = NodeUtil.getStringValue(firstArg); + String searchValue = getSideEffectFreeStringValue(firstArg); // searchValue must be a valid string. if (searchValue == null) { return n; @@ -567,6 +567,8 @@ private Node tryFoldArrayJoin(Node n) { reportChangeToEnclosingScope(n); } + // logic above ensures that `right` is immutable, so no need to check for + // side effects with getSideEffectFreeStringValue(right) String joinString = (right == null) ? "," : NodeUtil.getStringValue(right); List arrayFoldedChildren = new ArrayList<>(); StringBuilder sb = null; @@ -667,7 +669,7 @@ private Node tryFoldStringSubstr(Node n, Node stringNode, Node arg1) { int length; String stringAsString = stringNode.getString(); - Double maybeStart = NodeUtil.getNumberValue(arg1); + Double maybeStart = getSideEffectFreeNumberValue(arg1); if (maybeStart != null) { start = maybeStart.intValue(); } else { @@ -676,7 +678,7 @@ private Node tryFoldStringSubstr(Node n, Node stringNode, Node arg1) { Node arg2 = arg1.getNext(); if (arg2 != null) { - Double maybeLength = NodeUtil.getNumberValue(arg2); + Double maybeLength = getSideEffectFreeNumberValue(arg2); if (maybeLength != null) { length = maybeLength.intValue(); } else { @@ -723,7 +725,7 @@ private Node tryFoldStringSubstringOrSlice(Node n, Node stringNode, Node arg1) { int end; String stringAsString = stringNode.getString(); - Double maybeStart = NodeUtil.getNumberValue(arg1); + Double maybeStart = getSideEffectFreeNumberValue(arg1); if (maybeStart != null) { start = maybeStart.intValue(); } else { @@ -732,7 +734,7 @@ private Node tryFoldStringSubstringOrSlice(Node n, Node stringNode, Node arg1) { Node arg2 = arg1.getNext(); if (arg2 != null) { - Double maybeEnd = NodeUtil.getNumberValue(arg2); + Double maybeEnd = getSideEffectFreeNumberValue(arg2); if (maybeEnd != null) { end = maybeEnd.intValue(); } else { diff --git a/src/com/google/javascript/jscomp/PeepholeSubstituteAlternateSyntax.java b/src/com/google/javascript/jscomp/PeepholeSubstituteAlternateSyntax.java index b6d5517594c..b52fc101192 100644 --- a/src/com/google/javascript/jscomp/PeepholeSubstituteAlternateSyntax.java +++ b/src/com/google/javascript/jscomp/PeepholeSubstituteAlternateSyntax.java @@ -669,7 +669,7 @@ private Node tryTurnTemplateStringsToStrings(Node n) { if (n.getParent().isTaggedTemplateLit()) { return n; } - String string = NodeUtil.getStringValue(n); + String string = getSideEffectFreeStringValue(n); if (string == null) { return n; } diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java index 067a8cc4424..159ff31575b 100644 --- a/test/com/google/javascript/jscomp/NodeUtilTest.java +++ b/test/com/google/javascript/jscomp/NodeUtilTest.java @@ -2065,8 +2065,8 @@ public void testGetNumberValue() { assertThat(NodeUtil.getNumberValue(parseExpr("null"))).isEqualTo(0.0); assertThat(NodeUtil.getNumberValue(parseExpr("void 0"))).isNaN(); assertThat(NodeUtil.getNumberValue(parseExpr("void f"))).isNaN(); - // values with side-effects are ignored. - assertThat(NodeUtil.getNumberValue(parseExpr("void f()"))).isNull(); + // we pay no attention to possible side effects. + assertThat(NodeUtil.getNumberValue(parseExpr("void f()"))).isNaN(); assertThat(NodeUtil.getNumberValue(parseExpr("NaN"))).isNaN(); assertThat(NodeUtil.getNumberValue(parseExpr("Infinity"))).isPositiveInfinity(); assertThat(NodeUtil.getNumberValue(parseExpr("-Infinity"))).isNegativeInfinity();