Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114760
MIME-Version: 1.0
References: <CAF+90c-W_g9Mt9yMNzBWfMEmQP287a7A6S75RyZo2fvUO2uGXQ@mail.gmail.com>
 <83a9d0ab-60b8-4939-8be9-bdd6ddeb46c5@www.fastmail.com>
In-Reply-To: <83a9d0ab-60b8-4939-8be9-bdd6ddeb46c5@www.fastmail.com>
Date: Mon, 7 Jun 2021 11:06:01 +0200
Message-ID: <CAF+90c-=y=t2o0O7MbcWjqGUK8Z_imGuw5St3n_fGcARMWZvxg@mail.gmail.com>
To: Larry Garfield <larry@garfieldtech.com>
Cc: php internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000b13a3d05c4295ae6"
Subject: Re: [PHP-DEV] [RFC] Readonly properties
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000b13a3d05c4295ae6
Content-Type: text/plain; charset="UTF-8"

On Sat, Jun 5, 2021 at 6:51 PM Larry Garfield <larry@garfieldtech.com>
wrote:

> On Fri, Jun 4, 2021, at 10:19 AM, Nikita Popov wrote:
> > Hi internals,
> >
> > I'd like to open the discussion on readonly properties:
> > https://wiki.php.net/rfc/readonly_properties_v2
> >
> > This proposal is similar to the
> > https://wiki.php.net/rfc/write_once_properties RFC that has been
> declined
> > previously. One significant difference is that the new RFC limits the
> scope
> > of initializing assignments. I think a key mistake of the previous RFC
> was
> > the confusing "write-once" framing, which is both technically correct and
> > quite irrelevant.
> >
> > Please see the rationale section (
> > https://wiki.php.net/rfc/readonly_properties_v2#rationale) for how this
> > proposal relates to other RFCs and alternatives.
> >
> > Regards,
> > Nikita
>
> Thank you for the detailed analysis in the rationale section.  I am,
> however, still skeptical of this approach, for a couple of reasons.
>
> 1. This does appear to address the "leakage" problem I noted in my earlier
> analysis around the start of the year, when considering the writeonce
> proposal[1][2].  That's great to see.
>
> 2. It doesn't address the larger question of cloning, however.  The answer
> for now seems "maybe we'll get clone-with at some point", which would be a
> way around it, but there is no active RFC for that right now.  I'm
> obviously very in favor of RFCs that complement each other to give more
> than the sum of their parts, but those RFCs need to be at least on the
> horizon to actually come together.  Right now that looks like it won't
> happen this cycle.  Absent clone-with, readonly would be effectively
> unusable in any evolvable object of any non-trivial complexity.


Compared to the general area of applicability of readonly properties, the
cases that require clone-with are rare. It's the intersection of
"not-really-immutable objects using wither evolution" and "has so many
properties that passing them to the constructor is infeasible". While this
intersection does include a couple of prominent examples, they are by no
means numerous. While it may be a problem worth solving, I *personally*
don't consider it neither particularly important nor time critical.

For what it's worth, I believe Mate wants to work on clone-with once this
RFC passes -- while there is no RFC for that, there is already an
implementation, though from a cursory look it would require some
adjustments to actually work with readonly properties.


> It also wouldn't work with objects that need properties that are not
> constructor arguments, such as PSR-7 type objects.  That's a no in my book.
>

I don't understand what you mean here. Why would properties that are not
constructor arguments be a problem?


> 3. As noted in my previous analysis, even with clone-with, asymmetric
> visibility results in a nicer syntax when evolving objects that have any
> complexity to them.  (See the examples in [2].)
>

I tend to agree. And I am not opposed to having asymmetric visibility as
well. For example, C# does both, and I think both do have value. "readonly"
is a very strong signal to the reader. Asymmetric visibility is not.
Asymmetric visibility (without proper readonly support) could mean that the
property is actually readonly, or it could mean that it's exactly what it
says on the tin: The property is internally mutable. I don't think these
two concepts should be conflated, though they certainly have overlap.


> 4. One detail left out of the rationale section is how much of a
> performance difference there is between readonly and implicit-accessors.
> It says the latter still has a performance hit, but not how much.  How
> significant is it?  If it's tiny,  then frankly the biggest argument for
> readonly goes away, since implicit-accessors gives us 98% the same
> functionality in a more forward-compatible way without the cloning issues.
> If it's twice as slow, then having a separate keyword for a common case
> makes sense.
>

The performance difference shouldn't be large, maybe 10-15%. I don't
consider performance a big deciding factor here. When comparing to
accessors the relevant difference I see is that readonly properties cover a
large use case at a small fraction of language complexity.


> 5. I would have to experiment a bit with hydration, as others have noted,
> because unconventional object construction mechanisms are a critically
> important workflow.  The RFC even notes in passing that serialization is
> possibly made complicated.  Just how complicated?  There's no mention of
> __deserialize() and how it would interact, but cases like that need to be
> very clearly handled and documented.
>

The RFC notes in passing that *if* readonly default values were allowed, it
could have impact on userland deserialization. As-is, I believe the
proposal doesn't present any problem for userland hydrators or
deserializers, or PHP's own unserialization mechanisms. This should get
mentioned in the proposal, though it will be along the lines of "yes,
everything works as expected".


> 6. I'm OK with the approach to constructor promotion and default values.
> That seems reasonable in context, especially if combined with
> New-in-initializers[3], which I am hoping you plan to finish this cycle. :-)
>
> 7. Though, it just occurred to me, this may result in issues around
> optional values.
>
> class URL {
>   public function __construct(
>     public readonly string $scheme = 'https',
>     public readonly string $domain = '',
>     public readonly int $port = '443',
>     public readonly string $path = '',
>   ) {}
> }
>
> Now, if you want to hydrate the object externally, you need to ensure
> those properties are not set by the constructor.  However, a promoted value
> is always set by the constructor; either it has a default or it is
> required.  From previous replies it sounds like the workaround for that is
> reflection and newWithoutConstructor(), but I'm not sure how I feel about
> that, because that would also then preclude any *other* logic in the
> constructor, as that would also get skipped.  Perhaps this isn't as big of
> an issue as I think it is, and if so I'd love to hear why, but it makes me
> concerned.
>

Using newInstanceWithoutConstructor() is not a workaround, to the best of
my knowledge these things are always implemented this way. You let the
constructor initialize the properties, or you bypass the constructor and
manually initialize them. I don't really see how it could work in any other
way, unless you have some very specific contract between your constructor
and a hydrator library (in which case you should make that contract with a
method other than the constructor).

8. Although the RFC says it does not preclude accessors or explicit
> asymmetric visibility in the future, and I absolutely believe that Nikita
> is genuine about that, I am still concerned that should more robust
> proposals come forward later, it will be met with "we don't need another
> still-fancier syntax here, readonly is good enough."  It's good to know
> that C# manages to have both, but that doesn't mean the same logic would
> apply in PHP, or to PHP voters, specifically.
>

To clarify, I think your argument here goes roughly like this (correct me
if I'm wrong): If we already have one of readonly properties or asymmetric
visibility, an argument could be made for not adding the other one because
it doesn't provide sufficient marginal value. So, if we assume that we can
only have one of these features, is readonly properties the right one to
add? Shouldn't we add asymmetric visibility instead, which can be used for
a wider range of use cases?

I think this is a legitimate concern to have, but (under the assumption
that we can have only one) it's not easy to say which would be preferable.
Yes, asymmetric visibility is more widely applicable, but at the same time
I would expect most applications to be bonafide readonly properties. So
while asymmetric visibility is more widely applicable, it's also imprecise
for the majority use case. Which is better? Of course, my assumption here
could also be wrong.

9. I know that the variance for properties in child classes was a source of
> discussion, and the end result seems to be that readonly-ness is
> invariant.  However, that precludes having an easy way to have both a
> mutable and immutable version of a class, easily.  (If you wanted to have,
> say, a read-only class most of the time for safety, but for writing you use
> an alternate "open" version that can be updated and then persisted to the
> database.)  That's a style I've experimented with on and off for a while
> and already don't have a great solution to, but it feels like readonly
> would make that even harder.  Again, I'd be very happy to hear alternatives
> around that.
>

I don't believe you can't have an immutable class extend a mutable one or a
mutable extend an immutable one, both result in LSP violations (though not
on the type level). There was a lot of discussion around this when
DateTimeImmutable was introduced, which notably is not in an inheritance
relationship (in other direction) with DateTime, for good reason.


> Depending on the answers to the above, I could be convinced of this as a
> stop gap iff paired with clone-with in the same version.  However, on its
> own I think this is only a half-solution, and I'm not wild about a
> half-solution for a version or two.  That's why I prefer going straight to
> asymmetric visibility, as that would cover mostly the same use case in a
> single RFC while still being forward compatible; it would benefit from
> clone-with, but still be usable without it.
>

Just to reiterate this point: Readonly properties is not a half solution to
which asymmetric visibility is the full solution. They are *different*
solutions to different albeit overlapping problems. I'm not proposing
readonly properties here because it's the only thing I can get into 8.1,
I'm proposing it because I think it's the more important problem to solve.

Regards,
Nikita

--000000000000b13a3d05c4295ae6--