On Tue, May 30, 2023, at 7:34 PM, Andreas Hennings wrote:
> On Tue, 30 May 2023 at 19:12, Larry Garfield <la...@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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
