ReflectionClass fails to properly identify unimplemented interface methods as abstract

[colinodell]

I've identified a behavior difference / PHP incompatibility bug with ReflectionClass which occurs when an abstract method doesn't fully implement an interface: getMethods(ReflectionMethod::IS_ABSTRACT) in HHVM will not return the missing method whereas PHP does.

Here's some sample code:

interface TestInterface
{
    public function foo();

    public function bar();
}

abstract class AbstractTestClass implements TestInterface
{
    public function foo()
    {
        return 'foo';
    }

    // Notice we have not implemented bar()
}

And here's a quick test demonstrating the behavior mismatch:

echo "Testing " . phpversion() . ":\n";
$reflector = new ReflectionClass('ErrorTest\AbstractTestClass');
var_dump($reflector->getMethods(ReflectionMethod::IS_ABSTRACT));

Here's the PHP output:

Testing 5.6.6:
array(1) {
  [0] =>
  class ReflectionMethod#4 (2) {
    public $name =>
    string(3) "bar"
    public $class =>
    string(23) "ErrorTest\TestInterface"
  }
}

Whereas HHVM produces a different result:

Testing 5.6.99-hhvm:
array(0) {
}

This issue was originally discovered here: sebastianbergmann/phpunit-mock-objects#223

[colinodell]

I've also found that $reflector->getMethod('bar')->isAbstract() returns true in both PHP and HHVM, which makes me think the problem is located somewhere in getMethodOrder (or perhaps addInterfaceMethods). I'm not a C++ developer so there's a good chance I might be wrong haha

[paulbiss]

At a cursory glance, I think this is the problem. I don't see those constants defined differently for ReflectionMethod from ReflectionFunction anywhere, 0x2 and x04 should be the correct values.

[SiebelsTim]

@paulbiss That's not the problem. ReflectionMethod uses IS_ABSTRACT = 2; while ReflectionClass uses IS_EXPLICIT_ABSTRACT = 32;.
I guess when implementing an interface, the abstract attribute from all methods are removed. Even for still undefined methods.

[SiebelsTim]

Relevant: http://3v4l.org/gD3G2

This is the part responsible for that.

hhvm/hphp/runtime/ext/reflection/ext_reflection.cpp

Lines 1166 to 1178 in fcdc18a

	if ((AttrPublic & mask) &&
	cls->attrs() & (AttrInterface | AttrAbstract | AttrTrait)) {
	for (auto const& interface: cls->declInterfaces()) {
	addInterfaceMethods(interface.get(), st);
	}
	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);
	}
	}
	}

First of all, we don't need the AttrPublic & mask check. That breaks http://3v4l.org/rQbp9 and http://3v4l.org/i1dBp for instance.
Then we want to add all interface methods, matching our mask. The mask matching is not implemented.

I don't have time right now to figure out all the edge cases. Maybe we even need to check, not to add implemented methods twice.

This should be a pretty good start for anyone wanting to take this. :)

---

Edit: another case that is broken: http://3v4l.org/hCDTr

[jwatzman]

@meghana1811 oops, looks like someone already sent a PR for this that landed -- sorry for not noticing that, should have seen the open PR :) You can look into the other small starter task.