diff --git a/dev/core/src/com/google/gwt/dev/js/JsStaticEval.java b/dev/core/src/com/google/gwt/dev/js/JsStaticEval.java index aeab3b28cd..a86a5613ec 100644 --- a/dev/core/src/com/google/gwt/dev/js/JsStaticEval.java +++ b/dev/core/src/com/google/gwt/dev/js/JsStaticEval.java @@ -50,11 +50,11 @@ import com.google.gwt.dev.js.ast.JsWhile; import com.google.gwt.dev.js.rhino.ScriptRuntime; import com.google.gwt.dev.util.Ieee754_64_Arithmetic; +import com.google.gwt.thirdparty.guava.common.annotations.VisibleForTesting; import java.util.ArrayList; import java.util.EnumSet; -import java.util.HashSet; -import java.util.IdentityHashMap; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -104,7 +104,7 @@ private static class MustExecVisitor extends JsVisitor { private final List mustExec = new ArrayList(); - public MustExecVisitor() { + MustExecVisitor() { } @Override @@ -133,7 +133,7 @@ public void endVisit(JsVars x, JsContext ctx) { } } - public List getStatements() { + private List getStatements() { return mustExec; } @@ -144,6 +144,17 @@ public boolean visit(JsFunction x, JsContext ctx) { } } + /** + * Describes how is the result of evaluation used. + * + *

In the future also string and number mode might be considered. + * #10261 removed unused string mode optimizations

+ */ + private enum EvalMode { + BOOL, // result will be coerced to a boolean + VOID // result will be discarded + } + /** * Does static evals. * @@ -151,14 +162,35 @@ public boolean visit(JsFunction x, JsContext ctx) { * {@link com.google.gwt.dev.jjs.impl.DeadCodeElimination}, such as ignored * expression results. */ - private class StaticEvalVisitor extends JsModVisitor { - - private Set evalBooleanContext = new HashSet(); + @VisibleForTesting + static class StaticEvalVisitor extends JsModVisitor { /** - * This is used by {@link #additionCoercesToString}. + * Stores how are expression evaluations used. + * Missing entry = no coercion, difference between null and false matters. */ - private Map coercesToStringMap = new IdentityHashMap(); + private final Map evalContext = new HashMap<>(); + + @Override + public boolean visit(JsExprStmt x, JsContext ctx) { + evalContext.put(x.getExpression(), EvalMode.VOID); + return true; + } + + @Override + public boolean visit(JsBinaryOperation x, JsContext ctx) { + if (evalContext.containsKey(x) + && (x.getOperator() == JsBinaryOperator.AND || x.getOperator() == JsBinaryOperator.OR)) { + evalContext.put(x.getArg1(), EvalMode.BOOL); + evalContext.put(x.getArg2(), evalContext.get(x)); + } else if (x.getOperator() == JsBinaryOperator.COMMA) { + evalContext.put(x.getArg1(), EvalMode.VOID); + if (evalContext.containsKey(x)) { + evalContext.put(x.getArg2(), evalContext.get(x)); + } + } + return true; + } @Override public void endVisit(JsBinaryOperation x, JsContext ctx) { @@ -167,11 +199,13 @@ public void endVisit(JsBinaryOperation x, JsContext ctx) { JsExpression arg2 = x.getArg2(); JsExpression result = x; - - if (op == JsBinaryOperator.AND) { - result = shortCircuitAnd(x); + if (evalContext.get(x) == EvalMode.VOID + && !arg2.hasSideEffects() && !op.isAssignment()) { + result = arg1; + } else if (op == JsBinaryOperator.AND) { + result = shortCircuitAnd(x, evalContext); } else if (op == JsBinaryOperator.OR) { - result = shortCircuitOr(x); + result = shortCircuitOr(x, evalContext); } else if (op == JsBinaryOperator.COMMA) { result = trySimplifyComma(x); } else if (op == JsBinaryOperator.EQ || op == JsBinaryOperator.REF_EQ) { @@ -195,7 +229,8 @@ public void endVisit(JsBinaryOperation x, JsContext ctx) { break; } } - + evalContext.remove(arg1); + evalContext.remove(arg2); result = maybeReorderOperations(result); if (result != x) { @@ -256,21 +291,29 @@ public void endVisit(JsBlock x, JsContext ctx) { @Override public void endVisit(JsConditional x, JsContext ctx) { - evalBooleanContext.remove(x.getTestExpression()); + evalContext.remove(x.getTestExpression()); + evalContext.remove(x.getThenExpression()); + evalContext.remove(x.getElseExpression()); JsExpression condExpr = x.getTestExpression(); JsExpression thenExpr = x.getThenExpression(); JsExpression elseExpr = x.getElseExpression(); - if (condExpr instanceof CanBooleanEval) { + if (condExpr instanceof JsBinaryOperation + && ((JsBinaryOperation) condExpr).getOperator() == JsBinaryOperator.COMMA) { + JsBinaryOperation condition = (JsBinaryOperation) condExpr; + JsExpression newConditional = accept(new JsConditional(x.getSourceInfo(), + condition.getArg2(), thenExpr, elseExpr)); + ctx.replaceMe(withSideEffect(condition.getArg1(), newConditional, x.getSourceInfo())); + } else if (condExpr instanceof CanBooleanEval) { CanBooleanEval condEval = (CanBooleanEval) condExpr; if (condEval.isBooleanTrue()) { JsBinaryOperation binOp = new JsBinaryOperation(x.getSourceInfo(), - JsBinaryOperator.AND, condExpr, thenExpr); + JsBinaryOperator.COMMA, condExpr, thenExpr); ctx.replaceMe(accept(binOp)); } else if (condEval.isBooleanFalse()) { - // e.g. (false() ? then : else) -> false() || else + // e.g. (false() ? then : else) -> (false() , else) JsBinaryOperation binOp = new JsBinaryOperation(x.getSourceInfo(), - JsBinaryOperator.OR, condExpr, elseExpr); + JsBinaryOperator.COMMA, condExpr, elseExpr); ctx.replaceMe(accept(binOp)); } } @@ -281,7 +324,7 @@ public void endVisit(JsConditional x, JsContext ctx) { */ @Override public void endVisit(JsDoWhile x, JsContext ctx) { - evalBooleanContext.remove(x.getCondition()); + evalContext.remove(x.getCondition()); JsExpression expr = x.getCondition(); if (expr instanceof CanBooleanEval) { @@ -304,6 +347,7 @@ public void endVisit(JsDoWhile x, JsContext ctx) { @Override public void endVisit(JsExprStmt x, JsContext ctx) { + evalContext.remove(x.getExpression()); if (!x.getExpression().hasSideEffects()) { if (ctx.canRemove()) { ctx.removeMe(); @@ -318,7 +362,7 @@ public void endVisit(JsExprStmt x, JsContext ctx) { */ @Override public void endVisit(JsFor x, JsContext ctx) { - evalBooleanContext.remove(x.getCondition()); + evalContext.remove(x.getCondition()); JsExpression expr = x.getCondition(); if (expr instanceof CanBooleanEval) { @@ -348,7 +392,7 @@ public void endVisit(JsFor x, JsContext ctx) { */ @Override public void endVisit(JsIf x, JsContext ctx) { - evalBooleanContext.remove(x.getIfExpr()); + evalContext.remove(x.getIfExpr()); JsExpression condExpr = x.getIfExpr(); if (condExpr instanceof CanBooleanEval) { @@ -402,10 +446,10 @@ public void endVisit(JsIf x, JsContext ctx) { @Override public void endVisit(JsPrefixOperation x, JsContext ctx) { if (x.getOperator() == JsUnaryOperator.NOT) { - evalBooleanContext.remove(x.getArg()); + evalContext.remove(x.getArg()); } - if (evalBooleanContext.contains(x)) { + if (evalContext.containsKey(x)) { if ((x.getOperator() == JsUnaryOperator.NOT) && (x.getArg() instanceof JsPrefixOperation)) { JsPrefixOperation arg = (JsPrefixOperation) x.getArg(); @@ -422,7 +466,7 @@ public void endVisit(JsPrefixOperation x, JsContext ctx) { */ @Override public void endVisit(JsWhile x, JsContext ctx) { - evalBooleanContext.remove(x.getCondition()); + evalContext.remove(x.getCondition()); JsExpression expr = x.getCondition(); if (expr instanceof CanBooleanEval) { @@ -443,88 +487,47 @@ public void endVisit(JsWhile x, JsContext ctx) { @Override public boolean visit(JsConditional x, JsContext ctx) { - evalBooleanContext.add(x.getTestExpression()); + evalContext.put(x.getTestExpression(), EvalMode.BOOL); + EvalMode evalMode = evalContext.get(x); + if (evalMode != null) { + evalContext.put(x.getThenExpression(), evalMode); + evalContext.put(x.getElseExpression(), evalMode); + } return true; } @Override public boolean visit(JsDoWhile x, JsContext ctx) { - evalBooleanContext.add(x.getCondition()); + evalContext.put(x.getCondition(), EvalMode.BOOL); return true; } @Override public boolean visit(JsFor x, JsContext ctx) { - evalBooleanContext.add(x.getCondition()); + evalContext.put(x.getCondition(), EvalMode.BOOL); return true; } @Override public boolean visit(JsIf x, JsContext ctx) { - evalBooleanContext.add(x.getIfExpr()); + evalContext.put(x.getIfExpr(), EvalMode.BOOL); return true; } @Override public boolean visit(JsPrefixOperation x, JsContext ctx) { if (x.getOperator() == JsUnaryOperator.NOT) { - evalBooleanContext.add(x.getArg()); + evalContext.put(x.getArg(), EvalMode.BOOL); } return true; } @Override public boolean visit(JsWhile x, JsContext ctx) { - evalBooleanContext.add(x.getCondition()); + evalContext.put(x.getCondition(), EvalMode.BOOL); return true; } - /** - * Given an expression, determine if the addition operator would cause a - * string coercion to happen. - */ - private boolean additionCoercesToString(JsExpression expr) { - if (expr instanceof JsStringLiteral) { - return true; - } - - /* - * Because the nodes passed into this method are visited on exit, it is - * worthwhile to memoize the result for this function. - */ - Boolean toReturn = coercesToStringMap.get(expr); - if (toReturn != null) { - return toReturn; - } - toReturn = false; - - if (expr instanceof JsBinaryOperation) { - JsBinaryOperation op = (JsBinaryOperation) expr; - switch (op.getOperator()) { - case ADD: - toReturn = additionCoercesToString(op.getArg1()) - || additionCoercesToString(op.getArg2()); - break; - case COMMA: - toReturn = additionCoercesToString(op.getArg2()); - break; - } - - if (op.getOperator().isAssignment()) { - toReturn = additionCoercesToString(op.getArg2()); - } - } - - /* - * TODO: Consider adding heuristics to detect String(foo), typeof(foo), - * and foo.toString(). The latter is debatable, since an implementation - * might not actually return a string. - */ - - coercesToStringMap.put(expr, toReturn); - return toReturn; - } - /** * This method MUST be called whenever any statements are removed from a * function. This is because some statements, such as JsVars or JsFunction @@ -606,42 +609,86 @@ public static int exec(JsProgram program) { /** * Simplify short circuit AND expressions. * + *

In all contexts apply the following simplifications:

* 
-   * if (true && isWhatever()) -> if (isWhatever()), unless side effects
-   * if (false() && isWhatever()) -> if (false())
+   * return true() && isWhatever() -> return isWhatever(), unless true() has side effects
+   * return false() && isWhatever() -> return false()
+   *
+ * In boolean context also apply these: + * 
+   * if (isWhatever() && true()) -> if (isWhatever()) unless true() has side effects
+   * if (isWhatever() && false()) -> if (false()) unless isWhatever() side effects
    *
*/ - protected static JsExpression shortCircuitAnd(JsBinaryOperation expr) { + protected static JsExpression shortCircuitAnd(JsBinaryOperation expr, + Map evalContext) { JsExpression arg1 = expr.getArg1(); JsExpression arg2 = expr.getArg2(); if (arg1 instanceof CanBooleanEval) { CanBooleanEval eval1 = (CanBooleanEval) arg1; - if (eval1.isBooleanTrue() && !arg1.hasSideEffects()) { - return arg2; + if (eval1.isBooleanTrue()) { + return withSideEffect(arg1, arg2, expr.getSourceInfo()); } else if (eval1.isBooleanFalse()) { return arg1; } } + // technically this operation works in VOID mode too, + // but void expressions are optimized early in endVisit + if (arg2 instanceof CanBooleanEval && evalContext.get(expr) == EvalMode.BOOL) { + CanBooleanEval eval2 = (CanBooleanEval) arg2; + if (eval2.isBooleanTrue() && !arg2.hasSideEffects()) { + return arg1; + } else if (eval2.isBooleanFalse()) { + return withSideEffect(arg1, arg2, expr.getSourceInfo()); + } + } return expr; } + /** + * Returns either just {@code main} or {@code (sideEffect, main)}, depending on whether + * {@code sideEffect} actually has side effects. + */ + private static JsExpression withSideEffect(JsExpression sideEffect, JsExpression main, + SourceInfo sourceInfo) { + return !sideEffect.hasSideEffects() ? main + : new JsBinaryOperation(sourceInfo, JsBinaryOperator.COMMA, sideEffect, main); + } + /** * Simplify short circuit OR expressions. * + *

In all contexts apply the following simplifications:

* 
-   * if (true() || isWhatever()) -> if (true())
-   * if (false || isWhatever()) -> if (isWhatever()), unless side effects
+   * return false() || isWhatever() -> return isWhatever(), unless false() has side effects
+   * return true() && isWhatever() -> return true()
+   *
+ * In boolean context also apply these: + * 
+   * if (isWhatever() || false()) -> if (isWhatever()) unless false() has side effects
+   * if (isWhatever() || true()) -> if (true()) unless isWhatever() side effects
    *
*/ - protected static JsExpression shortCircuitOr(JsBinaryOperation expr) { + protected static JsExpression shortCircuitOr(JsBinaryOperation expr, + Map evalContext) { JsExpression arg1 = expr.getArg1(); JsExpression arg2 = expr.getArg2(); if (arg1 instanceof CanBooleanEval) { CanBooleanEval eval1 = (CanBooleanEval) arg1; - if (eval1.isBooleanTrue()) { + if (eval1.isBooleanFalse()) { + return withSideEffect(arg1, arg2, expr.getSourceInfo()); + } else if (eval1.isBooleanTrue()) { + return arg1; + } + } + // technically this operation works in VOID mode too, + // but void expressions are optimized early in endVisit + if (arg2 instanceof CanBooleanEval && evalContext.get(expr) == EvalMode.BOOL) { + CanBooleanEval eval2 = (CanBooleanEval) arg2; + if (eval2.isBooleanFalse() && !arg2.hasSideEffects()) { return arg1; - } else if (eval1.isBooleanFalse() && !arg1.hasSideEffects()) { - return arg2; + } else if (eval2.isBooleanTrue()) { + return withSideEffect(arg1, arg2, expr.getSourceInfo()); } } return expr; diff --git a/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java b/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java index 3177b09319..d2e7ca5c5f 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsStaticEvalTest.java @@ -43,7 +43,8 @@ public void testAssociativity() throws Exception { assertEquals("alert(a&&b||c&&d);", optimize("alert((a && b) || ( c && d));")); assertEquals("a(),b&&c();", optimize("a(), b && c()")); - assertEquals("a()&&b,c();", optimize("a() && b, c()")); + assertEquals("a()&&b(),c();", optimize("a() && b(), c()")); + assertEquals("a(),c();", optimize("a() && b, c()")); // Don't damage math expressions assertEquals("alert(seconds/3600);", @@ -53,8 +54,8 @@ public void testAssociativity() throws Exception { assertEquals("alert(1-(1-foo));", optimize("alert(1 - (1 - foo))")); // Don't damage assignments - assertEquals("alert((a=0,b=foo));", - optimize("alert((a = 0, b = (bar, foo)))")); + assertEquals("alert((a=7,b=foo));", + optimize("alert((a = 7, b = (bar, foo)))")); assertEquals("alert(1+(a='2')+3+4);", optimize("alert(1 + (a = '2') + 3 + 4);")); assertEquals("alert(1+(a='2')+7);", @@ -87,7 +88,8 @@ public void testAssociativity() throws Exception { /** * Test for issue 7088. JsStatic eval infinite loop in - * {@link JsStaticEval.StaticEvalVisitor#endVisit(JsBlock, JsContext)} + * {@link JsStaticEval.StaticEvalVisitor#endVisit(com.google.gwt.dev.js.ast.JsBlock, + * com.google.gwt.dev.js.ast.JsContext)} */ public void testDeclareAfterReturn() throws Exception { // TODO(rluble): Note that the source output has the wrong precedence for function definition @@ -154,6 +156,68 @@ public void testLiteralCompares() throws Exception { assertEquals("alert(true);", optimize("alert(\"a\" !== null)")); } + public void testShortCircuitAnd() throws Exception { + assertEquals("alert(a);", optimize("alert(true && a)")); + assertEquals("alert(false);", optimize("alert(false && a)")); + + // these can't be simplified to maintain type + assertEquals("alert(a&&true);", optimize("alert(a && true)")); + assertEquals("alert(a&&false);", optimize("alert(a && false)")); + assertEquals("alert(!!a&&!!b);", optimize("alert(!!a && !!b)")); + assertEquals("alert(c|(bits&&1));", optimize("alert(c | (a, bits && 1))")); + + // in boolean context we can simplify more + assertEquals("alert(a&&b?c:d);", optimize("alert(!!a && !!b ? c :d)")); + assertEquals("alert(d);", optimize("alert(false && !!b ? c :d)")); + assertEquals("alert(b?c:d);", optimize("alert(true && !!b ? c :d)")); + assertEquals("alert(b?c:d1);", optimize("alert(b && true ? c :d1)")); + assertEquals("alert(d1);", optimize("alert(b && false ? c :d1)")); + assertEquals("alert(d2);", optimize("alert(a && false && b ? c :d2)")); + } + + public void testShortCircuitAndWithSideEffects() throws Exception { + assertEquals("a&&b();", optimize("!!a && !!b()")); + assertEquals("a();", optimize("a() && false && c();")); + assertEquals("a(),e();", optimize("a() && false && c() ? d() : e();")); + assertEquals("a()&&c()?d():e();", optimize("a() && true && c() ? d() : e();")); + } + + public void testSimplifyCommaInVoidContext() throws Exception { + assertEquals("a()&&b();", optimize("a()&&(b(),undefined)")); + assertEquals("a()?b():c();", optimize("a()?(b(),undefined):(c(),undefined)")); + } + + public void testSimplifyComma() throws Exception { + assertEquals("alert(!!a());", optimize("alert((true, !!a()))")); + assertEquals("alert((a(),true));", optimize("alert((!!a(), true))")); + } + + public void testShortCircuitOr() throws Exception { + assertEquals("alert(true);", optimize("alert(true || a)")); + assertEquals("alert(a);", optimize("alert(false || a)")); + + // these can't be simplified to maintain type + assertEquals("alert(a||true);", optimize("alert(a || true)")); + assertEquals("alert(a||false);", optimize("alert(a || false)")); + assertEquals("alert(!!a||!!b);", optimize("alert(!!a || !!b)")); + assertEquals("alert(c|(bits||0));", optimize("alert(c | (a, bits || 0))")); + + // in boolean context we can simplify more + assertEquals("alert(a||b?c:d);", optimize("alert(!!a || !!b ? c :d)")); + assertEquals("alert(c);", optimize("alert(true || !!b ? c :d)")); + assertEquals("alert(b?c:d);", optimize("alert(false || !!b ? c :d)")); + assertEquals("alert(b?c:d1);", optimize("alert(b || false ? c :d1)")); + assertEquals("alert(c1);", optimize("alert(b || true ? c1 :d)")); + assertEquals("alert(c2);", optimize("alert(a || true || b ? c2 :d)")); + } + + public void testShortCircuitOrWithSideEffects() throws Exception { + assertEquals("a||b();", optimize("!!a || !!b()")); + assertEquals("a();", optimize("a() || true || c();")); + assertEquals("a(),d();", optimize("a() || true || c() ? d() : e();")); + assertEquals("a()||c()?d():e();", optimize("a() || false || c() ? d() : e();")); + } + public void testLiteralEqNull() throws Exception { assertEquals("alert(false);", optimize("alert('test' == null)")); }