Re: [RFC[ Property accessor hooks, take 2

On Mon, Mar 4, 2024 at 4:40 PM Stephen Reay <php-lists@koalephant.com> wrote: > > > > > On 28 Feb 2024, at 06:17, Larry Garfield <larry@garfieldtech.com> wrote: > > > > On Sun, Feb 25, 2024, at 10:16 PM, Rowan Tommins [IMSoP] wrote: > >> [Including my full previous reply, since the list and gmail currently > >> aren't being friends. Apologies that this leads to rather a lot of > >> reading in one go...] > > > > Eh, I'd prefer a few big emails that come in slowly to lots of little emails that > > come in fast. :-) > > > >>> On 21/02/2024 18:55, Larry Garfield wrote: > >>>> Hello again, fine Internalians. > >>>> > >>>> After much on-again/off-again work, Ilija and I are back with a more polished > >>>> property access hooks/interface properties RFC. > >>> > >>> > >>> Hello, and a huge thanks to both you and Ilija for the continued work > >>> on this. I'd really like to see this feature make it into PHP, and > >>> agree with a lot of the RFC. > >>> > >>> > >>> My main concern is the proliferation of things that look the same but > >>> act differently, and things that look different but act the same: > > > > *snip* > > > >>> - a and b are both what we might call "traditional" properties, and > >>> equivalent to each other; a uses legacy syntax which we haven't > >>> removed for some reason > > > > I don't know why we haven't removed var either. > > I can't recall the last time I saw it in real code. But that's out of scope here. > > > > *snip* > > > >> I think there's some really great functionality in the RFC, and would > >> love for it to succeed in some form, but I think it would benefit from > >> removing some of the "magic". > >> > >> > >> Regards, > >> > >> -- > >> Rowan Tommins > >> [IMSoP] > > > > > > I'm going to try and respond to a couple of different points together here, including > > from later in the thread, as it's just easier. > > > > == Re, design philosophy: > > > >> In C#, all "properties" are virtual - as soon as you have any > >> non-default "get", "set" or "init" definition, it's > >> up to you to declare > >> a separate "field" to store the value in. Swift's "computed > >> properties" > >> are similar: if you have a custom getter or setter, there is no backing > >> store; to add behaviour to a "stored property", you use the separate > >> "property observer" hooks. > >> > >> Kotlin's approach is philosophically the opposite: there are no fields, > >> only properties, but properties can access a hidden "backing field" via > >> the special keyword "field". Importantly, omitting the setter doesn't > >> make the property read-only, it implies set(value) { field = value } > > > > A little history here to help clarify how we ended up where we are: The original RFC as we > > designed it modeled very closely on Swift, with 4 hooks.. Using get/set at all would create a > > virtual property and you were on your own, while the beforeSet/afterSet hooks would not. We ran > > that design by some PHP Foundation sponsors a year ago (I don't actually know who, Roman did it > > for us), and the general feedback was "we like the idea, but woof this is complicated with all > > these hooks and having to make my own backing property for all these little things. Couldn't > > this be simplified?" We thought a bit more, and I off-handedly suggested to Ilija "I > > mean, would it be possible to just detect if a get/set hook is using a backing store and make it > > automatically? Then we could get rid of the before/after hooks." He gave it a quick try and > > found that was straightforward, so we pivoted to that simplified version. We then realized that we > > had... mostly just recreated Kotlin's design, so shrugged happily and went on with life. > > > > As noted in an earlier email, C#, Kotlin, and Swift all have different stances on the > > variable name for the incoming value. We originally modeled on Swift so had that model (optional > > newVal name), and also because we liked how compact it was. When we switched to the simplified, > > incidentally Kotlin-esque approach, we just kept the optional variable as it works. > > > > I think where that ended up is pretty nice, personally, even if it is not a direct map of > > any particular other language. > > > > == Re asymmetric typing: > > > > This is capability already present today if using a setter method. > > > > class Person { > > private $name; > > > > public function setName(UnicodeString|string $name) > > { > > $this->name = $value instanceof UnicodeString ? $value : new > > UnicodeString($value); > > } > > } > > > > And widening the parameter type in a child class is also entirely legal.. As the goal of > > the RFC is, essentially, "make most common getter/setter patterns easy to add to a property > > without making an API-breaking method, so people don't have to add redundant just-in-case > > getters and setters all the time," covering an easy-to-cover use case seems like a good thing > > to do. > > > > It also ties into the question of the explict/implicit name, for the reason you mentioned > > earlier (unspecified means mixed), not by intent. More on that in another section. > > > > == Re virtual properties: > > > > Ilija and I talked this through, and there's pros and cons to a > > virtual keyword. Ilija also suggested a Ô > > k°„ëèUÅaμqôbacked keyword, which forces a backed property to exist even if > > it's not used in the hook itself. > > > > * Adding virtual adds more work for the developer, but > > more clarity. It would also mean $this->$propName or $this->{__PROPERTY__} would work > > "as expected", since there's no auto-detection for virtual-ness. On the downside, if > > you have a could-be-virtual property but never actually use the backing value, you have an extra > > backing value hanging around in memory that is inaccessible normally, but will still show up in some > > serialization formats, which could be unexpected. If you omit one of the hooks and forget to mark > > it virtual, you'll still get the default of the other operation, which could be unexpected. > > (Mostly this would be for a virtual-get that accidentally has a default setter because you forgot to > > mark it virtual.) > > * Doing autodetection as now, but with an added "make a backing value anyway" > > flag would resolve the use case of "My set hook just calls a method, and that method sets the > > property, but since the hook doesn't mention the property it doesn't get created" > > problem. It would also allow for $this->$propName to work if a property is explicitly backed. > > On the flipside, it's one more thing to think about, and the above example it solves would be > > trivially solved by having the method just return the value to set and letting the set hook do the > > actual write, which is arguably better and more reliable code anyway. > > * The status quo (auto-detection based on the presence of $this->propName). This has > > the advantage it "just works" in the 95% case, without having to think about anything > > extra. The downside is it does have some odd edge cases, like needing $this->propName to be > > explicitly used. > > > > I don't think any is an obvious winner. My personal preference would be for status > > quo (auto-detect) or explicit-virtual always. I could probably live with either, though I think > > I'd personally favor status quo. Thoughts from others? > > > > I agree that a flag to make the field *virtual* (thus disabling the backing store) makes more > sense than a flag to make it backed; It's also easier to understand when comparing hooked > properties with regular properties (essentially, backed is the default, you have to opt-in to it > being virtual). I don't think the edge cases of "auto" make it worthwhile just to not > need "virtual". > > > == Re reference-get > > > > Allowing backed properties to have a reference return creates a situation where any writes > > would then bypass the set hook, and thus any validation implemented there. That is, it makes the > > validation unreliable. A major footgun. The question is, do we favor caveat-emptor flexibility or > > correct-by-construction safety? Personally I always lead toward the latter, though PHP in general > > is... schizophrenic about it, I'd say. :-) > > > > At this point, we'd much rather leave it blocked to avoid the issue; it's easier > > to enable that loophole in the future if we really want it than to get rid of it if it turns out to > > have been a bad idea. > > > > There is one edge case that *might* make sense: If there is no set hook defined, then > > there's no set hook to worry about bypassing. So it may be safe to allow &get on backed > > properties IFF there is no set hook. I worry that is "one more quirky edge case", though, > > so as above it may be better to skip for now as it's easier to add later than remove. But if > > the consensus is to do that, we're open to it. (Question for everyone.) > > > > I don't have strong feeling about this, but in general I usually tend to prefer options > that are consistent, and give power/options to the developer.. If references are opt-in anyway, I > see that as accepting the trade-offs. If a developer doesn't want to allow by-ref modifications > of the property, why would they make it referenceable in the first place? This sounds a bit like > disallowing regular public properties because they might be modified outside the class - that's > kind of the point, surely. > > > == Re > > > > == Re arrays > > > >> Regarding arrays, have you considered allowing array-index writes if > > an &get hook is defined? i.e. "$x->foo['bar'] = 42;" could be > > treated > > as semantically equivalent to "$_temp =& $x->foo; $_temp['bar'] = > > 42; > > unset($_temp);" > > > > That's already discussed in the RFC: > > > >> The simplest approach would be to copy the array, modify it accordingly, and pass it > >> to set hook. This would have a large and obscure performance penalty, and a set implementation would > >> have no way of knowing which values had changed, leading to, for instance, code needing to > >> revalidate all elements even if only one has changed. > > > > Unless we were OK with that bypassing the set hook entirely if defined, which, as noted > > above, means any safety guarantees provided by a set hook are bypassed, leading to untrustworthy > > code. > > > > == Re hook shorthands and return values > > > > Ilija and I have been discussing this for a bit, and we've both budged a little. :-) > > Here's our counter-proposal: > > > > - Drop the "top level" shorthand, for get-only hooks. > > - Keep the => shorthand for the get hook itself. > > - For a set hook, the {} form has no return value; set the value yourself however you > > want. > > - For a set hook, the => form implies a backed value and will set the property to > > whatever value that evaluates to. > > > > So these are equivalent: > > > > public $foo { set { $this->foo = $value; } } > > public $foo { set => $value; } > > > > These are equivalent: > > > > public string $foo { > > get { > > return strtoupper($this->foo); > > } > > } > > public string $foo { get => strtoupper($this->foo); } > > > > And this goes away: > > > > public string $foo => strtoupper($this->foo); > > > > That covers the common cases with an arrow-function-like syntax that behaves as you'd > > expect (it returns things), and allows a longer version with arbitrarily complex logic if desired. > > It also means that each syntax variant does mean something importantly different. > > > > Would that be an acceptable compromise? (Question for everyone.) > > > > I think the examples given are clear, and the lack of the top-level short closure-esque version > makes it more obvious. Forgive me, I must have missed some of the previous comments - is there a > reason the 'full' setter can't return a value, for the sake of consistency? I > understand that you don't want "return to set" to be the *only* option, for the sake > of e.g. change/audit logging type functionality (i.e. set and then some action to record that the > change was made), but it seems a little odd and inconsistent to me that the return value of a short > closure would be used when the return value of the long version isn't. This isn't really a > major issue, I'm just curious if there was some explanation about it? > > > == Re the $value variable in set > > > > Honestly, Rowan's earlier point here is the strongest argument in favor for me of the > > current RFC approach. Anywhere else in PHP, something that looks like a parameter and has no type, > > like ($param), means its type is > > mixed. It would be weird and confusing to be different here. > > That's above and beyond the issue of forcing people to retype something obvious every time. (I > > cite again, recent PHP's trend toward removing needless boilerplate, which is very good.) > > Requiring that the type be specified, for consistency, makes little sense if the type is not allowed > > to vary. You're just repeating a string from earlier on the same line, for no particular > > benefit.. > > > > I genuinely don't understand the pushback on $value. It's something you learn > > once and never have to think about again. It's consistent. > > > > Ilija jokingly suggested making it always $value, unconditionally, and allowing only the > > type to be specified if widening: > > > > public int $foo { set(int|float) => floor($value); } > > > > Though I suspect that won't go over well, either. :-) > > > > So what makes the most sense to me is to keep $value optional, but IF you specify an > > alternate name, you must also specify a type (which may be wider). So these are equivalent: > > > > public int $foo { set (int $value) => $value + 1 } > > public int $foo { set => $value + 1 } > > > > And only those forms are legal. But you could also do this, if the situation called for > > it: > > > > public int $foo { set(int|float $num) => floor($num) + 1; } > > > > This "all or nothing" approach seems like it strikes the best balance, gives the > > most flexibility where needed while still having the least redundancy when not needed, and when a > > name/type is provided, its behavior is the same as for a method being inherited. > > > > Does that sound acceptable? (Again, question for everyone.) > > > > My only question with this is the same as I had in an earlier reply (and I'm not sure it > was ever answered directly?), and you allude to this yourself: everywhere *else*, > ($var) means a parameter with type > mixed. Why is the type *required* here, when you've > specifically said you want to avoid boilerplate? If we're going to assume people can understand > that (implicit property-type $value) is implicit, surely we can also assume that > they will understand "specifying a parameter without a type" means the parameter has no > type (i.e. is mixed`). > > Again, for myself I'd be likely to type it (or regular parameters, properties, etc) as > mixed if that's what I want *anyway*, but the inconsistency > here seems odd, unless there's some until-now unknown drive to deprecate type-less > parameters/properties/etc. > > > > The alternative that gives the most future-flexibility is to do neither: The variable is > > called $value, period, you can't change it, and you can't change the type, either. There > > is no () after set, ever. Punt both of those to a later follow-up. I'd prefer to include both > > now, but including neither now is the next-safer option. > > > > > > ## Regarding $field > > > > Sigh, now y'all like it. :-P Most of the feedback on this has been negative, so > > I'm inclined to leave it out at this point, unless there's a major swing in feedback to > > bring it back. But the RFC seems more likely to pass without it than with right now. > > > > --Larry Garfield > > > > > Cheers > > > Stephen I would think that simply using return-to-set would be the simplest solution, if you need to run something after it's set, you can use the regular way of running code after a return: try { return $value + 100; } finally { // this runs after returning } Robert Landers Software Engineer Utrecht NL