Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123104
Precedence: bulk
MIME-Version: 1.0
References: <CAPyj-LBMQEJ=2BF=xk_on4O2xTU+MM2XRAZ5g=uPwVOf=bh+sw@mail.gmail.com>
In-Reply-To: <CAPyj-LBMQEJ=2BF=xk_on4O2xTU+MM2XRAZ5g=uPwVOf=bh+sw@mail.gmail.com>
Date: Wed, 10 Apr 2024 17:07:53 -0500
Message-ID: <CAJp_myUm_aPF8ZTehwYPcybEJ1KQ9zZnoa5OKNda9cqgw4VMQQ@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC][Vote announcement] Property hooks
To: Ilija Tovilo <tovilo.ilija@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000e1c8240615c545f0"
From: mweierophinney@gmail.com ("Matthew Weier O'Phinney")

--000000000000e1c8240615c545f0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 8, 2024 at 4:41=E2=80=AFPM Ilija Tovilo <tovilo.ilija@gmail.com=
> wrote:

> Hi everyone
>
> Heads-up: Larry and I would like to start the vote of the property
> hooks RFC tomorrow:
> https://wiki.php.net/rfc/property-hooks
>
> We have worked long and hard on this RFC, and hope that we have found
> some middle-ground that works for the majority. One last concern we
> have not officially clarified on the list:
>
> https://externals.io/message/122445#122667
>
>
After reading JRF's reply, I found myself nodding along. I feel like while
a lot of the _details_ of my original feedback were addressed, the _themes_
largely were not, and many overlap with what Juliette noted.

I've taken some time again today to review the proposal, and I think I can
boil my main concern down to _consistency_, and _ensuring reviewer
comprehension_ when somebody is reviewing code that includes hooks. New
features in PHP really should veer away from allowing _implicit_ behavior,
and should be internally consistent with existing syntax (unless they are
attempting to make an explicit change to existing syntax).

On this latest review, I've got fewer concerns, but I still have some.

1. I'm not a fan of _implicit_. I strongly feel that there's no use case
for `set` without an argument. Requiring the argument makes explicit that
it is _receiving_ a value, and also makes it explicit _at the definition
point_ what value _type_ is accepted, and what it is _named_. (I realize I
might be in a minority here, though, and the fact that I can require this
as a CS rule (eventually) mitigates it to a degree. It feels like an
opportunity to reduce errors by requiring it, however.)

2. I'm not a huge fan of the short syntax, but the improvements in the most
recent draft are _mostly_ ones I can live with. The part that's still
unclear is when and where hooks need a `;` termination, and _why_ the `;`
is used, instead of `,`. When using `match()` expressions, you use `,` to
separate the expressions, but for some reason, the proposal uses `;` ...
but only when using short expressions. And if you have a full-form mixed
with a short form, the `;` is only needed for the short-form expression.
This feels arbitrary, and it will be easy to get it wrong for people
comfortable with `match()` statements.

My gut take is that the syntax should use `,` to separate hooks in ALL
cases:

```
public string $content {
    get {
        $content =3D str_replace('  ', ' ', $this->content);
        return $this->convert($content);
    },
    set(string $value) =3D> $value,
}

public string $summary {
    get =3D> $this->convert($this->summary),
    set(string $value) {
        $value =3D str_replace(('  ', ' ', $value);
        $this->summary =3D $value;
    },
}
```

This more closely matches `match()` expressions (pardon the pun), and
provides a template for how we might allow blocks in `match()` expressions
in the future.

(Larry tells me that it's `match()` being weird here, but considering that
for many developers, their only point of reference for this sort of syntax
IS `match()`, making it feel like the language is ignoring its own syntax
when creating new syntax.)

3. While I'd likely prefer Marco's approach to references (just don't allow
them), the fact that they mirror how `__get()` and `__set()` _currently_
work gives a migration path for users who are familiar with that paradigm's
gotchas. In other words, it's consistent with the current language, and
will make migrating from `__set/__get` to hooks easier. It's a lot of
complexity, but the table you created helps with that. That table MUST make
it to the docs for the feature!

4. The interaction with serialization has a LOT of different cases, and
looks like a great place to observe foot guns in the future. I'd like to
see a more consistent, predictable approach here, but knowing how many
pitfalls there are in serialization normally, I'm not expecting one; at
least it's being consistent with how each of the serialization methods
already work. What I DO expect is that you create a table detailing the
behavior for the docs, similar to the one you did for references/backed
values/virtual.

While I definitely feel for QA/CS tool makers (this is a HUGE chunk of new
syntax, and non-trivial), I also don't know how this could be done in a way
that would be simpler. My only suggestions would be to remove short-form
syntax and require type/varname for `set` always, but even with those
changes, I think similar levels of complexity will still exist to write
parsers/formatters for these anyways.

I personally have wanted these features in the language for likely 15
years. Maybe the foundation might be able to sponsor some developer time
towards helping the QA/CS tool makers with these features once merged
(assuming the RFC passes)?


> >> I personally do not feel strongly about whether asymmetric types make
> it into the initial implementation. Larry does, however, and I think it i=
s
> not fair to exclude them without providing any concrete reasons not to.
> [snip]
> >
> > My concern is more about the external impact of what is effectively a
> change to the type system of the language: [snip] will tools like PhpStan
> and Psalm require complex changes to analyse code using such properties?
>
> In particular, this paragraph is referencing the ability to widen the
> accepted $value parameter type of the set hook, described at the
> bottom of https://wiki.php.net/rfc/property-hooks#set. I have talked
> to Ond=C5=99ej Mirtes, the maintainer of PHPStan, and he confirmed that
> this should not be complex to implement in PHPStan. In fact, PHPStan
> already offers the @property-read and @property-write class
> annotations, which can be used to describe "virtual" properties
> handled within __get/__set, already providing asymmetric types of
> sorts. Hence, this concern should be a non-issue.
>
> Thank you to everybody who has contributed to the discussion!
>
> Ilija
>


--=20
Matthew Weier O'Phinney
mweierophinney@gmail.com
https://mwop.net/
he/him

--000000000000e1c8240615c545f0
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">=
<div dir=3D"ltr" class=3D"gmail_attr">On Mon, Apr 8, 2024 at 4:41=E2=80=AFP=
M Ilija Tovilo &lt;<a href=3D"mailto:tovilo.ilija@gmail.com">tovilo.ilija@g=
mail.com</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D=
"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-le=
ft:1ex">Hi everyone<br>
<br>
Heads-up: Larry and I would like to start the vote of the property<br>
hooks RFC tomorrow:<br>
<a href=3D"https://wiki.php.net/rfc/property-hooks" rel=3D"noreferrer" targ=
et=3D"_blank">https://wiki.php.net/rfc/property-hooks</a><br>
<br>
We have worked long and hard on this RFC, and hope that we have found<br>
some middle-ground that works for the majority. One last concern we<br>
have not officially clarified on the list:<br>
<br>
<a href=3D"https://externals.io/message/122445#122667" rel=3D"noreferrer" t=
arget=3D"_blank">https://externals.io/message/122445#122667</a><br>
<br></blockquote><div><br></div><div>After reading JRF&#39;s reply, I found=
 myself nodding along. I feel like while a lot of the _details_ of my origi=
nal feedback were addressed, the _themes_ largely were not, and many overla=
p with what Juliette noted.</div><div><br></div><div>I&#39;ve taken some ti=
me again today to review the proposal, and I think I can boil my main conce=
rn down to _consistency_, and _ensuring reviewer comprehension_ when somebo=
dy is reviewing code that includes hooks. New features in PHP really should=
 veer away from allowing _implicit_ behavior, and should be internally cons=
istent with existing syntax (unless they are attempting to make an explicit=
 change to existing syntax).</div><div><br></div><div>On this latest review=
, I&#39;ve got fewer concerns, but I still have some.<br></div><div><br></d=
iv><div>1. I&#39;m not a fan of _implicit_. I strongly feel that there&#39;=
s no use case for `set` without an argument. Requiring the argument makes e=
xplicit that it is _receiving_ a value, and also makes it explicit _at the =
definition point_ what value _type_ is accepted, and what it is _named_. (I=
 realize I might be in a minority here, though, and the fact that I can req=
uire this as a CS rule (eventually) mitigates it to a degree. It feels like=
 an opportunity to reduce errors by requiring it, however.)<br><br>2. I&#39=
;m not a huge fan of the short syntax, but the improvements in the most rec=
ent draft are _mostly_ ones I can live with. The part that&#39;s still uncl=
ear is when and where hooks need a `;` termination, and _why_ the `;` is us=
ed, instead of `,`. When using `match()` expressions, you use `,` to separa=
te the expressions, but for some reason, the proposal uses `;` ... but only=
 when using short expressions. And if you have a full-form mixed with a sho=
rt form, the `;` is only needed for the short-form expression. This feels a=
rbitrary, and it will be easy to get it wrong for people comfortable with `=
match()` statements.<br><br>My gut take is that the syntax should use `,` t=
o separate hooks in ALL cases:<br><br>```<br>public string $content {<br>=
=C2=A0 =C2=A0 get {<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 $content =3D str_replace=
(&#39; =C2=A0&#39;, &#39; &#39;, $this-&gt;content);<br>=C2=A0 =C2=A0 =C2=
=A0 =C2=A0 return $this-&gt;convert($content);<br>=C2=A0 =C2=A0 },<br>=C2=
=A0 =C2=A0 set(string $value) =3D&gt; $value,<br>}<br><br>public string $su=
mmary {<br>=C2=A0 =C2=A0 get =3D&gt; $this-&gt;convert($this-&gt;summary),<=
br>=C2=A0 =C2=A0 set(string $value) {<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 $value=
 =3D str_replace((&#39; =C2=A0&#39;, &#39; &#39;, $value);<br>=C2=A0 =C2=A0=
 =C2=A0 =C2=A0 $this-&gt;summary =3D $value;<br>=C2=A0 =C2=A0 },<br>}<br>``=
`<br><br>This more closely matches `match()` expressions (pardon the pun), =
and provides a template for how we might allow blocks in `match()` expressi=
ons in the future.</div><div><br></div><div>(Larry tells me that it&#39;s `=
match()` being weird here, but considering that for many developers, their =
only point of reference for this sort of syntax IS `match()`, making it fee=
l like the language is ignoring its own syntax when creating new syntax.)<b=
r></div><div>=C2=A0 <br>3. While I&#39;d likely prefer Marco&#39;s approach=
 to references (just don&#39;t allow them), the fact that they mirror how `=
__get()` and `__set()` _currently_ work gives a migration path for users wh=
o are familiar with that paradigm&#39;s gotchas. In other words, it&#39;s c=
onsistent with the current language, and will make migrating from `__set/__=
get` to hooks easier. It&#39;s a lot of complexity, but the table you creat=
ed helps with that. That table MUST make it to the docs for the feature!<br=
><br>4. The interaction with serialization has a LOT of different cases, an=
d looks like a great place to observe foot guns in the future. I&#39;d like=
 to see a more consistent, predictable approach here, but knowing how many =
pitfalls there are in serialization normally, I&#39;m not expecting one; at=
 least it&#39;s being consistent with how each of the serialization methods=
 already work. What I DO expect is that you create a table detailing the be=
havior for the docs, similar to the one you did for references/backed value=
s/virtual.<br></div><div><br></div><div>While I definitely feel for QA/CS t=
ool makers (this is a HUGE chunk of new syntax, and non-trivial), I also do=
n&#39;t know how this could be done in a way that would be simpler. My only=
 suggestions would be to remove short-form syntax and require type/varname =
for `set` always, but even with those changes, I think similar levels of co=
mplexity will still exist to write parsers/formatters for these anyways.</d=
iv><div><br></div><div>I personally have wanted these features in the langu=
age for likely 15 years. Maybe the foundation might be able to sponsor some=
 developer time towards helping the QA/CS tool makers with these features o=
nce merged (assuming the RFC passes)?<br></div><div>=C2=A0</div><blockquote=
 class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px so=
lid rgb(204,204,204);padding-left:1ex">
&gt;&gt; I personally do not feel strongly about whether asymmetric types m=
ake it into the initial implementation. Larry does, however, and I think it=
 is not fair to exclude them without providing any concrete reasons not to.=
 [snip]<br>
&gt;<br>
&gt; My concern is more about the external impact of what is effectively a =
change to the type system of the language: [snip] will tools like PhpStan a=
nd Psalm require complex changes to analyse code using such properties?<br>
<br>
In particular, this paragraph is referencing the ability to widen the<br>
accepted $value parameter type of the set hook, described at the<br>
bottom of <a href=3D"https://wiki.php.net/rfc/property-hooks#set" rel=3D"no=
referrer" target=3D"_blank">https://wiki.php.net/rfc/property-hooks#set</a>=
. I have talked<br>
to Ond=C5=99ej Mirtes, the maintainer of PHPStan, and he confirmed that<br>
this should not be complex to implement in PHPStan. In fact, PHPStan<br>
already offers the @property-read and @property-write class<br>
annotations, which can be used to describe &quot;virtual&quot; properties<b=
r>
handled within __get/__set, already providing asymmetric types of<br>
sorts. Hence, this concern should be a non-issue.<br>
<br>
Thank you to everybody who has contributed to the discussion!<br>
<br>
Ilija<br>
</blockquote></div><br clear=3D"all"><br><span class=3D"gmail_signature_pre=
fix">-- </span><br><div dir=3D"ltr" class=3D"gmail_signature"><div dir=3D"l=
tr"><div>Matthew Weier O&#39;Phinney<br><a href=3D"mailto:mweierophinney@gm=
ail.com" target=3D"_blank">mweierophinney@gmail.com</a><br><a href=3D"https=
://mwop.net/" target=3D"_blank">https://mwop.net/</a></div><div>he/him<br><=
/div></div></div></div>

--000000000000e1c8240615c545f0--