Various fixes for enum class

paulbiss · facebook-github-bot · commit 6762285701de · 2021-04-05T23:05:22.000-07:00
Summary:
This diff fixes the following issues that arise from the fact that enum classes are tagged with `AttrEnum` by making `AttrEnum` and `AttrEnumClass` mutually exclusive:

 - Jumpstart serialization assumed that any `AttrEnum` class had a base type that was either `int` or `string` (this was actually doubly wrong as even for enums this type could also have been `arraykey`)
 - A variety of places in hhbbc, the jit, and the runtime treat any `AttrEnum` type name as though it's an alias for the enum's base type. This is not true of enum classes.
 - Checks were missing to prevent enum classes from including enums and vice-versa
 - BuiltinEnumClass was the only class marked AttrEnumClass but not AttrEnum, now it is marked neither to match BuiltinEnum (which is not marked AttrEnum). This made the fix to Class::setParent below possible

Additionally the following mostly benign issues were fixed:

 - Dead code in Class::classof (which would have been broken had it been possible to instantiate an enum class) was removed
 - Dead code in Class::setParent that assumed it was possible to extend an enum class (by means other than enum inclusion) was removed (all enums extend BuiltinEnum and all enum classes extend BuiltinEnumClass)
 - In index.cpp a check for enums which include other enums had erroneously negated a check for AttrEnum
 - In prof-data-serialize.cpp we weren't reading/writing included enums. While not strictly a requirement for correctness it is better to do this here.

Reviewed By: jano

Differential Revision: D27574956

fbshipit-source-id: 36910981a3daf7d5c5c01a48df2f62fd22fd8608
diff --git a/hphp/hack/src/hhbc/print.rs b/hphp/hack/src/hhbc/print.rs
@@ -751,7 +751,7 @@ fn print_class_special_attributes<W: Write>(
     if c.is_sealed() {
         special_attributes.push("sealed");
     }
-    if c.enum_type.is_some() {
+    if c.enum_type.is_some() && !hhas_attribute::has_enum_class(user_attrs) {
         special_attributes.push("enum");
     }
     if c.is_abstract() {
diff --git a/hphp/hack/test/enum_atom/compilation/test.php.exp b/hphp/hack/test/enum_atom/compilation/test.php.exp
@@ -116,7 +116,7 @@
   }
 }
 
-.class {} [enum enum_class "__EnumClass"("""v:0:{}""")] E (16,19) extends HH\BuiltinEnumClass {
+.class {} [enum_class "__EnumClass"("""v:0:{}""")] E (16,19) extends HH\BuiltinEnumClass {
   .enum_ty <"I" extended_hint>;
   .const A = uninit;
   .const B = uninit;
@@ -181,7 +181,7 @@
   }
 }
 
-.class {} [enum enum_class "__EnumClass"("""v:0:{}""")] F (25,27) extends HH\BuiltinEnumClass enum_includes (E) {
+.class {} [enum_class "__EnumClass"("""v:0:{}""")] F (25,27) extends HH\BuiltinEnumClass enum_includes (E) {
   .enum_ty <"I" extended_hint>;
   .const C = uninit;
   .method {}{} [private static no_injection] (25,27) 86cinit($constName) {
diff --git a/hphp/hhbbc/index.cpp b/hphp/hhbbc/index.cpp
@@ -1564,7 +1564,7 @@ bool build_class_constants(BuildClsInfo& info,
 
       // A constant from an interface or from an included enum collides
       // with an existing constant.
-      if (rparent->cls->attrs & (AttrInterface | AttrEnum)) {
+      if (rparent->cls->attrs & (AttrInterface | AttrEnum | AttrEnumClass)) {
         ITRACE(2,
                "build_cls_info_rec failed for `{}' because "
                "`{}' was defined by both `{}' and `{}'\n",
@@ -2658,11 +2658,13 @@ void resolve_combinations(ClassNamingEnv& env,
 
   for (auto& included_enum_name : cls->includedEnumNames) {
     auto const included_enum = map.at(included_enum_name);
-    if (!(included_enum->cls->attrs & AttrEnum)) {
+    auto const want_attr = cls->attrs & (AttrEnum | AttrEnumClass);
+    if (!(included_enum->cls->attrs & want_attr)) {
       ITRACE(2,
              "Resolve combinations failed for `{}' because `{}' "
-             "is not an enum\n",
-             cls->name, included_enum_name);
+             "is not an enum{}\n",
+             cls->name, included_enum_name,
+             want_attr & AttrEnumClass ? " class" : "");
       return;
     }
     cinfo->includedEnums.push_back(included_enum);
@@ -2780,6 +2782,7 @@ void compute_subclass_list(IndexData& index) {
   trace_time _("compute subclass list");
   auto fixupTraits = false;
   auto fixupEnums = false;
+  auto const AnyEnum = AttrEnum | AttrEnumClass;
   for (auto& cinfo : index.allClassInfos) {
     if (cinfo->cls->attrs & AttrInterface) continue;
     for (auto& cparent : cinfo->baseList) {
@@ -2791,13 +2794,13 @@ void compute_subclass_list(IndexData& index) {
       compute_subclass_list_rec(index, cinfo.get(), cinfo.get());
     }
     // Add the included enum lists if cinfo is an enum
-    if (!(cinfo->cls->attrs & AttrEnum) &&
+    if ((cinfo->cls->attrs & AnyEnum) &&
         cinfo->cls->includedEnumNames.size()) {
       fixupEnums = true;
       compute_included_enums_list_rec(index, cinfo.get(), cinfo.get());
     }
     // Also add instantiable classes to their interface's subclassLists
-    if (cinfo->cls->attrs & (AttrTrait | AttrEnum | AttrAbstract)) continue;
+    if (cinfo->cls->attrs & (AttrTrait | AnyEnum | AttrAbstract)) continue;
     for (auto& ipair : cinfo->implInterfaces) {
       auto impl = const_cast<ClassInfo*>(ipair.second);
       impl->subclassList.push_back(cinfo.get());
@@ -2807,7 +2810,7 @@ void compute_subclass_list(IndexData& index) {
   for (auto& cinfo : index.allClassInfos) {
     auto& sub = cinfo->subclassList;
     if ((fixupTraits && cinfo->cls->attrs & AttrTrait) ||
-        (fixupEnums && cinfo->cls->attrs & AttrEnum)) {
+        (fixupEnums && cinfo->cls->attrs & AnyEnum)) {
       // traits and enums can be reached by multiple paths, so we need to
       // uniquify their subclassLists.
       std::sort(begin(sub), end(sub));
@@ -3027,7 +3030,7 @@ void trace_interfaces(const IndexData& index, const ConflictGraph& cg) {
       ifaces.emplace_back(cinfo->cls);
       continue;
     }
-    if (cinfo->cls->attrs & (AttrTrait | AttrEnum)) continue;
+    if (cinfo->cls->attrs & (AttrTrait | AttrEnum | AttrEnumClass)) continue;
 
     classes.emplace_back(Cls{cinfo.get()});
     auto& vtable = classes.back().vtable;
@@ -3150,7 +3153,7 @@ void compute_iface_vtables(IndexData& index) {
     }
 
     // Only worry about classes with methods that can be called.
-    if (cinfo->cls->attrs & (AttrTrait | AttrEnum)) continue;
+    if (cinfo->cls->attrs & (AttrTrait | AttrEnum | AttrEnumClass)) continue;
 
     for (auto& ipair : cinfo->implInterfaces) {
       ++iface_uses[ipair.second->cls];
diff --git a/hphp/runtime/base/enum-cache.cpp b/hphp/runtime/base/enum-cache.cpp
@@ -43,13 +43,13 @@ const EnumValues* EnumCache::getValues(const Class* klass,
 }
 
 const EnumValues* EnumCache::getValuesBuiltin(const Class* klass) {
-  assertx(isEnum(klass));
+  assertx(isAnyEnum(klass));
   if (auto const values = klass->getEnumValues()) return values;
   return s_cache.getEnumValues(klass, false);
 }
 
 const EnumValues* EnumCache::getValuesStatic(const Class* klass) {
-  assertx(isEnum(klass));
+  assertx(isAnyEnum(klass));
   auto const result = [&]() -> const EnumValues* {
     if (auto const values = klass->getEnumValues()) return values;
     return s_cache.getEnumValues(klass, false, true);
@@ -144,7 +144,7 @@ const EnumValues* EnumCache::loadEnumValues(
     }
     // The outer condition below enables caching of enum constants defined
     // in enums included by the current class.
-    if (!(isEnum(klass)
+    if (!(isAnyEnum(klass)
         && klass->hasIncludedEnums()
         && klass->allIncludedEnums().contains(consts[i].cls->name()))) {
       if (consts[i].cls != klass && !recurse) {
diff --git a/hphp/runtime/base/object-data-inl.h b/hphp/runtime/base/object-data-inl.h
@@ -120,7 +120,8 @@ template <bool Unlocked>
 NEVER_INLINE ObjectData* ObjectData::newInstanceSlow(Class* cls) {
   assertx(cls);
   if (UNLIKELY(cls->attrs() &
-               (AttrAbstract | AttrInterface | AttrTrait | AttrEnum))) {
+               (AttrAbstract | AttrInterface | AttrTrait | AttrEnum |
+                AttrEnumClass))) {
     raiseAbstractClassError(cls);
   }
   if (cls->hasReifiedGenerics()) {
@@ -158,7 +159,8 @@ inline ObjectData* ObjectData::newInstance(Class* cls) {
     return newInstanceSlow<Unlocked>(cls);
   }
   if (UNLIKELY(cls->attrs() &
-               (AttrAbstract | AttrInterface | AttrTrait | AttrEnum))) {
+               (AttrAbstract | AttrInterface | AttrTrait | AttrEnum |
+                AttrEnumClass))) {
     raiseAbstractClassError(cls);
   }
   auto obj = ObjectData::newInstanceImpl<Unlocked>(
@@ -176,7 +178,8 @@ inline ObjectData* ObjectData::newInstanceReified(Class* cls,
                                                   ArrayData* reifiedTypes) {
   assertx(cls);
   if (UNLIKELY(cls->attrs() &
-               (AttrAbstract | AttrInterface | AttrTrait | AttrEnum))) {
+               (AttrAbstract | AttrInterface | AttrTrait | AttrEnum |
+                AttrEnumClass))) {
     raiseAbstractClassError(cls);
   }
   if (cls->hasReifiedGenerics()) {
@@ -207,7 +210,8 @@ inline ObjectData* ObjectData::newInstanceReified(Class* cls,
 
 inline ObjectData* ObjectData::newInstanceNoPropInit(Class* cls) {
   assertx(!(cls->attrs() &
-            (AttrAbstract | AttrInterface | AttrTrait | AttrEnum)));
+            (AttrAbstract | AttrInterface | AttrTrait | AttrEnum |
+             AttrEnumClass)));
   return ObjectData::newInstanceImpl<false>(
     cls,
     [&](void* mem, uint8_t sizeFlag) {
diff --git a/hphp/runtime/base/object-data.cpp b/hphp/runtime/base/object-data.cpp
@@ -1554,7 +1554,7 @@ void ObjectData::raiseAbstractClassError(Class* cls) {
   raise_error("Cannot instantiate %s %s",
               (attrs & AttrInterface) ? "interface" :
               (attrs & AttrTrait)     ? "trait" :
-              (attrs & AttrEnum)      ? "enum" : "abstract class",
+              (attrs & (AttrEnum|AttrEnumClass)) ? "enum" : "abstract class",
               cls->preClass()->name()->data());
 }
 
diff --git a/hphp/runtime/base/type-structure.cpp b/hphp/runtime/base/type-structure.cpp
@@ -602,7 +602,7 @@ bool resolveClass(TSEnv& env, const TSCtx& ctx, Array& ret,
   if (!cls) return false;
 
   TypeStructure::Kind resolvedKind;
-  if (isNormalClass(cls)) {
+  if (isNormalClass(cls) || isEnumClass(cls)) {
     resolvedKind = TypeStructure::Kind::T_class;
   } else if (isInterface(cls)) {
     resolvedKind = TypeStructure::Kind::T_interface;
diff --git a/hphp/runtime/ext/enum/ext_enum.php b/hphp/runtime/ext/enum/ext_enum.php
@@ -87,7 +87,6 @@ final public static function assertAll(
  * definition below is not actually used at run time; it is simply
  * provided for the typechecker and for developer reference.
  */
-<<__EnumClass>>
 abstract class BuiltinEnumClass<+T> {
   /**
    * Get the values of the public consts defined on this class,
diff --git a/hphp/runtime/ext/reflection/ext_reflection.cpp b/hphp/runtime/ext/reflection/ext_reflection.cpp
@@ -1142,7 +1142,8 @@ static bool HHVM_METHOD(ReflectionClass, isInternal) {
 
 static bool HHVM_METHOD(ReflectionClass, isInstantiable) {
   auto const cls = ReflectionClassHandle::GetClassFor(this_);
-  return !(cls->attrs() & (AttrAbstract | AttrInterface | AttrTrait | AttrEnum))
+  return !(cls->attrs() & (AttrAbstract | AttrInterface | AttrTrait | AttrEnum |
+                           AttrEnumClass))
     && (cls->getCtor()->attrs() & AttrPublic);
 }
 
@@ -1168,12 +1169,12 @@ static bool HHVM_METHOD(ReflectionClass, isTrait) {
 
 static bool HHVM_METHOD(ReflectionClass, isEnum) {
   auto const cls = ReflectionClassHandle::GetClassFor(this_);
-  return cls->attrs() & AttrEnum;
+  return cls->attrs() & (AttrEnum|AttrEnumClass);
 }
 
 static String HHVM_METHOD(ReflectionClass, getEnumUnderlyingType) {
   auto const cls = ReflectionClassHandle::GetClassFor(this_);
-  if (!(cls->attrs() & AttrEnum)) {
+  if (!(cls->attrs() & (AttrEnum|AttrEnumClass))) {
     Reflection::ThrowReflectionExceptionObject(
       "Trying to read the Enum-type of a non-Enum");
   }
diff --git a/hphp/runtime/ext/std/ext_std_classobj.cpp b/hphp/runtime/ext/std/ext_std_classobj.cpp
@@ -96,7 +96,7 @@ bool HHVM_FUNCTION(trait_exists, const String& trait_name,
 bool HHVM_FUNCTION(enum_exists, const String& enum_name,
                    bool autoload /* = true */) {
   Class* cls = Class::get(enum_name.get(), autoload);
-  return cls && isEnum(cls);
+  return cls && isAnyEnum(cls);
 }
 
 Variant HHVM_FUNCTION(get_class_methods, const Variant& class_or_object) {
diff --git a/hphp/runtime/ext/std/ext_std_closure.cpp b/hphp/runtime/ext/std/ext_std_closure.cpp
@@ -194,7 +194,8 @@ void c_Closure::instanceDtor(ObjectData* obj, const Class* cls) {
 
 ObjectData* createClosureRepoAuthRawSmall(Class* cls, size_t size,
                                           size_t index) {
-  assertx(!(cls->attrs() & (AttrAbstract|AttrInterface|AttrTrait|AttrEnum)));
+  assertx(!(cls->attrs() & (AttrAbstract|AttrInterface|AttrTrait|AttrEnum|
+                            AttrEnumClass)));
   assertx(!cls->needInitialization());
   assertx(cls->parent() == c_Closure::classof() && cls != c_Closure::classof());
   auto mem = tl_heap->mallocSmallIndexSize(index, size);
@@ -205,7 +206,8 @@ ObjectData* createClosureRepoAuthRawSmall(Class* cls, size_t size,
 }
 
 ObjectData* createClosureRepoAuth(Class* cls) {
-  assertx(!(cls->attrs() & (AttrAbstract|AttrInterface|AttrTrait|AttrEnum)));
+  assertx(!(cls->attrs() & (AttrAbstract|AttrInterface|AttrTrait|AttrEnum|
+                            AttrEnumClass)));
   assertx(!cls->needInitialization());
   assertx(cls->parent() == c_Closure::classof() || cls == c_Closure::classof());
   auto const nProps = cls->numDeclProperties();
diff --git a/hphp/runtime/vm/as.cpp b/hphp/runtime/vm/as.cpp
@@ -3245,7 +3245,7 @@ PreClass::Hoistable compute_hoistable(AsmState& as,
     if (!pce.interfaces().empty() ||
         !pce.usedTraits().empty() ||
         !pce.requirements().empty() ||
-        (pce.attrs() & AttrEnum)) {
+        (pce.attrs() & (AttrEnum|AttrEnumClass))) {
       return PreClass::Mergeable;
     }
     if (!parentName.empty() && !as.hoistables.count(parentName)) {
@@ -3284,7 +3284,7 @@ void parse_class(AsmState& as) {
     attrs |= AttrUnique | AttrPersistent | AttrBuiltin;
   }
   if (attrs & AttrIsConst) {
-    if (attrs & (AttrEnum | AttrInterface | AttrTrait)) {
+    if (attrs & (AttrEnum | AttrEnumClass | AttrInterface | AttrTrait)) {
       as.error("interfaces, traits and enums may not be const");
     }
     if (!(attrs & AttrForbidDynamicProps)) {
diff --git a/hphp/runtime/vm/class-inl.h b/hphp/runtime/vm/class-inl.h
@@ -210,13 +210,6 @@ inline bool Class::classof(const Class* cls) const {
     return this == cls ||
       m_interfaces.lookupDefault(cls->m_preClass->name(), nullptr) == cls;
   }
-  if (UNLIKELY(cls->attrs() & this->attrs() & AttrEnum)) {
-    if (this == cls) {
-      return true;
-    } else if (cls->m_extra && cls->m_extra->m_includedEnums.size() != 0) {
-      cls->m_extra->m_includedEnums.lookupDefault(this->m_preClass->name(), nullptr) == this;
-    }
-  }
   return classofNonIFace(cls);
 }
 
@@ -692,12 +685,21 @@ inline bool isEnum(const Class* cls) {
   return cls->attrs() & AttrEnum;
 }
 
+inline bool isEnumClass(const Class* cls) {
+  return cls->attrs() & AttrEnumClass;
+}
+
+inline bool isAnyEnum(const Class* cls) {
+  return isEnum(cls) || isEnumClass(cls);
+}
+
 inline bool isInterface(const Class* cls) {
   return cls->attrs() & AttrInterface;
 }
 
 inline bool isNormalClass(const Class* cls) {
-  return !(cls->attrs() & (AttrTrait | AttrInterface | AttrEnum));
+  return !(cls->attrs() & (AttrTrait | AttrInterface | AttrEnum |
+                           AttrEnumClass));
 }
 
 inline bool isAbstract(const Class* cls) {
diff --git a/hphp/runtime/vm/class.cpp b/hphp/runtime/vm/class.cpp
@@ -668,7 +668,7 @@ Class::Avail Class::avail(Class*& parent,
     }
   }
 
-  if (m_preClass->attrs() & AttrEnum) {
+  if (m_preClass->attrs() & (AttrEnum|AttrEnumClass)) {
     auto const& ie = m_extra->m_includedEnums;
     for (size_t i = 0; i < ie.size(); ++i) {
       auto const penum = ie[i]->m_preClass.get();
@@ -1819,15 +1819,16 @@ void Class::setParent() {
   if (m_parent.get() != nullptr) {
     Attr parentAttrs = m_parent->attrs();
     if (UNLIKELY(parentAttrs &
-                 (AttrFinal | AttrInterface | AttrTrait | AttrEnum))) {
+                 (AttrFinal | AttrInterface | AttrTrait | AttrEnum |
+                  AttrEnumClass))) {
       if (!(parentAttrs & AttrFinal) ||
-          (parentAttrs & AttrEnum) ||
+          (parentAttrs & (AttrEnum|AttrEnumClass)) ||
           m_preClass->userAttributes().find(s___MockClass.get()) ==
           m_preClass->userAttributes().end() ||
           m_parent->isCollectionClass()) {
         raise_error("Class %s may not inherit from %s (%s)",
                     m_preClass->name()->data(),
-                    ((parentAttrs & AttrEnum)      ? "enum" :
+                    ((parentAttrs & (AttrEnum|AttrEnumClass)) ? "enum" :
                      (parentAttrs & AttrFinal)     ? "final class" :
                      (parentAttrs & AttrInterface) ? "interface"   : "trait"),
                     m_parent->name()->data());
@@ -1850,20 +1851,6 @@ void Class::setParent() {
         m_parent->name()->data()
       );
     }
-    if (!(m_attrCopy & AttrEnumClass) && (parentAttrs & AttrEnumClass)) {
-      raise_error(
-        "Non-enum class %s cannot extend enum class parent %s",
-        m_preClass->name()->data(),
-        m_parent->name()->data()
-      );
-    }
-    if ((m_attrCopy & AttrEnumClass) && !(parentAttrs & AttrEnumClass)){
-      raise_error(
-        "Enum class %s cannot extend non-enum class parent %s",
-        m_preClass->name()->data(),
-        m_parent->name()->data()
-      );
-    }
 
     m_preClass->enforceInMaybeSealedParentWhitelist(m_parent->preClass());
     if (m_parent->m_maybeRedefsPropTy) m_maybeRedefsPropTy = true;
@@ -2493,7 +2480,7 @@ void Class::setConstants() {
         }
       }
       // Forbid redefining constants from included enums
-      if ((definingClass->attrs() & AttrEnum) && m_extra) {
+      if ((definingClass->attrs() & (AttrEnum|AttrEnumClass)) && m_extra) {
         for (int j = 0, size = m_extra->m_includedEnums.size(); j < size; ++j) {
           const Class* includedEnum = m_extra->m_includedEnums[j];
           if (includedEnum->hasConstant(preConst->name())) {
@@ -3917,7 +3904,6 @@ void Class::setEnumType() {
 
     // Make sure we've loaded a valid underlying type.
     if (m_enumBaseTy &&
-        !((attrs() & AttrEnumClass) && isObjectType(*m_enumBaseTy)) &&
         !isIntType(*m_enumBaseTy) &&
         !isStringType(*m_enumBaseTy)) {
       raise_error("Invalid base type for enum %s",
@@ -3929,7 +3915,7 @@ void Class::setEnumType() {
 void Class::setIncludedEnums() {
   IncludedEnumMap::Builder includedEnumsBuilder;
 
-  if (!isEnum(this) || m_preClass->includedEnums().empty()) {
+  if (!isAnyEnum(this) || m_preClass->includedEnums().empty()) {
     return;
   }
 
@@ -3941,9 +3927,10 @@ void Class::setIncludedEnums() {
     if (cp == nullptr) {
       raise_error("Undefined enum: %s", (*it)->data());
     }
-    if (!isEnum(cp)) {
-      raise_error("%s cannot include %s - it is not an enum",
-                  m_preClass->name()->data(), cp->name()->data());
+    if (isEnum(this) != isEnum(cp) || isEnumClass(this) != isEnumClass(cp)) {
+      raise_error("%s cannot include %s - it is not an enum%s",
+                  m_preClass->name()->data(), cp->name()->data(),
+                  isEnumClass(this) ? " class" : "");
     }
     m_preClass->enforceInMaybeSealedParentWhitelist(cp->preClass());
     auto cp_baseType = cp->m_enumBaseTy;
diff --git a/hphp/runtime/vm/class.h b/hphp/runtime/vm/class.h
diff --git a/hphp/runtime/vm/disas.cpp b/hphp/runtime/vm/disas.cpp
diff --git a/hphp/runtime/vm/jit/prof-data-serialize.cpp b/hphp/runtime/vm/jit/prof-data-serialize.cpp
diff --git a/hphp/test/slow/enum_class/enum_class05.php.expectf b/hphp/test/slow/enum_class/enum_class05.php.expectf
diff --git a/hphp/test/slow/enum_class/enum_class06.php.expectf b/hphp/test/slow/enum_class/enum_class06.php.expectf
diff --git a/hphp/test/slow/enum_class/enum_class07.php.expectf b/hphp/test/slow/enum_class/enum_class07.php.expectf
