Fix overly aggressive pruning when expressions may have side-effects.

	Change on 2015/10/26 by kstanger <kstanger@google.com>
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=106314151

translator/src/main/java/com/google/devtools/j2objc/translate/ConstantBranchPruner.java

@@ -27,6 +27,7 @@
 import com.google.devtools.j2objc.ast.PrefixExpression;
 import com.google.devtools.j2objc.ast.TreeVisitor;
 import com.google.devtools.j2objc.ast.WhileStatement;
+import com.google.devtools.j2objc.util.TranslationUtil;
 
 import java.util.Iterator;
 import java.util.List;
@@ -60,23 +61,26 @@ public void endVisit(InfixExpression node) {
       return;
     }
     List<Expression> operands = node.getOperands();
+    boolean foundSideEffect = false;
     for (Iterator<Expression> it = operands.iterator(); it.hasNext(); ) {
-      Boolean constantVal = getValue(it.next());
+      Expression expr = it.next();
+      foundSideEffect |= TranslationUtil.hasSideEffect(expr);
+      Boolean constantVal = getValue(expr);
       if (constantVal == null) {
         continue;
       }
-      if (constantVal && operator == CONDITIONAL_OR) {
-        // Whole expression is true.
-        node.replaceWith(new BooleanLiteral(true, typeEnv));
-        return;
-      }
-      if (!constantVal && operator == CONDITIONAL_AND) {
-        // Whole expression is false.
-        node.replaceWith(new BooleanLiteral(false, typeEnv));
-        return;
+      if (constantVal == (operator == CONDITIONAL_OR)) {
+        // Whole expression evaluates to 'constantVal'.
+        if (!foundSideEffect) {
+          // We can safely prune the whole infix if the precededing expressions
+          // have no side effects.
+          node.replaceWith(new BooleanLiteral(constantVal, typeEnv));
+          return;
+        }
+      } else {
+        // Else remove unnecessary constant value.
+        it.remove();
       }
-      // Else remove unnecessary constant value.
-      it.remove();
     }
 
     if (operands.size() == 0) {

translator/src/main/java/com/google/devtools/j2objc/translate/StaticVarRewriter.java

@@ -77,22 +77,6 @@ private void rewriteStaticAccess(Expression node) {
     node.replaceWith(newNode);
   }
 
-  /**
-   * Reterns whether the expression of a FieldAccess node has any side effects.
-   * If false, the expression can be removed from the tree.
-   * @param expr The expression of a FieldAccess node for a static variable.
-   */
-  private boolean fieldAccessExpressionHasSideEffect(Expression expr) {
-    switch (expr.getKind()) {
-      case QUALIFIED_NAME:
-      case SIMPLE_NAME:
-      case THIS_EXPRESSION:
-        return false;
-      default:
-        return true;
-    }
-  }
-
   @Override
   public boolean visit(FieldAccess node) {
     IVariableBinding var = node.getVariableBinding();
@@ -103,7 +87,7 @@ public boolean visit(FieldAccess node) {
 
     Expression expr = TreeUtil.remove(node.getExpression());
     Expression varNode = TreeUtil.remove(node.getName());
-    if (!fieldAccessExpressionHasSideEffect(expr)) {
+    if (!TranslationUtil.hasSideEffect(expr)) {
       node.replaceWith(varNode);
       varNode.accept(this);
       return false;

translator/src/main/java/com/google/devtools/j2objc/util/TranslationUtil.java

@@ -20,8 +20,12 @@
 import com.google.devtools.j2objc.ast.ArrayAccess;
 import com.google.devtools.j2objc.ast.ArrayCreation;
 import com.google.devtools.j2objc.ast.Assignment;
+import com.google.devtools.j2objc.ast.CastExpression;
 import com.google.devtools.j2objc.ast.ClassInstanceCreation;
+import com.google.devtools.j2objc.ast.ConditionalExpression;
 import com.google.devtools.j2objc.ast.Expression;
+import com.google.devtools.j2objc.ast.FieldAccess;
+import com.google.devtools.j2objc.ast.InfixExpression;
 import com.google.devtools.j2objc.ast.MethodInvocation;
 import com.google.devtools.j2objc.ast.PackageDeclaration;
 import com.google.devtools.j2objc.ast.ParenthesizedExpression;
@@ -180,6 +184,55 @@ public static boolean isAssigned(Expression node) {
     return false;
   }
 
+  /**
+   * Reterns whether the expression might have any side effects. If true, it
+   * would be unsafe to prune the given node from the tree.
+   */
+  public static boolean hasSideEffect(Expression expr) {
+    switch (expr.getKind()) {
+      case BOOLEAN_LITERAL:
+      case CHARACTER_LITERAL:
+      case NULL_LITERAL:
+      case NUMBER_LITERAL:
+      case QUALIFIED_NAME:
+      case SIMPLE_NAME:
+      case STRING_LITERAL:
+      case SUPER_FIELD_ACCESS:
+      case THIS_EXPRESSION:
+        return false;
+      case CAST_EXPRESSION:
+        return hasSideEffect(((CastExpression) expr).getExpression());
+      case CONDITIONAL_EXPRESSION:
+        {
+          ConditionalExpression condExpr = (ConditionalExpression) expr;
+          return hasSideEffect(condExpr.getExpression())
+              || hasSideEffect(condExpr.getThenExpression())
+              || hasSideEffect(condExpr.getElseExpression());
+        }
+      case FIELD_ACCESS:
+        return hasSideEffect(((FieldAccess) expr).getExpression());
+      case INFIX_EXPRESSION:
+        for (Expression operand : ((InfixExpression) expr).getOperands()) {
+          if (hasSideEffect(operand)) {
+            return true;
+          }
+        }
+        return false;
+      case PARENTHESIZED_EXPRESSION:
+        return hasSideEffect(((ParenthesizedExpression) expr).getExpression());
+      case PREFIX_EXPRESSION:
+        {
+          PrefixExpression preExpr = (PrefixExpression) expr;
+          PrefixExpression.Operator op = preExpr.getOperator();
+          return op == PrefixExpression.Operator.INCREMENT
+              || op == PrefixExpression.Operator.DECREMENT
+              || hasSideEffect(preExpr.getOperand());
+        }
+      default:
+        return true;
+    }
+  }
+
   /**
    * Returns the modifier for an assignment expression being converted to a
    * function. The result will be "Array" if the lhs is an array access,

translator/src/test/java/com/google/devtools/j2objc/translate/ConstantBranchPrunerTest.java

@@ -148,8 +148,11 @@ public void testOr() throws IOException {
         "{", "result = 8;", "}");
   }
 
-  // Verify method invocation paired with constant is pruned.
+  // Verify method invocation paired with constant is not pruned because methods
+  // may have side effects.
   public void testMethodAnd() throws IOException {
+    // TODO(kstanger): Find a way to prune the unreachable branches while
+    // preserving the portions of the expression that have side effects.
     String translation = translateSourceFile(
         "class Test { "
         + "static final boolean DEBUG = true; "
@@ -160,9 +163,8 @@ public void testMethodAnd() throws IOException {
         + "  if (enabled() || DEBUG) { result = 3; } else { result = 4; }"
         + "  return result; }}",
         "Test", "Test.m");
-    assertTranslatedLines(translation,
-        "{", "result = 2;", "}",
-        "{", "result = 3;", "}");
+    assertTranslation(translation, "if ([self enabled] && Test_NDEBUG)");
+    assertTranslation(translation, "if ([self enabled] || Test_DEBUG)");
   }
 
   public void testExpressionPruning() throws IOException {