From 6e973e6a07cbf2a9b82411498e4d4a3e934e7d6f Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sat, 16 May 2020 17:24:08 -0400 Subject: [PATCH 01/22] Upgrade PHPCS requirement to 3.5 --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 72f888a4..64f53c1a 100644 --- a/composer.json +++ b/composer.json @@ -40,7 +40,7 @@ }, "require" : { "php" : ">=5.6.0", - "squizlabs/php_codesniffer": "^3.1", + "squizlabs/php_codesniffer": "^3.5", "phpcsstandards/phpcsutils": "^1.0" }, "require-dev": { From c62ea4aad55063b57866c3d10937e78943e227c9 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sat, 16 May 2020 17:24:31 -0400 Subject: [PATCH 02/22] Upgrade PHPCS requirement in README to 3.5 --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3bb2a33b..6b84793d 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ Plugin for PHP_CodeSniffer static analysis tool that adds analysis of problemati ### Requirements -VariableAnalysis requires PHP 5.6 or higher and [PHP CodeSniffer](https://github.com/squizlabs/PHP_CodeSniffer) version 3.1.0 or higher. +VariableAnalysis requires PHP 5.6 or higher and [PHP CodeSniffer](https://github.com/squizlabs/PHP_CodeSniffer) version 3.5.0 or higher. It also requires [PHPCSUtils](https://phpcsutils.com/) which must be installed as a PHPCS standard. If you are using composer, this will be done automatically (see below). From f9a28327dd8e88d24668131a0351fa1e64ef49d9 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sat, 16 May 2020 17:22:36 -0400 Subject: [PATCH 03/22] Add basic tests for arrow functions --- .../ArrowFunctionTest.php | 59 +++++++++++++++ .../fixtures/ArrowFunctionFixture.php | 72 +++++++++++++++++++ 2 files changed, 131 insertions(+) create mode 100644 Tests/VariableAnalysisSniff/ArrowFunctionTest.php create mode 100644 Tests/VariableAnalysisSniff/fixtures/ArrowFunctionFixture.php diff --git a/Tests/VariableAnalysisSniff/ArrowFunctionTest.php b/Tests/VariableAnalysisSniff/ArrowFunctionTest.php new file mode 100644 index 00000000..43d1399c --- /dev/null +++ b/Tests/VariableAnalysisSniff/ArrowFunctionTest.php @@ -0,0 +1,59 @@ +getFixture('ArrowFunctionFixture.php'); + $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->ruleset->setSniffProperty( + 'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff', + 'allowUnusedParametersBeforeUsed', + 'true' + ); + $phpcsFile->process(); + $lines = $this->getWarningLineNumbersFromFile($phpcsFile); + $expectedWarnings = [ + 9, + 14, + 19, + 24, + 30, + 34, + 39, + 51, + 57, + 71, + ]; + $this->assertEquals($expectedWarnings, $lines); + } + + public function testArrowFunctionsWithoutUnusedBeforeUsed() { + $fixtureFile = $this->getFixture('ArrowFunctionFixture.php'); + $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->ruleset->setSniffProperty( + 'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff', + 'allowUnusedParametersBeforeUsed', + 'false' + ); + $phpcsFile->process(); + $lines = $this->getWarningLineNumbersFromFile($phpcsFile); + $expectedWarnings = [ + 9, + 14, + 19, + 24, + 30, + 34, + 39, + 51, + 57, + 61, + 63, + 67, + 71, + ]; + $this->assertEquals($expectedWarnings, $lines); + } +} diff --git a/Tests/VariableAnalysisSniff/fixtures/ArrowFunctionFixture.php b/Tests/VariableAnalysisSniff/fixtures/ArrowFunctionFixture.php new file mode 100644 index 00000000..e243812f --- /dev/null +++ b/Tests/VariableAnalysisSniff/fixtures/ArrowFunctionFixture.php @@ -0,0 +1,72 @@ + $foo . $subject; + echo $arrowFunc('hello'); +} + +function arrowFunctionAsVariableWithUndefinedInside($subject) { + $arrowFunc = fn($foo) => $foo . $bar . $subject; // undefined variable $bar + echo $arrowFunc('hello'); +} + +function arrowFunctionAsVariableWithUndefinedInClosure() { + $arrowFunc = fn($foo) => $foo . $subject; // undefined variable $subject + echo $arrowFunc('hello'); +} + +function arrowFunctionAsVariableWithUnusedInside($subject) { + $arrowFunc = fn($foo) => $subject; // unused variable $foo + echo $arrowFunc('hello'); +} + +function unusedArrowFunctionVariable($subject) { + $arrowFunc = fn($foo) => $foo . $subject; // unused variable $arrowFunc +} + +function arrowFunctionAsVariableUsingOutsideArrow($subject) { + $arrowFunc = fn($foo) => $foo . $subject; + echo $arrowFunc('hello'); + echo $foo; // undefined variable $foo +} + +function arrowFunctionAsVariableWithUnusedInsideAfterUsed($subject) { + $arrowFunc = fn($foo, $bar) => $foo . $subject; // unused variable $bar + echo $arrowFunc('hello'); +} + +function arrowFunctionAsVariableWithUsedInsideAfterUnused($subject) { + $arrowFunc = fn($foo, $bar) => $bar . $subject; // unused variable $foo (but before used) + echo $arrowFunc('hello'); +} + +function arrowFunctionAsExpressionWithNoWarnings() { + $posts = []; + $ids = array_map(fn($post) => $post->id, $posts); + echo $ids; +} + +function arrowFunctionAsExpressionWithUndefinedVariableInside() { + $posts = []; + $ids = array_map(fn($post) => $post->id . $foo, $posts); // undefined variable $foo + echo $ids; +} + +function arrowFunctionAsExpressionWithUnusedVariableInside($subject) { + $posts = []; + $ids = array_map(fn($post) => $subject, $posts); // unused variable $post + echo $ids; +} + +function arrowFunctionAsExpressionWithUsedAfterUnused($subject) { + $posts = []; + $ids = array_map(fn($foo, $post) => $post->id, $posts); // unused variable $foo (but before used) + echo $ids; +} + +function arrowFunctionAsExpressionWithUnusedVariableOutsideArrow($subject) { + $posts = []; + $ids = array_map(fn($post) => $post->id, $posts); + echo $ids; + echo $post; // undefined variable $post; +} \ No newline at end of file From 895b177cbd750a9d2ec46e7868c61516e80732fc Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 18:02:22 -0400 Subject: [PATCH 04/22] Add tests for variable argument lists --- .../VariableArgumentListTest.php | 48 +++++++++++++++++++ .../fixtures/VariableArgumentListFixture.php | 45 +++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 Tests/VariableAnalysisSniff/VariableArgumentListTest.php create mode 100644 Tests/VariableAnalysisSniff/fixtures/VariableArgumentListFixture.php diff --git a/Tests/VariableAnalysisSniff/VariableArgumentListTest.php b/Tests/VariableAnalysisSniff/VariableArgumentListTest.php new file mode 100644 index 00000000..7023a6c8 --- /dev/null +++ b/Tests/VariableAnalysisSniff/VariableArgumentListTest.php @@ -0,0 +1,48 @@ +getFixture('VariableArgumentListFixture.php'); + $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->ruleset->setSniffProperty( + 'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff', + 'allowUnusedParametersBeforeUsed', + 'true' + ); + $phpcsFile->process(); + $lines = $this->getWarningLineNumbersFromFile($phpcsFile); + $expectedWarnings = [ + 6, + 15, + 23, + 33, + 38, + ]; + $this->assertEquals($expectedWarnings, $lines); + } + + public function testVariableArgumentListWithoutUnusedBeforeUsed() { + $fixtureFile = $this->getFixture('VariableArgumentListFixture.php'); + $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->ruleset->setSniffProperty( + 'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff', + 'allowUnusedParametersBeforeUsed', + 'false' + ); + $phpcsFile->process(); + $lines = $this->getWarningLineNumbersFromFile($phpcsFile); + $expectedWarnings = [ + 6, + 15, + 19, + 23, + 33, + 38, + 43, + ]; + $this->assertEquals($expectedWarnings, $lines); + } +} diff --git a/Tests/VariableAnalysisSniff/fixtures/VariableArgumentListFixture.php b/Tests/VariableAnalysisSniff/fixtures/VariableArgumentListFixture.php new file mode 100644 index 00000000..a5e9ce4a --- /dev/null +++ b/Tests/VariableAnalysisSniff/fixtures/VariableArgumentListFixture.php @@ -0,0 +1,45 @@ + $foo[0] . $subject; + echo $arrowFunc('hello'); +} + +function functionWithArrowVariableArgumentsAloneUnused($subject) { + $arrowFunc = fn(...$foo) => $subject; // unused variable $foo + echo $arrowFunc('hello'); +} + +function functionWithArrowVariableArgumentsUnusedAfterOther() { + $arrowFunc = fn($first, ...$foo) => $first; // unused variable $foo + echo $arrowFunc('hello'); +} + +function functionWithArrowVariableArgumentsAfterOtherUnused() { + $arrowFunc = fn($first, ...$foo) => $foo[0]; // unused variable $first (but before used) + echo $arrowFunc('hello'); +} From 7ec29a49e4b0a840cac9c1631c2d6a0a02855323 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sat, 16 May 2020 18:14:08 -0400 Subject: [PATCH 05/22] Make debug function variadic and work for objects --- VariableAnalysis/Lib/Helpers.php | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index 89a97f66..f4e677a2 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -318,17 +318,25 @@ public static function findVariableScope(File $phpcsFile, $stackPtr) { } /** - * @param string $message - * * @return void */ - public static function debug($message) { + public static function debug(...$messages) { if (! defined('PHP_CODESNIFFER_VERBOSITY')) { return; } - if (PHP_CODESNIFFER_VERBOSITY > 3) { - echo PHP_EOL . "VariableAnalysisSniff: DEBUG: $message" . PHP_EOL; + if (PHP_CODESNIFFER_VERBOSITY <= 3) { + return; + } + $output = PHP_EOL . "VariableAnalysisSniff: DEBUG:"; + foreach ($messages as $message) { + if (is_string($message) || is_numeric($message)) { + $output .= ' "' . $message . '"'; + continue; + } + $output .= PHP_EOL . var_export($message, true) . PHP_EOL; } + $output .= PHP_EOL; + echo $output; } /** From 2f38fa70b212b193e0963153c96ca22b3944895a Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sat, 16 May 2020 18:24:03 -0400 Subject: [PATCH 06/22] Add ScopeType constants --- VariableAnalysis/Lib/ScopeType.php | 11 ++++++++++ VariableAnalysis/Lib/VariableInfo.php | 10 ++++----- .../CodeAnalysis/VariableAnalysisSniff.php | 21 ++++++++++--------- 3 files changed, 27 insertions(+), 15 deletions(-) create mode 100644 VariableAnalysis/Lib/ScopeType.php diff --git a/VariableAnalysis/Lib/ScopeType.php b/VariableAnalysis/Lib/ScopeType.php new file mode 100644 index 00000000..c06c0f35 --- /dev/null +++ b/VariableAnalysis/Lib/ScopeType.php @@ -0,0 +1,11 @@ + 'variable', - 'param' => 'function parameter', - 'static' => 'static variable', - 'global' => 'global variable', - 'bound' => 'bound variable', + ScopeType::LOCAL => 'variable', + ScopeType::PARAM => 'function parameter', + ScopeType::STATICSCOPE => 'static variable', + ScopeType::GLOBALSCOPE => 'global variable', + ScopeType::BOUND => 'bound variable', ); public function __construct($varName) { diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 90ad91db..dcd7533f 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -3,6 +3,7 @@ namespace VariableAnalysis\Sniffs\CodeAnalysis; use VariableAnalysis\Lib\ScopeInfo; +use VariableAnalysis\Lib\ScopeType; use VariableAnalysis\Lib\VariableInfo; use VariableAnalysis\Lib\Constants; use VariableAnalysis\Lib\Helpers; @@ -296,7 +297,7 @@ protected function areFollowingArgumentsUsed($varInfo, $scopeInfo) { if (! $foundVarPosition) { continue; } - if ($variable->scopeType !== 'param') { + if ($variable->scopeType !== ScopeType::PARAM) { continue; } if ($variable->firstRead) { @@ -316,7 +317,7 @@ protected function areFollowingArgumentsUsed($varInfo, $scopeInfo) { protected function markVariableAssignment($varName, $stackPtr, $currScope) { $varInfo = $this->getOrCreateVariableInfo($varName, $currScope); if (!isset($varInfo->scopeType)) { - $varInfo->scopeType = 'local'; + $varInfo->scopeType = ScopeType::LOCAL; } if (isset($varInfo->firstInitialized) && ($varInfo->firstInitialized <= $stackPtr)) { return; @@ -493,7 +494,7 @@ protected function checkForFunctionPrototype(File $phpcsFile, $stackPtr, $varNam && (($tokens[$functionPtr]['code'] === T_FUNCTION) || ($tokens[$functionPtr]['code'] === T_CLOSURE)) ) { - $this->markVariableDeclaration($varName, 'param', null, $stackPtr, $functionPtr); + $this->markVariableDeclaration($varName, ScopeType::PARAM, null, $stackPtr, $functionPtr); // Are we pass-by-reference? $referencePtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true, null, true); if (($referencePtr !== false) && ($tokens[$referencePtr]['code'] === T_BITWISE_AND)) { @@ -519,7 +520,7 @@ protected function checkForFunctionPrototype(File $phpcsFile, $stackPtr, $varNam // $functionPtr is at the use, we need the function keyword for start of scope. $functionPtr = $phpcsFile->findPrevious([T_CLOSURE], $functionPtr - 1, $currScope + 1, false, null, true); if (! is_bool($functionPtr)) { - $this->markVariableDeclaration($varName, 'bound', null, $stackPtr, $functionPtr); + $this->markVariableDeclaration($varName, ScopeType::BOUND, null, $stackPtr, $functionPtr); $this->markVariableAssignment($varName, $stackPtr, $functionPtr); // Are we pass-by-reference? @@ -593,7 +594,7 @@ protected function checkForCatchBlock(File $phpcsFile, $stackPtr, $varName, $cur $catchPtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $openPtr - 1, null, true, null, true); if (($catchPtr !== false) && ($tokens[$catchPtr]['code'] === T_CATCH)) { // Scope of the exception var is actually the function, not just the catch block. - $this->markVariableDeclaration($varName, 'local', null, $stackPtr, $currScope, true); + $this->markVariableDeclaration($varName, ScopeType::LOCAL, null, $stackPtr, $currScope, true); $this->markVariableAssignment($varName, $stackPtr, $currScope); if ($this->allowUnusedCaughtExceptions) { $varInfo = $this->getOrCreateVariableInfo($varName, $currScope); @@ -916,7 +917,7 @@ protected function checkForGlobalDeclaration(File $phpcsFile, $stackPtr, $varNam } // It's a global declaration. - $this->markVariableDeclaration($varName, 'global', null, $stackPtr, $currScope); + $this->markVariableDeclaration($varName, ScopeType::GLOBALSCOPE, null, $stackPtr, $currScope); return true; } @@ -981,7 +982,7 @@ protected function checkForStaticDeclaration(File $phpcsFile, $stackPtr, $varNam } // It's a static declaration. - $this->markVariableDeclaration($varName, 'static', null, $stackPtr, $currScope); + $this->markVariableDeclaration($varName, ScopeType::STATICSCOPE, null, $stackPtr, $currScope); if (Helpers::getNextAssignPointer($phpcsFile, $stackPtr) !== null) { $this->markVariableAssignment($varName, $stackPtr, $currScope); } @@ -1421,10 +1422,10 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v if ($varInfo->ignoreUnused || isset($varInfo->firstRead)) { return; } - if ($this->allowUnusedFunctionParameters && $varInfo->scopeType === 'param') { + if ($this->allowUnusedFunctionParameters && $varInfo->scopeType === ScopeType::PARAM) { return; } - if ($this->allowUnusedParametersBeforeUsed && $varInfo->scopeType === 'param' && $this->areFollowingArgumentsUsed($varInfo, $scopeInfo)) { + if ($this->allowUnusedParametersBeforeUsed && $varInfo->scopeType === ScopeType::PARAM && $this->areFollowingArgumentsUsed($varInfo, $scopeInfo)) { Helpers::debug("variable {$varInfo->name} at end of scope has unused following args"); return; } @@ -1438,7 +1439,7 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v // of "unused variable" warnings. return; } - if ($varInfo->scopeType === 'global' && isset($varInfo->firstInitialized)) { + if ($varInfo->scopeType === ScopeType::GLOBALSCOPE && isset($varInfo->firstInitialized)) { // If we imported this variable from the global scope, any further use of // the variable, including assignment, should count as "variable use" for // the purposes of "unused variable" warnings. From 6d401f138f7460cbeed379693a88f372a81c1097 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sat, 16 May 2020 18:24:26 -0400 Subject: [PATCH 07/22] Remove unused variables in checkForSuperGlobal --- .../Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index dcd7533f..6389f8fb 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -645,13 +645,11 @@ protected function checkForThisWithinClass(File $phpcsFile, $stackPtr, $varName) } /** - * @param File $phpcsFile - * @param int $stackPtr * @param string $varName * * @return bool */ - protected function checkForSuperGlobal(File $phpcsFile, $stackPtr, $varName) { + protected function checkForSuperGlobal($varName) { // Are we a superglobal variable? if (in_array($varName, [ 'GLOBALS', @@ -1201,7 +1199,7 @@ protected function processVariable(File $phpcsFile, $stackPtr) { } // Are we a $GLOBALS, $_REQUEST, etc superglobal? - if ($this->checkForSuperGlobal($phpcsFile, $stackPtr, $varName)) { + if ($this->checkForSuperGlobal($varName)) { Helpers::debug('found superglobal'); return; } @@ -1306,7 +1304,7 @@ protected function processVariableInString(File $phpcsFile, $stackPtr) { continue; } - if ($this->checkForSuperGlobal($phpcsFile, $stackPtr, $varName)) { + if ($this->checkForSuperGlobal($varName)) { continue; } From 1dc004c79f7196930d729c901f5da11202d985a4 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sat, 4 Jul 2020 20:53:04 -0400 Subject: [PATCH 08/22] Add description to processVariable --- .../CodeAnalysis/VariableAnalysisSniff.php | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 6389f8fb..df3609d3 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -1135,7 +1135,25 @@ protected function checkForSymbolicObjectProperty(File $phpcsFile, $stackPtr, $v } /** - * Called to process normal member vars. + * Process a normal variable in the code + * + * Most importantly, this function determines if the variable use is a "read" + * (using the variable for something) or a "write" (an assignment) or, + * sometimes, both at once. + * + * It also determines the scope of the variable (where it begins and ends). + * + * Using these two pieces of information, we can determine if the variable is + * being used ("read") without having been defined ("write"). + * + * We can also determine, once the scan has hit the end of a scope, if any of + * the variables within that scope have been defined ("write") without being + * used ("read"). That behavior, however, happens in the `processScopeClose` + * function using the data gathered by this function. + * + * Some variables are used in more complex ways, so there are other similar + * functions to this one, like `processVariableInString`, and + * `processCompact`. They have the same purpose as this function, though. * * @param File $phpcsFile The PHP_CodeSniffer file where this token was found. * @param int $stackPtr The position where the token was found. @@ -1154,7 +1172,7 @@ protected function processVariable(File $phpcsFile, $stackPtr) { return; } - // Determine if variable is being assigned or read. + // Determine if variable is being assigned ("write") or used ("read"). // Read methods that preempt assignment: // Are we a $object->$property type symbolic reference? From 4382eb89ed910e8c4b1ff1adedc419bbde46df73 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sun, 17 May 2020 00:45:15 -0400 Subject: [PATCH 09/22] Refactor scope detection to handle arrow functions --- .../ArrowFunctionTest.php | 21 +- .../fixtures/ArrowFunctionFixture.php | 7 +- VariableAnalysis/Lib/Helpers.php | 282 +++++++++++++++--- VariableAnalysis/Lib/ScopeInfo.php | 4 +- VariableAnalysis/Lib/VariableInfo.php | 12 + .../CodeAnalysis/VariableAnalysisSniff.php | 275 +++++++++++------ 6 files changed, 452 insertions(+), 149 deletions(-) diff --git a/Tests/VariableAnalysisSniff/ArrowFunctionTest.php b/Tests/VariableAnalysisSniff/ArrowFunctionTest.php index 43d1399c..78115375 100644 --- a/Tests/VariableAnalysisSniff/ArrowFunctionTest.php +++ b/Tests/VariableAnalysisSniff/ArrowFunctionTest.php @@ -15,16 +15,17 @@ public function testArrowFunctions() { $phpcsFile->process(); $lines = $this->getWarningLineNumbersFromFile($phpcsFile); $expectedWarnings = [ - 9, - 14, - 19, - 24, - 30, - 34, - 39, - 51, - 57, - 71, + 9, + 14, + 19, + 24, + 30, + 34, + 51, + 57, + 61, + 67, + 71, ]; $this->assertEquals($expectedWarnings, $lines); } diff --git a/Tests/VariableAnalysisSniff/fixtures/ArrowFunctionFixture.php b/Tests/VariableAnalysisSniff/fixtures/ArrowFunctionFixture.php index e243812f..0d882a47 100644 --- a/Tests/VariableAnalysisSniff/fixtures/ArrowFunctionFixture.php +++ b/Tests/VariableAnalysisSniff/fixtures/ArrowFunctionFixture.php @@ -58,15 +58,16 @@ function arrowFunctionAsExpressionWithUnusedVariableInside($subject) { echo $ids; } -function arrowFunctionAsExpressionWithUsedAfterUnused($subject) { +function arrowFunctionAsExpressionWithUsedAfterUnused($subject) { // unused variable $subject $posts = []; $ids = array_map(fn($foo, $post) => $post->id, $posts); // unused variable $foo (but before used) echo $ids; } -function arrowFunctionAsExpressionWithUnusedVariableOutsideArrow($subject) { +function arrowFunctionAsExpressionWithUnusedVariableOutsideArrow($subject) { //unused variable $subject $posts = []; $ids = array_map(fn($post) => $post->id, $posts); echo $ids; echo $post; // undefined variable $post; -} \ No newline at end of file +} + diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index f4e677a2..7e90308b 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -4,6 +4,7 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Utils\FunctionDeclarations; class Helpers { /** @@ -112,20 +113,84 @@ public static function areConditionsWithinFunctionBeforeClass(array $conditions) /** * @param File $phpcsFile - * @param int $openPtr + * @param int $stackPtr + * + * @return bool + */ + public static function isTokenInsideFunctionDefinitionArgumentList(File $phpcsFile, $stackPtr) { + return (bool) self::getFunctionIndexForFunctionArgument($phpcsFile, $stackPtr); + } + + /** + * Find the index of the function keyword for a token in a function definition's arguments + * + * Does not work for tokens inside the "use". + * + * @param File $phpcsFile + * @param int $stackPtr + * + * @return ?int + */ + public static function getFunctionIndexForFunctionArgument(File $phpcsFile, $stackPtr) { + $tokens = $phpcsFile->getTokens(); + + $nonFunctionTokenTypes = array_values(Tokens::$emptyTokens); + $nonFunctionTokenTypes[] = T_OPEN_PARENTHESIS; + $nonFunctionTokenTypes[] = T_VARIABLE; + $nonFunctionTokenTypes[] = T_COMMA; + $nonFunctionTokenTypes[] = T_STRING; + $nonFunctionTokenTypes[] = T_BITWISE_AND; + $functionPtr = self::getIntOrNull($phpcsFile->findPrevious($nonFunctionTokenTypes, $stackPtr - 1, null, true, null, true)); + if (! is_int($functionPtr)) { + return null; + } + + $functionTokenTypes = [ + T_FUNCTION, + T_CLOSURE, + T_FN, // TODO: cannot use this before PHP 7.4 + ]; + if (!in_array($tokens[$functionPtr]['code'], $functionTokenTypes, true)) { + return null; + } + return $functionPtr; + } + + /** + * @param File $phpcsFile + * @param int $stackPtr + * + * @return bool + */ + public static function isTokenInsideFunctionUseImport(File $phpcsFile, $stackPtr) { + return (bool) self::getUseIndexForUseImport($phpcsFile, $stackPtr); + } + + /** + * Find the token index of the "use" for a token inside a function use import + * + * @param File $phpcsFile + * @param int $stackPtr * * @return ?int */ - public static function findPreviousFunctionPtr(File $phpcsFile, $openPtr) { - // Function names are T_STRING, and return-by-reference is T_BITWISE_AND, - // so we look backwards from the opening bracket for the first thing that - // isn't a function name, reference sigil or whitespace and check if it's a - // function keyword. - $functionPtrTypes = Tokens::$emptyTokens; - $functionPtrTypes[T_STRING] = T_STRING; - $functionPtrTypes[T_BITWISE_AND] = T_BITWISE_AND; - - return self::getIntOrNull($phpcsFile->findPrevious($functionPtrTypes, $openPtr - 1, null, true, null, true)); + public static function getUseIndexForUseImport(File $phpcsFile, $stackPtr) { + $tokens = $phpcsFile->getTokens(); + + $nonUseTokenTypes = array_values(Tokens::$emptyTokens); + $nonUseTokenTypes[] = T_VARIABLE; + $nonUseTokenTypes[] = T_COMMA; + $nonUseTokenTypes[] = T_BITWISE_AND; + $openParenPtr = self::getIntOrNull($phpcsFile->findPrevious($nonUseTokenTypes, $stackPtr - 1, null, true, null, true)); + if (! is_int($openParenPtr) || $tokens[$openParenPtr]['code'] !== T_OPEN_PARENTHESIS) { + return null; + } + + $usePtr = self::getIntOrNull($phpcsFile->findPrevious(array_values($nonUseTokenTypes), $openParenPtr - 1, null, true, null, true)); + if (! is_int($usePtr) || $tokens[$usePtr]['code'] !== T_USE) { + return null; + } + return $usePtr; } /** @@ -267,56 +332,201 @@ public static function normalizeVarName($varName) { * * @return ?int */ - public static function findFunctionPrototype(File $phpcsFile, $stackPtr) { + public static function findVariableScope(File $phpcsFile, $stackPtr) { $tokens = $phpcsFile->getTokens(); + $token = $tokens[$stackPtr]; + + if (self::isTokenInsideArrowFunctionBody($phpcsFile, $stackPtr)) { + // Get the list of variables defined by the arrow function + // If this matches any of them, the scope is the arrow function, + // otherwise, it uses the enclosing scope. + $arrowFunctionIndex = self::getContainingArrowFunctionIndex($phpcsFile, $stackPtr); + if ($arrowFunctionIndex) { + $variableNames = self::getVariablesDefinedByArrowFunction($phpcsFile, $arrowFunctionIndex); + if (in_array($token['content'], $variableNames, true)) { + return $arrowFunctionIndex; + } + } + } - $openPtr = Helpers::findContainingOpeningBracket($phpcsFile, $stackPtr); - if (! is_int($openPtr)) { - return null; + return self::findVariableScopeExceptArrowFunctions($phpcsFile, $stackPtr); + } + + /** + * Return the token index of the scope start for a token + * + * For a variable within a function body, or a variable within a function + * definition argument list, this will return the function keyword's index. + * + * For a variable within a "use" import list within a function definition, + * this will return the enclosing scope, not the function keyword. This is + * important to note because the "use" keyword performs double-duty, defining + * variables for the function's scope, and consuming the variables in the + * enclosing scope. Use `getUseIndexForUseImport` to determine if this + * token needs to be treated as a "use". + * + * For a variable within an arrow function definition argument list, + * this will return the arrow function's keyword index. + * + * For a variable in an arrow function body, this will return the enclosing + * function's index, which may be incorrect. + * + * Since a variable in an arrow function's body may be imported from the + * enclosing scope, it's important to test to see if the variable is in an + * arrow function and also check its enclosing scope separately. + * + * @param File $phpcsFile + * @param int $stackPtr + * + * @return ?int + */ + public static function findVariableScopeExceptArrowFunctions(File $phpcsFile, $stackPtr) { + $tokens = $phpcsFile->getTokens(); + $allowedTypes = [ + T_VARIABLE, + T_DOUBLE_QUOTED_STRING, + T_HEREDOC, + T_STRING, + ]; + if (! in_array($tokens[$stackPtr]['code'], $allowedTypes, true)) { + throw new \Exception("Cannot find variable scope for non-variable {$tokens[$stackPtr]['type']}"); + } + + $startOfTokenScope = self::getStartOfTokenScope($phpcsFile, $stackPtr); + if (is_int($startOfTokenScope) && $startOfTokenScope > 0) { + return $startOfTokenScope; } - $functionPtr = Helpers::findPreviousFunctionPtr($phpcsFile, $openPtr); - if (($functionPtr !== null) && ($tokens[$functionPtr]['code'] === T_FUNCTION)) { - return $functionPtr; + + // If there is no "conditions" array, this is a function definition argument. + if (self::isTokenInsideFunctionDefinitionArgumentList($phpcsFile, $stackPtr)) { + $functionPtr = self::getFunctionIndexForFunctionArgument($phpcsFile, $stackPtr); + if (! is_int($functionPtr)) { + throw new \Exception("Function index not found for function argument index {$stackPtr}"); + } + $functionToken = $tokens[$functionPtr]; + return $functionToken['scope_condition']; } - return null; + + self::debug('Cannot find function scope for variable at', $stackPtr); + return $startOfTokenScope; } /** + * Return the token index of the scope start for a variable token + * + * This will only work for a variable within a function's body. Otherwise, + * see `findVariableScope`, which is more complex. + * + * Note that if used on a variable in an arrow function, it will return the + * enclosing function's scope, which may be incorrect. + * * @param File $phpcsFile * @param int $stackPtr * * @return ?int */ - public static function findVariableScope(File $phpcsFile, $stackPtr) { + private static function getStartOfTokenScope(File $phpcsFile, $stackPtr) { $tokens = $phpcsFile->getTokens(); - $token = $tokens[$stackPtr]; + $token = $tokens[$stackPtr]; $in_class = false; - if (!empty($token['conditions'])) { - foreach (array_reverse($token['conditions'], true) as $scopePtr => $scopeCode) { - if (($scopeCode === T_FUNCTION) || ($scopeCode === T_CLOSURE)) { - return $scopePtr; - } - if (isset(Tokens::$ooScopeTokens[$scopeCode]) === true) { - $in_class = true; - } + $conditions = $token['conditions'] ?? []; + $functionTokenTypes = [ + T_FUNCTION, + T_CLOSURE, + T_FN, // TODO: cannot use this before PHP 7.4 + ]; + foreach (array_reverse($conditions, true) as $scopePtr => $scopeCode) { + if (in_array($scopeCode, $functionTokenTypes, true)) { + return $scopePtr; + } + if (isset(Tokens::$ooScopeTokens[$scopeCode]) === true) { + $in_class = true; } - } - - $scopePtr = Helpers::findFunctionPrototype($phpcsFile, $stackPtr); - if (is_int($scopePtr)) { - return $scopePtr; } if ($in_class) { - // Member var of a class, we don't care. + // If this is inside a class and not inside a function, this is either a + // class member variable definition, or a function argument. If it is a + // variable definition, it has no scope on its own (it can only be used + // with an object reference). If it is a function argument, we need to do + // more work (see `findVariableScopeExceptArrowFunctions`). return null; } - // File scope, hmm, lets use first token of file? + // If we can't find a scope, let's use the first token of the file. return 0; } + /** + * @param File $phpcsFile + * @param int $stackPtr + * + * @return bool + */ + public static function isTokenInsideArrowFunctionDefinition(File $phpcsFile, $stackPtr) { + $tokens = $phpcsFile->getTokens(); + $token = $tokens[$stackPtr]; + $openParenIndices = $token['nested_parenthesis'] ?? []; + if ($openParenIndices) { + return false; + } + $openParenPtr = $openParenIndices[0]; + return FunctionDeclarations::isArrowFunction($phpcsFile, $openParenPtr - 1); + } + + /** + * @param File $phpcsFile + * @param int $stackPtr + * + * @return bool + */ + public static function isTokenInsideArrowFunctionBody(File $phpcsFile, $stackPtr) { + return (bool) self::getContainingArrowFunctionIndex($phpcsFile, $stackPtr); + } + + /** + * @param File $phpcsFile + * @param int $stackPtr + * + * @return ?int + */ + public static function getContainingArrowFunctionIndex(File $phpcsFile, $stackPtr) { + $tokens = $phpcsFile->getTokens(); + $enclosingScopeIndex = self::findVariableScopeExceptArrowFunctions($phpcsFile, $stackPtr); + $arrowFunctionIndex = $phpcsFile->findPrevious([T_FN], $stackPtr - 1, $enclosingScopeIndex); // TODO: cannot use T_FN before PHP 7.4 + if (! is_int($arrowFunctionIndex)) { + return null; + } + $arrowFunctionToken = $tokens[$arrowFunctionIndex]; + $arrowFunctionScopeStart = $arrowFunctionToken['scope_opener']; + $arrowFunctionScopeEnd = $arrowFunctionToken['scope_closer']; + if ($stackPtr > $arrowFunctionScopeStart && $stackPtr < $arrowFunctionScopeEnd) { + return $arrowFunctionIndex; + } + return null; + } + + /** + * @param File $phpcsFile + * @param int $stackPtr + * + * @return string[] + */ + public static function getVariablesDefinedByArrowFunction(File $phpcsFile, $stackPtr) { + $tokens = $phpcsFile->getTokens(); + $arrowFunctionToken = $tokens[$stackPtr]; + $variableNames = []; + self::debug('looking for variables in arrow function token', $arrowFunctionToken); + for ($index = $arrowFunctionToken['parenthesis_opener']; $index < $arrowFunctionToken['parenthesis_closer']; $index++) { + $token = $tokens[$index]; + if ($token['code'] === T_VARIABLE) { + $variableNames[] = $token['content']; + } + } + return $variableNames; + } + /** * @return void */ diff --git a/VariableAnalysis/Lib/ScopeInfo.php b/VariableAnalysis/Lib/ScopeInfo.php index d541fc2a..04b435bd 100644 --- a/VariableAnalysis/Lib/ScopeInfo.php +++ b/VariableAnalysis/Lib/ScopeInfo.php @@ -16,7 +16,7 @@ class ScopeInfo { */ public $variables = []; - public function __construct($currScope) { - $this->owner = $currScope; + public function __construct($scopeStartIndex) { + $this->owner = $scopeStartIndex; } } diff --git a/VariableAnalysis/Lib/VariableInfo.php b/VariableAnalysis/Lib/VariableInfo.php index 806621ce..2467c45f 100644 --- a/VariableAnalysis/Lib/VariableInfo.php +++ b/VariableAnalysis/Lib/VariableInfo.php @@ -2,6 +2,8 @@ namespace VariableAnalysis\Lib; +use VariableAnalysis\Lib\ScopeType; + /** * Holds details of a variable within a scope. */ @@ -28,6 +30,16 @@ class VariableInfo { */ public $passByReference = false; + /** + * @var self | null + */ + public $referencedVariable; + + /** + * @var int | null + */ + public $referencedVariableScope; + /** * @var bool */ diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index df3609d3..ac1b9922 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -27,6 +27,13 @@ class VariableAnalysisSniff implements Sniff { */ private $scopes = []; + /** + * A list of token indices which start scopes and will be used to check for unused variables. + * + * @var int[] + */ + private $scopeStartIndices = []; + /** * A list of custom functions which pass in variables to be initialized by * reference (eg `preg_match()`) and therefore should not require those @@ -125,7 +132,13 @@ public function register() { T_DOUBLE_QUOTED_STRING, T_HEREDOC, T_CLOSE_CURLY_BRACKET, + T_FUNCTION, + T_CLOSURE, T_STRING, + T_COMMA, + T_SEMICOLON, + T_CLOSE_PARENTHESIS, + T_FN, // TODO: we can't use this before php 7.4 so we need to replace it somehow ]; } @@ -157,7 +170,25 @@ private function getPassByReferenceFunction($functionName) { */ public function process(File $phpcsFile, $stackPtr) { $tokens = $phpcsFile->getTokens(); - $token = $tokens[$stackPtr]; + + $scopeStartTokenTypes = [ + T_FUNCTION, + T_CLOSURE, + T_FN, // TODO: we cannot use this before PHP 7.4 + ]; + + $scopeIndexThisCloses = array_reduce($this->scopeStartIndices, function ($found, $index) use ($stackPtr, $tokens) { + if ($stackPtr === $tokens[$index]['scope_closer']) { + return $index; + } + return $found; + }, null); + if ($scopeIndexThisCloses) { + Helpers::debug('found closing scope at', $stackPtr, 'for scope', $scopeIndexThisCloses); + $this->processScopeClose($phpcsFile, $scopeIndexThisCloses); + } + + $token = $tokens[$stackPtr]; if ($this->currentFile !== $phpcsFile) { $this->currentFile = $phpcsFile; @@ -180,8 +211,9 @@ public function process(File $phpcsFile, $stackPtr) { $this->markAllVariablesRead($phpcsFile, $stackPtr); return; } - if (($token['code'] === T_CLOSE_CURLY_BRACKET) && isset($token['scope_condition'])) { - $this->processScopeClose($phpcsFile, $token['scope_condition']); + if (isset($token['scope_condition']) && in_array($token['code'], $scopeStartTokenTypes, true)) { + Helpers::debug('found scope condition', $token); + $this->scopeStartIndices[] = $token['scope_condition']; return; } } @@ -256,8 +288,10 @@ protected function getVariableInfo($varName, $currScope) { * @return VariableInfo */ protected function getOrCreateVariableInfo($varName, $currScope) { + // TODO: this needs to find the scope of an arrow function if we are inside one, which must also include the scope of its parent! $scopeInfo = $this->getOrCreateScopeInfo($currScope); if (!isset($scopeInfo->variables[$varName])) { + Helpers::debug("creating a new variable for '{$varName}' in scope", $scopeInfo); $scopeInfo->variables[$varName] = new VariableInfo($varName); $validUnusedVariableNames = (empty($this->validUnusedVariableNames)) ? [] @@ -278,6 +312,7 @@ protected function getOrCreateVariableInfo($varName, $currScope) { $scopeInfo->variables[$varName]->ignoreUndefined = true; } } + Helpers::debug("scope for '{$varName}' is now", $scopeInfo); return $scopeInfo->variables[$varName]; } @@ -316,12 +351,20 @@ protected function areFollowingArgumentsUsed($varInfo, $scopeInfo) { */ protected function markVariableAssignment($varName, $stackPtr, $currScope) { $varInfo = $this->getOrCreateVariableInfo($varName, $currScope); + + // Is the variable referencing another variable? If so, mark that variable used also. + if ($varInfo->referencedVariable && $varInfo->referencedVariableScope) { + $this->markVariableAssignment($varInfo->name, $stackPtr, $varInfo->referencedVariableScope); + } + if (!isset($varInfo->scopeType)) { $varInfo->scopeType = ScopeType::LOCAL; } if (isset($varInfo->firstInitialized) && ($varInfo->firstInitialized <= $stackPtr)) { + Helpers::debug('markVariableAssignment failed; already initialized', $varName); return; } + Helpers::debug('markVariableAssignment', $varName); $varInfo->firstInitialized = $stackPtr; } @@ -343,6 +386,7 @@ protected function markVariableDeclaration( $currScope, $permitMatchingRedeclaration = false ) { + Helpers::debug("marking variable '{$varName}' declared in scope starting at token", $currScope); $varInfo = $this->getOrCreateVariableInfo($varName, $currScope); if (isset($varInfo->scopeType)) { if (($permitMatchingRedeclaration === false) || ($varInfo->scopeType !== $scopeType)) { @@ -367,9 +411,11 @@ protected function markVariableDeclaration( $varInfo->typeHint = $typeHint; } if (isset($varInfo->firstDeclared) && ($varInfo->firstDeclared <= $stackPtr)) { + Helpers::debug("variable '{$varName}' was already marked declared", $varInfo); return; } $varInfo->firstDeclared = $stackPtr; + Helpers::debug("variable '{$varName}' marked declared", $varInfo); } /** @@ -416,6 +462,7 @@ protected function markVariableRead($varName, $stackPtr, $currScope) { */ protected function isVariableUndefined($varName, $stackPtr, $currScope) { $varInfo = $this->getOrCreateVariableInfo($varName, $currScope); + Helpers::debug('isVariableUndefined', $varInfo); if ($varInfo->ignoreUndefined) { return false; } @@ -425,6 +472,7 @@ protected function isVariableUndefined($varName, $stackPtr, $currScope) { if (isset($varInfo->firstInitialized) && $varInfo->firstInitialized <= $stackPtr) { return false; } + // TODO: if the variable is in an arrow function body, also check the enclosing scope return true; } @@ -469,71 +517,89 @@ protected function markAllVariablesRead(File $phpcsFile, $stackPtr) { } /** + * Process a variable if it is inside a function definition + * + * This does not include variables imported by a "use" statement. + * * @param File $phpcsFile * @param int $stackPtr * @param string $varName - * @param int $currScope * - * @return bool + * @return void */ - protected function checkForFunctionPrototype(File $phpcsFile, $stackPtr, $varName, $currScope) { + protected function processVariableAsFunctionDefinitionArgument(File $phpcsFile, $stackPtr, $varName) { + Helpers::debug("processVariableAsFunctionDefinitionArgument", $stackPtr, $varName); $tokens = $phpcsFile->getTokens(); - // Are we a function or closure parameter? - // It would be nice to get the list of function parameters from watching for - // T_FUNCTION, but AbstractVariableSniff and AbstractScopeSniff define everything - // we need to do that as private or final, so we have to do it this hackish way. - $openPtr = Helpers::findContainingOpeningBracket($phpcsFile, $stackPtr); - if (! is_int($openPtr)) { - return false; + $functionPtr = Helpers::getFunctionIndexForFunctionArgument($phpcsFile, $stackPtr); + if (! is_int($functionPtr)) { + throw new \Exception("Function index not found for function argument index {$stackPtr}"); } - $functionPtr = Helpers::findPreviousFunctionPtr($phpcsFile, $openPtr); - if (// phpcs:ignore PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace - (is_int($functionPtr)) - && (($tokens[$functionPtr]['code'] === T_FUNCTION) - || ($tokens[$functionPtr]['code'] === T_CLOSURE)) - ) { - $this->markVariableDeclaration($varName, ScopeType::PARAM, null, $stackPtr, $functionPtr); - // Are we pass-by-reference? - $referencePtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true, null, true); - if (($referencePtr !== false) && ($tokens[$referencePtr]['code'] === T_BITWISE_AND)) { - $varInfo = $this->getOrCreateVariableInfo($varName, $functionPtr); - $varInfo->passByReference = true; - } - // Are we optional with a default? - if (Helpers::getNextAssignPointer($phpcsFile, $stackPtr) !== null) { - $this->markVariableAssignment($varName, $stackPtr, $functionPtr); - } - return true; + Helpers::debug("processVariableAsFunctionDefinitionArgument found function definition", $tokens[$functionPtr]); + $this->markVariableDeclaration($varName, ScopeType::PARAM, null, $stackPtr, $functionPtr); + + // Are we pass-by-reference? + $referencePtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true, null, true); + if (($referencePtr !== false) && ($tokens[$referencePtr]['code'] === T_BITWISE_AND)) { + $varInfo = $this->getOrCreateVariableInfo($varName, $functionPtr); + $varInfo->passByReference = true; } - // Is it a use keyword? Use is both a read and a define, fun! - if (($functionPtr !== false) && ($tokens[$functionPtr]['code'] === T_USE)) { - $this->markVariableRead($varName, $stackPtr, $currScope); - if ($this->isVariableUndefined($varName, $stackPtr, $currScope) === true) { - // We haven't been defined by this point. - Helpers::debug("variable $varName in function prototype looks undefined"); - $phpcsFile->addWarning("Variable %s is undefined.", $stackPtr, 'UndefinedVariable', ["\${$varName}"]); - return true; - } - // $functionPtr is at the use, we need the function keyword for start of scope. - $functionPtr = $phpcsFile->findPrevious([T_CLOSURE], $functionPtr - 1, $currScope + 1, false, null, true); - if (! is_bool($functionPtr)) { - $this->markVariableDeclaration($varName, ScopeType::BOUND, null, $stackPtr, $functionPtr); - $this->markVariableAssignment($varName, $stackPtr, $functionPtr); - - // Are we pass-by-reference? - $referencePtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true, null, true); - if ((! is_bool($referencePtr)) && ($tokens[$referencePtr]['code'] === T_BITWISE_AND)) { - $varInfo = $this->getOrCreateVariableInfo($varName, $functionPtr); - $varInfo->passByReference = true; - } + // Are we optional with a default? + if (Helpers::getNextAssignPointer($phpcsFile, $stackPtr) !== null) { + $this->markVariableAssignment($varName, $stackPtr, $functionPtr); + } + } - return true; - } + /** + * Process a variable if it is inside a function's "use" import + * + * @param File $phpcsFile + * @param int $stackPtr + * @param string $varName + * @param int $outerScope The start of the scope outside the function definition + * + * @return void + */ + protected function processVariableAsUseImportDefinition(File $phpcsFile, $stackPtr, $varName, $outerScope) { + $tokens = $phpcsFile->getTokens(); + + Helpers::debug("processVariableAsUseImportDefinition", $stackPtr, $varName); + + $endOfArgsPtr = $phpcsFile->findPrevious([T_CLOSE_PARENTHESIS], $stackPtr - 1, null); + if (! is_int($endOfArgsPtr)) { + throw new \Exception("Arguments index not found for function use index {$stackPtr}"); + } + $functionPtr = Helpers::getFunctionIndexForFunctionArgument($phpcsFile, $endOfArgsPtr); + if (! is_int($functionPtr)) { + throw new \Exception("Function index not found for function use index {$stackPtr}"); + } + + // Use is both a read (in the enclosing scope) and a define (in the function scope) + $this->markVariableRead($varName, $stackPtr, $outerScope); + + // If it's undefined in the enclosing scope, the use is wrong + if ($this->isVariableUndefined($varName, $stackPtr, $outerScope) === true) { + Helpers::debug("variable '{$varName}' in function definition looks undefined in scope", $outerScope); + $phpcsFile->addWarning("Variable %s is undefined.", $stackPtr, 'UndefinedVariable', ["\${$varName}"]); + return; + } + + $this->markVariableDeclaration($varName, ScopeType::BOUND, null, $stackPtr, $functionPtr); + $this->markVariableAssignment($varName, $stackPtr, $functionPtr); + + // Are we pass-by-reference? If so, then any assignment to the variable in + // the function scope also should count for the enclosing scope. + $referencePtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true, null, true); + if (is_int($referencePtr) && $tokens[$referencePtr]['code'] === T_BITWISE_AND) { + Helpers::debug("variable '{$varName}' in function definition looks passed by reference"); + $varInfo = $this->getOrCreateVariableInfo($varName, $functionPtr); + $varInfo->passByReference = true; + $referencedVariable = $this->getVariableInfo($varName, $outerScope); + $varInfo->referencedVariable = $referencedVariable; + $varInfo->referencedVariableScope = $outerScope; } - return false; } /** @@ -542,7 +608,7 @@ protected function checkForFunctionPrototype(File $phpcsFile, $stackPtr, $varNam * * @return bool */ - protected function checkForClassProperty(File $phpcsFile, $stackPtr) { + protected function processVariableAsClassProperty(File $phpcsFile, $stackPtr) { $propertyDeclarationKeywords = [ T_PUBLIC, T_PRIVATE, @@ -582,7 +648,7 @@ protected function checkForClassProperty(File $phpcsFile, $stackPtr) { * * @return bool */ - protected function checkForCatchBlock(File $phpcsFile, $stackPtr, $varName, $currScope) { + protected function processVariableAsCatchBlock(File $phpcsFile, $stackPtr, $varName, $currScope) { $tokens = $phpcsFile->getTokens(); // Are we a catch block parameter? @@ -612,7 +678,7 @@ protected function checkForCatchBlock(File $phpcsFile, $stackPtr, $varName, $cur * * @return bool */ - protected function checkForThisWithinClass(File $phpcsFile, $stackPtr, $varName) { + protected function processVariableAsThisWithinClass(File $phpcsFile, $stackPtr, $varName) { $tokens = $phpcsFile->getTokens(); $token = $tokens[$stackPtr]; @@ -649,7 +715,7 @@ protected function checkForThisWithinClass(File $phpcsFile, $stackPtr, $varName) * * @return bool */ - protected function checkForSuperGlobal($varName) { + protected function processVariableAsSuperGlobal($varName) { // Are we a superglobal variable? if (in_array($varName, [ 'GLOBALS', @@ -676,7 +742,7 @@ protected function checkForSuperGlobal($varName) { * * @return bool */ - protected function checkForStaticMember(File $phpcsFile, $stackPtr) { + protected function processVariableAsStaticMember(File $phpcsFile, $stackPtr) { $tokens = $phpcsFile->getTokens(); $doubleColonPtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true); @@ -710,7 +776,7 @@ protected function checkForStaticMember(File $phpcsFile, $stackPtr) { * * @return bool */ - protected function checkForStaticOutsideClass(File $phpcsFile, $stackPtr, $varName) { + protected function processVariableAsStaticOutsideClass(File $phpcsFile, $stackPtr, $varName) { // Are we refering to self:: outside a class? $tokens = $phpcsFile->getTokens(); @@ -754,7 +820,7 @@ protected function checkForStaticOutsideClass(File $phpcsFile, $stackPtr, $varNa * * @return bool */ - protected function checkForAssignment(File $phpcsFile, $stackPtr, $varName, $currScope) { + protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varName, $currScope) { // Is the next non-whitespace an assignment? $assignPtr = Helpers::getNextAssignPointer($phpcsFile, $stackPtr); if (! is_int($assignPtr)) { @@ -762,7 +828,7 @@ protected function checkForAssignment(File $phpcsFile, $stackPtr, $varName, $cur } // Is this a variable variable? If so, it's not an assignment to the current variable. - if ($this->checkForVariableVariable($phpcsFile, $stackPtr)) { + if ($this->processVariableAsVariableVariable($phpcsFile, $stackPtr)) { Helpers::debug('found variable variable'); return false; } @@ -792,7 +858,7 @@ protected function checkForAssignment(File $phpcsFile, $stackPtr, $varName, $cur * * @return bool */ - protected function checkForVariableVariable(File $phpcsFile, $stackPtr) { + protected function processVariableAsVariableVariable(File $phpcsFile, $stackPtr) { $tokens = $phpcsFile->getTokens(); $prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($stackPtr - 1), null, true); @@ -821,7 +887,7 @@ protected function checkForVariableVariable(File $phpcsFile, $stackPtr) { * * @return bool */ - protected function checkForListShorthandAssignment(File $phpcsFile, $stackPtr, $varName, $currScope) { + protected function processVariableAsListShorthandAssignment(File $phpcsFile, $stackPtr, $varName, $currScope) { // OK, are we within a [ ... ] construct? $openPtr = Helpers::findContainingOpeningSquareBracket($phpcsFile, $stackPtr); if (! is_int($openPtr)) { @@ -857,7 +923,7 @@ protected function checkForListShorthandAssignment(File $phpcsFile, $stackPtr, $ * * @return bool */ - protected function checkForListAssignment(File $phpcsFile, $stackPtr, $varName, $currScope) { + protected function processVariableAsListAssignment(File $phpcsFile, $stackPtr, $varName, $currScope) { $tokens = $phpcsFile->getTokens(); // OK, are we within a list (...) construct? @@ -900,7 +966,7 @@ protected function checkForListAssignment(File $phpcsFile, $stackPtr, $varName, * * @return bool */ - protected function checkForGlobalDeclaration(File $phpcsFile, $stackPtr, $varName, $currScope) { + protected function processVariableAsGlobalDeclaration(File $phpcsFile, $stackPtr, $varName, $currScope) { $tokens = $phpcsFile->getTokens(); // Are we a global declaration? @@ -927,7 +993,7 @@ protected function checkForGlobalDeclaration(File $phpcsFile, $stackPtr, $varNam * * @return bool */ - protected function checkForStaticDeclaration(File $phpcsFile, $stackPtr, $varName, $currScope) { + protected function processVariableAsStaticDeclaration(File $phpcsFile, $stackPtr, $varName, $currScope) { $tokens = $phpcsFile->getTokens(); // Are we a static declaration? @@ -992,7 +1058,7 @@ protected function checkForStaticDeclaration(File $phpcsFile, $stackPtr, $varNam * * @return bool */ - protected function checkForNumericVariable($varName) { + protected function processVariableAsNumericVariable($varName) { return is_numeric(substr($varName, 0, 1)); } @@ -1004,7 +1070,7 @@ protected function checkForNumericVariable($varName) { * * @return bool */ - protected function checkForForeachLoopVar(File $phpcsFile, $stackPtr, $varName, $currScope) { + protected function processVariableAsForeachLoopVar(File $phpcsFile, $stackPtr, $varName, $currScope) { $tokens = $phpcsFile->getTokens(); // Are we a foreach loopvar? @@ -1053,7 +1119,7 @@ protected function checkForForeachLoopVar(File $phpcsFile, $stackPtr, $varName, * * @return bool */ - protected function checkForPassByReferenceFunctionCall(File $phpcsFile, $stackPtr, $varName, $currScope) { + protected function processVariableAsPassByReferenceFunctionCall(File $phpcsFile, $stackPtr, $varName, $currScope) { $tokens = $phpcsFile->getTokens(); // Are we pass-by-reference to known pass-by-reference function? @@ -1120,7 +1186,7 @@ protected function checkForPassByReferenceFunctionCall(File $phpcsFile, $stackPt * * @return bool */ - protected function checkForSymbolicObjectProperty(File $phpcsFile, $stackPtr, $varName, $currScope) { + protected function processVariableAsSymbolicObjectProperty(File $phpcsFile, $stackPtr, $varName, $currScope) { $tokens = $phpcsFile->getTokens(); // Are we a symbolic object property/function derefeference? @@ -1165,12 +1231,13 @@ protected function processVariable(File $phpcsFile, $stackPtr) { $token = $tokens[$stackPtr]; $varName = Helpers::normalizeVarName($token['content']); - Helpers::debug('examining token ' . $varName); + Helpers::debug("examining token for variable '{$varName}'", $token); $currScope = Helpers::findVariableScope($phpcsFile, $stackPtr); if ($currScope === null) { Helpers::debug('no scope found'); return; } + Helpers::debug("start of scope for variable '{$varName}' is", $currScope); // Determine if variable is being assigned ("write") or used ("read"). @@ -1193,96 +1260,104 @@ protected function processVariable(File $phpcsFile, $stackPtr) { // Pass-by-reference to known pass-by-reference function // Are we a $object->$property type symbolic reference? - if ($this->checkForSymbolicObjectProperty($phpcsFile, $stackPtr, $varName, $currScope)) { + if ($this->processVariableAsSymbolicObjectProperty($phpcsFile, $stackPtr, $varName, $currScope)) { Helpers::debug('found symbolic object property'); return; } // Are we a function or closure parameter? - if ($this->checkForFunctionPrototype($phpcsFile, $stackPtr, $varName, $currScope)) { - Helpers::debug('found function prototype'); + if (Helpers::isTokenInsideFunctionDefinitionArgumentList($phpcsFile, $stackPtr)) { + Helpers::debug('found function definition argument'); + $this->processVariableAsFunctionDefinitionArgument($phpcsFile, $stackPtr, $varName); + return; + } + + // Are we a variable being imported into a function's scope with "use"? + if (Helpers::isTokenInsideFunctionUseImport($phpcsFile, $stackPtr)) { + Helpers::debug('found use scope import definition'); + $this->processVariableAsUseImportDefinition($phpcsFile, $stackPtr, $varName, $currScope); return; } // Are we a catch parameter? - if ($this->checkForCatchBlock($phpcsFile, $stackPtr, $varName, $currScope)) { + if ($this->processVariableAsCatchBlock($phpcsFile, $stackPtr, $varName, $currScope)) { Helpers::debug('found catch block'); return; } // Are we $this within a class? - if ($this->checkForThisWithinClass($phpcsFile, $stackPtr, $varName)) { + if ($this->processVariableAsThisWithinClass($phpcsFile, $stackPtr, $varName)) { Helpers::debug('found this usage within a class'); return; } // Are we a $GLOBALS, $_REQUEST, etc superglobal? - if ($this->checkForSuperGlobal($varName)) { + if ($this->processVariableAsSuperGlobal($varName)) { Helpers::debug('found superglobal'); return; } // Check for static members used outside a class - if ($this->checkForStaticOutsideClass($phpcsFile, $stackPtr, $varName)) { + if ($this->processVariableAsStaticOutsideClass($phpcsFile, $stackPtr, $varName)) { Helpers::debug('found static usage outside of class'); return; } // $var part of class::$var static member - if ($this->checkForStaticMember($phpcsFile, $stackPtr)) { + if ($this->processVariableAsStaticMember($phpcsFile, $stackPtr)) { Helpers::debug('found static member'); return; } - if ($this->checkForClassProperty($phpcsFile, $stackPtr)) { + if ($this->processVariableAsClassProperty($phpcsFile, $stackPtr)) { Helpers::debug('found class property definition'); return; } // Is the next non-whitespace an assignment? - if ($this->checkForAssignment($phpcsFile, $stackPtr, $varName, $currScope)) { + if ($this->processVariableAsAssignment($phpcsFile, $stackPtr, $varName, $currScope)) { Helpers::debug('found assignment'); return; } // OK, are we within a list (...) = construct? - if ($this->checkForListAssignment($phpcsFile, $stackPtr, $varName, $currScope)) { + if ($this->processVariableAsListAssignment($phpcsFile, $stackPtr, $varName, $currScope)) { Helpers::debug('found list assignment'); return; } // OK, are we within a [...] = construct? - if ($this->checkForListShorthandAssignment($phpcsFile, $stackPtr, $varName, $currScope)) { + if ($this->processVariableAsListShorthandAssignment($phpcsFile, $stackPtr, $varName, $currScope)) { Helpers::debug('found list shorthand assignment'); return; } // Are we a global declaration? - if ($this->checkForGlobalDeclaration($phpcsFile, $stackPtr, $varName, $currScope)) { + if ($this->processVariableAsGlobalDeclaration($phpcsFile, $stackPtr, $varName, $currScope)) { Helpers::debug('found global declaration'); return; } // Are we a static declaration? - if ($this->checkForStaticDeclaration($phpcsFile, $stackPtr, $varName, $currScope)) { + if ($this->processVariableAsStaticDeclaration($phpcsFile, $stackPtr, $varName, $currScope)) { Helpers::debug('found static declaration'); return; } // Are we a foreach loopvar? - if ($this->checkForForeachLoopVar($phpcsFile, $stackPtr, $varName, $currScope)) { + if ($this->processVariableAsForeachLoopVar($phpcsFile, $stackPtr, $varName, $currScope)) { Helpers::debug('found foreach loop variable'); return; } // Are we pass-by-reference to known pass-by-reference function? - if ($this->checkForPassByReferenceFunctionCall($phpcsFile, $stackPtr, $varName, $currScope)) { + if ($this->processVariableAsPassByReferenceFunctionCall($phpcsFile, $stackPtr, $varName, $currScope)) { Helpers::debug('found pass by reference'); return; } // Are we a numeric variable used for constructs like preg_replace? - if ($this->checkForNumericVariable($varName)) { + if ($this->processVariableAsNumericVariable($varName)) { Helpers::debug('found numeric variable'); return; } @@ -1310,6 +1385,7 @@ protected function processVariableInString(File $phpcsFile, $stackPtr) { if (!preg_match_all(Constants::getDoubleQuotedVarRegexp(), $token['content'], $matches)) { return; } + Helpers::debug("examining token for variable in string", $token); $currScope = Helpers::findVariableScope($phpcsFile, $stackPtr); if ($currScope === null) { @@ -1318,16 +1394,16 @@ protected function processVariableInString(File $phpcsFile, $stackPtr) { foreach ($matches[1] as $varName) { $varName = Helpers::normalizeVarName($varName); // Are we $this within a class? - if ($this->checkForThisWithinClass($phpcsFile, $stackPtr, $varName)) { + if ($this->processVariableAsThisWithinClass($phpcsFile, $stackPtr, $varName)) { continue; } - if ($this->checkForSuperGlobal($varName)) { + if ($this->processVariableAsSuperGlobal($varName)) { continue; } // Are we a numeric variable used for constructs like preg_replace? - if ($this->checkForNumericVariable($varName)) { + if ($this->processVariableAsNumericVariable($varName)) { continue; } @@ -1409,8 +1485,8 @@ protected function processCompact(File $phpcsFile, $stackPtr) { /** * Called to process the end of a scope. * - * Note that although triggered by the closing curly brace of the scope, $stackPtr is - * the scope conditional, not the closing curly brace. + * Note that although triggered by the closing curly brace of the scope, + * $stackPtr is the scope conditional, not the closing curly brace. * * @param File $phpcsFile The PHP_CodeSniffer file where this token was found. * @param int $stackPtr The position of the scope conditional. @@ -1435,6 +1511,7 @@ protected function processScopeClose(File $phpcsFile, $stackPtr) { * @return void */ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $varInfo, ScopeInfo $scopeInfo) { + Helpers::debug('processScopeCloseForVariable', $varInfo); if ($varInfo->ignoreUnused || isset($varInfo->firstRead)) { return; } @@ -1461,14 +1538,16 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v // the purposes of "unused variable" warnings. return; } + $stackPtr = null; - if (!empty($varInfo->firstDeclared)) { + if (! empty($varInfo->firstDeclared)) { $stackPtr = $varInfo->firstDeclared; - } elseif (!empty($varInfo->firstInitialized)) { + } elseif (! empty($varInfo->firstInitialized)) { $stackPtr = $varInfo->firstInitialized; } + if ($stackPtr) { - Helpers::debug("variable {$varInfo->name} at end of scope looks undefined"); + Helpers::debug("variable {$varInfo->name} at end of scope looks unused"); $phpcsFile->addWarning( "Unused %s %s.", $stackPtr, From f4e7a67edffa5c7ac034547fa386cc13b723480d Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 18:07:26 -0400 Subject: [PATCH 10/22] Allow ellipses in argument lists --- VariableAnalysis/Lib/Helpers.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index 7e90308b..e8121fda 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -137,6 +137,7 @@ public static function getFunctionIndexForFunctionArgument(File $phpcsFile, $sta $nonFunctionTokenTypes = array_values(Tokens::$emptyTokens); $nonFunctionTokenTypes[] = T_OPEN_PARENTHESIS; $nonFunctionTokenTypes[] = T_VARIABLE; + $nonFunctionTokenTypes[] = T_ELLIPSIS; $nonFunctionTokenTypes[] = T_COMMA; $nonFunctionTokenTypes[] = T_STRING; $nonFunctionTokenTypes[] = T_BITWISE_AND; @@ -179,6 +180,7 @@ public static function getUseIndexForUseImport(File $phpcsFile, $stackPtr) { $nonUseTokenTypes = array_values(Tokens::$emptyTokens); $nonUseTokenTypes[] = T_VARIABLE; + $nonUseTokenTypes[] = T_ELLIPSIS; $nonUseTokenTypes[] = T_COMMA; $nonUseTokenTypes[] = T_BITWISE_AND; $openParenPtr = self::getIntOrNull($phpcsFile->findPrevious($nonUseTokenTypes, $stackPtr - 1, null, true, null, true)); From b7d3566aa3fb5b3197dfd4276c1f3f65e5da7303 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 18:42:26 -0400 Subject: [PATCH 11/22] Only use T_FN if it exists --- VariableAnalysis/Lib/Helpers.php | 13 ++++++++++--- .../Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 13 ++++++++----- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index e8121fda..82519988 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -149,8 +149,10 @@ public static function getFunctionIndexForFunctionArgument(File $phpcsFile, $sta $functionTokenTypes = [ T_FUNCTION, T_CLOSURE, - T_FN, // TODO: cannot use this before PHP 7.4 ]; + if (defined('T_FN')) { + $functionTokenTypes[] = T_FN; + } if (!in_array($tokens[$functionPtr]['code'], $functionTokenTypes, true)) { return null; } @@ -436,8 +438,10 @@ private static function getStartOfTokenScope(File $phpcsFile, $stackPtr) { $functionTokenTypes = [ T_FUNCTION, T_CLOSURE, - T_FN, // TODO: cannot use this before PHP 7.4 ]; + if (defined('T_FN')) { + $functionTokenTypes[] = T_FN; + } foreach (array_reverse($conditions, true) as $scopePtr => $scopeCode) { if (in_array($scopeCode, $functionTokenTypes, true)) { return $scopePtr; @@ -494,9 +498,12 @@ public static function isTokenInsideArrowFunctionBody(File $phpcsFile, $stackPtr * @return ?int */ public static function getContainingArrowFunctionIndex(File $phpcsFile, $stackPtr) { + if (! defined('T_FN')) { + return null; + } $tokens = $phpcsFile->getTokens(); $enclosingScopeIndex = self::findVariableScopeExceptArrowFunctions($phpcsFile, $stackPtr); - $arrowFunctionIndex = $phpcsFile->findPrevious([T_FN], $stackPtr - 1, $enclosingScopeIndex); // TODO: cannot use T_FN before PHP 7.4 + $arrowFunctionIndex = $phpcsFile->findPrevious([T_FN], $stackPtr - 1, $enclosingScopeIndex); if (! is_int($arrowFunctionIndex)) { return null; } diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index ac1b9922..4794fef1 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -127,7 +127,7 @@ class VariableAnalysisSniff implements Sniff { * @return (int|string)[] */ public function register() { - return [ + $types = [ T_VARIABLE, T_DOUBLE_QUOTED_STRING, T_HEREDOC, @@ -138,8 +138,11 @@ public function register() { T_COMMA, T_SEMICOLON, T_CLOSE_PARENTHESIS, - T_FN, // TODO: we can't use this before php 7.4 so we need to replace it somehow ]; + if (defined('T_FN')) { + $types[] = T_FN; + } + return $types; } /** @@ -174,8 +177,10 @@ public function process(File $phpcsFile, $stackPtr) { $scopeStartTokenTypes = [ T_FUNCTION, T_CLOSURE, - T_FN, // TODO: we cannot use this before PHP 7.4 ]; + if (defined('T_FN')) { + $scopeStartTokenTypes[] = T_FN; + } $scopeIndexThisCloses = array_reduce($this->scopeStartIndices, function ($found, $index) use ($stackPtr, $tokens) { if ($stackPtr === $tokens[$index]['scope_closer']) { @@ -288,7 +293,6 @@ protected function getVariableInfo($varName, $currScope) { * @return VariableInfo */ protected function getOrCreateVariableInfo($varName, $currScope) { - // TODO: this needs to find the scope of an arrow function if we are inside one, which must also include the scope of its parent! $scopeInfo = $this->getOrCreateScopeInfo($currScope); if (!isset($scopeInfo->variables[$varName])) { Helpers::debug("creating a new variable for '{$varName}' in scope", $scopeInfo); @@ -472,7 +476,6 @@ protected function isVariableUndefined($varName, $stackPtr, $currScope) { if (isset($varInfo->firstInitialized) && $varInfo->firstInitialized <= $stackPtr) { return false; } - // TODO: if the variable is in an arrow function body, also check the enclosing scope return true; } From bf5a3ad27cf3e6841c27432c5bd07c5fab7260da Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 18:53:55 -0400 Subject: [PATCH 12/22] Don't assume token still exists when looking for closer --- .../Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 4794fef1..55d224d3 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -183,7 +183,11 @@ public function process(File $phpcsFile, $stackPtr) { } $scopeIndexThisCloses = array_reduce($this->scopeStartIndices, function ($found, $index) use ($stackPtr, $tokens) { - if ($stackPtr === $tokens[$index]['scope_closer']) { + $scopeCloserIndex = $tokens[$index]['scope_closer'] ?? null; + if (!$scopeCloserIndex) { + Helpers::debug('No scope closer found for scope start', $index); + } + if ($stackPtr === $scopeCloserIndex) { return $index; } return $found; From 77a01e23a62e952042bb9aea9beee0e8d84bf682 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 18:59:05 -0400 Subject: [PATCH 13/22] Remove null coalesce --- VariableAnalysis/Lib/Helpers.php | 4 ++-- .../Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index 82519988..d2157a10 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -434,7 +434,7 @@ private static function getStartOfTokenScope(File $phpcsFile, $stackPtr) { $token = $tokens[$stackPtr]; $in_class = false; - $conditions = $token['conditions'] ?? []; + $conditions = isset($token['conditions']) ? $token['conditions'] : []; $functionTokenTypes = [ T_FUNCTION, T_CLOSURE, @@ -473,7 +473,7 @@ private static function getStartOfTokenScope(File $phpcsFile, $stackPtr) { public static function isTokenInsideArrowFunctionDefinition(File $phpcsFile, $stackPtr) { $tokens = $phpcsFile->getTokens(); $token = $tokens[$stackPtr]; - $openParenIndices = $token['nested_parenthesis'] ?? []; + $openParenIndices = isset($token['nested_parenthesis']) ? $token['nested_parenthesis'] : []; if ($openParenIndices) { return false; } diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 55d224d3..0ef722aa 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -183,7 +183,7 @@ public function process(File $phpcsFile, $stackPtr) { } $scopeIndexThisCloses = array_reduce($this->scopeStartIndices, function ($found, $index) use ($stackPtr, $tokens) { - $scopeCloserIndex = $tokens[$index]['scope_closer'] ?? null; + $scopeCloserIndex = isset($tokens[$index]['scope_closer']) ? $tokens[$index]['scope_closer'] : null; if (!$scopeCloserIndex) { Helpers::debug('No scope closer found for scope start', $index); } From 4d3786e7f335ba8abf81a5e1d599c7eef45085ec Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 19:34:48 -0400 Subject: [PATCH 14/22] Remove unused functions --- VariableAnalysis/Lib/Helpers.php | 24 ------------------- .../CodeAnalysis/VariableAnalysisSniff.php | 4 ++-- 2 files changed, 2 insertions(+), 26 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index d2157a10..64d9ad57 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -27,20 +27,6 @@ public static function findContainingOpeningSquareBracket(File $phpcsFile, $stac return self::getIntOrNull($phpcsFile->findPrevious([T_OPEN_SHORT_ARRAY], $stackPtr - 1, $previousStatementPtr)); } - /** - * @param File $phpcsFile - * @param int $stackPtr - * - * @return ?int - */ - public static function findContainingClosingSquareBracket(File $phpcsFile, $stackPtr) { - $endOfStatementPtr = self::getIntOrNull($phpcsFile->findNext([T_SEMICOLON], $stackPtr + 1)); - if (! $endOfStatementPtr) { - return null; - } - return self::getIntOrNull($phpcsFile->findNext([T_CLOSE_SHORT_ARRAY], $stackPtr + 1, $endOfStatementPtr)); - } - /** * @param File $phpcsFile * @param int $stackPtr @@ -67,16 +53,6 @@ public static function findContainingOpeningBracket(File $phpcsFile, $stackPtr) return null; } - /** - * @param File $phpcsFile - * @param int $stackPtr - * - * @return ?int - */ - public static function findParenthesisOwner(File $phpcsFile, $stackPtr) { - return self::getIntOrNull($phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true)); - } - /** * @param (int|string)[] $conditions * diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 0ef722aa..b0049205 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -1085,7 +1085,7 @@ protected function processVariableAsForeachLoopVar(File $phpcsFile, $stackPtr, $ if (! is_int($openParenPtr)) { return false; } - $foreachPtr = Helpers::findParenthesisOwner($phpcsFile, $openParenPtr); + $foreachPtr = Helpers::getIntOrNull($phpcsFile->findPrevious(Tokens::$emptyTokens, $openParenPtr - 1, null, true)); if (! is_int($foreachPtr)) { return false; } @@ -1094,7 +1094,7 @@ protected function processVariableAsForeachLoopVar(File $phpcsFile, $stackPtr, $ if (! is_int($openParenPtr)) { return false; } - $foreachPtr = Helpers::findParenthesisOwner($phpcsFile, $openParenPtr); + $foreachPtr = Helpers::getIntOrNull($phpcsFile->findPrevious(Tokens::$emptyTokens, $openParenPtr - 1, null, true)); if (! is_int($foreachPtr)) { return false; } From 401b59ed2dae3dd5cc3ac3a8afe48f2a98761e81 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 19:40:46 -0400 Subject: [PATCH 15/22] Move isVariableANumericVariable to Helpers --- VariableAnalysis/Lib/Helpers.php | 9 +++++++++ .../Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 13 ++----------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index 64d9ad57..ffa8247c 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -544,4 +544,13 @@ public static function splitStringToArray($pattern, $value) { $result = preg_split($pattern, $value); return is_array($result) ? $result : []; } + + /** + * @param string $varName + * + * @return bool + */ + public static function isVariableANumericVariable($varName) { + return is_numeric(substr($varName, 0, 1)); + } } diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index b0049205..5fdff967 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -1060,15 +1060,6 @@ protected function processVariableAsStaticDeclaration(File $phpcsFile, $stackPtr return true; } - /** - * @param string $varName - * - * @return bool - */ - protected function processVariableAsNumericVariable($varName) { - return is_numeric(substr($varName, 0, 1)); - } - /** * @param File $phpcsFile * @param int $stackPtr @@ -1364,7 +1355,7 @@ protected function processVariable(File $phpcsFile, $stackPtr) { } // Are we a numeric variable used for constructs like preg_replace? - if ($this->processVariableAsNumericVariable($varName)) { + if (Helpers::isVariableANumericVariable($varName)) { Helpers::debug('found numeric variable'); return; } @@ -1410,7 +1401,7 @@ protected function processVariableInString(File $phpcsFile, $stackPtr) { } // Are we a numeric variable used for constructs like preg_replace? - if ($this->processVariableAsNumericVariable($varName)) { + if (Helpers::isVariableANumericVariable($varName)) { continue; } From 7e782f39b449f97652b1fb52f7c303f84b14f34c Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 20:11:55 -0400 Subject: [PATCH 16/22] Use isArrowFunction rather than T_FN in getContainingArrowFunctionIndex --- VariableAnalysis/Lib/Helpers.php | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index ffa8247c..74a1cfd0 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -474,12 +474,8 @@ public static function isTokenInsideArrowFunctionBody(File $phpcsFile, $stackPtr * @return ?int */ public static function getContainingArrowFunctionIndex(File $phpcsFile, $stackPtr) { - if (! defined('T_FN')) { - return null; - } $tokens = $phpcsFile->getTokens(); - $enclosingScopeIndex = self::findVariableScopeExceptArrowFunctions($phpcsFile, $stackPtr); - $arrowFunctionIndex = $phpcsFile->findPrevious([T_FN], $stackPtr - 1, $enclosingScopeIndex); + $arrowFunctionIndex = self::getPreviousArrowFunctionIndex($phpcsFile, $stackPtr); if (! is_int($arrowFunctionIndex)) { return null; } @@ -492,6 +488,22 @@ public static function getContainingArrowFunctionIndex(File $phpcsFile, $stackPt return null; } + /** + * @param File $phpcsFile + * @param int $stackPtr + * + * @return ?int + */ + private static function getPreviousArrowFunctionIndex(File $phpcsFile, $stackPtr) { + $enclosingScopeIndex = self::findVariableScopeExceptArrowFunctions($phpcsFile, $stackPtr); + for ($index = $stackPtr - 1; $index > $enclosingScopeIndex; $index--) { + if (FunctionDeclarations::isArrowFunction($phpcsFile, $index)) { + return $index; + } + } + return null; + } + /** * @param File $phpcsFile * @param int $stackPtr From 0bd2b147b396b60bd594ed3f124b13628e93602d Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 20:14:52 -0400 Subject: [PATCH 17/22] Use isArrowFunction in getFunctionIndexForFunctionArgument for T_FN --- VariableAnalysis/Lib/Helpers.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index 74a1cfd0..c63cb03c 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -126,10 +126,7 @@ public static function getFunctionIndexForFunctionArgument(File $phpcsFile, $sta T_FUNCTION, T_CLOSURE, ]; - if (defined('T_FN')) { - $functionTokenTypes[] = T_FN; - } - if (!in_array($tokens[$functionPtr]['code'], $functionTokenTypes, true)) { + if (!in_array($tokens[$functionPtr]['code'], $functionTokenTypes, true) && ! FunctionDeclarations::isArrowFunction($phpcsFile, $functionPtr)) { return null; } return $functionPtr; From f8ef8edb65db5902d3769b9a724ec3c536c20f34 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 20:16:26 -0400 Subject: [PATCH 18/22] Replace T_FN with isArrowFunction in getStartOfTokenScope --- VariableAnalysis/Lib/Helpers.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index c63cb03c..8fe09241 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -412,11 +412,8 @@ private static function getStartOfTokenScope(File $phpcsFile, $stackPtr) { T_FUNCTION, T_CLOSURE, ]; - if (defined('T_FN')) { - $functionTokenTypes[] = T_FN; - } foreach (array_reverse($conditions, true) as $scopePtr => $scopeCode) { - if (in_array($scopeCode, $functionTokenTypes, true)) { + if (in_array($scopeCode, $functionTokenTypes, true) || FunctionDeclarations::isArrowFunction($phpcsFile, $scopePtr)) { return $scopePtr; } if (isset(Tokens::$ooScopeTokens[$scopeCode]) === true) { From 39ddb52e116e2c3cc0f49b8a9d3e00ac92b6b7c1 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 20:18:09 -0400 Subject: [PATCH 19/22] Replace T_FN in process --- .../Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 5fdff967..76a1eea7 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -11,6 +11,7 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\Lists; +use PHPCSUtils\Utils\FunctionDeclarations; class VariableAnalysisSniff implements Sniff { /** @@ -178,9 +179,6 @@ public function process(File $phpcsFile, $stackPtr) { T_FUNCTION, T_CLOSURE, ]; - if (defined('T_FN')) { - $scopeStartTokenTypes[] = T_FN; - } $scopeIndexThisCloses = array_reduce($this->scopeStartIndices, function ($found, $index) use ($stackPtr, $tokens) { $scopeCloserIndex = isset($tokens[$index]['scope_closer']) ? $tokens[$index]['scope_closer'] : null; @@ -220,7 +218,7 @@ public function process(File $phpcsFile, $stackPtr) { $this->markAllVariablesRead($phpcsFile, $stackPtr); return; } - if (isset($token['scope_condition']) && in_array($token['code'], $scopeStartTokenTypes, true)) { + if (isset($token['scope_condition']) && (in_array($token['code'], $scopeStartTokenTypes, true) || FunctionDeclarations::isArrowFunction($phpcsFile, $stackPtr))) { Helpers::debug('found scope condition', $token); $this->scopeStartIndices[] = $token['scope_condition']; return; From 2a59db1de8d258271bec98a250affe52473eaa46 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 20:22:25 -0400 Subject: [PATCH 20/22] Stop using scope_condition for scope conditions in process --- .../Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 76a1eea7..27b78d46 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -218,9 +218,11 @@ public function process(File $phpcsFile, $stackPtr) { $this->markAllVariablesRead($phpcsFile, $stackPtr); return; } - if (isset($token['scope_condition']) && (in_array($token['code'], $scopeStartTokenTypes, true) || FunctionDeclarations::isArrowFunction($phpcsFile, $stackPtr))) { + if (in_array($token['code'], $scopeStartTokenTypes, true) + || FunctionDeclarations::isArrowFunction($phpcsFile, $stackPtr) + ) { Helpers::debug('found scope condition', $token); - $this->scopeStartIndices[] = $token['scope_condition']; + $this->scopeStartIndices[] = $stackPtr; return; } } From e9fce57d1d9268746db10a6c979a48aa7b5633ed Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 20:23:48 -0400 Subject: [PATCH 21/22] Stop using scope_condition in findVariableScopeExceptArrowFunctions --- VariableAnalysis/Lib/Helpers.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index 8fe09241..87b4fc88 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -380,8 +380,7 @@ public static function findVariableScopeExceptArrowFunctions(File $phpcsFile, $s if (! is_int($functionPtr)) { throw new \Exception("Function index not found for function argument index {$stackPtr}"); } - $functionToken = $tokens[$functionPtr]; - return $functionToken['scope_condition']; + return $functionPtr; } self::debug('Cannot find function scope for variable at', $stackPtr); From 79dec4c558eb87e5e8f4d299f5f9ebadc8b10561 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Mon, 6 Jul 2020 20:29:07 -0400 Subject: [PATCH 22/22] Replace scope_opener/closer with getArrowFunctionOpenClose --- VariableAnalysis/Lib/Helpers.php | 10 ++++++---- .../Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 6 +++++- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index 87b4fc88..574c4538 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -467,14 +467,16 @@ public static function isTokenInsideArrowFunctionBody(File $phpcsFile, $stackPtr * @return ?int */ public static function getContainingArrowFunctionIndex(File $phpcsFile, $stackPtr) { - $tokens = $phpcsFile->getTokens(); $arrowFunctionIndex = self::getPreviousArrowFunctionIndex($phpcsFile, $stackPtr); if (! is_int($arrowFunctionIndex)) { return null; } - $arrowFunctionToken = $tokens[$arrowFunctionIndex]; - $arrowFunctionScopeStart = $arrowFunctionToken['scope_opener']; - $arrowFunctionScopeEnd = $arrowFunctionToken['scope_closer']; + $arrowFunctionInfo = FunctionDeclarations::getArrowFunctionOpenClose($phpcsFile, $arrowFunctionIndex); + if (! $arrowFunctionInfo) { + return null; + } + $arrowFunctionScopeStart = $arrowFunctionInfo['scope_opener']; + $arrowFunctionScopeEnd = $arrowFunctionInfo['scope_closer']; if ($stackPtr > $arrowFunctionScopeStart && $stackPtr < $arrowFunctionScopeEnd) { return $arrowFunctionIndex; } diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 27b78d46..307e239d 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -180,8 +180,12 @@ public function process(File $phpcsFile, $stackPtr) { T_CLOSURE, ]; - $scopeIndexThisCloses = array_reduce($this->scopeStartIndices, function ($found, $index) use ($stackPtr, $tokens) { + $scopeIndexThisCloses = array_reduce($this->scopeStartIndices, function ($found, $index) use ($phpcsFile, $stackPtr, $tokens) { $scopeCloserIndex = isset($tokens[$index]['scope_closer']) ? $tokens[$index]['scope_closer'] : null; + if (FunctionDeclarations::isArrowFunction($phpcsFile, $index)) { + $arrowFunctionInfo = FunctionDeclarations::getArrowFunctionOpenClose($phpcsFile, $index); + $scopeCloserIndex = $arrowFunctionInfo ? $arrowFunctionInfo['scope_closer'] : $scopeCloserIndex; + } if (!$scopeCloserIndex) { Helpers::debug('No scope closer found for scope start', $index); }