Re: [RFC] Asymmetric Visibility, v2

On Fri, May 31, 2024, at 12:04 PM, Alexandru Pătrănescu wrote: > On Fri, May 31, 2024 at 10:30 AM Claude Pache <claude.pache@gmail.com> wrote: >> >> >>> Le 30 mai 2024 à 17:07, Derick Rethans <derick@php.net> a écrit : >>> >>>> >>>> Now, if I define the property as public private(set) with similar >>>> intentions, to make sure that there is no way for external scope or >>>> extending classes scope to write to the property, while allowing >>>> reading from external scope (or extending classes scope). >>>> >>>> But the problem is that an extending class can define the property as >>>> public protected(set), and that will easily allow the property that I >>>> wanted to make sure it is private for writing to be changed by an >>>> extending class to be protected. >>> >>> public private(set) properties aren't really private, so you don't get >>> the shadowing, but you do have a point wrt to the expectation that an >>> inherited class can't easily override the private(set) part (with >>> protected(set) or public(set)). >> >> >> Note that the issue already exists today with readonly properties: those are basically >> private(set); but if you redeclare a non-private readonly property in a subclass, you can in fact >> initialise it from the subclass bypassing the initial private(set) restriction of the superclass: >> https://3v4l.org/9AV4r > > Yes, thank you; that's a good point. > Seems like another issue with readonly, but then again, the > private(set) part of readonly is not really explicitly designed, I > guess. > >> >> If you want a property not to be overridable, end of story, you can mark 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 > private(set)" to ensure only the current class will be able to set the > 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 > being allowed for constructor-promoted properties. > And maybe we can have this fixed in the current RFC if this ends up > 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 spent a little time looking at how other languages handle this. Kotlin just disallows using private-set on a non-final property. (Kotlin properties are final by default, unless otherwise specified.) cf: https://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/classes-and-structs/restricting-accessor-accessibility#access-modifiers-on-overriding-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 doesn't appear that you can widen the set visibility, I think. So since we now have final properties (yay, hooks RFC!), we could probably mostly solve this by just only allowing private(set) on a final property, like Kotlin. That's probably the easiest approach. We can also make private(set) properties implicitly final to avoid lots of boilerplate.. (Having to always type final with private(set) every time is kinda silly.) It would mean that proxy objects implemented using child classes with hooks would only work with a public protected(set) class, not with a private(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 properties (in 8.3) DO allow redeclaration, essentially adjusting the property scope (the class that declares it) to make the visibility check pass. That is, the definition of the class it is private to changes, which is different from how inheritance works elsewhere. When the parent writes to the same property, a special check is needed to verify the two properties 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 code, neither of which are good. If a property is declared private(set), then 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 already. If you have private(set), you can control when it gets written anyway, so if you want it to be write-once, it’s write once. If you have protected(set), you’re 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 string $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 emulated with a set hook now. In terms of code length, thanks to the optional public-get visibility, the 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 flexibility, 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 supported in constructor promotion. That's an oversight from the hooks implementation, not a design choice, so Ilija will just fix that as part of the hooks PR before it gets fully merged. --Larry Garfield