Re: Declaration-aware attributes

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> 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.) 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 we 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 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. 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() 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 >