[RFC] [Discussion] Readonly class amendments View as Markdown

As all the invariants will be maintained in the child classes

The invariant here is the immutability from a behavioural PoV.
LSP has 100% been broken in that example.

I've gone back to the example, and if we are going to use bad code to
"argue" against certain features, then let's use this argument to its
natural conclusion.

But before that, we really need to get everyone clear on what LSP is.
It is a subtyping relationship that states:
if a property P on an object X of type T is provable to be true, then for
an object Y of type S to be considered a proper subtype of T, this property
P must also be true for all object Y of type S.
Let's rephrase this slightly in terms more appropriate to PHP:
if a characteristic P on an instance X of class T is provable to be true,
then for an instance Y of class S to be considered a proper subclass of T,
this characteristic P must also be true for all instances Y of class S.

Having this in mind, let me show how easy it is to break LSP:
class T {
public function __construct(public readonly int $i) {}
public function add(self $t): self { return new self($this->i + $t->i);
}
}

$op1 = new T(5);
$op2 = new T(14);
$r = $op1->add($op2);
var_dump($r);

class S extends T {
public function add(parent $t): self { return new self($this->i -
$t->i); }
}

$op1 = new S(5);
$op2 = new T(14);
$r = $op1->add($op2);
var_dump($r);

Clearly, the characteristic that we can "add two T together" has been
broken in class S.
This is all to say, the type system can only provide a level of code
correctness, and that remains even in highly sophisticated type systems.
If you bear with me and the introduction of the following imaginary type
dependent syntax: Matrix<nm> which declares a Matrix type which is an n by
m matrix.
Thus, we can write the signature of a function that does define matrix
multiplication in the following way:
function multiplication(Matrix<nm> $op1, Matrix<mk> $op2): Matrix<nk>
Which would guarantee us that the 2 matrices provided can indeed be
multiplied together as they have compatible dimensions, and it even gives
us the dimensions of the output matrix.
However, the one thing it cannot guarantee is that the operation we are
actually doing is a matrix multiplication, I could just as well write a
function that returns a new n by k matrix filled with zeroes.

How can one be sure that a subclass will not alter the behaviour? By making
the method final.
That's mainly why personally I think final by default, and using
composition over inheritance, is "better".
And I think from what we've seen in language design over the last couple of
decades is the OO inheritance is just a bad way to do subtyping and
something like Haskell type-classes are way better. [1] But that ship has
somewhat sailed for PHP.
The LSP checks PHP (and for the matter any type system) can do and does, is
checking the logical correctness of the implementation, not behavioural
correctness that onus still remains on the developer.

Now, let's have another look at your example:
/* readonly */ class ImmutableCounter {
public function __construct(private readonly int $count) {}
public function add1(): self { return new self($this->count + 1); }
public function value(): int { return $this->count; }
}

IMHO the fact that the count property is private is the biggest issue, just
changing it to protected would solve most of the issues, as now you've
given the tools to PHP to verify and ensure this constraint holds for the
whole class hierarchy.
But more importantly, it means I don't need to reimplement the whole
parent class and (risk me messing up)/(keeping in sync) the behaviour of
the parent class if I want to do something like:

class SubstractableImmutableCounter extends ImmutableCounter {
public function sub1(): self { return new self($this->count - 1); }
}

As it's easy to mess up even just writing a function properly that does the
expected behaviour, I suppose we should remove the ability for users to
write them, as they cannot be trusted with.
So please, can we stop using the argument that this proposal breaks LSP if
you write dumb code, as that's already the case, and it doesn't if you
provide PHP with the actual information it needs to verify in child classes.

Moreover, if one expects a readonly class to be immutable, then that is
already not the case if one of the properties is a non-readonly object.
It is also not necessarily stateless, as can be seen in the following
example: https://3v4l.org/A2Lbs

readonly class ImmutableBadNumberGenerator {
public function __construct(private readonly int $i) {}

public function generate(): int {
    static $j = 1;
    ++$j;
    return $this->i + $j;
}

}

$o = new ImmutableBadNumberGenerator(10);
var_dump($o->generate());
var_dump($o->generate());
var_dump($o->generate());

Now what is up to debate is what the expectation around a class extending
a readonly class is.
And that I don't really have strong opinions.
However, what this discussion has told me more is that the expectation
around readonly is more like the immutable keyword in D than the behaviour
of the readonly keyword in other languages (as PHP and C# is the same
AFAIK).

Best regards,

George P. Banyard

[1] https://diogocastro.com/blog/2018/06/17/typeclasses-in-perspective/

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 us 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 going 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 should
be very telling.

This comes from two different avenues that compound with each other to
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 honestly
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 designed
in a somewhat counterintuitive way that I feel will be almost completely
replaced in actual code in the wild by "immutable" if PHP ever gets that.

Working on asymmetric visibility, I have come to agree. Nikita proposed readonly as a "junior version" of asymmetric visibility, to cover the most 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, because the quick-and-easy creates problems down the line that a more thoughtful, holistic view would have avoided.

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 the earlier sentiment, "readonly is broken enough as is, let's not break it even further."

--Larry Garfield