Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123475
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-491-g033e30d24-fm-20240520.001-g033e30d2
Precedence: bulk
MIME-Version: 1.0
Message-ID: <734bb8e8-2fdf-4e50-9039-e53c99ee4930@app.fastmail.com>
In-Reply-To: 
 <CAAwdEzBrRJMBQbY0R1Tp8RWpUP_DW6twNvGqnKDp3jjbiBFE-Q@mail.gmail.com>
References: <0a6a61cd-f203-4dea-a7f8-97e6b885c52d@app.fastmail.com>
 <CAAwdEzB0rFhrai8W+B3PhnYEoOR-QEZYUm=R9+8+18Yo9WOgyQ@mail.gmail.com>
 <aadcd4ed-5f73-fbb9-93fb-42f2710e61fe@php.net>
 <ED20EE91-9BBB-4FED-8440-BAE748CBF5AB@gmail.com>
 <CAAwdEzBrRJMBQbY0R1Tp8RWpUP_DW6twNvGqnKDp3jjbiBFE-Q@mail.gmail.com>
Date: Fri, 31 May 2024 16:08:53 +0000
To: "php internals" <internals@lists.php.net>
Subject: Re: [PHP-DEV] [RFC] Asymmetric Visibility, v2
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
From: larry@garfieldtech.com ("Larry Garfield")

On Fri, May 31, 2024, at 12:04 PM, Alexandru P=C4=83tr=C4=83nescu wrote:
> On Fri, May 31, 2024 at 10:30=E2=80=AFAM Claude Pache <claude.pache@gm=
ail.com> wrote:
>>=20
>>=20
>>> Le 30 mai 2024 =C3=A0 17:07, Derick Rethans <derick@php.net> a =C3=A9=
crit :
>>>=20
>>>>=20
>>>> Now, if I define the property as public private(set) with similar=20
>>>> intentions, to make sure that there is no way for external scope or=20
>>>> extending classes scope to write to the property, while allowing=20
>>>> reading from external scope (or extending classes scope).
>>>>=20
>>>> But the problem is that an extending class can define the property =
as=20
>>>> public protected(set), and that will easily allow the property that=
 I=20
>>>> wanted to make sure it is private for writing to be changed by an=20
>>>> extending class to be protected.
>>>=20
>>> public private(set) properties aren't really private, so you don't g=
et=20
>>> the shadowing, but you do have a point wrt to the expectation that a=
n=20
>>> inherited class can't easily override the private(set) part (with=20
>>> protected(set) or public(set)).
>>=20
>>=20
>> Note that the issue already exists today with readonly properties: th=
ose are basically private(set); but if you redeclare a non-private reado=
nly property in a subclass, you can in fact initialise it from the subcl=
ass bypassing the initial private(set) restriction of the superclass: ht=
tps://3v4l.org/9AV4r
>
> Yes, thank you; that's a good point.
> Seems like another issue with readonly, but then again, the=20
> private(set) part of readonly is not really explicitly designed, I=20
> guess.
>=20
>>=20
>> If you want a property not to be overridable, end of story, you can m=
ark it as `final` (the final marker for properties was added as part of =
the hooks RFC, but it works also with non-hooked properties).
>
> Yes, it seems like a good enough option, and use "final public=20
> private(set)" to ensure only the current class will be able to set the=20
> value.
> If we all agree, I think this should be documented in the RFC.
>
> There is another small problem, I think, with "final" modifier not=20
> being allowed for constructor-promoted properties.
> And maybe we can have this fixed in the current RFC if this ends up=20
> being "the correct" way to define public-read private-write properties.
>
> Regards,
> Alex

Mm, yeah, this is an interesting corner case we'd not thought of.  We sp=
ent a little time looking at how other languages handle this.

Kotlin just disallows using private-set on a non-final property.  (Kotli=
n properties are final by default, unless otherwise specified.)  cf: htt=
ps://kotlinlang.org/spec/inheritance.html#overriding

C# disallows changing property visibility in child classes entirely.  cf=
: https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/clas=
ses-and-structs/restricting-accessor-accessibility#access-modifiers-on-o=
verriding-accessors

Swift is... weird.  If you have a public private(set) property, you can =
override it by turning it into a virtual property with only a get hook. =
 If you also define a set hook, though, it just never gets called.  It d=
oesn't appear that you can widen the set visibility, I think.

So since we now have final properties (yay, hooks RFC!), we could probab=
ly mostly solve this by just only allowing private(set) on a final prope=
rty, like Kotlin.  That's probably the easiest approach.  We can also ma=
ke private(set) properties implicitly final to avoid lots of boilerplate=
.  (Having to always type final with private(set) every time is kinda si=
lly.)

It would mean that proxy objects implemented using child classes with ho=
oks would only work with a public protected(set) class, not with a priva=
te(set), but that's probably fine, and since no one is doing that quite =
yet, obviously, there's no existing code to be concerned about.

However, this also brings up another interesting issue: readonly propert=
ies (in 8.3) DO allow redeclaration, essentially adjusting the property =
scope (the class that declares it) to make the visibility check pass. Th=
at is, the definition of the class it is private to changes, which is di=
fferent from how inheritance works elsewhere.  When the parent writes to=
 the same property, a special check is needed to verify the two properti=
es are related.  All that special casing effectively means that readonly=
 in 8.4 wouldn't really be "write once + private(set)", but "write once =
+ private(set) - final", which is... just kinda screwy.  That means our =
options are:

* A BC break on readonly (not allowing it to be overridden)
* Make readonly an exception to the implicit final.
* Just don't allow readonly with aviz after all.

After some consideration, we believe the third option is best (least bad=
).  The other options add still-more special cases or break existing cod=
e, neither of which are good.  If a property is declared private(set), t=
hen child classes simply should not be allowed to change that, period.  =
Even adding a get hook in a child is potentially a problem, as it would =
change the "round trip" behavior in ways the parent class doesn't expect=
.  Disallowing readonly and making private(set) implicitly final avoids =
the loopholes in the language that would violate that expectation.

With aviz, all the use cases for readonly are essentially covered alread=
y.  If you have private(set), you can control when it gets written anywa=
y, so if you want it to be write-once, it=E2=80=99s write once.  If you =
have protected(set), you=E2=80=99re allowing a child to set it, so they =
can do so whenever they want anyway, and could be doing it out of order =
from the parent class to begin with.

So if you wanted the equivalent of "public protected(set) readonly strin=
g $foo", in practice, the readonly is not giving you much to begin with.=
  And "public public(set) readonly string $foo"... we just can't really =
think of a good use case for.  Even if one exists, that could be emulate=
d with a set hook now.

In terms of code length, thanks to the optional public-get visibility, t=
he amount of typing is roughly the same:

readonly class ReadonlyPoint {
    public function __construct(
        public int $x,
        public int $y,
    ) {}
}

class AvizPoint {
    public function __construct(
        private(set) int $x,
        private(set) int $y,
    ) {}
}

(Or protected, if you prefer.)  The latter provides a lot more flexibili=
ty, at the cost of "enforce write-once yourself in the class if you care=
", which seems an entirely reasonable tradeoff.

So we feel the best way forward is to make the following changes:

* private(set) implicitly means "final".  (You can declare it explicitly=
 if you want, but it isn't necessary.)
* Make readonly incompatible with aviz again.

Thoughts?

Also, Alexandru noted earlier that final properties don't seem to be sup=
ported in constructor promotion.  That's an oversight from the hooks imp=
lementation, not a design choice, so Ilija will just fix that as part of=
 the hooks PR before it gets fully merged.

--Larry Garfield