Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:125740
Precedence: bulk
MIME-Version: 1.0
Date: Thu, 3 Oct 2024 16:32:27 +0300
Message-ID: <CAPyeZ4ML6nvmGWwH=p8Dko-mnPV1-tcgs7EF_P7yA-X_KLHpsA@mail.gmail.com>
Subject: [PHP-DEV] Asymmetric visibility Reflection API problems
To: php internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
From: udaltsov.valentin@gmail.com (Valentin Udaltsov)

Hi, internals!

Recently I've started testing the new features of the PHP 8.4
Reflection API and found some of them unintuitive.
At first, I reported this as a bug [1], but then Ilija explained that
the behavior is intentional. So I've decided to discuss
the issue publicly.

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() = true (OK)
    public private(set) mixed $public_private_set;

    // isPrivateSet() = false (I would expect true)
    private private(set) mixed $private_private_set;

    // isPrivateSet() = false (I would expect true)
    private mixed $private;

    // isProtectedSet() = true (OK, explained in the RFC)
    public readonly mixed $public_readonly;

    // isProtectedSet() = false (I would expect true)
    protected readonly mixed $protected_readonly;

    // isProtectedSet() = false (I would expect true)
    protected protected(set) readonly mixed $protected_protected_set_readonly;

    // isPrivateSet() = false (OK), isProtectedSet() = false (OK)
    public bool $virtual_no_set_hook { get => true; }
}
```

Here's why it works this way. Internally properties with symmetric
visibility do not have an asymmetric flag. That is
why `private private(set)` and `private` both have `isPrivateSet() =
false`. Readonly properties without explicit `set`
imply `protected(set)` [4] and thus they are symmetric when protected
and asymmetric otherwise. I personally don't
agree that Reflection API should stick to the internal implementation
details. On the contrary, it should hide them and
make it easier to study the facts about the code.

Alright. Let's keep that in mind and jump into another issue. I've
suddenly realised, that the new Reflection API breaks
backward compatibility. In PHP <8.4 you can check
`$reflectionProperty->isPublic() &&
!$reflectionProperty->isReadonly()`
and then safely write to that property from global scope. Now this is
not the case anymore: if a public property has a
`private set()`, a simple Hydrator working on these assumptions will
break in 8.4 and require refactoring. The reason
for this is that before 8.4 `ReflectionProperty::isPublic() = true`
guaranteed that the property has public get and set,
now it only guarantees that property has a public get.

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.

```php
class A
{
    // isPublic() = false, isPrivate() = false, isPublicGet() = true,
isPrivateSet() = true
    public private(set) mixed $public_private_set;

    // isPrivate() = true, isPrivateGet() = true, isPrivateSet() = true
    private private(set) mixed $private_private_set;

    // isPrivate() = true, isPrivateGet() = true, isPrivateSet() = true
    private mixed $private;

    // isPublic() = false, isProtected() = false, isPublicGet() =
true, isProtectedSet() = true
    public readonly mixed $public_readonly;

    // isProtected() = true, isProtectedGet() = true, isProtectedSet() = true
    protected readonly mixed $protected_readonly;

    // isProtected() = true, isProtectedGet() = true, isProtectedSet() = true
    protected protected(set) readonly mixed $protected_protected_set_readonly;

    // isPublic() = false, isPublicGet() = true, isPublicSet() =
false, isProtectedSet() = false, isPrivateSet() = false
    public bool $virtual_no_set_hook { get => true; }
}
```

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.

1. https://github.com/php/php-src/issues/16175
2. https://wiki.php.net/rfc/asymmetric-visibility-v2
3. https://3v4l.org/O3FNN/rfc#vgit.master
4. https://wiki.php.net/rfc/asymmetric-visibility-v2#relationship_with_readonly
5. https://github.com/php/php-src/issues/16175#issuecomment-2389966021
-- 
Best regards,
Valentin