On Wed, Mar 30, 2016 at 11:29 AM, Chris Riley <t.carn...@gmail.com> wrote:
> On 29 March 2016 at 23:21, Nikita Popov <nikita....@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
