Re: Asymmetric visibility Reflection API problems

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]: > > ```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; } > } > ``` 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: ```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) 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��n�:G�� > �U� > 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,�10��On8 > ��, > 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