Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119043
MIME-Version: 1.0
References: <CAH5C8xUd4ueJ62pR6vT2haz9rFgrgspJQui1rPCqTneupD+tsQ@mail.gmail.com>
 <CADyq6sLFZpM1A+V-aa3dWjY1mL=65e-kxeiyBHABNPZR2GfpKQ@mail.gmail.com>
 <CADK1yX+9c3bkD8S1d-Ei3fRmWknQCHF2EXW9y96tFQkwUnse3Q@mail.gmail.com>
 <CAMrTa2GefQAMOe-9wcNrpx_UMSJebHPN9XeNNewQ-JvJX-czLw@mail.gmail.com> <38e576a2-a293-41b7-990f-9d4ecfad3b70@app.fastmail.com>
In-Reply-To: <38e576a2-a293-41b7-990f-9d4ecfad3b70@app.fastmail.com>
Date: Sun, 27 Nov 2022 18:55:15 +0100
Message-ID: <CAOWwgpnH+d_bk5uPijzfeaLEtxVjGOuUdDiuH9axQsAEmTtRLg@mail.gmail.com>
To: php internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000be6cd705ee77757e"
Subject: Re: [PHP-DEV] [RFC] [Discussion] Readonly class amendments
From: nicolas.grekas+php@gmail.com (Nicolas Grekas)

--000000000000be6cd705ee77757e
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

The code that Marco (Pivetta) shared was supposed to illustrate how
readonly classes can be useful to enforce some invariants on child classes.
Yet, here is another implementation that does use a readonly child class,
but still provides the behavior that was supposedly prevented by the
keyword:

readonly class MutableCounter extends ImmutableCounter {
    private stdClass $count;
    public function __construct(int $count) {
        $this->count =3D (object) ['count' =3D> $count];
    }
    public function add1(): self { return new self(++$this->count->count); =
}
    public function value(): int { return $this->count->count; }
}


This counterexample shows that readonly classes do not provide any extra
safeguards.

As it stands, the readonly keyword on classes has two consequences:
1. it forces writing dummy boilerplate to work around the current
limitation while failing to provide any real guarantees
2. it gives false expectations - the exact one that we're discussions about
here (no, readonly classes don't help enforce any aspects of LSP)

1. is why I call the current restriction arbitrary, and 2. might be
dangerous since it would make people build on non-existing grounds.

If we stay with the current way, we'll just have another WTF in the
language IMHO. One that will make it harder to master PHP.


Le dim. 27 nov. 2022 =C3=A0 17:51, Larry Garfield <larry@garfieldtech.com> =
a
=C3=A9crit :

> On Sat, Nov 26, 2022, at 6:35 PM, Jordan LeDoux wrote:
> > On Sat, Nov 26, 2022 at 3:40 PM Deleu <deleugyn@gmail.com> wrote:
> >
> >>
> >> As I think more about this, there's nothing about the current RFC in
> this
> >> code sample. What's breaking LSP here is the child class doing state
> >> modification, not PHP.  To further expand that rationale, PHP allows u=
s
> to
> >> create child classes. Whether that class will be LSP-safe or not is up
> to
> >> us, not up to PHP.
> >>
> >> However, the point still stands. Allowing child classes to break
> readonly
> >> will make it easier to build code that breaks LSP. The question then
> >> becomes: why is this being proposed and is it worth it?
> >>
> >
> > I cannot help but feel that the way `readonly` is being treated is goin=
g
> to
> > end up one of those things that is regretted. "Readonly does not imply
> > immutability". The fact that very nearly *every* single person who has
> not
> > worked on the RFCs has at some point been confused by this however shou=
ld
> > be very telling.
> >
> > This comes from two *different* avenues that compound with each other t=
o
> > *both* make this design head-scratching to me.
> >
> > First, in virtually all other technical contexts where the term
> "readonly"
> > is used, it means that the information/data cannot be altered. That is
> not
> > the case with readonly. In PHP, in this implementation, it is not
> > "readonly" in the sense that it is used everywhere else for computing, =
it
> > is "assign once".
> >
> > Second, the English words "read only", particularly to native speakers,
> > make this behavior very counterintuitive and confusing. I won't belabor
> > that point further.
> >
> > What "read only" really is, is "constructor initialize only". It honest=
ly
> > has nothing to do with "read" as it's implemented.
>
> Not quite.  It really is just write-once.  The idea that you can only do
> that in the constructor is not in the language; that's been invented by
> over-eager static analysis tools.  (Everyone should disable that check.)
>
> > I guess I worry that this RFC makes `readonly` even more of a minefield
> for
> > PHP developers, increasing the mental load of using it in code while
> *even
> > further* watering down the benefits it may provide. It's already design=
ed
> > in a somewhat counterintuitive way that I feel will be almost completel=
y
> > replaced in actual code in the wild by "immutable" if PHP ever gets tha=
t.
>
> Working on asymmetric visibility, I have come to agree.  Nikita proposed
> readonly as a "junior version" of asymmetric visibility, to cover the mos=
t
> common use case without introducing more complexity.  At the time, he was
> confident that it wouldn't preclude expanding to asymmetric visibility in
> the future.  Well... I can say with confidence at this point that is not
> correct, and the design of readonly is causing issues for asymmetric
> visibility, and for cloning, to the point that (based on feedback in the
> other thread) we're likely going to for now forbid readonly and a-viz on
> the same property.
>
> At this point, I think I view readonly as a cautionary tale about the
> costs of doing "quick and easy" design over something more robust, becaus=
e
> the quick-and-easy creates problems down the line that a more thoughtful,
> holistic view would have avoided.
>

To me, the best outcome of this discussion should be to retire readonly
*classes*. They were thought as a quick way to not repeat the readonly
keyword on every property, but they in fact introduce this behavior +
expectations related to child classes and these collide. From a conceptual
ground, I think we proved that there are no guarantees to pass to child
classes, but I can't argue against everybody's first understanding, even if
they're wrong.

Is retiring the keyword on classes an option? I feel like this ship has
sailed.
But then, if we can't retire them, we should at least fix them.



> > LSP doesn't exist because it is some objectively better way of
> programming
> > according to universal laws of entropy or something. It is instead
> > important because LSP helps programmers be able to predict the behavior
> of
> > the program they are writing and reduces the short-term memory load
> > involved in programming and architecture.
> >
> > Something that *technically* complies with LSP but makes the program
> harder
> > to predict and increases the mental load of programming violates the
> > *purpose* of LSP. We can argue about whether it is technically correct,
> but
> > I feel like that somewhat misses the point: making the language more
> > capable, more stable, and more predictable.
> >
> > In other words, I do not believe it is that important or care to argue
> > about whether this RFC violates LSP. It violates the *purpose* of LSP,
> and
> > that's a bigger problem to me personally.
> >
> > Jordan
>
> At this point, I am inclined to agree.  readonly is wonky enough as is.
> Making it semi-LSP (in spirit) just makes it even more wonky.  To flip th=
e
> earlier sentiment, "readonly is broken enough as is, let's not break it
> even further."
>

I'm not comfortable with this sort of "discrediting" of readonly as it
works now. At the time, I thought asymmetric visibility was the thing we
needed. Yet the community decided that we needed the "assign once" extra
guarantee. It's not really fair to now call this as "broken". It's not. It
works as designed, and to my experience, it is a useful feature, especially
for public/protected properties.

I don't see much discussion about the cloning part of the RFC. Marco wrote
"allowing mutation in `__clone()` is fine" and I feel like that's also fine
to most, am I correct? That's the most critical part of the proposal, since
as I just explained, the first part of the RFC is a behavior that we can
work around already (but that makes the language a maze =C2=AF\_(=E3=83=84)=
_/=C2=AF.)

Nicolas

--000000000000be6cd705ee77757e--