Skip to content

Commit

Permalink
BCFile/FunctionDeclarations::get[Method]Properties(): bug fix - skip …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@jrfnl
jrfnl committed Mar 29, 2024
1 parent f627b49 commit 9f63afb
Show file tree
Hide file tree
Showing 4 changed files with 323 additions and 2 deletions.
144 changes: 142 additions & 2 deletions PHPCSUtils/BackCompat/BCFile.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
];
}

/**
Expand Down
19 changes: 19 additions & 0 deletions PHPCSUtils/Utils/FunctionDeclarations.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
18 changes: 18 additions & 0 deletions Tests/BackCompat/BCFile/GetMethodPropertiesTest.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
144 changes: 144 additions & 0 deletions Tests/BackCompat/BCFile/GetMethodPropertiesTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down

0 comments on commit 9f63afb

Please # to comment.