Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:92018
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.161.181 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CABHuAWwFYOya0yqzMqOKwpxDp4UoR4Xi5QVByrGO8N2t8y2hKQ@mail.gmail.com>
References: <CAF+90c95i=u8rE_vA9WOuZ7VjSkWLxFPRoRykWAm2337mV=JeA@mail.gmail.com>
	<CABHuAWwFYOya0yqzMqOKwpxDp4UoR4Xi5QVByrGO8N2t8y2hKQ@mail.gmail.com>
Date: Wed, 30 Mar 2016 11:36:21 +0200
Message-ID: <CAF+90c9+2eZVar=6sAo5HM+LnzFGDkZqCDX6o8fSaJ1q_0eqag@mail.gmail.com>
To: Chris Riley <t.carnage@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11408754f3ce31052f40e0b2
Subject: Re: [PHP-DEV] Forbid binding methods to incompatible $this
From: nikita.ppv@gmail.com (Nikita Popov)

--001a11408754f3ce31052f40e0b2
Content-Type: text/plain; charset=UTF-8

On Wed, Mar 30, 2016 at 11:29 AM, Chris Riley <t.carnage@gmail.com> wrote:

> On 29 March 2016 at 23:21, Nikita Popov <nikita.ppv@gmail.com> 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

--001a11408754f3ce31052f40e0b2--