Skip to content

Commit

Permalink
Drop unsafe type based optimizations that distinguish between null an…
Browse files Browse the repository at this point in the history 
…d undefined. For optimization purposes we have to assume that all values may contain null or undefined as the type system is "leaky" with regard to accessing undefined properties on object used as maps or access elements off the end of an array.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=173182612
  • Loading branch information
@concavelenz @blickly
concavelenz authored and blickly committed Oct 24, 2017
1 parent 7647534 commit 633c46a
Show file tree
Hide file tree
Showing 11 changed files with 72 additions and 295 deletions.
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/DefaultPassConfig.java
View file
Expand Up @@ -1723,7 +1723,7 @@ private static CompilerPass createPeepholeOptimizationsPass(
final boolean useTypesForOptimization = compiler.getOptions().useTypesForLocalOptimization;
List<AbstractPeepholeOptimization> optimizations = new ArrayList<>();
optimizations.add(new MinimizeExitPoints(compiler));
optimizations.add(new PeepholeMinimizeConditions(late, useTypesForOptimization));
optimizations.add(new PeepholeMinimizeConditions(late));
optimizations.add(new PeepholeSubstituteAlternateSyntax(late));
optimizations.add(new PeepholeReplaceKnownMethods(late, useTypesForOptimization));
optimizations.add(new PeepholeRemoveDeadCode());
Expand Down Expand Up @@ -1775,7 +1775,7 @@ protected CompilerPass create(AbstractCompiler compiler) {
getName(),
new StatementFusion(options.aggressiveFusion),
new PeepholeRemoveDeadCode(),
new PeepholeMinimizeConditions(late, useTypesForOptimization),
new PeepholeMinimizeConditions(late),
new PeepholeSubstituteAlternateSyntax(late),
new PeepholeReplaceKnownMethods(late, useTypesForOptimization),
new PeepholeFoldConstants(late, useTypesForOptimization),
Expand Down
16 changes: 1 addition & 15 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -298,10 +298,6 @@ static String arrayToString(Node literal) {
return result.toString();
}

public static Double getNumberValue(Node n) {
return getNumberValue(n, false);
}

/**
* Gets the value of a node as a Number, or null if it cannot be converted.
* When it returns a non-null Double, this method effectively emulates the
Expand All @@ -311,7 +307,7 @@ public static Double getNumberValue(Node n) {
* @param useType If true, return 0.0 if the type is null, and NaN if the type is undefined.
* @return The value of a node as a Number, or null if it cannot be converted.
*/
static Double getNumberValue(Node n, boolean useType) {
static Double getNumberValue(Node n) {
switch (n.getToken()) {
case TRUE:
return 1.0;
Expand Down Expand Up @@ -342,16 +338,6 @@ static Double getNumberValue(Node n, boolean useType) {
if (name.equals("Infinity")) {
return Double.POSITIVE_INFINITY;
}
if (useType) {
TypeI type = n.getTypeI();
if (type != null) {
if (type.isVoidType()) {
return Double.NaN;
} else if (type.isNullType()) {
return 0.0;
}
}
}
return null;

case NEG:
Expand Down
56 changes: 27 additions & 29 deletions src/com/google/javascript/jscomp/PeepholeFoldConstants.java
View file
Expand Up @@ -262,7 +262,7 @@ private void tryConvertToNumber(Node n) {
break;
}

Double result = NodeUtil.getNumberValue(n, shouldUseTypes);
Double result = NodeUtil.getNumberValue(n);
if (result == null) {
return;
}
Expand Down Expand Up @@ -739,11 +739,11 @@ 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, shouldUseTypes);
Double lValObj = NodeUtil.getNumberValue(left);
if (lValObj == null) {
return null;
}
Double rValObj = NodeUtil.getNumberValue(right, shouldUseTypes);
Double rValObj = NodeUtil.getNumberValue(right);
if (rValObj == null) {
return null;
}
Expand Down Expand Up @@ -819,7 +819,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, shouldUseTypes);
Double rightValObj = NodeUtil.getNumberValue(right);
if (rightValObj != null && left.getToken() == opType) {
checkState(left.hasTwoChildren());

Expand Down Expand Up @@ -852,8 +852,7 @@ private Node tryFoldAdd(Node node, Node left, Node right) {
checkArgument(node.isAdd());

if (NodeUtil.mayBeString(node, shouldUseTypes)) {
if (NodeUtil.isLiteralValue(left, false) &&
NodeUtil.isLiteralValue(right, false)) {
if (NodeUtil.isLiteralValue(left, false) && NodeUtil.isLiteralValue(right, false)) {
// '6' + 7
return tryFoldAddConstantString(node, left, right);
} else {
Expand Down Expand Up @@ -930,7 +929,7 @@ private Node tryFoldShift(Node n, Node left, Node right) {
* Try to fold comparison nodes, e.g ==
*/
private Node tryFoldComparison(Node n, Node left, Node right) {
TernaryValue result = evaluateComparison(n.getToken(), left, right, shouldUseTypes);
TernaryValue result = evaluateComparison(n.getToken(), left, right);
if (result == TernaryValue.UNKNOWN) {
return n;
}
Expand All @@ -945,7 +944,7 @@ 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,
boolean useTypes, boolean willNegate) {
boolean willNegate) {
// First, try to evaluate based on the general type.
ValueType leftValueType = NodeUtil.getKnownValueType(left);
ValueType rightValueType = NodeUtil.getKnownValueType(right);
Expand All @@ -969,8 +968,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, useTypes);
Double rv = NodeUtil.getNumberValue(right, useTypes);
Double lv = NodeUtil.getNumberValue(left);
Double rv = NodeUtil.getNumberValue(right);
if (lv == null || rv == null) {
// Special case: `x < x` is always false.
//
Expand All @@ -991,33 +990,32 @@ 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,
boolean useTypes) {
private static TernaryValue tryAbstractEqualityComparison(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, useTypes);
return tryStrictEqualityComparison(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, useTypes);
Double rv = NodeUtil.getNumberValue(right);
return rv == null
? TernaryValue.UNKNOWN
: tryAbstractEqualityComparison(left, IR.number(rv), useTypes);
: tryAbstractEqualityComparison(left, IR.number(rv));
}
if ((leftValueType == ValueType.STRING && rightValueType == ValueType.NUMBER)
|| leftValueType == ValueType.BOOLEAN) {
Double lv = NodeUtil.getNumberValue(left, useTypes);
Double lv = NodeUtil.getNumberValue(left);
return lv == null
? TernaryValue.UNKNOWN
: tryAbstractEqualityComparison(IR.number(lv), right, useTypes);
: tryAbstractEqualityComparison(IR.number(lv), right);
}
if ((leftValueType == ValueType.STRING || leftValueType == ValueType.NUMBER)
&& rightValueType == ValueType.OBJECT) {
Expand All @@ -1034,7 +1032,7 @@ 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, boolean useTypes) {
private static TernaryValue tryStrictEqualityComparison(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 @@ -1054,8 +1052,8 @@ private static TernaryValue tryStrictEqualityComparison(Node left, Node right, b
if (NodeUtil.isNaN(right)) {
return TernaryValue.FALSE;
}
Double lv = NodeUtil.getNumberValue(left, useTypes);
Double rv = NodeUtil.getNumberValue(right, useTypes);
Double lv = NodeUtil.getNumberValue(left);
Double rv = NodeUtil.getNumberValue(right);
if (lv != null && rv != null) {
return TernaryValue.forBoolean(lv.doubleValue() == rv.doubleValue());
}
Expand Down Expand Up @@ -1098,29 +1096,29 @@ private static TernaryValue tryStrictEqualityComparison(Node left, Node right, b
return TernaryValue.UNKNOWN;
}

static TernaryValue evaluateComparison(Token op, Node left, Node right, boolean useTypes) {
static TernaryValue evaluateComparison(Token op, Node left, Node right) {
// Don't try to minimize side-effects here.
if (NodeUtil.mayHaveSideEffects(left) || NodeUtil.mayHaveSideEffects(right)) {
return TernaryValue.UNKNOWN;
}

switch (op) {
case EQ:
return tryAbstractEqualityComparison(left, right, useTypes);
return tryAbstractEqualityComparison(left, right);
case NE:
return tryAbstractEqualityComparison(left, right, useTypes).not();
return tryAbstractEqualityComparison(left, right).not();
case SHEQ:
return tryStrictEqualityComparison(left, right, useTypes);
return tryStrictEqualityComparison(left, right);
case SHNE:
return tryStrictEqualityComparison(left, right, useTypes).not();
return tryStrictEqualityComparison(left, right).not();
case LT:
return tryAbstractRelationalComparison(left, right, useTypes, false);
return tryAbstractRelationalComparison(left, right, false);
case GT:
return tryAbstractRelationalComparison(right, left, useTypes, false);
return tryAbstractRelationalComparison(right, left, false);
case LE:
return tryAbstractRelationalComparison(right, left, useTypes, true).not();
return tryAbstractRelationalComparison(right, left, true).not();
case GE:
return tryAbstractRelationalComparison(left, right, useTypes, true).not();
return tryAbstractRelationalComparison(left, right, true).not();
default:
break;
}
Expand Down

0 comments on commit 633c46a

Please sign in to comment.