[RFC] Property Hook improvements

Lynn

2 years ago

On Mon, Jul 1, 2024 at 7:05 PM Larry Garfield <larry@garfieldtech.com>
wrote:

> Hi folks.  As Ilija's been polishing off hooks to get the PR merged, we've
> run into two small revisions that should make life better for all
> involved.  One is a performance improvement that requires a very slight
> error handling behavior change, and the other is enabling readonly in
> selected (but probably all of the relevant) circumstances.
>
> I'd say we expect these to be uncontroversial, but this is PHP. :-)  So I
> will instead just note that it's a short RFC and open the discussion
> accordingly.
>
> https://wiki.php.net/rfc/hook_improvements
>
> --
>   Larry Garfield
>   larry@garfieldtech.com

"A side effect of that optimization, however, is that we cannot proactively
detect the bug above. Instead, it would result in an infinite loop, which
would eventually trigger a a stack overflow."

This got a small typo ("a a" at the end). This reads no different to me
than infinite recursion between 2 methods. It honestly does not bother me,
and any static analysis tool will be able to pick this anyway. Sounds like
a free performance gain to me!

Larry Garfield

2 years ago

On Mon, Jul 1, 2024, at 5:28 PM, Lynn wrote:

> "A side effect of that optimization, however, is that we cannot
> proactively detect the bug above. Instead, it would result in an
> infinite loop, which would eventually trigger a a stack overflow."
>
> This got a small typo ("a a" at the end).

Fixed, thanks.

> This reads no different to me
> than infinite recursion between 2 methods. It honestly does not bother
> me, and any static analysis tool will be able to pick this anyway.
> Sounds like a free performance gain to me!

Yep, it would be exactly 2 method recursion, and the engine would naturally treat it as such.

--Larry Garfield

Tim Düsterhus

2 years ago

Hi

On 7/1/24 19:02, Larry Garfield wrote:

> I'd say we expect these to be uncontroversial, but this is PHP. :-)  So I will instead just note that it's a short RFC and open the discussion accordingly.

Big fan of the performance improvement, given that I convinced Ilija to
look into that once more. Really happy with what he found there.

For the second part about supporting readonly: I don't want to rush this
in the last few weeks before feature freeze, especially since the word
'investigating' appears in the RFC text. Given that readonly is
currently entirely unsupported, this could very well be added with PHP
8.5, allowing folks to really think about the implications and how it
interacts with stuff. That's a "no" from my side.

Best regards
Tim Düsterhus

PS: Small typo: '$this-dbApi->loadCategory($this->categoryId);', '>'
missing there after 'this-'.

Larry Garfield

2 years ago

On Mon, Jul 1, 2024, at 8:32 PM, Tim Düsterhus wrote:

> Hi
>
> On 7/1/24 19:02, Larry Garfield wrote:
>> I'd say we expect these to be uncontroversial, but this is PHP. :-)  So I will instead just note that it's a short RFC and open the discussion accordingly.
>
> Big fan of the performance improvement, given that I convinced Ilija to
> look into that once more. Really happy with what he found there.
>
> For the second part about supporting readonly: I don't want to rush this
> in the last few weeks before feature freeze, especially since the word
> 'investigating' appears in the RFC text. Given that readonly is
> currently entirely unsupported, this could very well be added with PHP
> 8.5, allowing folks to really think about the implications and how it
> interacts with stuff. That's a "no" from my side.

"Investigating" here is a small term, honestly.  Ilija just hadn't had a chance to poke at it to see where is more feasible.  I expect it would be a day or so at most to sort that part out; we just wanted to get the discussion going given the timing, and he didn't have time over the weekend. :-)

Are there specific concerns you have about implications, or just a general "dude, it's July" sense?

> Best regards
> Tim Düsterhus
>
> PS: Small typo: '$this-dbApi->loadCategory($this->categoryId);', '>'
> missing there after 'this-'.

Fixed, thanks.

--Larry Garfield

Tim Düsterhus

2 years ago

Hi

On 7/1/24 22:57, Larry Garfield wrote:

> Are there specific concerns you have about implications, or just a general "dude, it's July" sense?
>

From my side primarily "dude, it's July", because I do not want to
think this through right now. Claude did the thinking, though and I
agree with the examples he gave to point out how this is a bad idea.

Best regards
Tim Düsterhus

Claude Pache

2 years ago

> Le 1 juil. 2024 à 19:02, Larry Garfield <larry@garfieldtech.com> a écrit :
>
> Hi folks.  As Ilija's been polishing off hooks to get the PR merged, we've run into two small revisions that should make life better for all involved.  One is a performance improvement that requires a very slight error handling behavior change, and the other is enabling readonly in selected (but probably all of the relevant) circumstances.
>
> I'd say we expect these to be uncontroversial, but this is PHP. :-)  So I will instead just note that it's a short RFC and open the discussion accordingly.
>
> https://wiki.php.net/rfc/hook_improvements
>
> --
>  Larry Garfield
>  larry@garfieldtech.com

Hi,

1. Removing the guard against recursion is sensible to me, as the sort of bug it is supposed to prevent is not specific to property accessors. IMO, a better solution to the issue is to implement a global upper limit on the call stack size. Currently, I am able to generate a call stack of more than 10 millions items before an OOM error occurs; PHP should have thrown a stack overflow error long before that. But this is entirely orthogonal to property hooks.

2. As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that `readonly` is only acceptable if there is no `get` hook.

The primary reason I would want readonly with `get` hook, is lazy initialisation, and I think that there is better design than to trust the user for not implementing bugs. For that use case, I think it is preferable to implement dedicated hook for lazy initialisation, orthogonal to the readonly feature. Here is what I would suggest (in a separate RFC, of course):

* An additional hook, called `init`, may be added to backed properties. When attempting to read the value of the backing store, if it is uninitialised, then the `init` hook is triggered, which is supposed to initialise it.

In general, a hooked property can be marked as `readonly` when it is backed and has no `get` hook. In particular, it can have an `init` hook, which is all we need in order to have a lazy readonly property that is robust against bugs by construction.

—Claude

Claude Pache

2 years ago

> Le 3 juil. 2024 à 11:54, Claude Pache <claude.pache@gmail.com> a écrit :
>
>
> 2. As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that `readonly` is only acceptable if there is no `get` hook.

Hi,

One more thing, why I think that we should be strict here. It is not just for preventing the user to write *dumb* code, it is also for preventing them to write *creative* code, e.g.

```php
class doc {
public readonly int page {
get => $this->page + $this->offset;
}
private int $offset = 0;
public function __construct(int $page) {
$this->page = $page;
}
public function foo() {
// $this->offset may be adjusted here
}
}
```

—Claude

Rob Landers

2 years ago

On Wed, Jul 3, 2024, at 14:28, Claude Pache wrote:

>
>
>> Le 3 juil. 2024 à 11:54, Claude Pache <claude.pache@gmail.com> a écrit :
>>
>>
>> 2. As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that `readonly` is only acceptable if there is no `get` hook.
>
> Hi,
>
> One more thing, why I think that we should be strict here. It is not just for preventing the user to write *dumb* code, it is also for preventing them to write *creative* code, e.g.
>
> ```php
> class doc {
>     public readonly int page {
>         get => $this->page + $this->offset;
>     }
>     private int $offset = 0;
>     public function __construct(int $page) {
>         $this->page = $page;
>     }
>     public function foo() {
>         // $this->offset may be adjusted here
>     }
> }
> ```
>
> —Claude

I don't see anything wrong with that code. $page doesn't say "immutable" it says "readonly", as in "cannot write to".

— Rob

Claude Pache

2 years ago

> Le 3 juil. 2024 à 14:42, Rob Landers <rob@bottled.codes> a écrit :
>
> On Wed, Jul 3, 2024, at 14:28, Claude Pache wrote:
>>
>>
>>> Le 3 juil. 2024 à 11:54, Claude Pache <claude.pache@gmail.com> a écrit :
>>>
>>>
>>> 2. As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that `readonly` is only acceptable if there is no `get` hook.
>>
>> Hi,
>>
>> One more thing, why I think that we should be strict here. It is not just for preventing the user to write *dumb* code, it is also for preventing them to write *creative* code, e.g.
>>
>> ```php
>> class doc {
>>     public readonly int page {
>>         get => $this->page + $this->offset;
>>     }
>>     private int $offset = 0;
>>     public function __construct(int $page) {
>>         $this->page = $page;
>>     }
>>     public function foo() {
>>         // $this->offset may be adjusted here
>>     }
>> }
>> ```
>>
>> —Claude
>
> I don't see anything wrong with that code. $page doesn't say "immutable" it says "readonly", as in "cannot write to".
>
> — Rob

The fact you don’t see anything wrong is *precisely* the issue. According to the original RFC [1], the intention of readonly properties was immutability, not just non-writability. For the weaker case of non-writability, it was explicitly mentioned the possibility of (future) asymmetric property visibility.

That said, I agree that “immutable” would have been a better word choice than “readonly” for conveying the original intention. We could also ignore the original intention and reinterpret the feature in case everyone agrees (personally, I prefer the original intention, but I won’t fight for that).

—Claude

[1]: https://wiki.php.net/rfc/readonly_properties_v2#rationale

Rob Landers

2 years ago

On Wed, Jul 3, 2024, at 15:29, Claude Pache wrote:

>
>
>> Le 3 juil. 2024 à 14:42, Rob Landers <rob@bottled.codes> a écrit :
>>
>> On Wed, Jul 3, 2024, at 14:28, Claude Pache wrote:
>>>
>>>
>>>> Le 3 juil. 2024 à 11:54, Claude Pache <claude.pache@gmail.com> a écrit :
>>>>
>>>>
>>>> 2. As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that `readonly` is only acceptable if there is no `get` hook.
>>>
>>> Hi,
>>>
>>> One more thing, why I think that we should be strict here. It is not just for preventing the user to write *dumb* code, it is also for preventing them to write *creative* code, e.g.
>>>
>>> ```php
>>> class doc {
>>>     public readonly int page {
>>>         get => $this->page + $this->offset;
>>>     }
>>>     private int $offset = 0;
>>>     public function __construct(int $page) {
>>>         $this->page = $page;
>>>     }
>>>     public function foo() {
>>>         // $this->offset may be adjusted here
>>>     }
>>> }
>>> ```
>>>
>>> —Claude
>>
>> I don't see anything wrong with that code. $page doesn't say "immutable" it says "readonly", as in "cannot write to".
>>
>> — Rob
>
> The fact you don’t see anything wrong is *precisely* the issue. According to the original RFC [1], the intention of readonly properties was immutability, not just non-writability. For the weaker case of non-writability, it was explicitly mentioned the possibility of (future) asymmetric property visibility.

That was before properties could be functions? Now I suppose we have to decide if the definition of read-only doesn't mean read-only, but immutability; despite the original intention.

>
> That said, I agree that “immutable” would have been a better word choice than “readonly” for conveying the original intention. We could also ignore the original intention and reinterpret the feature in case everyone agrees (personally, I prefer the original intention, but I won’t fight for that).

Agree. I just wanted to point out that there were two interpretations: the English definition, and the original intention of the RFC.

— Rob

Rob Landers

2 years ago

On Wed, Jul 3, 2024, at 16:22, Rob Landers wrote:

>
>
> On Wed, Jul 3, 2024, at 15:29, Claude Pache wrote:
>>
>>
>>> Le 3 juil. 2024 à 14:42, Rob Landers <rob@bottled.codes> a écrit :
>>>
>>> On Wed, Jul 3, 2024, at 14:28, Claude Pache wrote:
>>>>
>>>>
>>>>> Le 3 juil. 2024 à 11:54, Claude Pache <claude.pache@gmail.com> a écrit :
>>>>>
>>>>>
>>>>> 2. As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that `readonly` is only acceptable if there is no `get` hook.
>>>>
>>>> Hi,
>>>>
>>>> One more thing, why I think that we should be strict here. It is not just for preventing the user to write *dumb* code, it is also for preventing them to write *creative* code, e.g.
>>>>
>>>> ```php
>>>> class doc {
>>>>     public readonly int page {
>>>>         get => $this->page + $this->offset;
>>>>     }
>>>>     private int $offset = 0;
>>>>     public function __construct(int $page) {
>>>>         $this->page = $page;
>>>>     }
>>>>     public function foo() {
>>>>         // $this->offset may be adjusted here
>>>>     }
>>>> }
>>>> ```
>>>>
>>>> —Claude
>>>
>>> I don't see anything wrong with that code. $page doesn't say "immutable" it says "readonly", as in "cannot write to".
>>>
>>> — Rob
>>
>> The fact you don’t see anything wrong is *precisely* the issue. According to the original RFC [1], the intention of readonly properties was immutability, not just non-writability. For the weaker case of non-writability, it was explicitly mentioned the possibility of (future) asymmetric property visibility.
>
> That was before properties could be functions? Now I suppose we have to decide if the definition of read-only doesn't mean read-only, but immutability; despite the original intention.
>
>>
>> That said, I agree that “immutable” would have been a better word choice than “readonly” for conveying the original intention. We could also ignore the original intention and reinterpret the feature in case everyone agrees (personally, I prefer the original intention, but I won’t fight for that).
>
> Agree. I just wanted to point out that there were two interpretations: the English definition, and the original intention of the RFC.
>
> — Rob

In better words, as a dev, I don't care which interpretation is chosen, as long as it is consistent.

— Rob

Derick Rethans

2 years ago

On Wed, 3 Jul 2024, Claude Pache wrote:

> > Le 1 juil. 2024 à 19:02, Larry Garfield <larry@garfieldtech.com> a
> > écrit :
> >
> > Hi folks.  As Ilija's been polishing off hooks to get the PR merged,
> > we've run into two small revisions that should make life better for
> > all involved.  One is a performance improvement that requires a very
> > slight error handling behavior change, and the other is enabling
> > readonly in selected (but probably all of the relevant)
> > circumstances.
> >
> > I'd say we expect these to be uncontroversial, but this is PHP. :-)
> > So I will instead just note that it's a short RFC and open the
> > discussion accordingly.
> >
> > https://wiki.php.net/rfc/hook_improvements
>
> 1. Removing the guard against recursion is sensible to me, as the sort
> of bug it is supposed to prevent is not specific to property
> accessors. IMO, a better solution to the issue is to implement a
> global upper limit on the call stack size.

That is already done in PHP 8.4, if you end up using too much stack, PHP
will now bail out graciously:
https://github.com/php/php-src/blob/master/Zend/zend_call_stack.c

> Currently, I am able to generate a call stack of more than 10 millions
> items before an OOM error occurs; PHP should have thrown a stack
> overflow error long before that.

Why? Most actual function calls are not done on the stack any more. So
it is now perfectly okay to have that many nested calls. If you want it
to bail out earlier (during testing), then Xdebug will help you.

cheers,
Derick

--
https://derickrethans.nl | https://xdebug.org | https://dram.io

Author of Xdebug. Like it? Consider supporting me: https://xdebug.org/support

mastodon: @derickr@phpc.social @xdebug@phpc.social

Derick Rethans

2 years ago

On Mon, 1 Jul 2024, Larry Garfield wrote:

> Hi folks.  As Ilija's been polishing off hooks to get the PR merged,
> we've run into two small revisions that should make life better for
> all involved.  One is a performance improvement that requires a very
> slight error handling behavior change, and the other is enabling
> readonly in selected (but probably all of the relevant) circumstances.
>
> I'd say we expect these to be uncontroversial, but this is PHP. :-)
> So I will instead just note that it's a short RFC and open the
> discussion accordingly.
>
> https://wiki.php.net/rfc/hook_improvements

1. Remove the proactive guard against recursive backing value access

"which would eventually trigger a stack overflow"

Would it though? PHP 8.4 has an actual protection against this now,
instead of getting the white-screen-of-death.

Not that it matters much, but it's probably good to be precise in the
language.

cheers,
Derick

Ilija Tovilo

2 years ago

Hi Derick

On Wed, Jul 10, 2024 at 11:32 AM Derick Rethans <derick@php.net> wrote:

>
> On Mon, 1 Jul 2024, Larry Garfield wrote:
>
> >
> > https://wiki.php.net/rfc/hook_improvements
>
> 1. Remove the proactive guard against recursive backing value access
>
>         "which would eventually trigger a stack overflow"
>
> Would it though? PHP 8.4 has an actual protection against this now,
> instead of getting the white-screen-of-death.
>
> Not that it matters much, but it's probably good to be precise in the
> language.

Yes, this excerpt is referring to the PHP stack overflow detection,
which will throw an exception rather than segfault. That is, on all
the architectures that already support stack overflow detection. We're
just reusing the same mechanism already used for normal function
calls.

---

Hi Claude

> 1. Removing the guard against recursion is sensible to me, as the sort of bug it is supposed to prevent is not specific to property accessors. IMO, a better solution to the issue is to implement a global upper limit on the call stack size. Currently, I am able to generate a call stack of more than 10 millions items before an OOM error occurs; PHP should have thrown a stack overflow error long before that. But this is entirely orthogonal to property hooks.

PHP allows using recursion for algorithms on arbitrarily large data
because function calls do not cause the C stack to grow. Limiting the
PHP stack size would thus require these algorithms to be rewritten to
iterative ones, which can make them more complicated. What we could do
is check for stack size on OOM and indicate that the OOM was most
likely caused by infinite recursion.

Anyway, This does not really apply to hooks, because it will (usually)
cause VM reentry for recursion, growing the stack size and thus
triggering the "fast" stack overflow detection error.

Ilija

Larry Garfield

2 years ago

On Mon, Jul 1, 2024, at 12:02 PM, Larry Garfield wrote:

> Hi folks.  As Ilija's been polishing off hooks to get the PR merged,
> we've run into two small revisions that should make life better for all
> involved.  One is a performance improvement that requires a very slight
> error handling behavior change, and the other is enabling readonly in
> selected (but probably all of the relevant) circumstances.
>
> I'd say we expect these to be uncontroversial, but this is PHP. :-)  So
> I will instead just note that it's a short RFC and open the discussion
> accordingly.
>
> https://wiki.php.net/rfc/hook_improvements

Based on discussion here and off-list, and after confirming with Nicolas that lazy objects would be compatible with readonly properties already (which have very similar use cases in practice), we're going to hold off on the readonly hooks part of this RFC.  I've split it off to its own RFC for later:

https://wiki.php.net/rfc/readonly_hooks

The other half of this RFC, removing the recursion guard in return for performance, has had no objections.  Its discussion period ends Friday, so I will open the vote on that Monday-ish.

--Larry Garfield

Tim Düsterhus

2 years ago

Hi

On 7/10/24 15:47, Larry Garfield wrote:

> The other half of this RFC, removing the recursion guard in return for performance, has had no objections.  Its discussion period ends Friday, so I will open the vote on that Monday-ish.

Thank you, looks good to me, ship it.

One minor remark about the RFC page: You didn't remove the "(use today's
date here)" placeholder at the top.

Best regards
Tim Düsterhus