From 9f63afb93a7a626c44b0213208a24f778c9ab0ab Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 29 Mar 2024 04:50:56 +0100 Subject: [PATCH] BCFile/FunctionDeclarations::get[Method]Properties(): bug fix - skip over closure use statements Sister-PR to upstream PR PHPCSStandards/PHP_CodeSniffer 421 The short of it is that the `File::getMethodProperties()` method previously did not skip over closure `use` statements when walking the tokens to find the return type. As closure `use` statements can only contain variables, the only tokens which could be encountered in a closure `use` statement, which could also be encountered in a return type declaration are the `T_STRING`, `T_SELF`, `T_PARENT` and the `T_STATIC` tokens in the case when the variable being imported is a class property, i.e. the property name part of `$this->name` or `self` in `self::$name`. Either way, this patch fixes the issue. Includes unit tests. --- PHPCSUtils/BackCompat/BCFile.php | 144 +++++++++++++++++- PHPCSUtils/Utils/FunctionDeclarations.php | 19 +++ .../BCFile/GetMethodPropertiesTest.inc | 18 +++ .../BCFile/GetMethodPropertiesTest.php | 144 ++++++++++++++++++ 4 files changed, 323 insertions(+), 2 deletions(-) diff --git a/PHPCSUtils/BackCompat/BCFile.php b/PHPCSUtils/BackCompat/BCFile.php index 14c6adf6..87ccd0dc 100644 --- a/PHPCSUtils/BackCompat/BCFile.php +++ b/PHPCSUtils/BackCompat/BCFile.php @@ -462,7 +462,7 @@ public static function getMethodParameters(File $phpcsFile, $stackPtr) * * Changelog for the PHPCS native function: * - Introduced in PHPCS 0.0.5. - * - The upstream method has received no significant updates since PHPCS 3.9.0. + * - PHPCS 3.9.1: bug fix for closure use statements vs return types. PHPCS #421. * * @see \PHP_CodeSniffer\Files\File::getMethodProperties() Original source. * @see \PHPCSUtils\Utils\FunctionDeclarations::getProperties() PHPCSUtils native improved version. @@ -480,7 +480,147 @@ public static function getMethodParameters(File $phpcsFile, $stackPtr) */ public static function getMethodProperties(File $phpcsFile, $stackPtr) { - return $phpcsFile->getMethodProperties($stackPtr); + $tokens = $phpcsFile->getTokens(); + + if ($tokens[$stackPtr]['code'] !== T_FUNCTION + && $tokens[$stackPtr]['code'] !== T_CLOSURE + && $tokens[$stackPtr]['code'] !== T_FN + ) { + throw new RuntimeException('$stackPtr must be of type T_FUNCTION or T_CLOSURE or T_FN'); + } + + if ($tokens[$stackPtr]['code'] === T_FUNCTION) { + $valid = [ + T_PUBLIC => T_PUBLIC, + T_PRIVATE => T_PRIVATE, + T_PROTECTED => T_PROTECTED, + T_STATIC => T_STATIC, + T_FINAL => T_FINAL, + T_ABSTRACT => T_ABSTRACT, + T_WHITESPACE => T_WHITESPACE, + T_COMMENT => T_COMMENT, + T_DOC_COMMENT => T_DOC_COMMENT, + ]; + } else { + $valid = [ + T_STATIC => T_STATIC, + T_WHITESPACE => T_WHITESPACE, + T_COMMENT => T_COMMENT, + T_DOC_COMMENT => T_DOC_COMMENT, + ]; + } + + $scope = 'public'; + $scopeSpecified = false; + $isAbstract = false; + $isFinal = false; + $isStatic = false; + + for ($i = ($stackPtr - 1); $i > 0; $i--) { + if (isset($valid[$tokens[$i]['code']]) === false) { + break; + } + + switch ($tokens[$i]['code']) { + case T_PUBLIC: + $scope = 'public'; + $scopeSpecified = true; + break; + case T_PRIVATE: + $scope = 'private'; + $scopeSpecified = true; + break; + case T_PROTECTED: + $scope = 'protected'; + $scopeSpecified = true; + break; + case T_ABSTRACT: + $isAbstract = true; + break; + case T_FINAL: + $isFinal = true; + break; + case T_STATIC: + $isStatic = true; + break; + } + } + + $returnType = ''; + $returnTypeToken = false; + $returnTypeEndToken = false; + $nullableReturnType = false; + $hasBody = true; + $returnTypeTokens = Collections::returnTypeTokens(); + + if (isset($tokens[$stackPtr]['parenthesis_closer']) === true) { + $scopeOpener = null; + if (isset($tokens[$stackPtr]['scope_opener']) === true) { + $scopeOpener = $tokens[$stackPtr]['scope_opener']; + } + + for ($i = $tokens[$stackPtr]['parenthesis_closer']; $i < $phpcsFile->numTokens; $i++) { + if (($scopeOpener === null && $tokens[$i]['code'] === T_SEMICOLON) + || ($scopeOpener !== null && $i === $scopeOpener) + ) { + // End of function definition. + break; + } + + if ($tokens[$i]['code'] === T_USE) { + // Skip over closure use statements. + for ($j = ($i + 1); $j < $phpcsFile->numTokens && isset(Tokens::$emptyTokens[$tokens[$j]['code']]) === true; $j++); + if ($tokens[$j]['code'] === T_OPEN_PARENTHESIS) { + if (isset($tokens[$j]['parenthesis_closer']) === false) { + // Live coding/parse error, stop parsing. + break; + } + + $i = $tokens[$j]['parenthesis_closer']; + continue; + } + } + + if ($tokens[$i]['code'] === T_NULLABLE) { + $nullableReturnType = true; + } + + if (isset($returnTypeTokens[$tokens[$i]['code']]) === true) { + if ($returnTypeToken === false) { + $returnTypeToken = $i; + } + + $returnType .= $tokens[$i]['content']; + $returnTypeEndToken = $i; + } + } + + if ($tokens[$stackPtr]['code'] === T_FN) { + $bodyToken = T_FN_ARROW; + } else { + $bodyToken = T_OPEN_CURLY_BRACKET; + } + + $end = $phpcsFile->findNext([$bodyToken, T_SEMICOLON], $tokens[$stackPtr]['parenthesis_closer']); + $hasBody = ($end !== false && $tokens[$end]['code'] === $bodyToken); + } + + if ($returnType !== '' && $nullableReturnType === true) { + $returnType = '?' . $returnType; + } + + return [ + 'scope' => $scope, + 'scope_specified' => $scopeSpecified, + 'return_type' => $returnType, + 'return_type_token' => $returnTypeToken, + 'return_type_end_token' => $returnTypeEndToken, + 'nullable_return_type' => $nullableReturnType, + 'is_abstract' => $isAbstract, + 'is_final' => $isFinal, + 'is_static' => $isStatic, + 'has_body' => $hasBody, + ]; } /** diff --git a/PHPCSUtils/Utils/FunctionDeclarations.php b/PHPCSUtils/Utils/FunctionDeclarations.php index e24bcf27..353ffdb1 100644 --- a/PHPCSUtils/Utils/FunctionDeclarations.php +++ b/PHPCSUtils/Utils/FunctionDeclarations.php @@ -270,6 +270,25 @@ public static function getProperties(File $phpcsFile, $stackPtr) break; } + if ($tokens[$i]['code'] === \T_USE) { + // Skip over closure use statements. + for ( + $j = ($i + 1); + $j < $phpcsFile->numTokens && isset(Tokens::$emptyTokens[$tokens[$j]['code']]) === true; + $j++ + ); + + if ($tokens[$j]['code'] === \T_OPEN_PARENTHESIS) { + if (isset($tokens[$j]['parenthesis_closer']) === false) { + // Live coding/parse error, stop parsing. + break; + } + + $i = $tokens[$j]['parenthesis_closer']; + continue; + } + } + if ($tokens[$i]['code'] === \T_NULLABLE) { $nullableReturnType = true; } diff --git a/Tests/BackCompat/BCFile/GetMethodPropertiesTest.inc b/Tests/BackCompat/BCFile/GetMethodPropertiesTest.inc index 94ffdd1e..ec7cee66 100644 --- a/Tests/BackCompat/BCFile/GetMethodPropertiesTest.inc +++ b/Tests/BackCompat/BCFile/GetMethodPropertiesTest.inc @@ -185,6 +185,24 @@ $value = $obj->fn(true); /* testFunctionDeclarationNestedInTernaryPHPCS2975 */ return (!$a ? [ new class { public function b(): c {} } ] : []); +/* testClosureWithUseNoReturnType */ +$closure = function () use($a) /*comment*/ {}; + +/* testClosureWithUseNoReturnTypeUseProp */ +$closure = function () use ($this->prop){}; + +/* testClosureWithUseWithReturnType */ +$closure = function () use /*comment*/ ($a): Type {}; + +/* testClosureWithUseWithReturnTypeUseProp */ +$closure = function () use ($this->prop): ?array {}; + +/* testClosureWithUseWithReturnTypeUseStaticPropWithSelf */ +$closure = function() use(self::$prop) : int {}; + +/* testClosureWithUseWithReturnTypeUseStaticPropWithStatic */ +$closure = function () use (static::$prop): false|null {}; + /* testArrowFunctionLiveCoding */ // Intentional parse error. This has to be the last test in the file. $fn = fn diff --git a/Tests/BackCompat/BCFile/GetMethodPropertiesTest.php b/Tests/BackCompat/BCFile/GetMethodPropertiesTest.php index 93a61fa8..2b6d5e3b 100644 --- a/Tests/BackCompat/BCFile/GetMethodPropertiesTest.php +++ b/Tests/BackCompat/BCFile/GetMethodPropertiesTest.php @@ -1198,6 +1198,150 @@ public function testFunctionDeclarationNestedInTernaryPHPCS2975() $this->getMethodPropertiesTestHelper('/* ' . __FUNCTION__ . ' */', $expected); } + /** + * Test handling of closure declarations with a use variable import without a return type declaration. + * + * @return void + */ + public function testClosureWithUseNoReturnType() + { + // Offsets are relative to the T_CLOSURE token. + $expected = [ + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => '', + 'return_type_token' => false, + 'return_type_end_token' => false, + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, + 'has_body' => true, + ]; + + $this->getMethodPropertiesTestHelper('/* ' . __FUNCTION__ . ' */', $expected); + } + + /** + * Test handling of closure declarations with a use variable import without a return type declaration. + * + * @return void + */ + public function testClosureWithUseNoReturnTypeUseProp() + { + // Offsets are relative to the T_CLOSURE token. + $expected = [ + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => '', + 'return_type_token' => false, + 'return_type_end_token' => false, + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, + 'has_body' => true, + ]; + + $this->getMethodPropertiesTestHelper('/* ' . __FUNCTION__ . ' */', $expected); + } + + /** + * Test handling of closure declarations with a use variable import with a return type declaration. + * + * @return void + */ + public function testClosureWithUseWithReturnType() + { + // Offsets are relative to the T_CLOSURE token. + $expected = [ + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => 'Type', + 'return_type_token' => 14, + 'return_type_end_token' => 14, + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, + 'has_body' => true, + ]; + + $this->getMethodPropertiesTestHelper('/* ' . __FUNCTION__ . ' */', $expected); + } + + /** + * Test handling of closure declarations with a use variable import with a return type declaration. + * + * @return void + */ + public function testClosureWithUseWithReturnTypeUseProp() + { + // Offsets are relative to the T_CLOSURE token. + $expected = [ + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => '?array', + 'return_type_token' => 15, + 'return_type_end_token' => 15, + 'nullable_return_type' => true, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, + 'has_body' => true, + ]; + + $this->getMethodPropertiesTestHelper('/* ' . __FUNCTION__ . ' */', $expected); + } + + /** + * Test handling of closure declarations with a use variable import with a return type declaration. + * + * @return void + */ + public function testClosureWithUseWithReturnTypeUseStaticPropWithSelf() + { + // Offsets are relative to the T_CLOSURE token. + $expected = [ + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => 'int', + 'return_type_token' => 13, + 'return_type_end_token' => 13, + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, + 'has_body' => true, + ]; + + $this->getMethodPropertiesTestHelper('/* ' . __FUNCTION__ . ' */', $expected); + } + + /** + * Test handling of closure declarations with a use variable import with a return type declaration. + * + * @return void + */ + public function testClosureWithUseWithReturnTypeUseStaticPropWithStatic() + { + // Offsets are relative to the T_CLOSURE token. + $expected = [ + 'scope' => 'public', + 'scope_specified' => false, + 'return_type' => 'false|null', + 'return_type_token' => 14, + 'return_type_end_token' => 16, + 'nullable_return_type' => false, + 'is_abstract' => false, + 'is_final' => false, + 'is_static' => false, + 'has_body' => true, + ]; + + $this->getMethodPropertiesTestHelper('/* ' . __FUNCTION__ . ' */', $expected); + } + /** * Test helper. *