Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No support for namespace operator used in type declarations #3066

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Aug 30, 2020

Follow up on #3063 (comment)

The namespace keyword as an operator is perfectly valid to be used as part of a type declaration.
See: https://3v4l.org/hvjlL

This usage was so far not supported in the tokenizer for the determination whether a ? is a nullable type token or a ternary, nor was it taken into account for skipping over the return type for an arrow function in the backfill.

Lastly, this usage was so far not supported in the File::getMethodParameters(), File::getMethodProperties() and the File::getMemberProperties() methods.

Fixed now.

Includes select unit tests covering the functionality.


[Edit]
I've added an additional commit fixing the Tokenizer::recurseScopeMap() method as when the namespace keyword used as an operator was encountered, the scope openers/closers would incorrectly not be set.

The namespace keyword as a scope opener can never be within another scope, so we can safely ignore it when encountered as it will always be the namespace keyword used as an operator in that case.

Includes dedicated unit tests.

@jrfnl jrfnl force-pushed the feature/bugfixes-namespace-operator-in-typedeclarations branch 2 times, most recently from 632ba23 to 1faa467 Compare August 31, 2020 02:24
@gsherwood gsherwood added this to Idea Bank in PHPCS v3 Development via automation Aug 31, 2020
@gsherwood gsherwood added this to the 3.5.7 milestone Aug 31, 2020
@gsherwood
Copy link
Member

@jrfnl I've marked this for the 3.5.7 release but I'd appreciate it if you could give me a second opinion on the risk. I'm wondering if this is better included in 3.6.0.

@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 1, 2020

@gsherwood I don't see much risk here as this is a straight bug fix: valid PHP not being tokenized correctly in PHPCS.

Running the unit tests of various external standards with this branch is successful, but as this syntax was so far basically unsupported by PHPCS, that's not really surprising as they probably don't have any tests in place covering this syntax.

All the same, if you deem it risky, I'm fine with it being marked for PHPCS 3.6.0.
Note: this PR should be merged + merged to PHPCS 4.x, before I can pull the PHPCS 4.x change to backfill the PHP 8.0 "namespaced names as single token". This issue was discovered while working on that change. (see #3041)

As for the impact:

  • The namespace keyword is rarely used as an operator in PHP code. While it is perfectly valid PHP, most people don't know about it and don't use it, so even if a few sniffs would start misbehaving (which I don't expect), the impact would be limited to the few projects out there which do use it.
  • Sniffs which bowed out when a class/function etc does not have a scope opener set (live coding/parse error), will now do so less often as there is one less reason for the scope opener not to be set. So, in effect, they will function correctly more often.
  • Sniffs which examine type declarations using the utility methods in File will now receive the correct type declaration when the namespace keyword as an operator is used in the declaration.
    Previously, they would receive an empty value for the "type". or possibly even just the nullable operator without the actual type.
    If the sniff would walk the tokens of the type, the sniff should probably be adjusted to allow for the T_NAMESPACE token, but if the sniff is not adjusted, in most cases, it will just work the same as before, which is typically - bow out when an unexpected token/type declaration value is encountered.
  • Sniffs which do namespace resolution would need to take the namespace keyword into account for type declarations and extends/implements, but this is barely done in PHPCS itself and rarely in external standards.

Future scope, building on top of this change:

  • Adjust the File::findExtendedClassName() method to take the namespace operator into account.
  • Adjust the File::findImplementedInterfaceNames() method to take the namespace operator into account.
  • Adjust various sniffs to take the namespace keyword into account for type declarations and extends/implements.

Note: the changes mentioned in the "Future scope" would only be relevant for PHPCS 3.x as PHPCS 4.x will use different tokens - per #3041 -, which is why I haven't made those changes.

The `namespace` keyword as an operator is perfectly valid to be used as part of a type declaration.

This usage was so far not supported in the tokenizer for the determination whether a `?` is a nullable type token or a ternary.

Fixed now.
…tions

The `namespace` keyword as an operator is perfectly valid to be used as part of a type declaration.

This usage was so far not supported in the tokenizer for the arrow function backfill.

Fixed now.
…mespace relative type declarations

The `namespace` keyword as an operator is perfectly valid to be used as part of a type declaration.

This usage was so far not supported in the `File::getMethodParameters()`, `File::getMethodProperties()` and the `File::getMemberProperties()` methods.

Fixes now. Including adding a unit test for each.
@jrfnl jrfnl force-pushed the feature/bugfixes-namespace-operator-in-typedeclarations branch from 1faa467 to 16d2cd1 Compare September 2, 2020 00:12
The namespace keyword as a scope opener can never be within another scope, so we can safely ignore it when encountered as it will always be the namespace keyword used as an operator in that case.

Includes dedicated unit tests.
@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 2, 2020

Rebased for merge conflicts.

@gsherwood gsherwood changed the title Tokenizer & File utilities: bugfixes for namespace operator in typedeclarations No support for namespace operator used in type declarations Sep 20, 2020
gsherwood added a commit that referenced this pull request Sep 20, 2020
@gsherwood gsherwood merged commit 4db6a74 into squizlabs:master Sep 20, 2020
PHPCS v3 Development automation moved this from Idea Bank to Ready for Release Sep 20, 2020
@gsherwood
Copy link
Member

I didn't know that was allowed - thanks for finding and fixing it. Have picked this into the 4.0 branch as well.

@jrfnl jrfnl deleted the feature/bugfixes-namespace-operator-in-typedeclarations branch September 20, 2020 22:52
@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 20, 2020

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
PHPCS v3 Development
Ready for Release
Development

Successfully merging this pull request may close these issues.

None yet

2 participants