Tokenizer/PHP: investigate root cause of a particular PHP 8.5 deprecation notice

[jrfnl]

Commit 6b82a86 in PR #1215 temporarily silences a PHP 8.5 "Using null as an array offset" deprecation in the Tokenizer layer dealing with the ? character.

As per the commit message:

When running the NullsafeObjectOperatorTest, the "Using null as an array offset" deprecation gets triggered in the tokenizer layer handling re-tokenization to T_NULLABLE and/or T_INLINE_THEN.

This should only be possible if the below code at the top of the loop would result in $tokenType being null, which shouldn't be possible....

PHP_CodeSniffer/src/Tokenizers/PHP.php

Lines 2254 to 2258 in f1c9394

	if (is_array($tokens[$i]) === true) {
	$tokenType = $tokens[$i][0];
	} else {
	$tokenType = $tokens[$i];
	}

With this conundrum in mind, I'm electing to (temporarily) silence the deprecation notice for now until there is more time to investigate in more depth.

Ref: https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_using_values_null_as_an_array_offset_and_when_calling_array_key_exists

This issue is a reminder to investigate the above before PHP 9.0 comes round and to determine a permanent solution.

As that part of the Tokenizer is not sufficiently covered by dedicated tests, more tests will need to be put in place as well to validate and safeguard the permanent solution.

[andrewnicols]

I'm on a long train journey and thought I'd take a look at this, but after reverting #1215 I still can't reproduce this on PHP 8.5.

[jrfnl]

@andrewnicols I appreciate you taking a look. Which version of PHP 8.5 did you use to test ? The deprecation was only merged last week, so if you are using pre-compiled versions of PHP 8.5, you'd need PHP 8.5-beta3, which was only tagged today (and won't be officially released until Thursday).

[andrewnicols]

Ah thanks - I've got an older version at the moment. I'll see about installing a newer version and looking when I get a chance.

[jrfnl]

@andrewnicols I just installed beta3 to investigate another one of these in the 4.x branch. Will update when I know more (just in case they turn out to be related).

[jrfnl]

@andrewnicols Update: the issue in 4.x is in the same "layer" of the Tokenizer, but it is not the same issue.

I'll add the tests to cover the 4.x change to both the master and the 4.x branch.

[jrfnl]

FYI: I pulled the tests for the issue I was looking into in #1225 and the fix for 4.x in #1226.

The above still needs investigation as it is not the same issue.

[andrewnicols]

Okay, so I've tracked the creation of the null to

PHP_CodeSniffer/src/Tokenizers/PHP.php

Lines 889 to 906 in 6b82a86

	if (PHP_VERSION_ID >= 80000
	&& $tokenIsArray === true
	&& ($token[0] === T_COMMENT && (strpos($token[1], '//') === 0 || strpos($token[1], '#') === 0))
	&& isset($tokens[($stackPtr + 1)]) === true
	&& is_array($tokens[($stackPtr + 1)]) === true
	&& $tokens[($stackPtr + 1)][0] === T_WHITESPACE
	) {
	$nextToken = $tokens[($stackPtr + 1)];
	// If the next token is a single new line, merge it into the comment token
	// and set to it up to be skipped.
	if ($nextToken[1] === "\n" || $nextToken[1] === "\r\n" || $nextToken[1] === "\n\r") {
	$token[1] .= $nextToken[1];
	$tokens[($stackPtr + 1)] = null;
	if (PHP_CODESNIFFER_VERBOSITY > 1) {
	echo "\t\t* merged newline after comment into comment token $stackPtr".PHP_EOL;
	}

The comment above the change is:

                // If the next token is a single new line, merge it into the comment token
                // and set to it up to be skipped.

The code being parsed at that point is this:

// Intentional parse error. This has to be the last test in the file.
echo $obj?

Later on, when we get to the ? on the final line it's looking for the next non-empty line:

PHP_CodeSniffer/src/Tokenizers/PHP.php

Lines 2186 to 2191 in 6b82a86

	/*
	* Check if the next non-empty token is one of the tokens which can be used
	* in type declarations. If not, it's definitely a ternary.
	* At this point, the only token types which need to be taken into consideration
	* as potential type declarations are identifier names, T_ARRAY, T_CALLABLE and T_NS_SEPARATOR.
	*/

There is a break there but we never get to it because it's linting invalid code at the end of the file. There is no next non-empty line.

So instead it works backwards to look for a nullable type or ternary:

PHP_CodeSniffer/src/Tokenizers/PHP.php

Lines 2255 to 2259 in 6b82a86

	for ($i = ($stackPtr - 1); $i >= 0; $i--) {
	if (is_array($tokens[$i]) === true) {
	$tokenType = $tokens[$i][0];
	} else {
	$tokenType = $tokens[$i];

But none of the previous tokens before the ? meet any of the break conditions:

• if ($tokenType === ':' || $tokenType === ',' || $tokenType === T_ATTRIBUTE_END) {

• PHP_CodeSniffer/src/Tokenizers/PHP.php

  Lines 2288 to 2294 in 6b82a86

  	if ($tokenType === T_FUNCTION
  	|| $tokenType === T_FN
  	|| @isset(Tokens::$methodPrefixes[$tokenType]) === true
  	|| @isset(Tokens::$scopeModifiers[$tokenType]) === true
  	|| $tokenType === T_VAR
  	|| $tokenType === T_READONLY
  	) {

• PHP_CodeSniffer/src/Tokenizers/PHP.php

  Line 2302 in 6b82a86

  } else if (in_array($tokenType, [T_DOUBLE_ARROW, T_OPEN_TAG, T_OPEN_TAG_WITH_ECHO, '=', '{', ';'], true) === true) {

So the combination of a comment on the line before, and the invalid code, and none of the other lines matching a break, mean that we hit the null token, which we expect to skip.

I'm not sure what the best solution is in this case. You know the code far better than me and the intent. I'm about to jump on a longhaul flight for the next 19 hours.

It feels wrong to add a specific check here for the value being null, but that may be the best solution in this case.

[jrfnl]

@andrewnicols Thank you for doing that analysis. I've followed your steps and agree with your conclusion about the cause.

Makes me wonder if there are other places in the Tokenizer which needs additional defensive coding for $tokens[$stackPtr] potentially being null, but that's for another ticket.

I'm not sure what the best solution is in this case.

It feels wrong to add a specific check here for the value being null, but that may be the best solution in this case.

I concur that it feels wrong to explicitly check for null here and I believe it can be avoided.

This is about determining whether a "?" character is a T_NULLABLE token or a T_INLINE_THEN.

Later on, when we get to the ? on the final line it's looking for the next non-empty line:

There is a break there but we never get to it because it's linting invalid code at the end of the file. There is no next non-empty line.

So instead it works backwards to look for a nullable type or ternary:

I think 👆🏻 this is the crucial part.

We first loop forward and if we didn't reach a conclusion, we loop backwards, but if the "?" is the last non-empty token in the file, I think we can already draw a conclusion after that first forward-loop, namely that we can never determine what the ? represents for definite, so we should short-circuit to the T_INLINE_THEN conclusion (= maintain existing behaviour).

So, I would suggest a check between the first (forward) loop and the second (backward) loop to see if the first loop reached the end of the file ($i === $numTokens) and if so, set the token to T_INLINE_THEN and continue to prevent it from even entering the second loop.

How does that sound to you ? I've locally tested a fix like described above and it seems to hold up fine.

If you like I could update your PR with the code I have locally (and add an extra test), though I'm also perfectly happy to let you fix it up further. Just let me know what you'd prefer.