Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118604
MIME-Version: 1.0
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>
In-Reply-To: <c20b9aae-fbbf-484e-b73b-b370e4783ada@www.fastmail.com>
Date: Sat, 10 Sep 2022 23:34:55 +0200
Message-ID: <CAOWwgpkp0ndSgnS4aOiWqf+omcfT0_AiP0P35SC4hDZw0KDoHw@mail.gmail.com>
To: Larry Garfield <larry@garfieldtech.com>
Cc: php internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000b2556d05e8596fa8"
Subject: Re: [PHP-DEV] Re: Issues with readonly classes
From: nicolas.grekas+php@gmail.com (Nicolas Grekas)

--000000000000b2556d05e8596fa8
Content-Type: text/plain; charset="UTF-8"

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";
  }
}


> Personally I'm undecided at the moment whether or not it should be
> allowed.  I'm sympathetic to the "it's easier to disallow and allow later
> than vice versa" argument, but still not sure where I stand.  The above at
> least gives a concrete example of where the problem would be.
>

If we want the safety you describe, we might want a stronger version of it.
Let's name it immutable. An immutable property/class would be like a
readonly property with the additional restriction that only an immutable
value could be assigned to it (scalars + array + immutable classes.) But
that's another topic.

Your example made me doubt for a moment, but without any convincing
purpose, this should help decide that child classes shouldn't have to be
readonly.

Nicolas

--000000000000b2556d05e8596fa8--