Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118564
MIME-Version: 1.0
References: <CAOWwgpk7uAkZD_CH5inbm8Ng8iVL5SZsz59Mpzgpc_iG42ei8Q@mail.gmail.com>
 <CAH5C8xU=eukkqn+PToG4-JT0vqjChhkY=kd_4t_ki9mXu1UMqQ@mail.gmail.com>
In-Reply-To: <CAH5C8xU=eukkqn+PToG4-JT0vqjChhkY=kd_4t_ki9mXu1UMqQ@mail.gmail.com>
Date: Mon, 5 Sep 2022 12:05:51 +0200
Message-ID: <CAOWwgp=YjUVO_AW70--tM6iSBK2NStY9UZDCYzc0hXiCeeUOjw@mail.gmail.com>
To: =?UTF-8?B?TcOhdMOpIEtvY3Npcw==?= <kocsismate90@gmail.com>
Cc: PHP Internals List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000027443b05e7eb3acd"
Subject: Re: Issues with readonly classes
From: nicolas.grekas+php@gmail.com (Nicolas Grekas)

--00000000000027443b05e7eb3acd
Content-Type: text/plain; charset="UTF-8"

Hi Mate

For 2.: static properties are useful e.g. to cache some shared state (info
>> extracted from reflection in my case) and I really don't see why readonly
>> classes should forbid static properties. Readonly is only useful for
>> instances, because it gives them immutability, but what's the link with
>> static properties? Note that this is less important than the previous point
>> as it's always possible to store shared global state in another class. But
>> still, this feels arbitrary to me.
>>
>
> First, I'll answer to your question about static properties, because the
> answer is simple: as readonly static properties are not supported (
> https://wiki.php.net/rfc/readonly_properties_v2#restrictions), I had to
> decide what to do with static properties of readonly classes (
> https://github.com/php/php-src/pull/7305#issuecomment-971381308), so
> Nikita and I agreed that forbidding them is the better option until we add
> support for static readonly properties. In my opinion, being able to set
> readwrite static properties for readonly classes would be very
> counterintuitive.
>

Thanks for the precision, works for me As I mentioned, there are ways
around anyway (storing in another class or in a static variable.)


But I'm also wondering about B: the RFC doesn't contain any rationale about
>> why this restriction exists (it says "Similarly how overriding of readonly
>> properties works", but the similarity is quite weak, readonly props don't
>> apply to other properties of child classes.) If there is no compelling
>> reason to have this limitation, it should be removed IMHO. Basically, this
>> would mean that child classes of readonly classes wouldn't have to be
>> readonly themselves (but the existing properties on the parent would have
>> to be of course.)
>
>
> Regarding your main question: I understand your problem with readonly
> classes, and I'd be happy if we found a solution which fits your use-cases
> and keeps consistency for the engine at the same time. To give you more
> context about the inheritance related restriction in the RFC: I went with
> forbidding the extension of readonly classes by non-readonly ones so that
> the invariants of the parent (no dynamic or mutable properties are allowed)
> don't occasionally get violated by the child. However, I cannot think about
> a proper scenario now where this would cause any problems... I'm wondering
> if anyone could come up with one?
>

Not only can I not find where removing this invariant would be a problem, I
also cannot think of where this invariant would be useful. I don't see what
I can build on top of it...



> And at last, regarding the interaction of readonly properties with
> __clone(): I'll definitely think more about this topic in the following
> weeks. Currently, it seems to me that readonly properties should be able to
> be reinitialized once *during* cloning, but not afterwards. Is this in line
> with what you want to achieve? If we once manage to add support for the
> "clone with" construct then readonly property reassignment(s) due to "with"
> arguments should also be allowed. But I'll need more time to also consider
> any undesirable side effects/edge cases, which could apply, should we
> settle on the approach described above.
>

During cloning only, yes, and we don't need any new syntax to allow this
(we might need one to make it easier to write wither APIs, but that's
another topic.)
That's exactly what I'm trying to achieve in
https://github.com/php/php-src/pull/9403. For the record, I tried 3
approaches to manage the transient state that is required to flag readonly
properties as once-writeable during cloning. I tried using zval.u1 but I
realized this is a read-only area right?, then tried using zval.u2.extra
but this is not a bitfield, then zval.u2.property_guard but without luck. I
didn't see any blocker either in the engine, so the only reason why I
failed is because my C-related skills are too weak and I'd need help to
figure out a proper patch.

Nicolas

>

--00000000000027443b05e7eb3acd--