PHPCS 4.x | Tokenizer: fix stray "parenthesis_..." indexes being set for non-closure use #3104
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 5, 2020
jrfnl
force-pushed
the
phpcs-4.x/tokenizer-fix-parenthesis-owners-for-use
branch
from
2fb2981 to
1bd5a12
Compare
Commit 08824f3 added support for `T_USE` tokens for closures being parentheses owners. However, the net effect was that *every* `T_USE` token - including import/trait use statement `T_USE` tokens, would have the `parenthesis_owner`, `parenthesis_opener` and `parenthesis_closer` indexes set. For closure `use` statements, those were set correctly. However, for import/trait `use` statements, the `parenthesis_owner` would point to the `T_USE` keyword which doesn't have parentheses and the `parenthesis_opener` and `parenthesis_closer` indexes would be `null` in most cases, but they would still be set. Also, in some cases, the `parenthesis_opener` and `parenthesis_closer` indexes for import/trait `use` statements would incorrectly be set if to arbitrary, unrelated parenthesis, if no open curly or open square bracket was encountered between the `use` statement and the next set of parenthesis. That makes the `parenthesis_...` indexes harder and less intuitive to work with as any of the `parenthesis_...` indexes could be set, even when the token has no parenthesis. I've fixed this now by: * Not just resetting the `$openOwner` variable when we know there won't be any parenthesis, but by also resetting the `parenthesis_...` indexes of the incorrectly set owner. * Doing the reset on more tokens, including doing it on a `T_OPEN_USE_GROUP` token, as the `tokenize()` method has already run, so group use tokens have already been retokenized to their dedicated token. I've also added a few `continue` statements to skip the rest of the code within the loop when we know in advance none of the other conditions would match anyway. Includes adding dedicated unit tests verifying the correct setting of the `parenthesis_...` indexes.
jrfnl
force-pushed
the
phpcs-4.x/tokenizer-fix-parenthesis-owners-for-use
branch
from
1bd5a12 to
44fba8d
Compare
|
Rebased yet again for merge conflicts. @gsherwood Would be great if this PR could get some attention as this bug is currently the main reason why external standards will fail against PHPCS 4.x when running tests. |
|
Sorry, merged now. Thanks a lot for fixing this up. |
|
@gsherwood Thanks for that! |
This was referenced May 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit 08824f3 (issue #2593) added support for
T_USEtokens for closures being parentheses owners.However, the net effect was that every
T_USEtoken - including import/trait use statementT_USEtokens, would have theparenthesis_owner,parenthesis_openerandparenthesis_closerindexes set.For closure
usestatements, those were set correctly.However, for import/trait
usestatements, theparenthesis_ownerwould point to theT_USEkeyword which doesn't have parentheses and theparenthesis_openerandparenthesis_closerindexes would benullin most cases, but they would still be set.Also, in some cases, the
parenthesis_openerandparenthesis_closerindexes for import/traitusestatements would incorrectly be set if to arbitrary, unrelated parenthesis, if no open curly or open square bracket was encountered between theusestatement and the next set of parenthesis.That makes the
parenthesis_...indexes harder and less intuitive to work with as any of theparenthesis_...indexes could be set, even when the token has no parenthesis.I've fixed this now by:
$openOwnervariable when we know there won't be any parenthesis, but by also resetting theparenthesis_...indexes of the incorrectly set owner.T_OPEN_USE_GROUPtoken, as thetokenize()method has already run, so group use tokens have already been retokenized to their dedicated token.Note: There may be some more tokens which can be added to this list, but this should be a reasonable start.
I've also added a few
continuestatements to skip the rest of the code within the loop when we know in advance none of the other conditions would match anyway.Includes adding dedicated unit tests verifying the correct setting of the
parenthesis_...indexes.