On Wed, Mar 30, 2016 at 11:29 AM, Chris Riley <[email protected]> wrote:
> On 29 March 2016 at 23:21, Nikita Popov <[email protected]> wrote:
>
>> 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
>>
>> [1]: http://markmail.org/message/sjihplebuwsmdwex
>> [2]:
>> https://github.com/php/php-src/compare/master...nikic:forbigIncompatThis
>>
>
>
> 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.
>
Nope. Rebinding on "real" closures continues to work as usual, including
using it to access private properties of objects.
This change only affects closures created by
ReflectionMethod::getClosure(), i.e. closures that are really methods in
disguise.
Nikita