Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114741
User-Agent: Cyrus-JMAP/3.5.0-alpha0-519-g27a961944e-fm-20210531.001-g27a96194
Mime-Version: 1.0
Message-ID: <83a9d0ab-60b8-4939-8be9-bdd6ddeb46c5@www.fastmail.com>
In-Reply-To: 
 <CAF+90c-W_g9Mt9yMNzBWfMEmQP287a7A6S75RyZo2fvUO2uGXQ@mail.gmail.com>
References: 
 <CAF+90c-W_g9Mt9yMNzBWfMEmQP287a7A6S75RyZo2fvUO2uGXQ@mail.gmail.com>
Date: Sat, 05 Jun 2021 11:51:10 -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 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.  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.

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].)

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.

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.

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.

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.

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.

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.

[1] https://peakd.com/hive-168588/@crell/object-properties-and-immutability
[2] https://peakd.com/hive-168588/@crell/object-properties-part-2-examples
[3] https://wiki.php.net/rfc/new_in_initializers

--Larry Garfield