Tokenizer/PHP: arrow function tokenization broken when true/false used in return type #453
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
These two tests were testing multiple test cases, but not using a data provider, which means that if one of the test cases would fail, the ones after the failing test case wouldn't even be tested. By using a data provider, that issue is avoided. It also allows for simplifying some of the test code for the `testKeywordReturnTypes()` test which duplicated the logic for the `scopePositionTestHelper()` just to allow for custom error messages mentioning the test marker. This is no longer needed when each marker is tested individually via the data provider.
…d in return type Since PHP 8.0, `false` and `null` can be included in a union return type. As of PHP 8.2, both `true`, `false` and `null` can be used as a stand-alone return type. The tokenizer layer handling arrow functions did not take this into account correctly. While `null` was handled correctly, `true` and `false` was not and would result in the arrow function `fn` keyword being tokenized as `T_STRING` across all PHP versions. As a result of that, the other typical tokenizer changes related to arrow functions (`=>` as `T_FN_ARROW`, scope/parenthesis owners etc) would also not be executed correctly. In practice, I suspect few people will have run into this bug as, after all, what's the point of declaring an arrow function which will only ever return `true` or `false` ? so in practice, it is likely to only have come into play for people using `true` or `false` as part of an arrow function union type. All the same, PHPCS should handle this correctly. Includes unit tests proving the bug and safeguarding the fix.
jrfnl
force-pushed
the
feature/tokenizer-php-fix-bug-arrow-functions-true-false-type
branch
from
3f5e10a
to
bd6356c
Compare
|
Rebased without changes. Merging once the build passes. |
jrfnl
deleted the
feature/tokenizer-php-fix-bug-arrow-functions-true-false-type
branch
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Description
BackfillFnTokenTest: use data providers when appropriate
These two tests were testing multiple test cases, but not using a data provider, which means that if one of the test cases would fail, the ones after the failing test case wouldn't even be tested.
By using a data provider, that issue is avoided.
It also allows for simplifying some of the test code for the
testKeywordReturnTypes()test which duplicated the logic for thescopePositionTestHelper()just to allow for custom error messages mentioning the test marker. This is no longer needed when each marker is tested individually via the data provider.Tokenizer/PHP: arrow function tokenization broken when true/false used in return type
Since PHP 8.0,
falseandnullcan be included in a union return type.As of PHP 8.2, both
true,falseandnullcan be used as a stand-alone return type.The tokenizer layer handling arrow functions did not take this into account correctly. While
nullwas handled correctly,trueandfalsewas not and would result in the arrow functionfnkeyword being tokenized asT_STRINGacross all PHP versions.As a result of that, the other typical tokenizer changes related to arrow functions (
=>asT_FN_ARROW, scope/parenthesis owners etc) would also not be executed correctly.In practice, I suspect few people will have run into this bug as, after all, what's the point of declaring an arrow function which will only ever return
trueorfalse? so in practice, it is likely to only have come into play for people usingtrueorfalseas part of an arrow function union type.All the same, PHPCS should handle this correctly.
Includes unit tests proving the bug and safeguarding the fix.
Suggested changelog entry
Fixed broken arrow function tokenization when the return type contained
trueorfalseTypes of changes