# Forbid binding methods to incompatible $this

- Thread: https://externals.io/message/91998
- Original archive: https://news-web.php.net/php.internals/91998
- Started by Nikita Popov on 2016-03-29T22:21:46+00:00
- Messages: 11
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #91998 — Nikita Popov — 2016-03-29T22:21:46+00:00

URL: https://externals.io/message/91998#91998

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

---

## #92015 — Chris Riley — 2016-03-30T09:29:04+00:00

URL: https://externals.io/message/91998#92015 · In reply to: #91998

> 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.

---

## #92017 — Dmitry Stogov — 2016-03-30T09:34:31+00:00

URL: https://externals.io/message/91998#92017 · In reply to: #91998

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.

---

## #92018 — Nikita Popov — 2016-03-30T09:36:21+00:00

URL: https://externals.io/message/91998#92018 · In reply to: #92015

> 
>
> > […]
>
>
> 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

---

## #92021 — Nikita Popov — 2016-03-30T11:02:11+00:00

URL: https://externals.io/message/91998#92021 · In reply to: #92017

> 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.
>

Is this referring to https://github.com/badoo/soft-mocks? If so, searching
the codebase for getClosure gives no results, so it shouldn't be affected.

I know that the Go! AOP framework used to do some odd things with closure
binding on methods. However their use case was *already* completely broken
by the limitations we added in 7.0 and the feature was removed.

Nikita

---

## #92022 — Alexander Lisachenko — 2016-03-30T12:07:49+00:00

URL: https://externals.io/message/91998#92022 · In reply to: #92021

Hello, internals!

Go! AOP was used closure rebinding to an incompatible context only for one
minor specific feature, called privileged advices, where method was
executing in the context of target class. But all main closure binding in
the framework core work only with compatible contexts, so everything is ok
with PHP7 now.

So, this new patch is ok for me. Anyway, if I need this functionality
again, I can convert methods to the closures via AST transformation of
classes.

2016-03-30 14:02 GMT+03:00 Nikita Popov <nikita.ppv@gmail.com>:

> 
>
> > […]
>
> Is this referring to https://github.com/badoo/soft-mocks? If so, searching
> the codebase for getClosure gives no results, so it shouldn't be affected.
>
> I know that the Go! AOP framework used to do some odd things with closure
> binding on methods. However their use case was *already* completely broken
> by the limitations we added in 7.0 and the feature was removed.
>
> Nikita

---

## #92027 — Dmitry Stogov — 2016-03-30T16:18:33+00:00

URL: https://externals.io/message/91998#92027

Great it's not used anymore [&#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.


Thanks. Dmitry.

---

## #92030 — Nikita Popov — 2016-03-30T19:46:08+00:00

URL: https://externals.io/message/91998#92030 · In reply to: #92027

> 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

---

## #92031 — Dmitry Stogov — 2016-03-30T20:15:32+00:00

URL: https://externals.io/message/91998#92031 · In reply to: #92030

> On Wed, Mar 30, 2016 at 6:18 PM, Dmitry Stogov <dmitry@zend.com 
> <mailto:dmitry@zend.com>> wrote:
>
>     Great it's not used anymore &#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.

thank you for discovering this.
Of course we can't drop or change SPL behavior.
Trampoline looks like a possible but too complex solution.
anyway, it would be great to remove this hack.

Thanks. Dmitry.


>
> Nikita

---

## #92150 — Nikita Popov — 2016-04-07T22:42:33+00:00

URL: https://externals.io/message/91998#92150 · In reply to: #91998

> 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
>

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

Nikita

---

## #92164 — Yasuo Ohgaki — 2016-04-08T09:09:29+00:00

URL: https://externals.io/message/91998#92164 · In reply to: #92150

Hi Nikita,


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

Could you write UPGRADING for this?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net