Re: [RFC][Vote announcement] Property hooks

On Mon, Apr 8, 2024 at 4:41 PM Ilija Tovilo <tovilo.ilija@gmail.com> wrote: > Hi everyone > > Heads-up: Larry and I would like to start the vote of the property > hooks RFC tomorrow: > https://wiki.php.net/rfc/property-hooks > > We have worked long and hard on this RFC, and hope that we have found > some middle-ground that works for the majority. One last concern we > have not officially clarified on the list: > > https://externals.io/message/122445#122667 > > After reading JRF's reply, I found myself nodding along. I feel like while a lot of the _details_ of my original feedback were addressed, the _themes_ largely were not, and many overlap with what Juliette noted. I've taken some time again today to review the proposal, and I think I can boil my main concern down to _consistency_, and _ensuring reviewer comprehension_ when somebody is reviewing code that includes hooks. New features in PHP really should veer away from allowing _implicit_ behavior, and should be internally consistent with existing syntax (unless they are attempting to make an explicit change to existing syntax). On this latest review, I've got fewer concerns, but I still have some. 1. I'm not a fan of _implicit_. I strongly feel that there's no use case for set without an argument. Requiring the argument makes explicit that it is _receiving_ a value, and also makes it explicit _at the definition point_ what value _type_ is accepted, and what it is _named_. (I realize I might be in a minority here, though, and the fact that I can require this as a CS rule (eventually) mitigates it to a degree. It feels like an opportunity to reduce errors by requiring it, however.) 2. I'm not a huge fan of the short syntax, but the improvements in the most recent draft are _mostly_ ones I can live with. The part that's still unclear is when and where hooks need a ; termination, and _why_ the ; is used, instead of ,. When using match() expressions, you use , to separate the expressions, but for some reason, the proposal uses ; ... but only when using short expressions. And if you have a full-form mixed with a short form, the ; is only needed for the short-form expression. This feels arbitrary, and it will be easy to get it wrong for people comfortable with match() statements. My gut take is that the syntax should use , to separate hooks in ALL cases: ``` public string $content { get { $content = str_replace(' ', ' ', $this->content); return $this->convert($content); }, set(string $value) => $value, } public string $summary { get => $this->convert($this->summary), set(string $value) { $value = str_replace((' ', ' ', $value); $this->summary = $value; }, } ``` This more closely matches match() expressions (pardon the pun), and provides a template for how we might allow blocks in match() expressions in the future. (Larry tells me that it's match() being weird here, but considering that for many developers, their only point of reference for this sort of syntax IS match(), making it feel like the language is ignoring its own syntax when creating new syntax.) 3. While I'd likely prefer Marco's approach to references (just don't allow them), the fact that they mirror how __get() and __set() _currently_ work gives a migration path for users who are familiar with that paradigm's gotchas. In other words, it's consistent with the current language, and will make migrating from __set/__get to hooks easier. It's a lot of complexity, but the table you created helps with that. That table MUST make it to the docs for the feature! 4. The interaction with serialization has a LOT of different cases, and looks like a great place to observe foot guns in the future. I'd like to see a more consistent, predictable approach here, but knowing how many pitfalls there are in serialization normally, I'm not expecting one; at least it's being consistent with how each of the serialization methods already work. What I DO expect is that you create a table detailing the behavior for the docs, similar to the one you did for references/backed values/virtual. While I definitely feel for QA/CS tool makers (this is a HUGE chunk of new syntax, and non-trivial), I also don't know how this could be done in a way that would be simpler. My only suggestions would be to remove short-form syntax and require type/varname for set always, but even with those changes, I think similar levels of complexity will still exist to write parsers/formatters for these anyways. I personally have wanted these features in the language for likely 15 years. Maybe the foundation might be able to sponsor some developer time towards helping the QA/CS tool makers with these features once merged (assuming the RFC passes)? > >> I personally do not feel strongly about whether asymmetric types make > it into the initial implementation. Larry does, however, and I think it is > not fair to exclude them without providing any concrete reasons not to. > [snip] > > > > My concern is more about the external impact of what is effectively a > change to the type system of the language: [snip] will tools like PhpStan > and Psalm require complex changes to analyse code using such properties? > > In particular, this paragraph is referencing the ability to widen the > accepted $value parameter type of the set hook, described at the > bottom of https://wiki.php.net/rfc/property-hooks#set. I have > talked > to Ondřej Mirtes, the maintainer of PHPStan, and he confirmed that > this should not be complex to implement in PHPStan. In fact, PHPStan > already offers the @property-read and @property-write class > annotations, which can be used to describe "virtual" properties > handled within __get/__set, already providing asymmetric types of > sorts. Hence, this concern should be a non-issue. > > Thank you to everybody who has contributed to the discussion! > > Ilija > -- Matthew Weier O'Phinney mweierophinney@gmail.com https://mwop.net/ he/him