Avoid PHP error on parse error in PSR12.Functions.ReturnTypeDeclaration

[fredden]

Description

When attempting to process a file with a git merge conflict marker with the PSR12 standard, there is a PHP notice (with phpcs) or PHP Fatal error (with phpcbf) due to an unvalidated assumption in the PSR12.Functions.ReturnTypeDeclaration sniff that there will be a colon in the token array when File::getMethodProperties() detects a return type.

$ phpcs --version
PHP_CodeSniffer version 4.0.1 (stable) by Squiz and PHPCSStandards
$
$ phpcs --standard=PSR12 --basepath=. src/Standards/Generic/Tests/Arrays/DisallowLongArraySyntaxUnitTest.2.inc

FILE: src/Standards/Generic/Tests/Arrays/DisallowLongArraySyntaxUnitTest.2.inc
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  1 | ERROR | [x] Header blocks must be separated by a single blank line
  1 | ERROR | [ ] An error occurred during processing; checking has been aborted. The error message was: Undefined array key -1 in
    |       |     ...../vendor/squizlabs/php_codesniffer/src/Standards/PSR12/Sniffs/Functions/ReturnTypeDeclarationSniff.php on line 87
    |       |     The error originated in the PSR12.Functions.ReturnTypeDeclaration sniff on line 87.
 11 | ERROR | [ ] There must be a single space between the colon and type in a return type declaration
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 35ms; Memory: 8MB
$
$ phpcbf --standard=PSR12 --basepath=. src/Standards/Generic/Tests/Arrays/DisallowLongArraySyntaxUnitTest.2.inc
PHP Fatal error:  Uncaught PHP_CodeSniffer\Exceptions\RuntimeException: Undefined array key -1 in ...../vendor/squizlabs/php_codesniffer/src/Standards/PSR12/Sniffs/Functions/ReturnTypeDeclarationSniff.php on line 87 in ...../vendor/squizlabs/php_codesniffer/src/Runner.php:543
Stack trace:
#0 ...../vendor/squizlabs/php_codesniffer/src/Standards/PSR12/Sniffs/Functions/ReturnTypeDeclarationSniff.php(87): PHP_CodeSniffer\Runner->handleErrors()
#1 ...../vendor/squizlabs/php_codesniffer/src/Files/File.php(497): PHP_CodeSniffer\Standards\PSR12\Sniffs\Functions\ReturnTypeDeclarationSniff->process()
#2 ...../vendor/squizlabs/php_codesniffer/src/Files/LocalFile.php(90): PHP_CodeSniffer\Files\File->process()
#3 ...../vendor/squizlabs/php_codesniffer/src/Fixer.php(181): PHP_CodeSniffer\Files\LocalFile->process()
#4 ...../vendor/squizlabs/php_codesniffer/src/Reports/Cbf.php(59): PHP_CodeSniffer\Fixer->fixFile()
#5 ...../vendor/squizlabs/php_codesniffer/src/Reporter.php(368): PHP_CodeSniffer\Reports\Cbf->generateFileReport()
#6 ...../vendor/squizlabs/php_codesniffer/src/Runner.php(627): PHP_CodeSniffer\Reporter->cacheFileReport()
#7 ...../vendor/squizlabs/php_codesniffer/src/Runner.php(390): PHP_CodeSniffer\Runner->processFile()
#8 ...../vendor/squizlabs/php_codesniffer/src/Runner.php(208): PHP_CodeSniffer\Runner->run()
#9 ...../vendor/squizlabs/php_codesniffer/bin/phpcbf(30): PHP_CodeSniffer\Runner->runPHPCBF()
#10 ...../vendor/bin/phpcbf(119): include('...')
#11 {main}
  thrown in ...../vendor/squizlabs/php_codesniffer/src/Runner.php on line 543
$

I initially opened #1350 to change the output of File::getMethodProperties(). While some discussion continues in that pull request, this pull request should unblock anyone attempting to use this sniff during a merge conflict.

Suggested changelog entry

Added defensive coding to PSR12.Functions.ReturnTypeDeclaration to avoid PHP errors.

Related issues/external references

Relates to #152

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.
• I have opened a sister-PR in the documentation repository to update the Wiki.

[jrfnl]

Thanks for setting this up @fredden !

I've left some questions in-line.

I also wonder (but haven't looked in detail yet) whether the code block on (new) line 70-90 could be simplified a little what with the $colon variable being set and known ahead of that code block.

Let me know what you think.

[fredden]

I also wonder (but haven't looked in detail yet) whether the code block on (new) line 70-90 could be simplified a little what with the $colon variable being set and known ahead of that code block.

I rewrote the block with this in mind, but don't find it more legible than the original. There is precedent in this sniff to check the 'code' rather than the pointer for the closing parenthesis in the following block (new line 92).

- if ($tokens[($colon - 1)]['code'] !== T_CLOSE_PARENTHESIS) {
+ if (($colon - 1) !== $tokens[$stackPtr]['parenthesis_closer']) {

 if ($tokens[($returnType - 1)]['code'] !== T_WHITESPACE
     || $tokens[($returnType - 1)]['content'] !== ' '
-    || $tokens[($returnType - 2)]['code'] !== T_COLON
+    || ($returnType - 2) !== $colon
 ) {

I'm happy to adjust according to your steer if you'd like.

[jrfnl]

I also wonder (but haven't looked in detail yet) whether the code block on (new) line 70-90 could be simplified a little what with the $colon variable being set and known ahead of that code block.

I rewrote the block with this in mind, but don't find it more legible than the original. There is precedent in this sniff to check the 'code' rather than the pointer for the closing parenthesis in the following block (new line 92).

@fredden Thanks for looking into this. I agree that the first one doesn't improve readability, though the second may and that condition also has a duplicate in two more places in that block. Not a blocker either way. Whatever you decide will work.