Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120478
MIME-Version: 1.0
References: <CAB-BujaDnogpq7=BwwVbbT6XmBAAiDqQ3mDqgNccdzG1nW=WuQ@mail.gmail.com>
 <64EAB991-7082-47B8-B546-73CD08243C6E@koalephant.com> <CAB-BujYA+CG1GkS176eqsVkMrgQVqX=P8xNPz9zNgijE1uMC6w@mail.gmail.com>
 <23DF3A9A-9F11-4786-A011-41AE0FA9CFA9@koalephant.com> <CAB-Bujb9Hp+UoPy2Uxft8SuYaYo5fX1N_SD5vLeiAWmFdUkLSA@mail.gmail.com>
 <75a8db86-c91d-4f5e-b19e-e968926b4e8a@app.fastmail.com> <CAB-Bujbmi7ns1Dqx8FdRrihsFa+6qWL=a0rmbK_HAW925T0VyA@mail.gmail.com>
 <e103fefd-0ee8-419d-9031-9635eeac0f85@app.fastmail.com>
In-Reply-To: <e103fefd-0ee8-419d-9031-9635eeac0f85@app.fastmail.com>
Date: Tue, 30 May 2023 23:46:04 +0200
Message-ID: <CAB-BujbQV3f_154voO7t-tMXkgcoJZ8YkB5KK1o-Z=S6vwuHnQ@mail.gmail.com>
To: Larry Garfield <larry@garfieldtech.com>
Cc: php internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Declaration-aware attributes
From: andreas@dqxtech.net (Andreas Hennings)

On Tue, 30 May 2023 at 22:45, Larry Garfield <larry@garfieldtech.com> wrote=
:
>
> On Tue, May 30, 2023, at 7:34 PM, Andreas Hennings wrote:
> > On Tue, 30 May 2023 at 19:12, Larry Garfield <larry@garfieldtech.com> w=
rote:
> >>
> >> I've run into this issue in my attribute library as well (https://gith=
ub.com/Crell/AttributeUtils).  What I do there is allow attributes to opt-i=
n to a callback method via interface.  For example:
> >>
> >> #[\Attribute]
> >> class AttribWithName implements FromReflectionClass
> >> {
> >>     public readonly string $name;
> >>
> >>     public function __construct(?string $name =3D null)
> >>     {
> >>         if ($name) {
> >>             $this->name =3D $name;
> >>         }
> >>     }
> >>
> >>     public function fromReflection(\ReflectionClass $subject): void
> >>     {
> >>         $this->name ??=3D $subject->getShortName();
> >>     }
> >> }
> >
> > So it is technically from outside it is a setter, whereas from inside
> > it is not really.
> > Technically, this means you need a version of the attribute object
> > that can exist without the values from the reflector.
> > The constructor needs to initialize some "stub" values, until the
> > setter is called.
> > Every other method also needs to support the case when the setter has
> > not been called yet, or possibly throw an exception.
> > Also, your property type has to allow null, so `?string`, not `string`.
>
> Except the property type is not null above?  The argument is, but not the=
 property.  (I could use CPP here with a null value instead, if we had asym=
metric visibility.)

True!
My conception of readonly was distorted by the same static analysis
tools that you complained about earlier!
https://3v4l.org/CUgYv
I think a more accurate label would be "write once".

>
> Also, if the interface is called by the reflection system itself as part =
of getInstance() then yes, we can guarantee that it's been run, just like w=
e can guarantee the constructor has run.

The concern was that you can also construct the attribute object with
a regular `new AttribWithName()` expression, and then you can omit the
->fromReflection() call.
But we could say it is part of the contract that you have to call the
method before the object is fully operational.

>
> >> (Side note: This is why static analysis tools that forbid writing to r=
eadonly properties except from the constructor are wrong; it's also an exam=
ple of where asymmetric visibility would be superior to readonly.  But I di=
gress.)
> >
> > Technically there is no guarantee that the setters will be called
> > before any other method, and only once.
> > If these methods can write on readonly properties, then any method can.
> > Unless we somehow mark these methods as special.
>
> There's nothing special about readonly properties there.  An uninitialize=
d non-readonly property is no more or less susceptible to still being unini=
tialized when you need it.  Singling out readonly here is just dumb, and ex=
acerbates the problems of readonly.
>
> > On the other hand, a wither method with "clone with" should be allowed
> > to work on readonly properties.
> > You could rewrite your method like this, once we have clone with:
> > (or use the old-school syntax but without readonly)
> >
> >     public function withReflector(\ReflectionClass $subject): static
> >     {
> >         return ($this->name !=3D=3D NULL)
> >             ? $this
> >             : clone $this with (name: $subject->getShortName();
> >     }
> >
> > Then in the discovery code:
> >
> >     $attribute =3D $reflectionClass->getAttributes()[0]->newInstance();
> >     $attribute =3D $attribute->withReflector($reflectionClass);
>
> In that library I actually have several opt-in post-constructor setup rou=
tines.  The documentation covers them all.  Making them all withers would b=
e just needlessly slow.  Basically it's an example of a "mutable and then l=
ocked" object, which I emulate by virtue of only calling the setup methods =
from the library, and using readonly properties.
>
> >> That's why I think an opt-in interface is the way to go.  It also avoi=
ds any confusion around the constructor parameters, which is, based on this=
 thread, a confusing area. :-)
> >
> > What do you think about a placeholder syntax to avoid confusion with a
> > skipped parameter?
> > Like
> >
> > #[A('x', ?', 'y')]
>
> Oh god no.  For one, function calls are already complicated enough and th=
e next thing we should add there is some kind of partial application, which=
 already discussed using ?.  Second, that the attribute cares about what it=
 is attached to is not something the caller should care about.  If the user=
 of the attribute needs to specify "and put the reflection object here" in =
the attribute usage, we've failed.  I'd vote against any such proposal in a=
ny form, whatever the syntax.

I was actually thinking of the partial application with the `?` symbol.
The metaphor would be that we create a partial callable from the `new
A(.., ?)` expression, and then PHP calls that callable with the
reflector argument.
It would all just be a metaphor to justify the `?` placeholder,
because PHP would actually just skip the extra step and evaluate the
expression in one go.

But the only reason was to have something to avoid a skipped parameter.
We could also put other placeholders that feel more like expressions,
but I am not happy with that either.

In the end I could also live with a method call as you propose.

>
> >> My second preference would be the ReflectionAttribute::inProgress() ca=
ll in the constructor, or something like that.  I like that less because it=
's a stateful call, but it would also reduce the issue with readonly proper=
ties (as in the example above) by making both alternatives available in the=
 constructor, so maybe it's an acceptable tradeoff.
> >
> > I would like to avoid anything that is stateful or that leaves an
> > incomplete stub object.
> > (But I think I made this clear enough, so..)
>
> I don't think it's going to be possible to have both.  We either have a s=
tatic call of some kind (the inProgress() method above or similar) that onl=
y works sometimes (ie, while an attribute is being constructed), or we have=
 multiple setup methods (which the engine can ensure are called, but techni=
cally that does mean the object has a moment where it's potentially incompl=
ete).  I doubt we can have our cake and eat it too, here.
>
> Personally I think the "thou shalt not ever have an object that isn't per=
fectly setup" rule is over-blown.  It's a good guideline, but it has edge c=
ases where it just simply doesn't work.  This is one of them.
>
> --Larry Garfield
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>