Skip to content

Commit

Permalink
Various fixes for enum class
Browse files Browse the repository at this point in the history
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
  • Loading branch information
paulbiss authored and facebook-github-bot committed Apr 6, 2021
1 parent 0460f25 commit 6762285
Show file tree
Hide file tree
Showing 20 changed files with 91 additions and 78 deletions.
2 changes: 1 addition & 1 deletion hphp/hack/src/hhbc/print.rs
Expand Up @@ -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() {
Expand Down
4 changes: 2 additions & 2 deletions hphp/hack/test/enum_atom/compilation/test.php.exp
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
21 changes: 12 additions & 9 deletions hphp/hhbbc/index.cpp
Expand Up @@ -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",
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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());
Expand All @@ -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));
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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];
Expand Down
6 changes: 3 additions & 3 deletions hphp/runtime/base/enum-cache.cpp
Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
12 changes: 8 additions & 4 deletions hphp/runtime/base/object-data-inl.h
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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>(
Expand All @@ -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()) {
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion hphp/runtime/base/object-data.cpp
Expand Up @@ -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());
}

Expand Down
2 changes: 1 addition & 1 deletion hphp/runtime/base/type-structure.cpp
Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion hphp/runtime/ext/enum/ext_enum.php
Expand Up @@ -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,
Expand Down
7 changes: 4 additions & 3 deletions hphp/runtime/ext/reflection/ext_reflection.cpp
Expand Up @@ -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);
}

Expand All @@ -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");
}
Expand Down
2 changes: 1 addition & 1 deletion hphp/runtime/ext/std/ext_std_classobj.cpp
Expand Up @@ -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) {
Expand Down
6 changes: 4 additions & 2 deletions hphp/runtime/ext/std/ext_std_closure.cpp
Expand Up @@ -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);
Expand All @@ -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();
Expand Down
4 changes: 2 additions & 2 deletions hphp/runtime/vm/as.cpp
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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)) {
Expand Down
18 changes: 10 additions & 8 deletions hphp/runtime/vm/class-inl.h
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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) {
Expand Down
35 changes: 11 additions & 24 deletions hphp/runtime/vm/class.cpp
Expand Up @@ -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();
Expand Down Expand Up @@ -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());
Expand All @@ -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;
Expand Down Expand Up @@ -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())) {
Expand Down Expand Up @@ -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",
Expand All @@ -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;
}

Expand All @@ -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;
Expand Down

0 comments on commit 6762285

Please sign in to comment.