Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120471
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-447-ge2460e13b3-fm-20230525.001-ge2460e13
Mime-Version: 1.0
Message-ID: <75a8db86-c91d-4f5e-b19e-e968926b4e8a@app.fastmail.com>
In-Reply-To: 
 <CAB-Bujb9Hp+UoPy2Uxft8SuYaYo5fX1N_SD5vLeiAWmFdUkLSA@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>
Date: Tue, 30 May 2023 17:11:54 +0000
To: "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: larry@garfieldtech.com ("Larry Garfield")

On Tue, May 30, 2023, at 2:49 PM, Andreas Hennings wrote:
> On Tue, 30 May 2023 at 15:14, Stephen Reay <php-lists@koalephant.com> =
wrote:
>>
>> (Resending to the list without all the history because qmail complain=
ed about message size)
>>
>>
>> >>
>> >> Hi Andreas,
>> >>
>> >> I too have wondered (and I think asked in room11?) about such a co=
ncept. >From memory the general response was =E2=80=9Cjust do it in user=
land with a wrapper=E2=80=9D so its good to see someone else is interest=
ed in this being part of the language.
>> >>
>> >> While I agree that it=E2=80=99s most useful if the `Reflector` ins=
tance is available in the constructor, I=E2=80=99m not keen on the propo=
sed magic =E2=80=9Cskipping=E2=80=9D of arguments as you suggest. It see=
ms way too easy to confuse someone (particularly if the attribute class =
itself has reason to be instantiated directly in code)
>> >
>> > Good point! Almost made me change my mind completely. But I already
>> > changed it back :)
>> >
>> > When instantiating in code, the "real" signature would have to be
>> > used, and the reflector argument passed explicitly.
>>
>>
>> That=E2=80=99s kind of my point: it=E2=80=99s not super intuitive why=
 (or the specifics of how) it=E2=80=99s being skipped when it=E2=80=99s =
an attribute, vs when it=E2=80=99s instantiated from code. What if someo=
ne specifies an argument with the same name? If they specify args withou=
t names, can they just use null for that? Etc.
>
> I agree it could be confusing.
> But for the named args, I think it is quite obvious:
>
> #[Attribute]
> class A {
>   public readonly array $moreArgs;
>   public function __construct(
>     public readonly string $x,
>     // Reflector parameter can be last required parameter, BUT
>     #[AttributeDeclaration] public readonly \ReflectionClass $class,
>     // Optional parameters have to be after the required reflector par=
ameter.
>     public readonly ?string $y =3D NULL,
>     // Variadic parameter must be last.
>     string ...$moreArgs,
>   ) {
>     $this->moreArgs =3D $moreArgs;
>   }
> }
>
> #[A('x', 'y', 'z')]  // -> new A('x', $reflector, 'y', 'z')
> #[A(x: 'x', y: 'y')]  // -> new A('x', $reflector, 'y')
> #[A(x: 'x', class: new ReflectionClass('C'))]  // -> new A('x', new
> ReflectionClass('C'))
>
> We _could_ say that explicitly passing a value for the reflector in an
> attribute declaration is forbidden.
> Or we allow it, then an explicit value would simply overwrite the
> implicit value.
>
> If we use placeholder syntax, the above examples would look like this:
>
> #[A('x', ?, 'y', 'z')]  // -> new A('x', $reflector, 'y', 'z')
> #[A(x: 'x', class: ?, y: 'y')]  // -> new A('x', $reflector, 'y')
> #[A(x: 'x', class: new ReflectionClass('C'))]  // -> new A('x', new
> ReflectionClass('C'))
>
>
>>
>> > This would be
>> > useful for unit tests that want to replicate the realistic behavior.
>> > Also it guarantees that the code of the attribute class can really
>> > count on this value to not be null, no matter how the class is
>> > instantiated.
>> >
>> >
>>
>> I would expect that whether the Reflector object is required is simpl=
y a matter of whether or not the parameter is nullable.
>> If it=E2=80=99s not nullable, then yes, the explicit instantiation ca=
ll will need to supply it at the correct location. If it=E2=80=99s only =
required when created from attribute usage, then it would accept null, a=
nd the constructor would have appropriate logic to handle that.
>>
>
> Yes.
> But I would expect the common practice to be to make it required,
> because then the constructor code will be simpler.
>
>>
>>
>> >>
>> >> I think a better approach would be to suggest authors put the para=
meter at the *end* of the parameter list, so that no =E2=80=98skipping' =
is required when passing arguments without names (or put it where you li=
ke if you=E2=80=99re always using named arguments)
>> >
>> > If I understand correctly, the proposal would technically not chang=
e,
>> > we just add a recommendation.
>>
>> Technically, yes =E2=80=9Cmy way=E2=80=9D would work fine with the pr=
oposal you=E2=80=99ve suggested, if I choose to always put the parameter=
 marked by #[ReflectionContext] last.
>
> As above, the problem with this would be optional and variadic
> parameters, which have to come after a required reflector parameter.
>
>>
>> I=E2=80=99m just concerned about confusing usage if =E2=80=9Cinsert t=
his parameter anywhere=E2=80=9D is the =E2=80=98recommended=E2=80=99 (i.=
e. documented example) way to use this feature.
>>
>> Even with that concern, I still prefer this to most other solutions m=
entioned so far, for the same reasons: they=E2=80=99re all some degree o=
f magic.
>>
>> The only other solution I can think of that=E2=80=99s less =E2=80=9Cm=
agic=E2=80=9D and more explicit, is (and I have no idea if this is even =
feasible technically) to introduce a builtin trait for attribute classes=
 to use, providing a protected method or property that gives access to t=
he Reflector (how the trait has access is not really important, I assume=
 it can be assigned to the object somehow before the constructor is call=
ed). I guess this could also be an abstract class, but a trait makes it =
much easier to adopt so that would be my preferred approach.
>>
>> So something like
>>
>> trait AttributeReflector {
>>     protected function getReflector(): \Reflector {
>>         // do internal stuff
>>     }
>> }
>>
>> #[Attribute]
>> class Foo {
>>     Use \AttributeReflector;
>>
>>     public readonly string $name;
>>
>>     function __construct(?string $name =3D null) {
>>         $this->name =3D $name ?? $this->getReflector()->name;
>>     }
>> }
>
> I was also considering this, but there is a problem.
> When you instantiate the attribute class outside an attribute
> declaration, how do you tell it about a required reflector?
> This would be relevant e.g. during unit tests.
>
> The constructor parameter provides a clean way to pass a custom reflec=
tor.
> But with the ->getReflector(), the reflector would already be
> magically added to the object before the constructor is executed. This
> would be impossible to replicate in custom code outside an attribute
> declaration.

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=20
{
    public readonly string $name;
   =20
    public function __construct(?string $name =3D null)=20
    {
        if ($name) {
            $this->name =3D $name;
        }
    }
   =20
    public function fromReflection(\ReflectionClass $subject): void
    {
        $this->name ??=3D $subject->getShortName();
    }
}

(Side note: This is why static analysis tools that forbid writing to rea=
donly properties except from the constructor are wrong; it's also an exa=
mple of where asymmetric visibility would be superior to readonly.  But =
I digress.)

My preference would be for something along those lines to be implemented=
 in core.

Importantly, we *MUST NOT* design it such that the reflection object get=
s assigned to a property of the attribute.  Reflection objects are not s=
erializable.  Attributes will frequently be cached.  That means it force=
s the attribute author to make the property nullable AND then unset it s=
ometime before the attribute gets serialized, or it will break.  That's =
a no-go.

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 thi=
s thread, a confusing area. :-)

My second preference would be the ReflectionAttribute::inProgress() call=
 in the constructor, or something like that.  I like that less because i=
t's a stateful call, but it would also reduce the issue with readonly pr=
operties (as in the example above) by making both alternatives available=
 in the constructor, so maybe it's an acceptable tradeoff.

I can see an argument either direction here.

--Larry Garfield