Stop Hack array compat notices from within BuiltinEnum methods

Summary: Stop Hack array compat notices from triggering from within BuiltinEnum methods by manually doing int-like key conversion. This also ensures that once darrays become dicts, their behavior won't change and can be dealt with separately. Reviewed By: paulbiss Differential Revision: D6886711 fbshipit-source-id: 92c44d7201c4904b06757044e1d5fb87cbc672d2
facebook · Feb 4, 2018 · 3e719a792ef2b92459c2fd820420a4a92085cd96 · 3e719a7
1 parent cc8e5dc
commit 3e719a792ef2b92459c2fd820420a4a92085cd96
diff --git a/hphp/runtime/base/enum-cache.cpp b/hphp/runtime/base/enum-cache.cpp
@@ -150,7 +150,15 @@ const EnumValues* EnumCache::loadEnumValues(const Class* klass,
       EnumCache::failLookup(msg);
     }
     values.set(StrNR(consts[i].name), cellAsCVarRef(value));
-    names.set(value, make_tv<KindOfPersistentString>(consts[i].name));
+
+    // Manually perform int-like key coercion even if names is a dict for
+    // backwards compatibility.
+    int64_t n;
+    if (tvIsString(&value) && value.m_data.pstr->isStrictlyInteger(n)) {
+      names.set(n, make_tv<KindOfPersistentString>(consts[i].name));
+    } else {
+      names.set(value, make_tv<KindOfPersistentString>(consts[i].name), true);
+    }
   }
 
   assertx(names.isDArray());

diff --git a/hphp/runtime/ext/enum/ext_enum.cpp b/hphp/runtime/ext/enum/ext_enum.cpp
@@ -46,6 +46,13 @@ static bool HHVM_STATIC_METHOD(BuiltinEnum, isValid, const Variant &value) {
   if (UNLIKELY(!value.isInteger() && !value.isString())) return false;
 
   const EnumValues* values = EnumCache::getValuesBuiltin(self_);
+
+  // Manually perform int-like key conversion even if names is a dict, for
+  // backwards compatibility.
+  int64_t num;
+  if (value.isString() && value.getStringData()->isStrictlyInteger(num)) {
+    return values->names.exists(num);
+  }
   return values->names.exists(value);
 }
 
@@ -54,26 +61,27 @@ static Variant HHVM_STATIC_METHOD(BuiltinEnum, coerce, const Variant &value) {
     return Variant(Variant::NullInit{});
   }
 
-  auto base = self_->enumBaseTy();
-  Variant res = value;
+  auto res = value;
 
-  // First, if the base type is an int and we have a string containing
-  // an int, do the coercion first. This saves having to also do it in
-  // the array lookup (since it will be stored as an int there).
+  // Manually do int-like string conversion. This is to ensure the lookup
+  // succeeds below (since the values array does int-like string key conversion
+  // when created, even if its a dict).
   int64_t num;
-  if (base == KindOfInt64 && value.isString() &&
-      value.getStringData()->isStrictlyInteger(num)) {
+  if (value.isString() && value.getStringData()->isStrictlyInteger(num)) {
     res = Variant(num);
   }
 
-  // Make sure that the value is in the map. Then, if we have an int
-  // and the underlying type is a string, convert it to a string so
-  // the output type is right.
-  const EnumValues* values = EnumCache::getValuesBuiltin(self_);
-  if (!values->names.exists(value)) {
+  auto values = EnumCache::getValuesBuiltin(self_);
+  if (!values->names.exists(res)) {
     res = Variant(Variant::NullInit{});
-  } else if (base && isStringType(*base) && value.isInteger()) {
-    res = Variant(value.toString());
+  } else if (auto base = self_->enumBaseTy()) {
+    if (isStringType(*base) && res.isInteger()) {
+      res = Variant(res.toString());
+    }
+  } else {
+    // If the value is present, but the enum has no base type, return the value
+    // as specified, undoing any int-like string conversion we did on it.
+    return value;
   }
 
   return res;

diff --git a/hphp/test/slow/dv_array/enums.php b/hphp/test/slow/dv_array/enums.php
@@ -24,3 +24,60 @@
 var_dump(is_array($x));
 var_dump(is_varray($x));
 var_dump(is_darray($x));
+
+echo "============================================\n";
+
+enum Enum1 : string {
+  ONE = '1';
+  TWO = '2';
+  THREE = '3';
+}
+
+enum Enum2 : int {
+  ONE = 1;
+  TWO = 2;
+  THREE = 3;
+}
+
+enum Enum3 : mixed {
+  ONE = '1';
+  TWO = '2';
+  THREE = '3';
+}
+
+var_dump(Enum1::getNames());
+var_dump(Enum1::getValues());
+var_dump(Enum1::isValid('1'));
+var_dump(Enum1::isValid(1));
+var_dump(Enum1::coerce('1'));
+var_dump(Enum1::coerce(1));
+var_dump(Enum1::assert('1'));
+var_dump(Enum1::assert(1));
+var_dump(Enum1::assertAll(vec['1']));
+var_dump(Enum1::assertAll(vec[1]));
+
+echo "==============================================\n";
+
+var_dump(Enum2::getNames());
+var_dump(Enum2::getValues());
+var_dump(Enum2::isValid('1'));
+var_dump(Enum2::isValid(1));
+var_dump(Enum2::coerce('1'));
+var_dump(Enum2::coerce(1));
+var_dump(Enum2::assert('1'));
+var_dump(Enum2::assert(1));
+var_dump(Enum2::assertAll(vec['1']));
+var_dump(Enum2::assertAll(vec[1]));
+
+echo "==============================================\n";
+
+var_dump(Enum3::getNames());
+var_dump(Enum3::getValues());
+var_dump(Enum3::isValid('1'));
+var_dump(Enum3::isValid(1));
+var_dump(Enum3::coerce('1'));
+var_dump(Enum3::coerce(1));
+var_dump(Enum3::assert('1'));
+var_dump(Enum3::assert(1));
+var_dump(Enum3::assertAll(vec['1']));
+var_dump(Enum3::assertAll(vec[1]));
diff --git a/hphp/test/slow/dv_array/enums.php.expect b/hphp/test/slow/dv_array/enums.php.expect
@@ -36,4 +36,97 @@ array(7) {
 }
 bool(true)
 bool(false)
-bool(true)
+bool(true)
+============================================
+array(3) {
+  [1]=>
+  string(3) "ONE"
+  [2]=>
+  string(3) "TWO"
+  [3]=>
+  string(5) "THREE"
+}
+array(3) {
+  ["ONE"]=>
+  string(1) "1"
+  ["TWO"]=>
+  string(1) "2"
+  ["THREE"]=>
+  string(1) "3"
+}
+bool(true)
+bool(true)
+string(1) "1"
+string(1) "1"
+string(1) "1"
+string(1) "1"
+array(1) {
+  [0]=>
+  string(1) "1"
+}
+array(1) {
+  [0]=>
+  string(1) "1"
+}
+==============================================
+array(3) {
+  [1]=>
+  string(3) "ONE"
+  [2]=>
+  string(3) "TWO"
+  [3]=>
+  string(5) "THREE"
+}
+array(3) {
+  ["ONE"]=>
+  int(1)
+  ["TWO"]=>
+  int(2)
+  ["THREE"]=>
+  int(3)
+}
+bool(true)
+bool(true)
+int(1)
+int(1)
+int(1)
+int(1)
+array(1) {
+  [0]=>
+  int(1)
+}
+array(1) {
+  [0]=>
+  int(1)
+}
+==============================================
+array(3) {
+  [1]=>
+  string(3) "ONE"
+  [2]=>
+  string(3) "TWO"
+  [3]=>
+  string(5) "THREE"
+}
+array(3) {
+  ["ONE"]=>
+  string(1) "1"
+  ["TWO"]=>
+  string(1) "2"
+  ["THREE"]=>
+  string(1) "3"
+}
+bool(true)
+bool(true)
+string(1) "1"
+int(1)
+string(1) "1"
+int(1)
+array(1) {
+  [0]=>
+  string(1) "1"
+}
+array(1) {
+  [0]=>
+  int(1)
+}
diff --git a/hphp/test/slow/hack_arr_compat/enums.php b/hphp/test/slow/hack_arr_compat/enums.php
@@ -0,0 +1,57 @@
+<?hh
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+enum Enum1 : string {
+  ONE = '1';
+  TWO = '2';
+  THREE = '3';
+}
+
+enum Enum2 : int {
+  ONE = 1;
+  TWO = 2;
+  THREE = 3;
+}
+
+enum Enum3 : mixed {
+  ONE = '1';
+  TWO = '2';
+  THREE = '3';
+}
+
+var_dump(Enum1::getNames());
+var_dump(Enum1::getValues());
+var_dump(Enum1::isValid('1'));
+var_dump(Enum1::isValid(1));
+var_dump(Enum1::coerce('1'));
+var_dump(Enum1::coerce(1));
+var_dump(Enum1::assert('1'));
+var_dump(Enum1::assert(1));
+var_dump(Enum1::assertAll(vec['1']));
+var_dump(Enum1::assertAll(vec[1]));
+
+echo "==============================================\n";
+
+var_dump(Enum2::getNames());
+var_dump(Enum2::getValues());
+var_dump(Enum2::isValid('1'));
+var_dump(Enum2::isValid(1));
+var_dump(Enum2::coerce('1'));
+var_dump(Enum2::coerce(1));
+var_dump(Enum2::assert('1'));
+var_dump(Enum2::assert(1));
+var_dump(Enum2::assertAll(vec['1']));
+var_dump(Enum2::assertAll(vec[1]));
+
+echo "==============================================\n";
+
+var_dump(Enum3::getNames());
+var_dump(Enum3::getValues());
+var_dump(Enum3::isValid('1'));
+var_dump(Enum3::isValid(1));
+var_dump(Enum3::coerce('1'));
+var_dump(Enum3::coerce(1));
+var_dump(Enum3::assert('1'));
+var_dump(Enum3::assert(1));
+var_dump(Enum3::assertAll(vec['1']));
+var_dump(Enum3::assertAll(vec[1]));
diff --git a/hphp/test/slow/hack_arr_compat/enums.php.expect b/hphp/test/slow/hack_arr_compat/enums.php.expect
@@ -0,0 +1,92 @@
+array(3) {
+  [1]=>
+  string(3) "ONE"
+  [2]=>
+  string(3) "TWO"
+  [3]=>
+  string(5) "THREE"
+}
+array(3) {
+  ["ONE"]=>
+  string(1) "1"
+  ["TWO"]=>
+  string(1) "2"
+  ["THREE"]=>
+  string(1) "3"
+}
+bool(true)
+bool(true)
+string(1) "1"
+string(1) "1"
+string(1) "1"
+string(1) "1"
+array(1) {
+  [0]=>
+  string(1) "1"
+}
+array(1) {
+  [0]=>
+  string(1) "1"
+}
+==============================================
+array(3) {
+  [1]=>
+  string(3) "ONE"
+  [2]=>
+  string(3) "TWO"
+  [3]=>
+  string(5) "THREE"
+}
+array(3) {
+  ["ONE"]=>
+  int(1)
+  ["TWO"]=>
+  int(2)
+  ["THREE"]=>
+  int(3)
+}
+bool(true)
+bool(true)
+int(1)
+int(1)
+int(1)
+int(1)
+array(1) {
+  [0]=>
+  int(1)
+}
+array(1) {
+  [0]=>
+  int(1)
+}
+==============================================
+array(3) {
+  [1]=>
+  string(3) "ONE"
+  [2]=>
+  string(3) "TWO"
+  [3]=>
+  string(5) "THREE"
+}
+array(3) {
+  ["ONE"]=>
+  string(1) "1"
+  ["TWO"]=>
+  string(1) "2"
+  ["THREE"]=>
+  string(1) "3"
+}
+bool(true)
+bool(true)
+string(1) "1"
+int(1)
+string(1) "1"
+int(1)
+array(1) {
+  [0]=>
+  string(1) "1"
+}
+array(1) {
+  [0]=>
+  int(1)
+}
diff --git a/hphp/test/slow/hack_arr_compat/enums.php.hphp_opts b/hphp/test/slow/hack_arr_compat/enums.php.hphp_opts
@@ -0,0 +1 @@
+-vRuntime.Eval.HackArrCompatNotices=true -d hhvm.php7.all=0
diff --git a/hphp/test/slow/hack_arr_compat/enums.php.opts b/hphp/test/slow/hack_arr_compat/enums.php.opts
@@ -0,0 +1 @@
+-vEval.HackArrCompatNotices=true
		@@ -0,0 +1 @@
		-vRuntime.Eval.HackArrCompatNotices=true -d hhvm.php7.all=0