Fix ReflectionClass::getMethods filter with abstract methods and inte…

…rfaces

Summary: This is a fix for #5170.
I've introduced a second Set because we don't want to return abstract methods in parent classes which are implemented in the child class.
Closes #5236

Reviewed By: @fredemmott

Differential Revision: D2035770

hphp/runtime/ext/reflection/ext_reflection.cpp

@@ -1092,46 +1092,29 @@ static bool HHVM_METHOD(ReflectionClass, hasMethod, const String& name) {
   return (get_method_func(cls, name) != nullptr);
 }
 
-static void addInterfaceMethods(const Class* iface, const SmartPtr<c_Set>& st) {
-  assert(iface && st);
-  assert(AttrInterface & iface->attrs());
-
-  size_t const numMethods = iface->preClass()->numMethods();
-  Func* const* methods = iface->preClass()->methods();
-  for (Slot i = 0; i < numMethods; ++i) {
-    const Func* m = methods[i];
-    if (m->isGenerated()) continue;
-
-    st->add(HHVM_FN(strtolower)(m->nameStr()).get());
-  }
-
-  for (auto const& parentIface: iface->declInterfaces()) {
-    addInterfaceMethods(parentIface.get(), st);
-  }
-  auto const& allIfaces = iface->allInterfaces();
-  if (allIfaces.size() > iface->declInterfaces().size()) {
-    for (int i = 0; i < allIfaces.size(); ++i) {
-      addInterfaceMethods(allIfaces[i].get(), st);
-    }
-  }
-}
-
 // helper for getMethods: returns a Set
 static Object HHVM_METHOD(ReflectionClass, getMethodOrder, int64_t filter) {
   auto const cls = ReflectionClassHandle::GetClassFor(this_);
   Attr mask = attrs_from_modifiers(filter, false);
 
   // At each step, we fetch from the PreClass is important because the
   // order in which getMethods returns matters
+  StringISet visitedMethods;
   auto st = makeSmartPtr<c_Set>();
   st->reserve(cls->numMethods());
 
   auto add = [&] (const Func* m) {
-    if (m->isGenerated() || !(m->attrs() & mask)) return;
-    st->add(HHVM_FN(strtolower)(m->nameStr()).get());
+    if (m->isGenerated()) return;
+    if (visitedMethods.count(m->nameStr())) return;
+
+    visitedMethods.insert(m->nameStr());
+    if (m->attrs() & mask) {
+      st->add(HHVM_FN(strtolower)(m->nameStr()).get());
+    }
   };
 
   std::function<void(const Class*)> collect;
+  std::function<void(const Class*)> collectInterface;
 
   collect = [&] (const Class* cls) {
     if (!cls) return;
@@ -1160,19 +1143,39 @@ static Object HHVM_METHOD(ReflectionClass, getMethodOrder, int64_t filter) {
     }
   };
 
+  collectInterface = [&] (const Class* iface) {
+    if (!iface) return;
+
+    size_t const numMethods = iface->preClass()->numMethods();
+    Func* const* methods = iface->preClass()->methods();
+    for (Slot i = 0; i < numMethods; ++i) {
+      add(methods[i]);
+    }
+
+    for (auto const& parentIface: iface->declInterfaces()) {
+      collectInterface(parentIface.get());
+    }
+    auto const& allIfaces = iface->allInterfaces();
+    if (allIfaces.size() > iface->declInterfaces().size()) {
+      for (int i = 0; i < allIfaces.size(); ++i) {
+        collectInterface(allIfaces[i].get());
+      }
+    }
+  };
+
   collect(const_cast<Class*>(cls));
 
   // concrete classes should already have all of their methods present
-  if ((AttrPublic & mask) &&
+  if (((AttrPublic | AttrAbstract | AttrStatic) & mask) &&
       cls->attrs() & (AttrInterface | AttrAbstract | AttrTrait)) {
     for (auto const& interface: cls->declInterfaces()) {
-      addInterfaceMethods(interface.get(), st);
+      collectInterface(interface.get());
     }
     auto const& allIfaces = cls->allInterfaces();
     if (allIfaces.size() > cls->declInterfaces().size()) {
       for (int i = 0; i < allIfaces.size(); ++i) {
         auto const& interface = allIfaces[i];
-        addInterfaceMethods(interface.get(), st);
+        collectInterface(interface.get());
       }
     }
   }

hphp/test/slow/reflection/class_methods_filtered.php

@@ -0,0 +1,22 @@
+<?php
+abstract class A {
+  private function f() {}
+  abstract protected function g();
+  abstract public function h();
+}
+
+interface I {
+    public function i();
+    public function j();
+    static function s();
+}
+
+abstract class B extends A implements I {
+  protected function g(){}
+  public function h(){}
+  public function j() {}
+}
+
+$ref = new ReflectionClass("B");
+var_dump($ref->getMethods(ReflectionMethod::IS_ABSTRACT));
+var_dump($ref->getMethods(ReflectionMethod::IS_STATIC));

hphp/test/slow/reflection/class_methods_filtered.php.expectf

@@ -0,0 +1,25 @@
+array(2) {
+  [0]=>
+  object(ReflectionMethod)#%d (2) {
+    ["name"]=>
+    string(1) "i"
+    ["class"]=>
+    string(1) "I"
+  }
+  [1]=>
+  object(ReflectionMethod)#%d (2) {
+    ["name"]=>
+    string(1) "s"
+    ["class"]=>
+    string(1) "I"
+  }
+}
+array(1) {
+  [0]=>
+  object(ReflectionMethod)#%d (2) {
+    ["name"]=>
+    string(1) "s"
+    ["class"]=>
+    string(1) "I"
+  }
+}