PHP 8.5 | Tokenizer/PHP: properly fix "Using null as an array offset" deprecation #1230
Conversation
If a comment is on its own line, the new line token is merged into the comment token, and the new line is skipped by setting it to `null`. Where the next line contains incomplete, or invalid, code which ends in an nullsafe operator (for example `$obj?`), the tokenizer will step backwards until it finds the next non-empty line. The skipped new line token should be skipped during this parsing. This can only occur during live coding or when a file has a parse error, but PHPCS should handle that situation gracefully. Fixed now. Fixes PHPCSStandards#1216. This change is already covered via the existing tests. Ref: https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_using_values_null_as_an_array_offset_and_when_calling_array_key_exists
…n array offset" deprecation" This reverts commit 6b82a86 / PR 1215 as it is no longer needed.
andrewnicols
changed the title
|
I've left feedback about this PR in the underlying ticket: #1216 (comment) |
… deprecation
If a slash/hash comment is on its own line, the new line token is merged into the comment token for PHP 8.0+, and the new line is skipped by setting it to `null`.
The PHP Tokenizer did not take the possibility of a token being set to `null` into account when retokening a `?` to either `T_NULLABLE` or `T_INLINE_THEN`, leading to the PHP 8.5 deprecation notice.
In that case, the sniff first looks forward to see if we can draw a conclusion based on the non-empty tokens following the `?` and if not, walks backward to look at the tokens before the `?`.
The problem occurs when a `null` token exists in the sequence before the `?`.
This `null` token will only have been injected with a specific code sequence: when there is a slash/hash comment followed by a new line in the token sequence before the `?` and there is no indentation/new line whitespace on the next line.
Also see the detailed analysis by andrewnicols in PHPCSStandards/PHP_CodeSniffer 1216#issuecomment-3274198443
There are only two situations in which this causes the tokenizer to hit the PHP 8.5 "Using null as an array offset" deprecation notice:
1. If the `?` token is the last token in the file (live coding - the issue was discovered via a test related to live coding).
2. If there are tokens after the `?`, but not tokens which allows us to draw a conclusion (yet) and there is a slash/hash comment between the `?` and the previous non-empty token.
This commit:
1. Adds dedicated tests for both situations.
2. Adds a new fix for situation [1] as if there are no tokens after a `?`, we cannot determine definitively whether the `?` is a nullable operator or an inline then. For BC, this should be tokenized as `T_INLINE_THEN`.
3. Makes a small tweak to the previously added fix which addressed situation [2].
Alternatively, we could consider switching to using the "$finalTokens" token stack to do the token walking instead, but as that is a bigger change and this part of the code has nowhere near sufficient tests, that change is left for later.
Fixes 1216
|
@andrewnicols I've taken the liberty to update this PR now to make it ready for merge and inclusion in the 3.13.5 release. Please let me know if you have questions about the additional changes I made. If I don't hear anything, I will merge this tomorrow. |
There was a problem hiding this comment.
This change looks good to me, though I do wonder if it would be better to have a new token to indicate incomplete code at some point.
Sorry for tkaing so long to get back to you. I spent most of September and half of October flying between England and Australia for personal reasons and I'm still catching up on everything I missed.
Thanks for looking it over.
Interesting idea... I'm not sure it would be doable, I mean, what with PHPCS being used in IDEs, there are so many flavours of potential parse errors/incomplete code, but might be worth opening an issue about to think and talk it through some more.
No worries. Life happens and sounds like you set the right priorities. 👍🏻 |
I've created #1303. Completely understand if it isn't possible, just a thought. |
jrfnl
changed the title
2 participants
Description
Fixes #1215 / #1216
Suggested changelog entry
Ignore skipped tokens in tokenizer.
Related issues/external references
Fixes #1215 / #1216
Types of changes
PR checklist