Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123086
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: Wed, 10 Apr 2024 12:03:58 +0200
Message-ID: <CAPzBOBPEwbsphUoeU=2489JtezJT-tnVxkVPUDc7eGA9TtqOKg@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC][Vote announcement] Property hooks
To: Juliette Reinders Folmer <php-internals_nospam@adviesenzo.nl>
Cc: internals@lists.php.net
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: landers.robert@gmail.com (Robert Landers)

On Wed, Apr 10, 2024 at 5:49=E2=80=AFAM Juliette Reinders Folmer
<php-internals_nospam@adviesenzo.nl> wrote:
>
> On 9-4-2024 16:03, Juliette Reinders Folmer wrote:
>
> On 8-4-2024 23:39, Ilija Tovilo 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
>
> I personally do not feel strongly about whether asymmetric types make 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]
>
> My concern is more about the external impact of what is effectively a cha=
nge 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
>
>
> Ilija,
>
> Heads-up: I'm still writing up an opinion and intend to send it to the li=
st before end of day (CET). I know I'm late to the party, but I've been hav=
ing trouble finding the words to express myself properly regarding this RFC=
 (and have been struggling to find the right words for months).
>
> Smile,
> Juliette
>
>
> Later than intended, but here goes....
>
> If there is one RFC which has been giving me nightmares since I first hea=
rd of it, it's this one.
>
> 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.
>
> I am going to try now anyway (before it is too late), so please bear with=
 me. Also, as I'm not a C-developer, please forgive me if I get the interna=
ls wrong. I'm writing this from a PHP-dev/user perspective, with my perspec=
tive being heavily influenced by my role as maintainer of PHP_CodeSniffer.
>
> ---
> 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.
> ---
>
> I can definitely see the use case and desirability of the property hooks =
functionality proposed in the RFC and compared to the initial RFC I read la=
st year, the current RFC is, IMO, much improved.
> Huge kudos to Ilija and Larry for all the work they have put in to this!
>
> I applaud the intention of this RFC to make it easier to avoid the magic =
__get()/__set() et al methods. What I have a problem with is the implementa=
tion details.
>
> Over the last few years, we've seen a movement to get rid of more and mor=
e of the _surprising_ behaviour of PHP, with subtle exceptions being deprec=
ated and slated for removal and (most) new syntaxes trying to use the princ=
iple of least surprise by design.
>
> This RFC, in my view, is in stark contrast to this as it introduces a ple=
thora of exceptions and subtle different behaviour in a way that will catch=
 developers out for years to come.
>
> At this moment (excluding property hooks), from a user perspective, there=
 are three different function syntaxes in PHP: named functions (and methods=
), anonymous functions and arrow functions.
>
> The semantics of these are very similar with subtle differences:
> * Can be static or non-static.
> * Take parameters, which can be typed, references, variadic, optional etc=
.
> * Can have a return type.
> * Can return by reference.
> * Have a function "body".
>
> The differences between the current syntaxes - from a user perspective - =
are as follows:
> =3D Named functions:
> * When declared in a class, can have visibility attached, can be abstract=
, can be final.
> * When declared in an interface or declared as abstract, will not have a =
function "body".
>
> =3D Anonymous functions:
> * Can import plain variables from outside its scope with a `use()` clause=
.
> * Are declared as an expression (can be assigned to a variable etc).
>
> =3D Arrow functions:
> * Have access to variables in the same scope.
> * Are declared as an expression.
> * Body of the function starts with a =3D> instead of being enclosed in cu=
rlies and can end on a range of characters.
> * Can only take one statement in the body.
> * Automagically returns.
>
> The property hooks RFC introduces a fourth flavour of function syntax. An=
d not just one syntax, but five and the differences in the semantics of the=
 function syntaxes are significant.
>
> The differences in semantics I see for "full" property hook functions com=
pared to other function syntaxes are as follows:
> * `get` cannot take parameters (and doesn't have the parentheses typicall=
y expected for a function declaration).
> * `get` cannot take return type (inherits this from the property, which i=
s logically sound, but does create a syntactic difference).
> * `set` can take one (typed or untyped) parameter.
> * `set` cannot take return type (silently set to void, which is logically=
 sound, but does create a syntactic difference).
> * `set` magically creates a $value variable for the "new" value, though t=
hat variable _may_ have an arbitrary different name depending on whether or=
 not the parameter was explicitly specified.
> * Has a function body.
>
> Then there are multiple short hand syntaxes, which each have yet more syn=
tactic differences:
> * The arrow function variant for `get` with all the above differences + t=
he differences inherent to arrow functions, combined, with the exception of=
 the access to variables in the same scope and a more clearly defined end o=
f the expression.
> * The implicit "set" parameter, which does not have a explicit parameter,=
 does not have parentheses and has the magically created $value variable.
> * 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).
> * The abstract/interface variants, which don't have a function body.
>
> Next there are the differences in semantics which are now being introduce=
d for properties:
> * Aside from the hooks themselves....
> * 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.
>
> And then there are the new features for properties (not just hooked ones)=
:
> * Properties can now be declared as final.
> * 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.
> * Abstract/Interface properties don't comply with the same covariance/con=
travariance rules as other properties
>
> 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.
>
> Additionally, when reading the RFC (in its current state), I see the foll=
owing exceptions being put in place, which impact the semantics for propert=
ies:
> * Only available for object properties, static properties are excluded.
> * `var` for property declarations is not supported according to the RFC. =
While I 100% agree using the var keyword is old-school, using `var` effecti=
vely makes a property a public property (which would satisfy the visibility=
 requirement for an interface/abstract class) and the `var` keyword is *not=
* deprecated, even though the RFC states it is: https://3v4l.org/3o50a
>    I'd fully support formally deprecating the `var` keyword for propertie=
s and eventually removing it, but not supporting it for this RFC, even thou=
gh it is still a supported language feature seems opinionated and arbitrary=
.
>    Note: when testing the RFC code, declaring a property using the var ke=
yword on an interface does not currently throw an error (while if it is not=
 supported, I would expect one): https://3v4l.org/KaLqe/rfc#vrfc.property-h=
ooks , same goes for using the var keyword with property hooks in a class: =
https://3v4l.org/mQ6pG/rfc#vrfc.property-hooks
> * 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.
> * 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.
>    Oh, and hang on, they *can* be read from a method in the same class as=
 long as that method is called from within the set hook, so now the method =
will need to either do a backtrace or use Reflection to figure out whether =
it has access to the property value (now why does that remind me of the mag=
ic __...() methods ?).
> * 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.
> * 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 ?)
> * Changes the meaning of `$this->property` read/write access for all meth=
ods called from within a hook while executing the hook.
> * 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.
> * The range of different behaviours for various forms of serialization.
>
> Furthermore:
>
> [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
>
> 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
>
> [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;
> }
>
> [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.
>
> [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.
>
>
> All in all, I really do see the use case for this feature and I most defi=
nitely appreciate all the work Ilija and Larry have put into the RFC and th=
e implementation.
>
> However, in its current state, I'm concerned that the feature introduces =
so many exceptional situations and WTF moments that it is a worse solution =
than the magic methods which are already available. (and no, I'm definitely=
 not a fan of those magic methods either).
>
> I think property hooks as currently proposed with all its catches will be=
 hard to teach, will raise the barrier of entry to the language, can catch =
people out in dozens of different ways and introduces way too many peculiar=
ities, while the tendency of the language has been to try to eliminate thos=
e kind of surprising behaviours.
>
> Irrelevant side-note: I kind of feel like I could create a completely new=
 "Why Equal doesn't Equal" Quiz just and only based on property hooks and p=
eople will still not get it afterwards.
>
> ---
>
> As for the impact on static analysis tools....
>
> I can't speak for other tools, but I can tell you that for PHP_CodeSniffe=
r the impact of this RFC will be horrendous. Aside from the impact on the s=
yntax recognition layer (Tokenizer), my rough estimate is that about 70% of=
 all sniffs ever written will need to be adjusted for these five syntaxes a=
nd all the peculiarities around them - either to ignore hooks, work around =
hooks, skip over hooks, or to take hooks into account and examine them in a=
ll their variations.
> I estimate it will take a good 5-6 years before all actively used sniffs =
will have been adjusted and that in the mean time, maintainers of both PHPC=
S itself as well as the popular external standards will have to deal with a=
 significant number of extra support requests/bug reports related to this.
>
> The sheer amount of new syntaxes which have been introduced starting with=
 PHP 7.4, has already brought innovation in a large part of the PHP_CodeSni=
ffer field to a near complete halt for the past four years as I'm continuou=
sly trying to play catch-up. This RFC will mean that innovation, like new P=
SR-5/19 Docs standards, QA sniffs for tests, significantly improving the Se=
curity standard etc, will all be put on hold for another few years while I =
have to deal with this change.
>
> While I fully recognize that the impact on userland tooling is not a reas=
on to reject a new PHP feature, nor should it be, I do believe that the imp=
act of this RFC could be far less and easier to mitigate if different desig=
n choices were made or if the new syntaxes were introduced in a far slower,=
 more staged approach across multiple PHP versions.
>
> Smile,
> Juliette
>
> P.S.: nitpick: in the first two code examples under "Detailed Implementat=
ion" - "Set" the `$u` variable is missing the dollar sign for the `new User=
` line.


As someone who has spent a number of years writing C#, the RFC looks
quite sane. I do agree that it does quite a lot in one go though. If
you look at how fields were introduced into C#, the "short syntax"
version didn't come along until years after the long version and
people had a good grasp of what they wanted: a shorter way to write
fields because writing it out all the time was annoying af. Getting it
from the start will be really nice!

Personally, I'm not a big fan of calling arbitrary methods from inside
setters/getters. I'm pretty sure this is allowed in C#, however, it is
an anti-pattern and avoided except for very narrow edge cases. I'm
sure we'll see some of that here, where people will get bitten, and a
consensus of anti-patterns will emerge. However, having that power
available is immensely useful /when you need it/! Removing it because
"it complicates things" isn't really a good reason, IMHO.

To me, as a non-voting outsider, it smells like the RFC process breaks
down past a certain point. I've watched operator overloading fail ...
and I am implementing an RFC that could have been done 1000% in
user-space if operator overloading had passed. But it didn't, so now
we need to literally change the language to support it. Then watching
the feedback here, at the 11th hour, is super saddening. Will we never
be able to make great changes to the language due to a tragedy of the
commons?

Robert Landers
Software Engineer
Utrecht NL