Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114769
User-Agent: Cyrus-JMAP/3.5.0-alpha0-519-g27a961944e-fm-20210531.001-g27a96194
Mime-Version: 1.0
Message-ID: <9e921668-369c-41f2-a207-856cd098a3f1@www.fastmail.com>
In-Reply-To: 
 <CAF+90c-=y=t2o0O7MbcWjqGUK8Z_imGuw5St3n_fGcARMWZvxg@mail.gmail.com>
References: 
 <CAF+90c-W_g9Mt9yMNzBWfMEmQP287a7A6S75RyZo2fvUO2uGXQ@mail.gmail.com>
 <83a9d0ab-60b8-4939-8be9-bdd6ddeb46c5@www.fastmail.com>
 <CAF+90c-=y=t2o0O7MbcWjqGUK8Z_imGuw5St3n_fGcARMWZvxg@mail.gmail.com>
Date: Mon, 07 Jun 2021 09:08:42 -0500
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] [RFC] Readonly properties
From: larry@garfieldtech.com ("Larry Garfield")

On Mon, Jun 7, 2021, at 4:06 AM, Nikita Popov wrote:
> On Sat, Jun 5, 2021 at 6:51 PM Larry Garfield <larry@garfieldtech.com>
> wrote:

> > 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.

I look forward to it.

I don't think the use cases for non-trivial structs (where a withX() method can just return new static(list all properties here) ) are as few as you think.  One difficulty in determining that is that, if they are made easier/more robust (via any of the mechanisms under discussion), their usage is likely to increase.  That's a good thing, and a benefit of all of these pieces floating around, but it does make it harder to gauge the net-use of any particular feature.  I prefer to err on the side of enabling more flexibility and power rather than less.

> > 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?

Without clone-with, if you wanted a with-er method, it would necessarily look like this:

class Point {
  public function __construct(
    public readonly int $x, 
    public readonly int $y, 
    public readonly int $z,
  ) {}

  public function withX(int $newX) {
    return new static($newX, $this->y, $this->z);
  }
}

That is, you're populating the entire property set via the constructor.  In some cases that's fine; the larger the object, though, the more needlessly verbose redundancy that has.

Now let's add a "rendered" flag property, that is public readonly bool.  (Maybe not the best example; it's before 9 am and I'm trying to think on the fly here.)  It's not in the constructor, but for later processing.  It would get set as a side effect of some other method, to indicate the point has already been drawn.  Now make a new version of the object with a different Z.  What do you do with $rendered?

> > 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.

I think this is the crux of the disagreement.  There's a large swath of cases that could be well-handled by either.  I feel the cases that are only or better handled by asymmetric visibility is larger than the cases that are only or better handled by readonly.  As noted above, I don't know that either of us have any data with which to determine which is actually the case.

(Maybe someone with more C# experience can speak to that?  What's typical there, where both mechanisms are available?)

> > 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. :-)

> 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?

That is essentially correct.  I know you're not making that argument, and I'm not making that argument, but it's an argument I can absolutely see being made, because it has been in the past for other things, and sometimes I agree with it as it can be a valid argument.

Hence why, in general, I prefer to go with the more robust, more "interacts nicely with other things" option from the get-go, to both not make more robust options harder to adopt in the future and to avoid user confusion down the road when they have 3 different tools that all do *mostly* the same thing and aren't sure of the subtle reasons why they'd use one over the other.

(Like, for example, if one had a readonly property but wanted to convert it to a custom accessor property... what are the subtle knock on effects they have to account for, then?  There are almost certainly more of them than if switching a property from implicit accessors to explicit accessors, although off hand I don't know what they would all be.)

> 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.

As above, I'd love to get some actual data from the C# community on this.  Lacking that, we're all just guessing.

> 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.

True.  Might that be feasible with asymmetric visibility, however?  (I'm honestly not sure.)

> > 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.

I was perhaps unclear here.  I don't think readonly is a half-solution and asymmetric visibility is the full solution.  I think readonly without clone-with is a half-solution.  readonly plus clone-with is a mostly-full solution that ends up in a very similar (competing?) place with asymmetric visibility, which is a mostly-full solution.  I can probably still vote for readonly plus clone-with, because that combination would be useful.  It's readonly without clone-with that I am concerned about being a half-solution.  (clone-with is a nice-to-have with asymmetric visibility, but IMO is an ergonomic necessity with readonly.)

--Larry Garfield