Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:125750
Precedence: bulk
MIME-Version: 1.0
References: <CAPyeZ4ML6nvmGWwH=p8Dko-mnPV1-tcgs7EF_P7yA-X_KLHpsA@mail.gmail.com>
In-Reply-To: <CAPyeZ4ML6nvmGWwH=p8Dko-mnPV1-tcgs7EF_P7yA-X_KLHpsA@mail.gmail.com>
Date: Fri, 4 Oct 2024 18:13:29 +0200
Message-ID: <CAPyj-LBUDVJsBJXvBxVtDGMf4KJ-MWgbZHhA_C-KeedQ2wZ0Jw@mail.gmail.com>
Subject: Re: [PHP-DEV] Asymmetric visibility Reflection API problems
To: php internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: tovilo.ilija@gmail.com (Ilija Tovilo)

Hi Valentin

Thank you for bringing up your concerns.

On Thu, Oct 3, 2024 at 3:32=E2=80=AFPM Valentin Udaltsov
<udaltsov.valentin@gmail.com> wrote:
>
> We are going to talk about the asymmetric visibility RFC [2] and the
> new `ReflectionProperty::is(Private|Protected)Set()`
> methods. Consider a class and reflected facts about it's properties [3]:
>
> ```php
> class A
> {
>     // isPrivateSet() =3D true (OK)
>     public private(set) mixed $public_private_set;
>
>     // isPrivateSet() =3D false (I would expect true)
>     private private(set) mixed $private_private_set;
>
>     // isPrivateSet() =3D false (I would expect true)
>     private mixed $private;
>
>     // isProtectedSet() =3D true (OK, explained in the RFC)
>     public readonly mixed $public_readonly;
>
>     // isProtectedSet() =3D false (I would expect true)
>     protected readonly mixed $protected_readonly;
>
>     // isProtectedSet() =3D false (I would expect true)
>     protected protected(set) readonly mixed $protected_protected_set_read=
only;
>
>     // isPrivateSet() =3D false (OK), isProtectedSet() =3D false (OK)
>     public bool $virtual_no_set_hook { get =3D> true; }
> }
> ```

As mentioned on GitHub, I do understand and agree that this may seem
confusing without more context.
However, this behavior for flags is really not unique to asymmetric visibil=
ity.
For example:

```php
interface I {
    public function test();
}

var_dump((new ReflectionMethod('I', 'test'))->isAbstract());
//> bool(true)
```

The `abstract` flag is not (and cannot) be specified explicitly, but it is =
added
implicitly. Similarly, the `protected(set)` in `protected protected(set)` i=
s
redundant, so the compiler removes it to omit additional visibility checks.=
 For
the same reason, we don't even have an internal flag (at runtime) for
`public(set)`, as it is always removed at compile time.

As mentioned, these methods are generally quite "dumb" and simply expose th=
ese
internal flags to userland. This is reflected by `getModifiers()`, which re=
turns
a bitmask over all flags. `getModifiers()` should stay consistent with
`isPublic()`, as well as all the other methods. This will be increasingly
difficult if we start tweaking the implementation of these methods.

I have proposed to instead solve this by introducing new functions,
`ReflectionProperty::isReadable()` and `isReadable()`. They would not only
handle visibility and asymmetric visibility, but also scope, virtual-ness,
hooks and readonly (including __clone). Additionally, if some new
concept is ever
added, the functions may adapt. I have created a simple PoC [1].

> Here's what I propose:
>
> - `ReflectionProperty::isPublic()` returns `true` only if the property
> is symmetrically public. For `public readonly $prop`
> it will return `false`, because it's implicitly `public protected(set)
> readonly $prop`. This is a BC break, but a
> smaller one, given that libraries now take "readonly" into account.
> - `ReflectionProperty::isProtected()` returns `true` only if the
> property is symmetrically protected. For `protected readonly $prop`
> it will return ``true``, because it's implicitly `protected
> protected(set) readonly $prop`. Fully backward compatible.
> - `ReflectionProperty::isPrivate()` returns `true` only if the
> property is symmetrically private. Fully backward compatible.
> - Add `ReflectionProperty::isPublicGet()`: returns `true` if property
> is `public` or `public XXX(set)`.
> - Add `ReflectionProperty::isProtectedGet()`: returns `true` if
> property is `protected` or `protected XXX(set)`.
> - Add `ReflectionProperty::isPrivateGet()`: returns `true` if property
> is `private` or `private private(set)`.
> - Add `ReflectionProperty::isPublicSet()`: returns `true` if property
> is symmetrically public (asymmetric public
> set is not possible).
> - Change `ReflectionProperty::isProtectedSet()`: returns `true` if
> property is symmetrically protected or has an
> asymmetric protected set.
> - Change `ReflectionProperty::isPrivate()`: returns `true` if property
> is symmetrically private or has an asymmetric
> private set.

I very much disagree with changing `isPublic` and friends. It would be a
considerable BC break which isn't acceptable in the RC phase. I also don't =
think
it's necessarily more correct. With this RFC, properties have two visibilit=
ies:

1. The visibility of the entire property, which we are all used to.
2. The visibility of the set operation, which was introduced with
asymmetric visibility.

Essentially, the get operation continues to inherit the visibility from the
property. This is what `isPublic()` and friends are referring to. Of course=
, you
may argue that an equally valid mental model is that the property no longer=
 has
a visibility, but an additional get visibility. However, this does not
accurately reflect the implementation, nor, more importantly, the syntax.

> The most difficult question is whether we can have these or similar
> changes in 8.4 after RC1... Does a BC break
> in the current implementation (if we agree that it exists) give us
> such an option?
>
> Also, Ilija has a brilliant idea to add `isReadable($scope): bool`,
> `isWritable($scope): bool` methods [5],
> but this can be done in PHP 9.

Normally: No. Breaking changes or new features (since very recently [2]) ar=
e not
allowed in the RC phase, which we have entered Sep 26th. If an exception we=
re to
be made, two new functions would have a much lower impact than tweaking the
behavior of some very old functions. I'm not sure whether this is something=
 we
necessarily need in 8.4. I have deferred this decision to the RMs.

Also note that such functions wouldn't need to wait for PHP 9, it may also =
go
into the next minor. The next version of PHP is likely going to be 8.5, unl=
ess
we find a good reason to bump the major version.

Regards,
Ilija

[1] https://github.com/php/php-src/pull/16209
[2] https://wiki.php.net/rfc/release_cycle_update#disallow_new_features_in_=
release_candidates_and_bug_fix_releases