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

PHP 8.0 | Support union types in PHP Tokenizer and File class utility methods #3032

Merged
merged 10 commits into from Nov 2, 2020

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Jul 23, 2020

Implements support for PHP 8.0 union types in the PHP tokenizer and the File::getMethodParameters(), File::getMethodProperties() and the File::getMemberProperties() methods.

Note: Individual sniffs which examine the parameter/return/property type returned by those methods will have to implement support for union types by using explode('|', ltrim($type, '?')) on the type returned by these methods to get to the individual types or walk the tokens from the start of the type to the end of the type.

Ref: https://wiki.php.net/rfc/union_types_v2

Implementation as discussed in #2968. Partially fixes #2968.

Commit details

Tokens: add new T_TYPE_UNION token

... which will indicate the | character in PHP 8.0 union types.

Tokenizer/PHP: tokenize the "|" for union types as T_TYPE_UNION

This adds a new block of logic to the PHP::processAdditional() method which changes the token code and type of T_BITWISE_OR | tokens in type declarations to T_TYPE_UNION.

As the PHP::processAdditional() method walks backwards through the token stack, the arrow function backfill will not have been done yet, so for those some special conditions have been put in place.

I've tried to limit the token walking within the new block as much as possible while still maintaining accuracy.

This includes changing all union type operators in a single type declaration in one go, instead of on each individual T_BITWISE_OR token, which prevents the same logic having to be executed multiple times for multi-union types like int|float|null.

Includes dedicated unit tests.

Tokenizer/PHP: array return type keyword to T_STRING vs PHP 8 union types

array keywords used as return types in PHP 8 union types would only be correctly changed to T_STRING if they were the first type in the union.

Fixed now.

Includes adding T_STATIC to the array of allowed tokens. While previously it wasn't an issue that the token was not included in the array, it is necessary for the token to be there to support union types.

This change will be tested via the union type related tests for the File::getMethodProperties() method.

Tokenizer/PHP: arrow function backfill vs PHP8 union types

As the PHP::processAdditional() method walks backwards through the file, by the time the fn keyword backfill logic is hit, any union type | tokens will have already been converted to T_TYPE_UNION.

So, to make the arrow function backfill compatible with PHP 8 union types, the T_TYPE_UNION token needs to be added to the "allowed tokens" ($ignore) array.

Includes unit tests.

File::getMethodParameters(): add support for "union" parameter types

This adds support for union types to the File::getMethodParameters() method.

Includes extensive unit tests.

File::getMethodProperties(): add support for "union" return types

This adds support for union types to the File::getMethodProperties() method.

Includes extensive unit tests.

File::getMemberProperties(): add support for "union" types

This adds support for union types to the File::getMemberProperties() method.

Includes extensive unit tests.

Docs: update "nullable_type" comments to clarify meaning


[EDIT] Added one more commit

Tokenizer/PHP: add new token to the $knownLengths property


[EDIT] And another one

Tests: update the Tokenizer/UndoNamespacedNameSingleToken test

... to allow for the new T_TYPE_UNION token. (Ref: #3063)

@gsherwood gsherwood added this to Idea Bank in PHPCS v3 Development via automation Jul 23, 2020
@gsherwood gsherwood added this to the 3.5.6 milestone Jul 23, 2020
@gsherwood
Copy link
Member

I've marked this for 3.5.6 as I didn't see anything that would be a BC break, but please let me know if I've missed something.

@jrfnl
Copy link
Contributor Author

jrfnl commented Jul 23, 2020

@gsherwood There's no BC-break, but it does introduce a new token, which depending on interpretation of semver could be considered a feature which should go into a minor rather than a patch version.

@jrfnl
Copy link
Contributor Author

jrfnl commented Jul 23, 2020

Oops.. just noticed some copy/pasta in the test file docblock, let me quickly fix that.

@jrfnl jrfnl force-pushed the feature/support-union-types branch from 6586584 to 95afd97 Compare July 23, 2020 23:22
@jrfnl
Copy link
Contributor Author

jrfnl commented Jul 23, 2020

Fixed now.

@gsherwood
Copy link
Member

but it does introduce a new token

Indeed it does. It should be in 3.6.0. Thanks.

@gsherwood gsherwood modified the milestones: 3.5.6, 3.6.0 Jul 23, 2020
@jrfnl
Copy link
Contributor Author

jrfnl commented Jul 23, 2020

Looks like one of the builds stalled and needs to be retriggered.

... which will indicate the `|` character in PHP 8.0 union types.
…NION

This adds a new block of logic to the `PHP::processAdditional()` method which changes the token code and type of `T_BITWISE_OR` `|` tokens in type declarations to `T_TYPE_UNION`.

As the `PHP::processAdditional()` method walks backwards through the token stack, the arrow function backfill will not have been done yet, so for those some special conditions have been put in place.

I've tried to limit the token walking within the new block as much as possible while still maintaining accuracy.

This includes changing all union type operators in a single type declaration in one go, instead of on each individual `T_BITWISE_OR` token, which prevents the same logic having to be executed multiple times for multi-union types like `int|float|null`.

Includes dedicated unit tests.

Ref: https://wiki.php.net/rfc/union_types_v2
…8 union types

`array` keywords used as return types in PHP 8 union types would only be correctly changed to `T_STRING` if they were the first type in the union.

Fixed now.

Includes adding `T_STATIC` to the array of allowed tokens. While previously it wasn't an issue that the token was not included in the array, it is necessary for the token to be there to support union types.

This change will be tested via the union type related tests for the `File::getMethodProperties()` method.
As the `PHP::processAdditional()` method walks backwards through the file, by the time the `fn` keyword backfill logic is hit, any union type `|` tokens will have already been converted to `T_TYPE_UNION`.

So, to make the arrow function backfill compatible with PHP 8 union types, the `T_TYPE_UNION` token needs to be added to the "allowed tokens" (`$ignore`) array.

Includes unit tests.
…ter types

This adds support for union types to the `File::getMethodParameters()` method.

Includes extensive unit tests.
… types

This adds support for union types to the `File::getMethodProperties()` method.

Includes extensive unit tests.
This adds support for union types to the `File::getMemberProperties()` method.

Includes extensive unit tests.
@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 20, 2020

Rebased for merge conflicts with merged PR #3066

... to allow for the new `T_TYPE_UNION` token.
gsherwood added a commit that referenced this pull request Nov 2, 2020
@gsherwood gsherwood merged commit ba4956b into squizlabs:master Nov 2, 2020
PHPCS v3 Development automation moved this from Idea Bank to Ready for Release Nov 2, 2020
@gsherwood
Copy link
Member

Merged 🎉 Thanks so much!

This has gone and closed #2968 as well, which I think is okay now as everything mentioned on there is done. The sniffs themselves will need changes, but those feel like different issues for me to look at later. Can you check my logic on that please.

@jrfnl jrfnl deleted the feature/support-union-types branch November 2, 2020 05:59
@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 2, 2020

This has gone and closed #2968 as well, which I think is okay now as everything mentioned on there is done. The sniffs themselves will need changes, but those feel like different issues for me to look at later. Can you check my logic on that please.

@gsherwood #2968 shouldn't be closed yet, but then again, the two remaining PRs I had ready locally for that issue have now been pulled:

Based on my earlier examination, the only other PHPCS native sniff which will need adjusting is the Squiz.Commenting.FunctionComment sniff, which I haven't done any work on.

I might, of course, have overlooked a sniff, but if that's the case, no doubt someone will report it soon enough 😄

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

Successfully merging this pull request may close these issues.

Support PHP 8's mixed type
2 participants