Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123117
Precedence: bulk
MIME-Version: 1.0
References: <CAPyj-LBMQEJ=2BF=xk_on4O2xTU+MM2XRAZ5g=uPwVOf=bh+sw@mail.gmail.com>
 <66154AA0.1040905@adviesenzo.nl> <66160A1D.4060409@adviesenzo.nl>
In-Reply-To: <66160A1D.4060409@adviesenzo.nl>
Date: Fri, 12 Apr 2024 01:00:25 +0200
Message-ID: <CAPyj-LDFKWut3cg0NzKAKZ6XFRnMd4YbC5TM2Fvb6Jop4mBB6g@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC][Vote announcement] Property hooks
To: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: tovilo.ilija@gmail.com (Ilija Tovilo)

Hi Juliette

On 9-4-2024 16:03, Juliette Reinders Folmer wrote:
> On 8-4-2024 23:39, Ilija Tovilo wrote:
>>
>> https://wiki.php.net/rfc/property-hooks
>>
>
> I realize it is late in the discussion period to speak up, but for months=
 I've been trying to find the words to express my concerns in a polite and =
constructive way and have failed.

First of all, thank you for taking the time to analyze the RFC and
form your thoughts. I know how much time and effort it takes.
Nonetheless, I hope you understand that receiving feedback of this
sort literally minutes before a vote is planned is close to the worst
way to go about it, both from a practical standpoint, but also RFC
author and reviewer morale.

Many of the points you raise have been there for many months to a
year, and many have been raised and discussed many times over. I will
try to address each, and give people some time to respond.

> And not just one syntax, but five and the differences in the semantics of=
 the function syntaxes are significant.

I think this is somewhat of a misrepresentation. For reference, you
can find the grammar here. It's only about <50 lines.

https://github.com/php/php-src/blob/bf390b47c7522fd4d130be26b8ee97520f98527=
5/Zend/zend_language_parser.y#L1088-L1131

Notably, `get` and `set` hooks don't actually have a different
grammar. Instead, hooks have names, optional parameter lists, and an
optional short (`=3D>`) or long (`{}`) body. Whether all possible
combinations are considered different syntax is open to
interpretation. It's true that additional rules apply to each hook,
e.g. whether they are allowed to declare a parameter list, and how the
return value is used. I could see an argument being made for allowing
an empty parameter list (`()`) for `get`, for consistency. Let us know
if you have any other ideas to make them more congruent.

In any case, I don't believe PHP_CodeSniffer would need to handle each
combination separately.

What would your optimal solution look like? I feel this discussion
cannot progress unless we have more concrete discussion points.

> * The implicit "set" parameter, which does not have a explicit parameter,=
 does not have parentheses and has the magically created $value variable.

To be a bit more exact, the parameter list is simply inferred to
`(<property type> $value)` so that the user does not need to spell it
out. This is not akin to other magic variables we had previously, like
`$http_response_header`.

> TL;DR: this RFC tries to do too much in one go and introduces a huge amou=
nt of cognitive complexity with all the exceptions and the differences in b=
ehaviour between virtual and backed properties. This cognitive complexity i=
s so high that I expect that the feature will catch most developers out a l=
ot of the time.

Many people still say that this RFC doesn't do enough because it
doesn't support x/y/z. This is including you:

> * The arrow function variant for `set` with all the above differences + t=
he differences inherent to arrow functions with the above mentioned excepti=
ons + the implicit assignment, which breaks the expected behaviour of arrow=
 functions by assigning the result of the expression instead of returning i=
t (totally understandable, but still a difference).
> * Properties _may_ or _may not_ have a default value anymore, depending o=
n whether the hooks cause it to be a backed or a virtual property.
> * Properties with hooks can _be_ readonly, but cannot be declared as read=
only.
> * Only available for object properties, static properties are excluded.
> * Readonly properties which are not denoted as readonly, but still are du=
e to their virtual nature (get without access to $this->prop), which can on=
ly be figured out by, again, studying the contents of the hook function.

There are more below that I will be commenting on in more detail.

I understand that, in a perfect world, each language feature would
automatically work with every other. In reality, this is either not
possible, or complex and time consuming. We opted for the middle
ground, allowing things that were workable and omitting things that
weren't, or required unrelated changes. If this is not acceptable,
then I genuinely don't know how to approach non-trivial RFCs.

> * Properties can now be declared as abstract, but only with explicit hook=
 requirements, otherwise the abstract keyword is not allowed.
> * Properties can now be declared on interfaces, but only with explicit ho=
ok requirements, otherwise they are not allowed.

The semantics of plain interface properties are not clear.
Intuitively, `public $prop;` looks equivalent to `public $prop { get;
set; }`, but due to reference semantics it is not. It's not even
equivalent to `{ &get; set; }`, because by-reference hooked properties
still have some inherent limitations
(https://wiki.php.net/rfc/property-hooks#assignment_by_reference).
Again, this could be addressed, but only at the cost of complexity.

> * Abstract/Interface properties don't comply with the same covariance/con=
travariance rules as other properties

Do you mean that they are not invariant as other properties? I think
it's generally accepted that LSP rules should be as lax as possible,
while being sound. Note that this rule isn't unique to
read-/write-only abstract properties either, but also applies to
read-/write-only virtual properties.

```php
class P {
    public string|int $readonly { get =3D> 'foo'; }
}

class C extends P {
    public int $readonly { get =3D> 42; }
}
```

> And last but not least, there is the ability to declare property hooks in=
 constructor property promotion ... where we can now basically have a funct=
ion declaration within the signature of a method declaration.... with all f=
ive possible syntax types.

The grammar used for hooks in constructor property promotion is the
same as the one used for properties. Am I wrong to assume this
shouldn't cause any additional work for PHP_CodeSniffer?

> * `var` for property declarations is not supported according to the RFC.

This is a misunderstanding between Larry and me. I personally never
intended to deprecate `var` in this RFC. We've briefly discussed this
and decided to treat `var` as normal, meaning as an alias of `public`.

> * Disallows references to properties, but only when there is a set hook.
> * Disallows indirect modification of a property, but only when there is a=
 set hook.

The RFC spends much time explaining these limitations. If the RFC is
ever accepted, in any form, then we must accept either of these
approaches:

1. The engine places limitations on by-reference usage of properties
when there is a `set` hook.
2. The user can trivially circumvent the `set` hook.

This limitation is not unique to PHP either.

> * Write-only (virtual) properties, which cannot be read and you need to s=
tudy the hook declaration in detail to figure out if a property is or is no=
t write-only.

Indeed. But this has also been discussed in much detail. One proposed
solution was explicitly marking properties as virtual. This comes with
the downside of changing the property signature, making conversion of
a backed parent property to virtual (and vice versa) a BC break. If
you have a different solution in mind, please share.

> Oh, and hang on, they *can* be read from a method in the same class as lo=
ng as that method is called from within the set hook, so now the method wil=
l need to either do a backtrace or use Reflection to figure out whether it =
has access to the property value.

Right. We use the same mechanism as `__get`/`__set` uses to protect
against recursion. In particular, when entering a magic method for a
particular property, PHP remembers this using a "property guard". It's
really just a marker in the object that sets the magic method type,
along with the property name.

When the same property is accessed again, the magic method or hook is
skipped, and PHP behaves as if the magic method or hook wasn't there
at all. For `__get`, `__set` and hooks of virtual properties, this
means accessing a dynamic property. For backed properties, this means
accessing its backing value.

After discussing it, we've decided to make virtual properties slightly
less surprising by throwing, instead of trying to create a dynamic
property. This would be consistent with how virtual read-, and
write-only properties are handled outside of hooks, when they are
written to or read, respectively.

> now why does that remind me of the magic __...() methods ?

That's no secret. We have mentioned many times that we use the same
mechanism for accessing backing value as `__get`/`__set` uses for
accessing dynamic properties.

> * Whether a type can be specified on the parameter on `set` depends on wh=
ether the property is typed. You cannot declare `set(mixed $value)` for an =
untyped property, even though it would effectively be compatible. This is i=
nconsistent with the behaviour for, for instance method overloads, where th=
is is acceptable: https://3v4l.org/hbCor/rfc#vrfc.property-hooks , though i=
t is consistent with the behaviour of property overloads, where this is not=
 acceptable: https://3v4l.org/seDWM (anyone up for an RFC to fix this incon=
sistency ?)

For child properties, it is not legal to add `mixed` to a previously
untyped property. https://3v4l.org/mScQS#v8.3.5 I was honestly not
aware of the case you outlined. Anyway, either approach we chose will
be inconsistent with the other.

> * Changes the meaning of `$this->property` read/write access for all meth=
ods called from within a hook while executing the hook.

What is the right solution to this in your opinion? Recursion surely
isn't it. Throwing when accessing the backing value outside of its
corresponding hook? I will investigate whether this is possible in an
efficient way, and whether it's worth considering.

> * Creates two different flavour of properties: "backed" properties and "v=
irtual" properties with significantly different behaviours, raising cogniti=
ve complexity.
>     This includes, but is not limited to the fact that set hooks can be b=
ypassed on virtual properties via a get reference.

Sure, but is there actually a solution to this? Making all properties
backed means that serialization includes backing values that aren't
used, and that the property supports useless reads/writes for
properties that are only read or only written to. Only supporting
virtual properties means that there's an inevitable BC break when
converting a backed property to a virtual one, because the explicit
backing property cannot share the same name as the virtual one, which
will once again affect serialization.

> * The range of different behaviours for various forms of serialization.

Indeed, we carefully picked the behavior that should make sense for
each case. Isn't that better than the user having to remember that
they need to handle serialization/JSON themselves, depending on the
case, which they also need to identify themselves?

> [1] The RFC also states in "Interaction with isset() and unset()" that th=
e behaviour of property hooks is consistent with how `isset()` interacts wi=
th `__get()` today. This is not true, as in: the behaviour as described for=
 isset with magic methods is different than what the RFC states it is: http=
s://3v4l.org/2Arkh/rfc#vrfc.property-hooks

Right, this paragraph is a bit misleading. In essence, `isset`
triggers both `__isset` and `__get` to 1. verify that the property
even exists and 2. if it does, that it is non-null. For hooked
properties, we don't need to verify the former, because the property
is explicitly declared. So we only call `get` and make sure its value
is non-null.

We'll make sure to clarify this better.

> Additionally, it proposes for isset() to throw an Error for virtual prope=
rties without a get hook. This is surprising behaviour as isset() by its na=
ture is expected to *not* throw errors, but return false for whatever is no=
t set or inaccessible from the current context: https://3v4l.org/3OlgM

Personally, I would rather know when checking `isset` on something
that can never return `true`. But I admit that this case is not
clear-cut. The throwing approach can be relaxed to return false
without a BC break though, but not the opposite.

> [2] PHP supports "multi-property declarations", but I see no mention of t=
hese in the RFC, which makes it unclear if and if so, how property hooks wo=
uld work with multi-property declarations.
> class Foo {
>     public string $bar, $baz, $bab;
> }

The implementation does not allow mixing multi-property declarations
with hooks. I find them confusing (e.g. are `public int $foo, $bar;`
two typed properties, or one typed and one untyped?). The same applies
for hooks. We'll make sure to spell this out explicitly.

> [3] If a `set` hook gets a named parameter, is the magic `$value` variabl=
e still available or not ? I'd expect not, but it is unclear from the RFC.

No. As mentioned above, on omission of the parameter list, it is
simply inferred. There's no "truly" magic `$value` variable.

> [4] Why should ReflectionProperty::getRawValue() throw an error for stati=
c properties ? Instead of returning the value, identically to ReflectionPro=
perty::getValue() ? This is surprising to me and I see no reason for it.

Because it's equivalent to `getValue`, and indicates the user is doing
something they don't fully understand.

> As for the impact on static analysis tools....

I won't comment too much on this, because it's your area of expertise.
Of course, I sympathize. If there's a solution that works well for us
_and_ you, let me know. But it doesn't sound like some simple tweaks
can significantly improve this problem for you.

Here are the things we're changing:

* `var` continues to be supported.
* We will change accessing a virtual property with the same name from
within a method called from a hook to throw, rather than create a
dynamic property.
* We will change accessing a backed property from within a method
called from a hook to throw as well, assuming it can be done
performantly. (We need to verify this.)

To summarize: I understand your concerns and why you raise them.
However, none of the complexity in this RFC is arbitrary. Most of it
is inherent in the problem space, and the RFC=E2=80=99s approach is the res=
ult
of extensive experimentation to determine possible solutions. While
some superficial changes are possible, the actual complex parts
(reference handling, inheritance, interfaces, virtual properties,
etc.) are all necessary complexity, not accidental complexity. Absent
a novel alternative, which after over a year of work and extensive
discussions on this list and elsewhere we do not believe exists, we
believe this RFC represents "the best hooks implementation that PHP
can support." And that implementation deserves a final vote to
determine if the internals community wants hooks at all or not.

Ilija