Skip to content

Commit

Permalink
Don't expect NodeUtil.getString|NumberValue() to check for side effects
Browse files Browse the repository at this point in the history 
This change helps to unblock a refactoring of mayHaveSideEffects()
s.t. it will always have access to the compiler object.

A recent change removed the one case where getStringValue() was actually
checking for side effects. The peephole optimizations may have been
relying on this behavior in some way that is hard to see clearly.
So, to reduce the risk of present or future bugs, I'm changing them
to explicitly check for side effects when calling getStringValue().

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=247508953
  • Loading branch information
@brad4d @EatingW
brad4d authored and EatingW committed May 10, 2019
1 parent 964833f commit a2ca418
Show file tree
Hide file tree
Showing 7 changed files with 140 additions and 91 deletions.
35 changes: 35 additions & 0 deletions src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java
View file
Expand Up @@ -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.
*
* <p>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.
*
* <p>This method effectively emulates the <code>String()</code> 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.
*
Expand Down
6 changes: 1 addition & 5 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/PeepholeCollectPropertyAssignments.java
View file
Expand Up @@ -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();
}
Expand Down
158 changes: 87 additions & 71 deletions src/com/google/javascript/jscomp/PeepholeFoldConstants.java
View file
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -1026,33 +1034,38 @@ 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) {
return TernaryValue.UNKNOWN;
} 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;
}
}
}
// 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.
//
Expand All @@ -1073,32 +1086,33 @@ 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)) {
return TernaryValue.TRUE;
}
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) {
Expand All @@ -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);
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit a2ca418

Please sign in to comment.