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

This PR improves performance of the `BCFile::getMethodProperties()` and the `FunctionDeclarations::getProperties()` methods and prevents incorrect return type information for closure `use` clauses containing invalid variable imports in the `use` clause (defensive coding).

Closure `use` statements can only import plain variables, not properties or other more complex variables.

As things were, when such "illegal" variables were imported in a closure `use`, the information for the return type could get mangled.
While this would be a parse error, for the purposes of static analysis, the `BCFile::getMethodProperties()` method and the `FunctionDeclarations::getProperties()` method should still handle this correctly.

This commit updates both methods to always skip over the complete `use` clause, which prevents the issue and improves performance as the same time (less token walking).

Includes unit tests.
  • Loading branch information
@jrfnl
jrfnl committed Mar 30, 2024
1 parent f627b49 commit 4e2bbe3
Show file tree
Hide file tree
Showing 7 changed files with 405 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: skip over closure use statements. 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
5 changes: 5 additions & 0 deletions Tests/BackCompat/BCFile/GetMethodPropertiesParseError1Test.inc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?php

/* testParseError */
// Intentional parse error.
$incompleteUse = function(int $a, string $b = '') use(&$import,
54 changes: 54 additions & 0 deletions Tests/BackCompat/BCFile/GetMethodPropertiesParseError1Test.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php
/**
* PHPCSUtils, utility functions and classes for PHP_CodeSniffer sniff developers.
*
* @package PHPCSUtils
* @copyright 2019-2020 PHPCSUtils Contributors
* @license https://opensource.org/licenses/LGPL-3.0 LGPL3
* @link https://github.com/PHPCSStandards/PHPCSUtils
*/

namespace PHPCSUtils\Tests\BackCompat\BCFile;

use PHPCSUtils\BackCompat\BCFile;
use PHPCSUtils\TestUtils\UtilityMethodTestCase;
use PHPCSUtils\Tokens\Collections;

/**
* Tests for the \PHPCSUtils\BackCompat\BCFile::getMethodProperties method.
*
* @covers \PHPCSUtils\BackCompat\BCFile::getMethodProperties
*
* @group functiondeclarations
*
* @since 1.0.11
*/
final class GetMethodPropertiesParseError1Test extends UtilityMethodTestCase
{

/**
* Test handling of closure declarations with an incomplete use clause.
*
* @return void
*/
public function testParseError()
{
$target = $this->getTargetToken('/* testParseError */', Collections::functionDeclarationTokens());
$result = BCFile::getMethodProperties(self::$phpcsFile, $target);

$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' => false,
];

$this->assertSame($expected, $result);
}
}
12 changes: 12 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,18 @@ $value = $obj->fn(true);
/* testFunctionDeclarationNestedInTernaryPHPCS2975 */
return (!$a ? [ new class { public function b(): c {} } ] : []);

/* testClosureWithUseNoReturnType */
$closure = function () use($a) /*comment*/ {};

/* testClosureWithUseNoReturnTypeIllegalUseProp */
$closure = function () use ($this->prop){};

/* testClosureWithUseWithReturnType */
$closure = function () use /*comment*/ ($a): Type {};

/* testClosureWithUseMultiParamWithReturnType */
$closure = function () use ($a, &$b, $c, $d, $e, $f, $g): ?array {};

/* testArrowFunctionLiveCoding */
// Intentional parse error. This has to be the last test in the file.
$fn = fn
97 changes: 97 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,103 @@ 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 an illegal use variable for a property import (not allowed in PHP)
* without a return type declaration.
*
* @return void
*/
public function testClosureWithUseNoReturnTypeIllegalUseProp()
{
// 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 testClosureWithUseMultiParamWithReturnType()
{
// Offsets are relative to the T_CLOSURE token.
$expected = [
'scope' => 'public',
'scope_specified' => false,
'return_type' => '?array',
'return_type_token' => 32,
'return_type_end_token' => 32,
'nullable_return_type' => true,
'is_abstract' => false,
'is_final' => false,
'is_static' => false,
'has_body' => true,
];

$this->getMethodPropertiesTestHelper('/* ' . __FUNCTION__ . ' */', $expected);
}

/**
* Test helper.
*
Expand Down
Loading

0 comments on commit 4e2bbe3

Please sign in to comment.