[Fix] Folding concats needs to preserve conversion to string

Summary:
The code to fold concats was checking the wrong condition (at one point, this
condition would have worked, but it assumed that nothing touched the expected
type on the node being replaced - this was too fragile, and is no longer true).
Instead, we check that the expected type on the concat is String - and if not we
insert a string cast.

There was a similar issue with $x + 0 and $x * 1, but those were sufficiently
conservative that I couldnt actually reproduce a problem. The new checks should
be both correct, and less conservative.

Test Plan: fast_tests slow_tests

Reviewers: qigao, myang, andrewparoski

Reviewed By: myang

CC: ps, mwilliams, myang

Differential Revision: 340359

Task ID: 752902

src/compiler/expression/binary_op_expression.cpp

@@ -267,25 +267,29 @@ ExpressionPtr BinaryOpExpression::simplifyArithmetic(
       // 1 * $a => $a, 0 + $a => $a
       if ((ival1 == 1 && m_op == '*') || (ival1 == 0 && m_op == '+')) {
         TypePtr actType2 = m_exp2->getActualType();
-        TypePtr expType2 = m_exp2->getExpectedType();
-        if ((actType2 && actType2->mustBe(Type::KindOfNumeric)
-                      && actType2->isExactType()) ||
-            (expType2 && expType2->mustBe(Type::KindOfNumeric)
-                      && Type::IsCastNeeded(ar, actType2, expType2))) {
+        TypePtr expType = getExpectedType();
+        if (actType2 &&
+            (actType2->mustBe(Type::KindOfNumeric) ||
+             (expType && expType->mustBe(Type::KindOfNumeric) &&
+              !actType2->couldBe(Type::KindOfArray) &&
+              Type::IsCastNeeded(ar, actType2, expType)))) {
           return m_exp2;
         }
       }
     } else if (v1.isString()) {
       String sval1 = v1.toString();
       if ((sval1.empty() && m_op == '.')) {
         TypePtr actType2 = m_exp2->getActualType();
-        TypePtr expType2 = m_exp2->getExpectedType();
+        TypePtr expType = getExpectedType();
         // '' . $a => $a
-        if ((actType2 && actType2->is(Type::KindOfString)) ||
-            (expType2 && expType2->is(Type::KindOfString) &&
-             Type::IsCastNeeded(ar, actType2, expType2))) {
+        if ((expType && expType->is(Type::KindOfString)) ||
+            (actType2 && actType2->is(Type::KindOfString))) {
           return m_exp2;
         }
+        ExpressionPtr rep(new UnaryOpExpression(
+                            getScope(), getLocation(),
+                            m_exp2, T_STRING_CAST, true));
+        return rep;
       }
     }
   }
@@ -295,25 +299,29 @@ ExpressionPtr BinaryOpExpression::simplifyArithmetic(
       // $a * 1 => $a, $a + 0 => $a
       if ((ival2 == 1 && m_op == '*') || (ival2 == 0 && m_op == '+')) {
         TypePtr actType1 = m_exp1->getActualType();
-        TypePtr expType1 = m_exp1->getExpectedType();
-        if ((actType1 && actType1->mustBe(Type::KindOfNumeric)
-                      && actType1->isExactType()) ||
-            (expType1 && expType1->mustBe(Type::KindOfNumeric)
-                      && Type::IsCastNeeded(ar, actType1, expType1))) {
+        TypePtr expType = getExpectedType();
+        if (actType1 &&
+            (actType1->mustBe(Type::KindOfNumeric) ||
+             (expType && expType->mustBe(Type::KindOfNumeric) &&
+              !actType1->couldBe(Type::KindOfArray) &&
+              Type::IsCastNeeded(ar, actType1, expType)))) {
           return m_exp1;
         }
       }
     } else if (v2.isString()) {
       String sval2 = v2.toString();
       if ((sval2.empty() && m_op == '.')) {
         TypePtr actType1 = m_exp1->getActualType();
-        TypePtr expType1 = m_exp1->getExpectedType();
+        TypePtr expType = getExpectedType();
         // $a . '' => $a
-        if ((actType1 && actType1->is(Type::KindOfString)) ||
-            (expType1 && expType1->is(Type::KindOfString) &&
-             Type::IsCastNeeded(ar, actType1, expType1))) {
+        if ((expType && expType->is(Type::KindOfString)) ||
+            (actType1 && actType1->is(Type::KindOfString))) {
           return m_exp1;
         }
+        ExpressionPtr rep(new UnaryOpExpression(
+                            getScope(), getLocation(),
+                            m_exp1, T_STRING_CAST, true));
+        return rep;
       }
     }
   }

src/test/test_code_run.cpp

@@ -14439,6 +14439,18 @@ bool TestCodeRun::TestConcat() {
        "$c = 0;"
        "var_dump($a . $b == $c);");
 
+  MVCR("<?php "
+       "class C {"
+       "  public function __toString() {"
+       "    return 'bar';"
+       "  }"
+       "}"
+       "function f($x) {"
+       "  var_dump($x . '');"
+       "}"
+       "f(123);"
+       "f(new C);");
+
   return true;
 }