Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120477
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-447-ge2460e13b3-fm-20230525.001-ge2460e13
Mime-Version: 1.0
Message-ID: <e103fefd-0ee8-419d-9031-9635eeac0f85@app.fastmail.com>
In-Reply-To: 
 <CAB-Bujbmi7ns1Dqx8FdRrihsFa+6qWL=a0rmbK_HAW925T0VyA@mail.gmail.com>
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>
Date: Tue, 30 May 2023 20:45:01 +0000
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] Declaration-aware attributes
From: larry@garfieldtech.com ("Larry Garfield")

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> wrote:
>>
>> I've run into this issue in my attribute library as well (https://github.com/Crell/AttributeUtils).  What I do there is allow attributes to opt-in to a callback method via interface.  For example:
>>
>> #[\Attribute]
>> class AttribWithName implements FromReflectionClass
>> {
>>     public readonly string $name;
>>
>>     public function __construct(?string $name = null)
>>     {
>>         if ($name) {
>>             $this->name = $name;
>>         }
>>     }
>>
>>     public function fromReflection(\ReflectionClass $subject): void
>>     {
>>         $this->name ??= $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 asymmetric visibility.)

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 we can guarantee the constructor has run.  

>> (Side note: This is why static analysis tools that forbid writing to readonly properties except from the constructor are wrong; it's also an example of where asymmetric visibility would be superior to readonly.  But I digress.)
>
> 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 uninitialized non-readonly property is no more or less susceptible to still being uninitialized when you need it.  Singling out readonly here is just dumb, and exacerbates 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 !== NULL)
>             ? $this
>             : clone $this with (name: $subject->getShortName();
>     }
>
> Then in the discovery code:
>
>     $attribute = $reflectionClass->getAttributes()[0]->newInstance();
>     $attribute = $attribute->withReflector($reflectionClass);

In that library I actually have several opt-in post-constructor setup routines.  The documentation covers them all.  Making them all withers would be just needlessly slow.  Basically it's an example of a "mutable and then locked" 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 avoids 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 the 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 any form, whatever the syntax.

>> My second preference would be the ReflectionAttribute::inProgress() call 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 properties (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 static call of some kind (the inProgress() method above or similar) that only works sometimes (ie, while an attribute is being constructed), or we have multiple setup methods (which the engine can ensure are called, but technically that does mean the object has a moment where it's potentially incomplete).  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 perfectly setup" rule is over-blown.  It's a good guideline, but it has edge cases where it just simply doesn't work.  This is one of them.

--Larry Garfield