8.4 | Asymmetric Visibility support

[DanielEScherzer]

I figured instead of putting all of the discussions on #734, it might be helpful to have a dedicated task to discuss this - let me know if I was wrong

---

• Asymmetric Visibility v2

  • [ ] Proposal needed on how to handle this for the tokenizer
  • [ ] Tokenizer changes needed - property declarations
  • [ ] Tokenizer changes needed - constructor property promotion
  • [ ] Updates needed to the predefined token collections in the Tokens class
  • [ ] Updates needed to utility functions - getMemberProperties(), getMethodParameters() (for constructor property promotion) These updates should include special handling for the abbreviated form.
  • [ ] Updates needed to abstract sniffs ? This needs investigation, but I imagine the AbstractScopeSniff and the AbstractVariableSniff might need updates.
  • [ ] Sniff updates needed

Proposal:

tokenizer

• New tokens T_PUBLIC_SET, T_PROTECTED_SET, and T_PRIVATE_SET are defined if not already defined by PHP (for use when running on lower versions)
• They are added to PHP_CodeSniffer\Util\Tokens::$scopeModifiers and ::$contextSensitiveKeywords
• They are added to PHP_CodeSniffer\Tokenizers\PHP::$knownLengths

getMemberProperties()

Currently: https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/be74da1a2727948c35e8862cc378fcf9e9345b42/src/Files/File.php#L1842-L1857

New: Returns (the is_static, is_readonly, is_final, type, type_token, type_end_token, and nullable_type are unchanged and not repeated here)

Does not use asymmetric visibilitity

scope would remain one of public, private, or protected. The get_scope, get_scope_specified, and set_scope are not included in the array if asymmetric visibility is not used.

return [
    'scope' => 'public', // or private or protected,
    'scope_specified' => true, // can also be false
    ...
];

Uses asymmetric visibility

If get and set visibility are both explicitly provided (or just set visibility public(set) is provided, and the get visibility of public is implicit) then even if the visibilities are the same asymmetric visibility handling is triggered:

• scope is set to asymmetric
• scope_specified is true
• get_scope is public/protected/private
• get_scope_specified is based on if the get scope is specified, or just implicitly public because only the set scope was specified
• set_scope is public/protected/private [there is no set_scope_specified because it must be specified for asymmetric properties]

For the

These updates should include special handling for the abbreviated form.

Abbreviated form results in get_scope_specified being false

return [
    'scope' => 'asymmetric', // always (for asymmetric properties)
    'scope_specified' => true, // always (for asymmetric properties)
    'get_scope' => 'public', // or protected, or private
    'get_scope_specified' => true, // can also be false
    'set_scope' => 'private', // or public, or protected
    ...
];

getMethodParameters()

Currently: https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/be74da1a2727948c35e8862cc378fcf9e9345b42/src/Files/File.php#L1365-L1370

Does not use asymmetric visibility

Unchanged

Uses asymmetric visibility

• property_visibility is set to asymmetric
• visibility_token is set to false
• new array key property_set_visibility is one of public/protected/private
• new array key set_visibility_token is an integer for the stack pointer, can never be false
• new array key property_get_visibility is one of public/protected/private
• new array key get_visibility_token is an integer for the stack pointer, or false if public is implied via short form

[jrfnl]

@DanielEScherzer Thank you for working on this proposal and yes, opening a dedicated issue, like this, to discuss proposed implementation details for the more complex syntaxes is exactly as intended 👍🏻 (this is also noted under "Contribution Process" in the original issue).

Tokenizer

• New tokens T_PUBLIC_SET, T_PROTECTED_SET, and T_PRIVATE_SET are defined if not already defined by PHP (for use when running on lower versions)
• They are added to PHP_CodeSniffer\Util\Tokens::$scopeModifiers and ::$contextSensitiveKeywords
• They are added to PHP_CodeSniffer\Tokenizers\PHP::$knownLengths

While not mentioned anywhere in the RFC (nor made explicitly above), I've just done some checking and it looks like PHP introduced three new PHP native tokens T_PUBLIC_SET, T_PROTECTED_SET, and T_PRIVATE_SET and that these tokens - even though they are "multi-part" - are whitespace and comment intolerant.

class Foo
{
    // This is supported.
    public private(set) string $barA = 'baz';
    private(set) protected string $barB = 'baz';

    // Invalid use (fatal error), but PHP tokenizer supported, so should be supported in PHPCS too.
    public(set) function foo() {}

    // All of the below are parse errors.
    public private (set) string $barE = 'baz';
    public private( set) string $barF = 'baz';
    public private(set ) string $barG = 'baz';
    public private ( set ) string $barH = 'baz';
    public private /*comment*/ (set) string $barI = 'baz';
}

Based on that, your proposal for the Tokenizer related changes is exactly what is needed and should be straight-forward to implement.

You have my blessing to go ahead with this part. ✅

File::getMemberProperties() and File::getMethodParameters()

Before I give an opinion about the current proposals, I'd like to understand the reasoning behind them better, so would you mind expanding a bit on the various "why"'s ?

Why introduce new *_get_* index keys ? Why change the behaviour of the pre-existing scope and scope_specified indexes ? Why change the behaviour of the pre-existing property_visibility and visibility_token indexes ? Why are the new array indexes proposed to be only included when asym visibility is used ? What will the impact of these changes be on pre-existing sniffs ?

I suspect there will be a reason behind proposing it like this and I'd like to understand this reason before discussing the implementation details.

[DanielEScherzer]

File::getMemberProperties() and File::getMethodParameters()

Before I give an opinion about the current proposals, I'd like to understand the reasoning behind them better, so would you mind expanding a bit on the various "why"'s ?

Why introduce new *_get_* index keys ?

So that it is clear what the get visibility is, separate from the set visibility

Why change the behaviour of the pre-existing scope and scope_specified indexes ? Why change the behaviour of the pre-existing property_visibility and visibility_token indexes ?

So when there is asymmetric visibility, the property is not "public", "protected", or "private" in the sense of pre-asymmetric visibility meaning, so I'm trying to not change the behavior of scope and property_visibility - it says "public", "protected", or "private" iff the property is full public/protected/private, like before. How then to indicate asymmetric visibility? We could introduce 6 new values for the different combinations, but I figured that it would be simplest to just say "asymmetrical" and access the different visibilities separately.

For scope_specified, the behavior isn't changed? It is still only true iff a scope is explicitly specified

For visibility_token, since there isn't just one token anymore, reusing this with an integer value would be potentially confusing

Why are the new array indexes proposed to be only included when asym visibility is used ?

Since I figured that would have the smallest impact for sniffs that are not expecting asymmetric visibility - I care ~0 if we decide to always include the new indexes

What will the impact of these changes be on pre-existing sniffs ?

The intent is that for any pre-existing sniff, it will work 100% the same when analyzing code that doesn't use asymmetric visibility; if it does use asymmetric visibility, then the sniff probably won't break, but it depends on if it checks for the visibility to be equal to something (is this private?) or not equal (is this non-public?) since that determines how the new "asymmetrical" value would be treated.

[jrfnl]

@DanielEScherzer Thank you for your thoughts on this, but I don't think this is the way to go and I fear that the currently proposed solution for getMemberProperties() and getMethodParameters() will lead to parse errors via the fixers, as sniffs may blindly take the value of scope to use in a string send to the fixer as they could always count on the value of that array key to be a valid visibility, and your proposal breaks that.

getMemberProperties()

I'd like to propose the following alternative instead:

• The behaviour of scope and scope_specified remains unchanged and for asym visibility applies to the "get" scope. After all, no (get) scope specified is already a valid situation without asym visibility. With asym visibility, it is just likely that it will start to occur more.
• If, and only if, a "set" scope is specified, then the following extra key will be added to the return array: set_scope with a value of '[public|protected|private]'.

This way, there is no breaking change for existing sniffs. And sniffs can assert everything they need anyway:

• Is this a property using asym visibility ?

  $isAsym = isset($memberProperties['set_scope']);
  

• Is the get scope specified ?

  if ($memberProperties['scope_specified'] === true) {...}
  

getMethodParameter()

Basically, I'd say this should behave the same, with the caveat that that method provides token positions too. So:

• The behaviour of property_visibility and visibility_token remains unchanged and for asym visibility applies to the "get" scope.
• If, and only if, a "set" scope is specified, then the following extra keys will be added to the return array: property_set_visibility with a value of '[public|protected|private]' and set_visibility_token (int).

Is there any reason you can think of why the above would not be sufficient ?

---

Adding the community cc-list for additional opinions on our competing proposals:

/cc @asispts @dingo-d @fredden @GaryJones @greg0ire @kukulich @michalbundyra @Ocramius @sirbrillig @stronk7 @weierophinney @wimg

[jrfnl]

I've now opened PR #1116 to implement the changes to the File methods. @DanielEScherzer would you like to review ?

[jrfnl]

I've now also opened PRs to add support for asym visibility to all sniffs which I could find would need it. Hope I didn't miss any. PRs are all linked above.