Skip to content

Commit

Permalink
raise notice if Reflection::getConstants is used on a enum/enum class…
Browse files Browse the repository at this point in the history
… that imports constants via enum_inclusion

Summary:
Goal: get data on how often `ReflectionClass::getConstants` is used on enums or enum classes that use/extend other enums.

This diffs updates `getConstants` to `raise_notice` whenever this happens.

Reviewed By: viratyosin

Differential Revision: D65018282

fbshipit-source-id: aeb0f3e286c6a596b3f6bc80acf4f31496dcbb8b
  • Loading branch information
Francesco Zappa Nardelli authored and facebook-github-bot committed Oct 29, 2024
1 parent c0f20fa commit 7a50223
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 3 deletions.
2 changes: 2 additions & 0 deletions hphp/runtime/base/strings.h
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ constexpr char MISSING_DYNAMICALLY_REFERENCED[] =
"Missing __DynamicallyReferenced attribute on class %s for classname_to_class";
constexpr char CLASSNAME_TO_CLASS_NOEXIST_EXCEPTION[] =
"Failed to load class from %s %s for classname_to_class.";
constexpr char REFLECTION_MISS_CONSTANTS_FROM_INCLUDED_ENUMS[] =
"ReflectionClass::getConstants() misses constants from the enums/enum classes included by %s";

} // namespace Strings
} // namespace HPHP
16 changes: 13 additions & 3 deletions hphp/runtime/ext/reflection/ext_reflection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1628,12 +1628,22 @@ void addClassConstantNames(const Class* cls,

auto numConsts = cls->numConstants();
const Class::Const* consts = cls->constants();
bool has_constants_from_included_enums = false;
for (size_t i = 0; i < numConsts; i++) {
if (consts[i].cls == cls && !consts[i].isAbstractAndUninit()
&& consts[i].kind() == ConstModifiers::Kind::Value) {
st->add(const_cast<StringData*>(consts[i].name.get()));
if (!consts[i].isAbstractAndUninit() &&
consts[i].kind() == ConstModifiers::Kind::Value) {
if (consts[i].cls == cls) {
st->add(const_cast<StringData*>(consts[i].name.get()));
} else if (cls->hasIncludedEnums()
&& cls->allIncludedEnums().contains(consts[i].cls->name())) {
has_constants_from_included_enums = true;
}
}
}
if (has_constants_from_included_enums) {
raise_notice(Strings::REFLECTION_MISS_CONSTANTS_FROM_INCLUDED_ENUMS,
cls->name()->data());
}

auto const& allTraits = cls->usedTraitClasses();
auto const numTraits = allTraits.size();
Expand Down
41 changes: 41 additions & 0 deletions hphp/test/slow/enum_supertyping/reflection.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?hh

enum Enum1: string as string {
A = 'A';
}

enum Enum2: string as string {
B = 'B';
}

enum Enum3: string as string {
C = 'C';
}

enum CombinedEnums: string as string {
use Enum1, Enum2, Enum3;
D = 'D';
}


enum class Base: int {
int BA = 1;
}

enum class Extd : int extends Base {
int BB = 2;
}

<<__EntryPoint>>
function main(): void {
$reflector = new ReflectionClass(Enum1::class);
var_dump($reflector->getConstants());
$reflector = new ReflectionClass(CombinedEnums::class);
var_dump($reflector->getConstants());

$reflector = new ReflectionClass(Base::class);
var_dump($reflector->getConstants());

$reflector = new ReflectionClass(Extd::class);
var_dump($reflector->getConstants());
}
20 changes: 20 additions & 0 deletions hphp/test/slow/enum_supertyping/reflection.php.expectf
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
dict(1) {
["A"]=>
string(1) "A"
}

Notice: ReflectionClass::getConstants() misses constants from the enums/enum classes included by CombinedEnums in %s/enum_supertyping/reflection.php on line 34
dict(1) {
["D"]=>
string(1) "D"
}
dict(1) {
["BA"]=>
int(1)
}

Notice: ReflectionClass::getConstants() misses constants from the enums/enum classes included by Extd in %s/reflection.php on line 40
dict(1) {
["BB"]=>
int(2)
}

0 comments on commit 7a50223

Please sign in to comment.