Fatal error: Call to undefined method invokeArgs() after closure rebinding

[lisachenko]

I have a strange issue with closure binding:

hhvm --version
HipHop VM 3.4.0-dev+2014.09.16 (rel)
Compiler: heads/master-0-g17096e60da27ae27e459cf632938cbd87330e6fd
Repo schema: 5ca2ae69cce0c21969efb6c148381ff90bf97fab
Extension API: 20140829

class A {
    public function test() {
        echo 'First';
    }
}

class B extends A {
    public function test() {
        echo 'Second';
    }
}

$instance   = new B;
$refClosure = (new ReflectionMethod(A::class, 'test'))
            ->getClosure($instance)
            ->bindTo($instance, A::class);
$refClosure();

Fatal error: Call to undefined method B::invokeArgs() in ./test.php on line 19
// It is a $refClosure() call line

Expected output is 'First' if everything is OK.

[lisachenko]

This issue is a result of testing #1203 with my test case. @paulbiss could you have a look into this issue? Thanks!

[paulbiss]

It looks like it's a result of the way that getClosure() is implemented, it wraps the reflection method in a closure rather than creating a new function, so attempting to bind it behaves, strangely.

https://github.com/facebook/hhvm/blob/master/hphp/runtime/ext/reflection/ext_reflection_hni.php#L985-L1006

[lisachenko]

Yes, you are right, getClosure() implementation is not fully correct, so rebinding results in error.

[lisachenko]

I know how to fix this from PHP side, need to rewrite last part of this method (I can send PR for that):

    public function getClosure($object = null)
    {
        // < original top part of code >

        $methodName = $this->name;
        $closure    = function (...$args) use ($methodName) {
            $isDynamic = isset($this);
            $result    = $isDynamic ? $this->$methodName(...$args) : static::$methodName(...$args);

            return $result;
        };

        return $closure->bindTo($object, $this->class);

With this patch closures from methods can work with rebinding, but scope handling is still wrong for static and dynamic closures:

class A {
    public static function test() {
        echo 'First', ',', get_called_class();
    }
}

class B extends A {
    public static function test() {
        echo 'Second';
    }
}

$refClosure = (new ReflectionMethod(A::class, 'test'))
            ->getClosure(null)
            ->bindTo(null, B::class);
$refClosure();
// Expected output:
First, B
// Real output is from B::test() invocation instead of A::test() with scope=B:
Second

For my first example (in the issue body) I got following:

// Expected output:
First
// Real output if from B->test() invocation
// Despite of getting a closure from A->test() and binding it explicitly to the A scope:
Second

Looks like this issue has common roots with #2486

[paulbiss]

The reason that isn't working is because static::$methodName(...$args) will call $methodName on the late-bound static class (B in your example). If I run the following in PHP5 (or HHVM)

class Foo {
  function test() { echo "In foo\n"; }
  public function getBaz() {
    return function () { static::test(); };
  }
}

class Bar {
  function test() { echo "In bar\n"; }
}

$c = (new Foo)->getBaz();
$c();

$d = $c->bindTo(null, Bar::class);
$d();

I get the following, illustrating that static:: will attach to the last scope that was bound.

In foo
In bar

Running the following example will give the same result in both HHVM and PHP5, showing that static scope binding is working consistently.

class Foo {
  public function getBaz() {
    return function () { echo get_called_class(); };
  }
}

class Bar {}

$c = (new Foo)->getBaz();
$c();
echo "\n";

$d = $c->bindTo(null, Bar::class);
$d();
echo "\n";

PHP 5 and HHVM both correctly output

Foo
Bar

[paulbiss]

W.r.t. submitting this as a PR, unfortunately Closure::bind is unavailable in Repo mode so baking it into ext_reflection is probably a bad idea. A check for scope-binding support via ini_get may be an acceptable way to add conditional support here.

The other issue (#2486) is something that actually came up while working on this. We don't support invoking instance methods from unrelated classes on arbitrary objects. Limited support could probably be added using logic from Closure::bind but would certainly break in Repo mode and may quite possibly break outside of Repo mode too.

[lisachenko]

Thank you, @paulbiss for explanation. Seems I should change static::$methodName into the self::$methodName() in the patch to respect current scope and not violate LSB.

We don't support invoking instance methods from unrelated classes on arbitrary objects. Limited support could probably be added using logic from Closure::bind but would certainly break in Repo mode and may quite possibly break outside of Repo mode too.

I know about limitations of Repo mode, so it's not an issue for me, it can be a requirement for my framework. However, I hope that there will be a partial support in HHVM for my issues in any mode :)

[lisachenko]

A check for scope-binding support via ini_get may be an acceptable way to add conditional support here.

Is there a documentation or recommendation about ini keys for hhvm? I will put a check into the getClosure() method. If closure binding is enabled in php.ini then switch to my patch, otherwise switch to the default one with wrapping reflection method into the closure.

[lisachenko]

Unfortunately, self::$methodName() can't be used, because it breaks LSB. But I found a solution ) I use it for intercepting invocation of static methods and proxying via closure:

        $className  = $this->getDeclaringClass()->name;
        $methodName = $this->name;
        $closure = function (...$args) use ($methodName, $className) {
            $result = forward_static_call_array(array($className, $methodName), $args);

            return $result;
        };

        return $closure->bindTo($object, $this->class);

Just checked this: it is working under HHVM for static and dynamic methods and equals to the standard PHP behaviour.

[paulbiss]

The runtime option I added for this is Eval.AllowScopeBinding, it should be accessible via ini_get('hhvm.allow_scope_binding'). I will look into the other issue when I have some spare cycles to see how feasible it would be.

[paulbiss]

Marking this as wishlist, as altering the scope of arbitrary member functions is likely to break things inside the JIT.

[lisachenko]

It's pity ... From PHP side I couldn't create a reliable code that will work for all possible cases (some of them still failing)

[lisachenko]

Ping

[paulbiss]

I don't think anyone is working on this, it's marked as wishlist.

[lisachenko]

@paulbiss ok, understood, hope it will be implemented in the next year :)

[Swahvay]

Just ran into this issue myself. Is there a workaround people have been using?

[lisachenko]

I'm going to close this issue because of dropping support for the HHVM.

Anyway, thanks guys! It was a nice try! And almost everything worked except of several issues.

[Swahvay]

This is still an issue I would like to see fixed. Does it hurt to leave it open until resolved?