Forbid binding methods to incompatible $this View as Markdown

Hi internals!

Currently, inside instance methods the following invariant holds:

assert(is_null($this) || is_object($this))

This is a problem. I'd like to guarantee the following invariant instead:

assert(is_null($this) || $this instanceof self)

That is, ensure that if $this is not null, then it must be an object
"compatible" with the class, i.e. be in its inheritance hierarchy.

The absence of this guarantee, apart from being a major WTF in itself, also
prevents us from optimizing operations involving $this. In particular, the
following optimizations are not possible:

a) If typed properties land, we will not be able to use the type of
$this->typedProperty for optimization, because $this->typedProperty might
actually be referencing a property of a totally unrelated class.
b) We are not able to early-bind arguments to private or final method
calls, i.e. determine at compile-time whether an argument will use by-value
or by-reference argument passing and optimize accordingly.
c) We are not able to inline calls to private or final methods. (The
possibility of null is an issue in this instance as well, though a lesser
one.)

The good news is that, as of PHP 7.0, we are very close to guaranteeing
that "$this instanceof self" already. There exists only one little known
loophole: Obtaining a closure object wrapping the method using
ReflectionMethod::getClosure() and binding to an unrelated $this using
Closure::bindTo().

In PHP 7.0 we already heavily cut down 1 on what bindTo() is to allowed
to do on fake method closures. Namely, it was completely forbidden to
rebind the scope of methods, as this was already interacting badly with
optimizations in 7.0. We did not forbid binding to an incompatible $this
because it was not yet causing immediate issues at the time.

I'd like to remedy this oversight now and restrict $this binding of fake
method closures to compatible contexts only, i.e. for a method A::foo()
allow only instances of A or one of its descendant classes to be bound.
This limitation already exists for fake method closures targeting internal
methods.

Are there any objections to this change? If not, I'll merge the patch 2.
If yes, I'll write an RFC, as this change, while of no direct consequence
for PHP programmers, is important for the PHP implementation.

Regards,
Nikita

Hi,

Will this patch break any use cases similar to:

class Bar {
private $a = 'hidden';
}

$x = new Bar();
$y = function() { return $this->a);
$z = $x->bindTo($x, $x);
echo $z(); //hidden

?

If not, I have no objections.

I agree, this ability is a dirty and annoying hack, but I'm sure, some people use it.

Tony, you don't use this in the new runkit replacement? :)

Thanks. Dmitry.

Hi internals!

Currently, inside instance methods the following invariant holds:

assert(is_null($this) || is_object($this))

This is a problem. I'd like to guarantee the following invariant instead:

assert(is_null($this) || $this instanceof self)

That is, ensure that if $this is not null, then it must be an object
"compatible" with the class, i.e. be in its inheritance hierarchy.

The absence of this guarantee, apart from being a major WTF in itself,
also prevents us from optimizing operations involving $this. In particular,
the following optimizations are not possible:

a) If typed properties land, we will not be able to use the type of
$this->typedProperty for optimization, because $this->typedProperty might
actually be referencing a property of a totally unrelated class.
b) We are not able to early-bind arguments to private or final method
calls, i.e. determine at compile-time whether an argument will use by-value
or by-reference argument passing and optimize accordingly.
c) We are not able to inline calls to private or final methods. (The
possibility of null is an issue in this instance as well, though a lesser
one.)

The good news is that, as of PHP 7.0, we are very close to guaranteeing
that "$this instanceof self" already. There exists only one little known
loophole: Obtaining a closure object wrapping the method using
ReflectionMethod::getClosure() and binding to an unrelated $this using
Closure::bindTo().

In PHP 7.0 we already heavily cut down 1 on what bindTo() is to allowed
to do on fake method closures. Namely, it was completely forbidden to
rebind the scope of methods, as this was already interacting badly with
optimizations in 7.0. We did not forbid binding to an incompatible $this
because it was not yet causing immediate issues at the time.

I'd like to remedy this oversight now and restrict $this binding of fake
method closures to compatible contexts only, i.e. for a method A::foo()
allow only instances of A or one of its descendant classes to be bound.
This limitation already exists for fake method closures targeting internal
methods.

Are there any objections to this change? If not, I'll merge the patch 2.
If yes, I'll write an RFC, as this change, while of no direct consequence
for PHP programmers, is important for the PHP implementation.

Regards,
Nikita

As the discussion was positive, this is now merged as
https://github.com/php/php-src/commit/75af8150f58fb55637ac12b33d469b27adef9d76
.

Nikita

Great it's not used anymore [image: &#X1f60a]

I personally agree to remove this.

Nikita, can you also investigate the need for double pointer to object in

typedef union _zend_function *(*zend_object_get_method_t)(zend_object
**object, zend_string *method, const zval *key);

As I remember, it's necessary only for some SPL hack, and looks like a
terrible design decision.

This is used for forwarding method calls to the the "inner" object in SPL
iterators. It's basically the internal equivalent of doing:

function __call($method, $args) {
return $this->inner->$method(...$args);
}

I don't think we can drop this feature at this point (deprecate?). We can
of course change the implementation to use a trampoline function to avoid
the zend_object** hack, but I'm not sure that would be worthwhile.

Nikita