From 5620b4360bbbe3032b75c515658b1e7de33eb1e4 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sat, 11 Jul 2020 16:56:15 -0400 Subject: [PATCH] Support global scope (#190) * Fix indentation in VariableAnalysisTest * Add global scope tests * Add global scope to processScopeClose * Fix AnonymousClassWithPropertiesFixture test * Add getScopeCloseForScopeOpen helper * Remove findWhereAssignExecuted * Fix ThisWithinNestedClosedScopeFixture test * Enable test for foreach in global scope * Disable allowUnusedForeachVariables in FunctionWithForeachFixture test * Allow a token to close more than one scope * Fix FunctionWithGlobalVarFixture test --- .../VariableAnalysisSniff/GlobalScopeTest.php | 18 +++++ .../VariableAnalysisTest.php | 47 ++++++------ .../AnonymousClassWithPropertiesFixture.php | 2 +- .../fixtures/FunctionWithGlobalVarFixture.php | 2 + .../fixtures/GlobalScopeFixture.php | 7 ++ VariableAnalysis/Lib/Helpers.php | 72 ++++++++++--------- .../CodeAnalysis/VariableAnalysisSniff.php | 29 ++++---- 7 files changed, 104 insertions(+), 73 deletions(-) create mode 100644 Tests/VariableAnalysisSniff/GlobalScopeTest.php create mode 100644 Tests/VariableAnalysisSniff/fixtures/GlobalScopeFixture.php diff --git a/Tests/VariableAnalysisSniff/GlobalScopeTest.php b/Tests/VariableAnalysisSniff/GlobalScopeTest.php new file mode 100644 index 00000000..eb34b11b --- /dev/null +++ b/Tests/VariableAnalysisSniff/GlobalScopeTest.php @@ -0,0 +1,18 @@ +getFixture('GlobalScopeFixture.php'); + $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->process(); + $lines = $this->getWarningLineNumbersFromFile($phpcsFile); + $expectedErrors = [ + 4, + 7, + ]; + $this->assertEquals($expectedErrors, $lines); + } +} diff --git a/Tests/VariableAnalysisSniff/VariableAnalysisTest.php b/Tests/VariableAnalysisSniff/VariableAnalysisTest.php index d148abda..2859b94a 100644 --- a/Tests/VariableAnalysisSniff/VariableAnalysisTest.php +++ b/Tests/VariableAnalysisSniff/VariableAnalysisTest.php @@ -114,6 +114,11 @@ public function testFunctionWithForeachErrors() { public function testFunctionWithForeachWarnings() { $fixtureFile = $this->getFixture('FunctionWithForeachFixture.php'); $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->ruleset->setSniffProperty( + 'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff', + 'allowUnusedForeachVariables', + 'false' + ); $phpcsFile->process(); $lines = $this->getWarningLineNumbersFromFile($phpcsFile); $expectedWarnings = [ @@ -129,10 +134,7 @@ public function testFunctionWithForeachWarnings() { 50, 52, 54, - // FIXME: this is an unused variable that needs to be fixed but for now - // we will ignore it. See - // https://github.com/sirbrillig/phpcs-variable-analysis/pull/36 - // 67, + 67, ]; $this->assertEquals($expectedWarnings, $lines); } @@ -585,6 +587,7 @@ public function testAnonymousClassAllowPropertyDefinitions() { $lines = $this->getWarningLineNumbersFromFile($phpcsFile); $expectedWarnings = [ 17, + 26, 38, ]; $this->assertEquals($expectedWarnings, $lines); @@ -767,7 +770,7 @@ public function testValidUndefinedVariableNamesIgnoresVarsInGlobalScope() { 7, 23, 39, - 54, + 54, ]; $this->assertEquals($expectedWarnings, $lines); } @@ -939,10 +942,10 @@ public function testGetDefinedVarsCountsAsRead() { $phpcsFile->process(); $lines = $this->getWarningLineNumbersFromFile($phpcsFile); $expectedWarnings = [ - 6, - 18, - 22, - 29, + 6, + 18, + 22, + 29, ]; $this->assertEquals($expectedWarnings, $lines); } @@ -953,12 +956,14 @@ public function testThisWithinNestedClosedScope() { $phpcsFile->process(); $lines = $this->getWarningLineNumbersFromFile($phpcsFile); $expectedWarnings = [ - 5, - 8, - 20, - 33, - 47, - 61, + 5, + 8, + 15, + 20, + 33, + 41, + 47, + 61, ]; $this->assertEquals($expectedWarnings, $lines); } @@ -969,12 +974,12 @@ public function testUnusedVarWithValueChange() { $phpcsFile->process(); $lines = $this->getWarningLineNumbersFromFile($phpcsFile); $expectedWarnings = [ - 5, - 6, - 8, - 9, - 11, - 12, + 5, + 6, + 8, + 9, + 11, + 12, ]; $this->assertEquals($expectedWarnings, $lines); } diff --git a/Tests/VariableAnalysisSniff/fixtures/AnonymousClassWithPropertiesFixture.php b/Tests/VariableAnalysisSniff/fixtures/AnonymousClassWithPropertiesFixture.php index 832b5399..ad59a105 100644 --- a/Tests/VariableAnalysisSniff/fixtures/AnonymousClassWithPropertiesFixture.php +++ b/Tests/VariableAnalysisSniff/fixtures/AnonymousClassWithPropertiesFixture.php @@ -23,7 +23,7 @@ public function methodWithStaticVar() { } } -$anonClass = new class() { +$anonClass = new class() { // should trigger unused warning protected $storedHello; private static $storedHello2; private $storedHello3; diff --git a/Tests/VariableAnalysisSniff/fixtures/FunctionWithGlobalVarFixture.php b/Tests/VariableAnalysisSniff/fixtures/FunctionWithGlobalVarFixture.php index 36e0f253..4874041d 100644 --- a/Tests/VariableAnalysisSniff/fixtures/FunctionWithGlobalVarFixture.php +++ b/Tests/VariableAnalysisSniff/fixtures/FunctionWithGlobalVarFixture.php @@ -55,3 +55,5 @@ function globalWithUnusedFunctionArg($user_type, $text, $testvar) { // should wa global $config; return $config . $text . $user_type; } + +echo $sunday; diff --git a/Tests/VariableAnalysisSniff/fixtures/GlobalScopeFixture.php b/Tests/VariableAnalysisSniff/fixtures/GlobalScopeFixture.php new file mode 100644 index 00000000..98958af0 --- /dev/null +++ b/Tests/VariableAnalysisSniff/fixtures/GlobalScopeFixture.php @@ -0,0 +1,7 @@ +getTokens(); - - // Write should be recorded at the next statement to ensure we treat the - // assign as happening after the RHS execution. - // eg: $var = $var + 1; -> RHS could still be undef. - // However, if we're within a bracketed expression, we take place at the - // closing bracket, if that's first. - // eg: echo (($var = 12) && ($var == 12)); - $semicolonPtr = $phpcsFile->findNext([T_SEMICOLON], $stackPtr + 1, null, false, null, true); - $commaPtr = $phpcsFile->findNext([T_COMMA], $stackPtr + 1, null, false, null, true); - $closePtr = false; - $openPtr = Helpers::findContainingOpeningBracket($phpcsFile, $stackPtr); - if ($openPtr !== null) { - if (isset($tokens[$openPtr]['parenthesis_closer'])) { - $closePtr = $tokens[$openPtr]['parenthesis_closer']; - } - } - - // Return the first thing: comma, semicolon, close-bracket, or stackPtr if nothing else - $assignEndTokens = [$commaPtr, $semicolonPtr, $closePtr]; - $assignEndTokens = array_filter($assignEndTokens); // remove false values - sort($assignEndTokens); - if (empty($assignEndTokens)) { - return $stackPtr; - } - return $assignEndTokens[0]; - } - /** * @param File $phpcsFile * @param int $stackPtr @@ -649,4 +614,41 @@ public static function getAttachedBlockIndicesForElse(File $phpcsFile, $stackPtr public static function isIndexInsideScope($needle, $scopeStart, $scopeEnd) { return ($needle > $scopeStart && $needle < $scopeEnd); } + + /** + * @param File $phpcsFile + * @param int $scopeStartIndex + * + * @return int + */ + public static function getScopeCloseForScopeOpen(File $phpcsFile, $scopeStartIndex) { + $tokens = $phpcsFile->getTokens(); + $scopeCloserIndex = isset($tokens[$scopeStartIndex]['scope_closer']) ? $tokens[$scopeStartIndex]['scope_closer'] : null; + + if (FunctionDeclarations::isArrowFunction($phpcsFile, $scopeStartIndex)) { + $arrowFunctionInfo = FunctionDeclarations::getArrowFunctionOpenClose($phpcsFile, $scopeStartIndex); + $scopeCloserIndex = $arrowFunctionInfo ? $arrowFunctionInfo['scope_closer'] : $scopeCloserIndex; + } + + if ($scopeStartIndex === 0) { + $scopeCloserIndex = Helpers::getLastNonEmptyTokenIndexInFile($phpcsFile); + } + return $scopeCloserIndex; + } + + /** + * @param File $phpcsFile + * + * @return int + */ + public static function getLastNonEmptyTokenIndexInFile(File $phpcsFile) { + $tokens = $phpcsFile->getTokens(); + foreach (array_reverse($tokens, true) as $index => $token) { + if (! in_array($token['code'], Tokens::$emptyTokens, true)) { + return $index; + } + } + self::debug('no non-empty token found for end of file'); + return 0; + } } diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 2d75e921..c6550703 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -33,7 +33,7 @@ class VariableAnalysisSniff implements Sniff { * * @var int[] */ - private $scopeStartIndices = []; + private $scopeStartIndices = [0]; /** * A list of custom functions which pass in variables to be initialized by @@ -180,21 +180,20 @@ public function process(File $phpcsFile, $stackPtr) { T_CLOSURE, ]; - $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; - } + $scopeIndicesThisCloses = array_reduce($this->scopeStartIndices, function ($found, $scopeStartIndex) use ($phpcsFile, $stackPtr) { + $scopeCloserIndex = Helpers::getScopeCloseForScopeOpen($phpcsFile, $scopeStartIndex); + if (!$scopeCloserIndex) { - Helpers::debug('No scope closer found for scope start', $index); + Helpers::debug('No scope closer found for scope start', $scopeStartIndex); } + if ($stackPtr === $scopeCloserIndex) { - return $index; + $found[] = $scopeStartIndex; } return $found; - }, null); - if ($scopeIndexThisCloses) { + }, []); + + foreach ($scopeIndicesThisCloses as $scopeIndexThisCloses) { Helpers::debug('found closing scope at', $stackPtr, 'for scope', $scopeIndexThisCloses); $this->processScopeClose($phpcsFile, $scopeIndexThisCloses); } @@ -856,8 +855,6 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN return false; } - $writtenPtr = Helpers::findWhereAssignExecuted($phpcsFile, $assignPtr); - // If the right-hand-side of the assignment to this variable is a reference // variable, then this variable is a reference to that one, and as such any // assignment to this variable (except another assignment by reference, @@ -877,14 +874,14 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN // actually need to mark it as used in this case because the act of this // assignment will mark it used on the next token. $varInfo->referencedVariableScope = $currScope; - $this->markVariableDeclaration($varName, ScopeType::LOCAL, null, $writtenPtr, $currScope, true); + $this->markVariableDeclaration($varName, ScopeType::LOCAL, null, $stackPtr, $currScope, true); // An assignment to a reference is a binding and should not count as // initialization since it doesn't change any values. - $this->markVariableAssignmentWithoutInitialization($varName, $writtenPtr, $currScope); + $this->markVariableAssignmentWithoutInitialization($varName, $stackPtr, $currScope); return true; } - $this->markVariableAssignment($varName, $writtenPtr, $currScope); + $this->markVariableAssignment($varName, $stackPtr, $currScope); return true; }