Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119062
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-1115-g8b801eadce-fm-20221102.001-g8b801ead
Mime-Version: 1.0
Message-ID: <44a8c249-b051-492b-9bca-b8f7baf5667a@app.fastmail.com>
In-Reply-To: <831b9906-dc0c-420c-b22f-8a0cc8a1ad64@app.fastmail.com>
References: <0854b030-c51c-4c1b-a7dd-22835a1e5da9@app.fastmail.com>
 <831b9906-dc0c-420c-b22f-8a0cc8a1ad64@app.fastmail.com>
Date: Wed, 30 Nov 2022 09:32:31 -0600
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] [RFC] Asymmetric Visibility, with readonly
From: larry@garfieldtech.com ("Larry Garfield")

On Tue, Nov 29, 2022, at 2:29 PM, Larry Garfield wrote:
> On Sun, Nov 13, 2022, at 2:08 PM, Larry Garfield wrote:
>> Hi folks.  Ilija is nearly done with the implementation for asymmetric 
>> visibility and flushing out edge cases, but we've run into one design 
>> question we'd like feedback on.
>>
>> There's two design decisions we've made at this point, both of which we 
>> think are logical and reasonable:
>>
>> 1. If specified, the set visibility must be tighter than the get 
>> visibility.  So `protected protected(set)` and `protected public(set)` 
>> are not permitted, for instance.
>>
>> 2. `readonly` is a "write once" flag that may be combined with 
>> asymmetric visibility.  If no set visibility is specified, `readoly` 
>> implies `private(set)`, but a different set visibility may also be 
>> provided.
>>
>> These are both reasonable rules.  However, it creates a conflict.  
>> Specifically, in the following cases:
>>
>> public public(set) readonly string $foo
>>
>> protected protected(set) readonly string $foo
>>
>> These would be the only way to have a non-private-set readonly 
>> property.  While the first is in practice quite unlikely, the second 
>> has valid use cases.  (In particular, a base class that provides 
>> properties expected to be set by a child constructor, and then used by 
>> a method in the parent class.)  However, it would not be allowed under 
>> the rules above.  Working around it would require specifying `public 
>> protected(set) readonly...`, which means exposing a property that 
>> likely should not be exposed.
>>
>> That creates an odd situation where readonly and asymmetric visibility 
>> may only be combined "sometimes."  That is not deesireable.  The only 
>> way to combine them in their current form is to allow `protected 
>> protected(set)` only if readonly is in use, which is excessively 
>> complicated both to implement and to explain/document/use.
>>
>> We see two possible ways to resolve this conflict:
>>
>> 1. Relax the set-is-tighter restriction.  That would allow `protected 
>> protected(set)` etc. on any property.  It wouldn't be particularly 
>> useful unless readonly is being used, but it would be syntactically 
>> legal and behave as you'd expect.  We could still disallow "set is more 
>> permissive" combinations (eg, `private public(set)`), as those have no 
>> apparent use case.
>>
>> 2. Disallow readonly and asymmetric visibility being combined, because 
>> readonly already has a hard-coded implied asymmetric visibility.  This 
>> option removes some potential use cases (they would most likely drop 
>> the readonly), but has the upside that it's easier to re-allow at some 
>> point in the future.
>>
>> 3. Some other brilliant idea we've not thought of.
>>
>>
>> Both are viable approaches with pros and cons.  We're split on which 
>> way to go with this, so we throw it out to the group for feedback.  
>> Which approach would you favor, or do you have some other brilliant 
>> idea to square this circle?
>
>
> Thank you everyone for the feedback.  Based on this thread, we've made 
> two changes to the RFC:
>
> 1. We've moved readonly back to forbidden with a-viz for now.  I've 
> added a section to Future Scope where we really should sort this out in 
> the future, but we'll do that in the future when we can all focus on 
> the various nuances of just that piece.
>
> 2. I rewrote the section on __set to make it clearer.  That also 
> included Ilija and I digging into all the nuances that are already 
> present.  The text may still look a bit complex, but that's because the 
> existing logic is already complex with readonly.  Long story short, the 
> a-viz RFC does not change anything in the way __set works vis a vis 
> asymmetric visibility; it just inherits and continues what readonly 
> already started, so it's consistent.
>
> The PR should be updated in the next week or two with the latest 
> changes.  Baring any major need for change, we expect to call a vote 
> for it shortly after New Years.
>
> Thanks all.
>
> --Larry Garfield


One other update: Nicolas poked me off list to remind me that the RFC didn't mention inheritance at all.  Oops. :-)  The patch already has inheritance well handled and tested, it just wasn't described in the RFC.  I have added a section that documents the behavior.  Short version: You can continue to expand, but not contract, the visibility in child classes, but you can do it for get and set separately.  It's pretty much exactly what you'd expect it to do.

--Larry Garfield