missing isset() on private(set) property (backing a get hook)

[hakre]

Description

i'm able to trigger an infinite recursion by using isset() on a private(set) property that has a get hook:

<?php

// https://github.com/php/php-src/issues/18318

$foo = new CatchMeIfYouCan();

var_dump($foo->bar);

final class CatchMeIfYouCan
{
    private(set) int $bar
    {
        get => $this->bar ??= $this->fooBar();
    }

    public function fooBar(): int
    {
        isset($this->bar);
    }
}

https://3v4l.org/LALTu

this came unexpected to me as it is normally possible to make use of isset() or ??= within a class safely.

expected is to fail as fooBar() does not return an integer.

---

and it seems to me that the use of ??= on the member is only possible within the hook, but not within a different place in the class's protocol.

example:

    ...

    public function fooBar(): int
    {
        return $this->bar ??= 1;
    }
}

PHP Version

PHP 8.4.5 (cli) & php-src/sapi/cli/php

Operating System

No response

[iluuu1994]

Hi @hakre. See https://wiki.php.net/rfc/hook_improvements. This is expected behavior. Accessing hooks from methods called within hooks will trigger hooks again. Previously (in the original RFC), this would error. While erroring was the preferred behavior, it was removed to improve performance for hooks to be on-par with a conventional getter method.

[iluuu1994]

Maybe this example clarifies what is happening:

final class CatchMeIfYouCan
{
    private int $bar;

    public function getBar(): int
    {
        return $this->bar ??= $this->fooBar();
    }

    public function fooBar(): int
    {
        $bar = $this->getBar();
        isset($bar);
    }
}

Access of the property outside of hooks always calls hooks. Access of the property within hooks always accesses the backing property. Hence, isset($this->bar); calls the get hook, leading to infinite recursion. It's not related to private(set).

[hakre]

Hmm, this is throwing me pretty much back in time even before ??= without introducing a private property only for making use of the hook.

I would love to see the isset() test (also via ?? or ??=) not affected by it.

But should have posted earlier, it's clear to me what happens, I have it also with the recursion warning which shows the stack.

Just for your better understanding from userland perspective:

refactoring from

    private(set) int $exit
    {
        get => $this->exit ??= proc_close($this-handle);
    }

to

    private(set) int $exit
    {
        get => $this->exit ??= $this->close();
    }

I guess I'd just need to get away from either the backing or the arrow.

If you don't spot any related error I think it would be nice to get a catchable Error there, but it is likely not a bug then.

[iluuu1994]

If you don't spot any related error

It depends on what close() does. If it accesses $this->exit, it's not safe to call. Can you show this code as well?

I think it would be nice to get a catchable Error there, but it is likely not a bug then.

If we can find a performant way to throw an error, I'm certainly open to it. But it was converted from an error to recursion specifically because checking for recursion is expensive, and shouldn't happen in correct code.

[hakre]

it's in the report: either isset($this->prop) or return $this->prop ??= some-integer; that's all.

[iluuu1994]

Well, I think it's reasonable for this not to work. By checking isset($this->prop) you're subtly leaking implementation details of $prop, namely that the property can be uninitialized. Property hooks are there to hide these implementation details. The correct thing to do here, IMO, is to check for isset within the hook. If you really need to check for the underlying value (or lack of value, in this case) of the property, you can use a separate backing property, as you have already indicated. https://3v4l.org/PFKim#v8.4.6

Nonetheless, let's hear what @Crell has to say, maybe he has a different opinion.

[Crell]

I'm inclined to agree with Ilija. Infinite loops are a logic problem. It would be nice to catch it more explicitly, but the performance cost of that was too high. If we can improve the error handling without hurting performance, great, but performance has priority here.

Conceptually, a property should now be seen as an "aspect of" an object. You don't need to say "is this aspect set yet," you just ask for its existence. Checking isset() on a property today is probably a bad idea generally. And looking at the original example, I cannot really envision in what non-contrived case this would even come up.

[hakre]

By checking isset($this->prop) you're subtly leaking implementation details of $prop, namely that the property can be uninitialized.

That's exactly the reason as I understand it why I'm missing it. In the original example we can see the backed property hook and the ??= is expressing that.

This is also what isset() is used for on a readonly attribute.

As we're in the implementation, therefore are the implementation, all implementation details do not leak but are in the information hiding. The backing of the property hook is just not working (in the sense that it does not work by implementing with information hiding).

The resolution is simple: Make the property hook entirely virtual and implement the information hiding as usual. The only thing we loose is the more brief/dense syntax. But that's nothing new we have in the language, so you don't find me complaining here.

There is another interesting aspect that turned out since yesterday: Property hooks get multiple invocation on a followed-up (unrelated) $this access in xdebug+phpstorm breakpoint. Was unable to entirely configure it away, solved it with a local static guard in the by-hook-invoked method. Found it generally useful to implement in method form and even to temporarily remove all hooked properties from the implementation and re-add @property-read __get()-ters temporarily as the debugability was totally messed up.

So for the moment, better endless recursion and fatal. (Seriously.)

For the theoretical thinking, my understanding is that isset() as a language construct should not trigger the hook if it is backed and private set and we come from private. No idea if that scenario has been taken into account in the original design, maybe @Crell remembers?

[hakre]

And thinking: More precisely, isset() should only work on properties, not on hooks (probably unless there is an isset hook which is not an existing thing, as there is no similar unset hook.)

[bwoebi]

@hakre isset() checks for null too, hence that's what it does on hooks. Just like what happens with __get() magic too.

[hakre]

@hakre isset() checks for null too, hence that's what it does on hooks. Just like what happens with __get() magic too.

Did you mean __isset()? ^^ And right: Different than null === $this->prop is isset($this->prop) for unset or null. Additionally it supports multiple expressions to test at once and returns the resolution testing the whole set. Isset() also does not yield diagnostics.

[hakre]

I found a gap that might come in handy for a different error provocation:

Fatal error: Cannot use isset() on the result of an expression (you can use "null !== expression" instead) in Standard input code on line 14

That is an unrelated message, but could potentially offer a small door on the isset() use on a hooked property as it is an expression now even it may look like it is not.

Just noting while running over it.