On Tue, 30 May 2023 at 22:45, Larry Garfield <la...@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 <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.)
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 > -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
