Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118606
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-927-gf4c98c8499-fm-20220826.002-gf4c98c84
Mime-Version: 1.0
Message-ID: <e82e0d67-bd0e-4f8d-a572-ac99f9bbbe06@www.fastmail.com>
In-Reply-To: 
 <CAOWwgpkp0ndSgnS4aOiWqf+omcfT0_AiP0P35SC4hDZw0KDoHw@mail.gmail.com>
References: 
 <CAOWwgpk7uAkZD_CH5inbm8Ng8iVL5SZsz59Mpzgpc_iG42ei8Q@mail.gmail.com>
 <CAH5C8xU=eukkqn+PToG4-JT0vqjChhkY=kd_4t_ki9mXu1UMqQ@mail.gmail.com>
 <c20b9aae-fbbf-484e-b73b-b370e4783ada@www.fastmail.com>
 <CAOWwgpkp0ndSgnS4aOiWqf+omcfT0_AiP0P35SC4hDZw0KDoHw@mail.gmail.com>
Date: Sun, 11 Sep 2022 10:21:49 -0500
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] Re: Issues with readonly classes
From: larry@garfieldtech.com ("Larry Garfield")

On Sat, Sep 10, 2022, at 4:34 PM, Nicolas Grekas wrote:
> Hello Larry,
>
>
>> Regarding your main question: I understand your problem with readonly
>> > classes, and I'd be happy if we found a solution which fits your
>> use-cases
>> > and keeps consistency for the engine at the same time. To give you more
>> > context about the inheritance related restriction in the RFC: I went with
>> > forbidding the extension of readonly classes by non-readonly ones so that
>> > the invariants of the parent (no dynamic or mutable properties are
>> allowed)
>> > don't occasionally get violated by the child. However, I cannot think
>> about
>> > a proper scenario now where this would cause any problems... I'm
>> wondering
>> > if anyone could come up with one?
>>
>> I don't have a real-world example, but a general pattern.  The advantage
>> of a readonly object (either conventionally like PSR-7 or enforced by the
>> `readonly` keyword) is that you can store a reference to it without having
>> to worry about it changing out from under you.  That means you can code on
>> the assumption that any data derived from that object is also not subject
>> to change.
>>
>> If you accept a readonly object of type Foo, you would write code around
>> it on that assumption.
>>
>> If you're then passed an object of type Bar extends Foo, which has an
>> extra non-readonly property, that assumption would now be broken.  Whether
>> that counts as a Liskov violation is... a bit squishy, I think.
>>
>> Where that might be a problem is a case like this:
>>
>> readonly class Person {
>>   public function __construct(public string $first, public string $last) {}
>>
>>   public function fullName() {
>>     return "$this->first $this->last";
>>   }
>> }
>>
>> class FancyPerson extends Person {
>>   public string $middle = '';
>>
>>   public function setMiddle(string $mid) {
>>     $this->middle = $mid;
>>   }
>>
>>   public function fullName() {
>>     return "$this->first $this-middle $this->last";
>>   }
>> }
>>
>> class Display {
>>   public function __construct(protected Person $person) {}
>>
>>   public function hello() {
>>     return "Hello " . $this->person->fullName();
>>   }
>> }
>>
>> Now, Display assumes Person is readonly, and thus fullName() is
>> idempotent, and thus Display::hello() is idempotent.  But if you pass a
>> FancyPerson to Display, then fullName() is not safely idempotent (it may
>> change out from under you) and thus hello() is no longer idempotent.
>>
>> Whether or not that problem actually exists in practice is another
>> question.  And to be fair, the same risk exists for conventionally-readonly
>> classes today (PSR-7, PSR-13, etc.), unless they're made final.  Arguably
>> the safety signaling of a lexically readonly class is stronger than for a
>> conventionally readonly class, so one would expect that to not be broken.
>> But that's the case where a non-readonly child of a readonly parent can
>> cause problems.
>>
>
> Thanks for constructing this example, that's good food for thoughts.
> Unfortunately, The code following readonly child class shows that this
> safety you describe doesn't exist:
>
> readonly class FancyPerson extends Person {
>   private readonly stdClass $middle;
>
>   public function setMiddle(string $mid) {
>     $this->middle ??= new stdClass;
>     $this->middle->name = $mid;
>   }
>
>   public function fullName() {
>     return "$this->first $this-middle $this->last";
>   }
> }

Hm.  I seem to recall during the discussion of readonly classes someone saying that object properties of a readonly class had to also be readonly classes, which would render the above code a compile error.  However, I just checked and that is not in the RFC.  Was it removed?  Am I imagining things? Anyone else know what I'm talking about? :-)

--Larry Garfield