Asymmetric visibility Reflection API problems

Hi

This thread is a good example why RFCs with a large impact should
ideally be implemented well in advance of feature freeze.

Am 2024-10-03 15:32, schrieb Valentin Udaltsov:

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.

I agree. The RFC specifies:

The ReflectionProperty object is given two new methods:
isProtectedSet(): bool and isPrivateSet(): bool. Their meaning should
be self-evident.

Clearly it's not self-evident, and I would intuitively expect that a
protected Type $name property is isProtectedSet(), because it
clearly is protected set.

• 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.

With this I disagree. The hydrator / code generation use case you
mention is generally intimately tied to the capabilities of a given PHP
version. Any new feature in PHP can affect the behavior of such code and
thus needs to be evaluated to see if it needs to be taken into account
to ensure correct behavior and asymmetric visibility is no different. I
would not expect my Hydrator to work correctly with a new PHP version,
unless specifically marked as compatible by the author.

Best regards
Tim Düsterhus

Hi Valentin

Thank you for bringing up your concerns.

On Thu, Oct 3, 2024 at 3:32 PM 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]:

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; }
}

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 visibility.
For example:

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) is
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 these
internal flags to userland. This is reflected by getModifiers(), which returns
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 visibilities:

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]) are not
allowed in the RC phase, which we have entered Sep 26th. If an exception were 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, unless
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