diff --git a/Tests/VariableAnalysisSniff/ForLoopTest.php b/Tests/VariableAnalysisSniff/ForLoopTest.php new file mode 100644 index 0000000..de10478 --- /dev/null +++ b/Tests/VariableAnalysisSniff/ForLoopTest.php @@ -0,0 +1,47 @@ +getFixture('FunctionWithForLoopFixture.php'); + $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->process(); + $lines = $this->getWarningLineNumbersFromFile($phpcsFile); + $expectedWarnings = [ + 22, + 65, + 80, + 94, + 110, + 112, + 113, + 118, + 123, + 129, + ]; + $this->assertSame($expectedWarnings, $lines); + } + + public function testFunctionWithForLoopWarningsHasCorrectSniffCodes() + { + $fixtureFile = $this->getFixture('FunctionWithForLoopFixture.php'); + $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile); + $phpcsFile->process(); + $warnings = $phpcsFile->getWarnings(); + $this->assertSame(self::UNDEFINED_ERROR_CODE, $warnings[22][17][0]['source']); + $this->assertSame(self::UNDEFINED_ERROR_CODE, $warnings[65][16][0]['source']); + $this->assertSame(self::UNDEFINED_ERROR_CODE, $warnings[80][16][0]['source']); + $this->assertSame(self::UNUSED_ERROR_CODE, $warnings[94][5][0]['source']); + $this->assertSame(self::UNUSED_ERROR_CODE, $warnings[110][7][0]['source']); + $this->assertSame(self::UNUSED_ERROR_CODE, $warnings[112][7][0]['source']); + $this->assertSame(self::UNDEFINED_ERROR_CODE, $warnings[113][16][0]['source']); + $this->assertSame(self::UNUSED_ERROR_CODE, $warnings[118][5][0]['source']); + $this->assertSame(self::UNDEFINED_ERROR_CODE, $warnings[123][5][0]['source']); + $this->assertSame(self::UNDEFINED_ERROR_CODE, $warnings[129][14][0]['source']); + } +} diff --git a/Tests/VariableAnalysisSniff/fixtures/FunctionWithForLoopFixture.php b/Tests/VariableAnalysisSniff/fixtures/FunctionWithForLoopFixture.php new file mode 100644 index 0000000..0f48f45 --- /dev/null +++ b/Tests/VariableAnalysisSniff/fixtures/FunctionWithForLoopFixture.php @@ -0,0 +1,164 @@ + 10) { + break; + } + } +} + +function closureInsideLoopInit() { + for ( + $closure = function( + $a, + $b, + $c // Unused variable $c + ) { + $m = 1; // Unused variable $m + var_dump($i); // Undefined variable $i + return $a * $b; + }, + $a = 0, + $b = 0, + $c = 0, // Unused variable $c + $i = 10; + $closure($a, $b, 4) < $i; + $i++, + $a++, + $t++, // Undefined variable $t + $b++ + ) + { + var_dump($a); + var_dump($b); + var_dump($m); // Undefined variable $m + } +} + +function veryBoringForLoop() { + for ( + ; + ; + ) { + } +} + +function reallySmallForLoop() { + for ($i = 1, $j = 0; $i <= 10; $j += $i, print $i, $i++); +} + +function colonSyntaxForLoop() { + for ($i = 0; $i < 10; $i++): + print $i; + endfor; +} + +function colonSyntaxForLoopAndLateDefinition() { + for ($i = 0; $i < 10; var_dump($foo), $i++): + $foo = 'hello'; + print $i; + endfor; +} + +function forLoopWithOneStatement() { + for ($i = 0; $i < 10; $i++) echo $i; +} + +function forLoopWithOneStatementAndLateDefinition() { + for ($i = 0; $i < 10; var_dump($foo), $i++) $foo = 'hello'; +} diff --git a/VariableAnalysis/Lib/ForLoopInfo.php b/VariableAnalysis/Lib/ForLoopInfo.php new file mode 100644 index 0000000..e067873 --- /dev/null +++ b/VariableAnalysis/Lib/ForLoopInfo.php @@ -0,0 +1,114 @@ + + */ + public $incrementVariables = []; + + /** + * @param int $forIndex + * @param int $blockStart + * @param int $blockEnd + * @param int $initStart + * @param int $initEnd + * @param int $conditionStart + * @param int $conditionEnd + * @param int $incrementStart + * @param int $incrementEnd + */ + public function __construct( + $forIndex, + $blockStart, + $blockEnd, + $initStart, + $initEnd, + $conditionStart, + $conditionEnd, + $incrementStart, + $incrementEnd + ) { + $this->forIndex = $forIndex; + $this->blockStart = $blockStart; + $this->blockEnd = $blockEnd; + $this->initStart = $initStart; + $this->initEnd = $initEnd; + $this->conditionStart = $conditionStart; + $this->conditionEnd = $conditionEnd; + $this->incrementStart = $incrementStart; + $this->incrementEnd = $incrementEnd; + } +} diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index 76fe005..b818bc5 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -4,6 +4,7 @@ use PHP_CodeSniffer\Files\File; use VariableAnalysis\Lib\ScopeInfo; +use VariableAnalysis\Lib\ForLoopInfo; use VariableAnalysis\Lib\ScopeType; use VariableAnalysis\Lib\VariableInfo; use PHP_CodeSniffer\Util\Tokens; @@ -142,12 +143,17 @@ public static function isTokenFunctionParameter(File $phpcsFile, $stackPtr) } /** + * Return true if the token is inside the arguments of a function call. + * + * For example, the variable `$foo` in `doSomething($foo)` is inside the + * arguments to the call to `doSomething()`. + * * @param File $phpcsFile * @param int $stackPtr * * @return bool */ - public static function isTokenInsideFunctionCall(File $phpcsFile, $stackPtr) + public static function isTokenInsideFunctionCallArgument(File $phpcsFile, $stackPtr) { return is_int(self::getFunctionIndexForFunctionCallArgument($phpcsFile, $stackPtr)); } @@ -631,7 +637,7 @@ public static function getArrowFunctionOpenClose(File $phpcsFile, $stackPtr) T_CLOSE_CURLY_BRACKET, T_CLOSE_SHORT_ARRAY, ]; - $scopeCloserIndex = $phpcsFile->findNext($endScopeTokens, $fatArrowIndex + 1); + $scopeCloserIndex = $phpcsFile->findNext($endScopeTokens, $fatArrowIndex + 1); if (! is_int($scopeCloserIndex)) { return null; } @@ -985,6 +991,9 @@ public static function isRequireInScopeAfter(File $phpcsFile, VariableInfo $varI /** * Find the index of the function keyword for a token in a function call's arguments * + * For the variable `$foo` in the expression `doSomething($foo)`, this will + * return the index of the `doSomething` token. + * * @param File $phpcsFile * @param int $stackPtr * @@ -1008,7 +1017,10 @@ public static function getFunctionIndexForFunctionCallArgument(File $phpcsFile, if (! is_int($functionPtr) || ! isset($tokens[$functionPtr]['code'])) { return null; } - if ($tokens[$functionPtr]['code'] === 'function' || ($tokens[$functionPtr]['content'] === 'fn' && self::isArrowFunction($phpcsFile, $functionPtr))) { + if ($tokens[$functionPtr]['content'] === 'function' || ($tokens[$functionPtr]['content'] === 'fn' && self::isArrowFunction($phpcsFile, $functionPtr))) { + return null; + } + if (! empty($tokens[$functionPtr]['scope_opener'])) { return null; } return $functionPtr; @@ -1168,4 +1180,102 @@ public static function isTokenVariableVariable(File $phpcsFile, $stackPtr) } return false; } + + /** + * @param File $phpcsFile + * @param int $stackPtr + * + * @return ForLoopInfo + */ + public static function makeForLoopInfo(File $phpcsFile, $stackPtr) + { + $tokens = $phpcsFile->getTokens(); + $token = $tokens[$stackPtr]; + $forIndex = $stackPtr; + $blockStart = $token['parenthesis_closer']; + if (isset($token['scope_opener'])) { + $blockStart = $token['scope_opener']; + $blockEnd = $token['scope_closer']; + } else { + // Some for loop blocks will not have scope positions because it they are + // inline (no curly braces) so we have to find the end of their scope by + // looking for the end of the next statement. + $nextSemicolonIndex = $phpcsFile->findNext([T_SEMICOLON], $token['parenthesis_closer']); + if (! is_int($nextSemicolonIndex)) { + $nextSemicolonIndex = $token['parenthesis_closer'] + 1; + } + $blockEnd = $nextSemicolonIndex; + } + $initStart = intval($token['parenthesis_opener']) + 1; + $initEnd = null; + $conditionStart = null; + $conditionEnd = null; + $incrementStart = null; + $incrementEnd = $token['parenthesis_closer'] - 1; + + $semicolonCount = 0; + $forLoopLevel = $tokens[$forIndex]['level']; + $forLoopNestedParensCount = 1; + + if (isset($tokens[$forIndex]['nested_parenthesis'])) { + $forLoopNestedParensCount = count($tokens[$forIndex]['nested_parenthesis']) + 1; + } + + for ($i = $initStart; ($i <= $incrementEnd && $semicolonCount < 2); $i++) { + if ($tokens[$i]['code'] !== T_SEMICOLON) { + continue; + } + + if ($tokens[$i]['level'] !== $forLoopLevel) { + continue; + } + + if (count($tokens[$i]['nested_parenthesis']) !== $forLoopNestedParensCount) { + continue; + } + + switch ($semicolonCount) { + case 0: + $initEnd = $i; + $conditionStart = $initEnd + 1; + break; + case 1: + $conditionEnd = $i; + $incrementStart = $conditionEnd + 1; + break; + } + $semicolonCount += 1; + } + + if ($initEnd === null || $conditionStart === null || $conditionEnd === null || $incrementStart === null) { + throw new \Exception("Cannot parse for loop at position {$forIndex}"); + } + + return new ForLoopInfo( + $forIndex, + $blockStart, + $blockEnd, + $initStart, + $initEnd, + $conditionStart, + $conditionEnd, + $incrementStart, + $incrementEnd + ); + } + + /** + * @param int $stackPtr + * @param array $forLoops + * @return ForLoopInfo|null + */ + public static function getForLoopForIncrementVariable($stackPtr, $forLoops) + { + foreach ($forLoops as $forLoop) { + if ($stackPtr > $forLoop->incrementStart && $stackPtr < $forLoop->incrementEnd) { + return $forLoop; + } + } + return null; + } } diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 6b5e2e5..a8f2b6c 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -26,6 +26,13 @@ class VariableAnalysisSniff implements Sniff */ private $scopeManager; + /** + * A list of for loops, keyed by the index of their first token in this file. + * + * @var array + */ + private $forLoops = []; + /** * A list of custom functions which pass in variables to be initialized by * reference (eg `preg_match()`) and therefore should not require those @@ -162,6 +169,8 @@ public function register() T_COMMA, T_SEMICOLON, T_CLOSE_PARENTHESIS, + T_FOR, + T_ENDFOR, ]; if (defined('T_FN')) { $types[] = T_FN; @@ -216,6 +225,7 @@ public function process(File $phpcsFile, $stackPtr) // easily accessed in other places which aren't passed the object. if ($this->currentFile !== $phpcsFile) { $this->currentFile = $phpcsFile; + $this->forLoops = []; } // Add the global scope for the current file to our scope indexes. @@ -228,6 +238,10 @@ public function process(File $phpcsFile, $stackPtr) // variables if the current token closes scopes. $this->searchForAndProcessClosingScopesAt($phpcsFile, $stackPtr); + // Scan variables that were postponed because they exist in the increment + // expression of a for loop if the current token closes a loop. + $this->processClosingForLoopsAt($phpcsFile, $stackPtr); + // Find and process variables to perform two jobs: to record variable // definition or use, and to report variables as undefined if they are used // without having been first defined. @@ -244,6 +258,13 @@ public function process(File $phpcsFile, $stackPtr) return; } + // Record for loop boundaries so we can delay scanning the third for loop + // expression until after the loop has been scanned. + if ($token['code'] === T_FOR) { + $this->recordForLoop($phpcsFile, $stackPtr); + return; + } + // If the current token is a call to `get_defined_vars()`, consider that a // usage of all variables in the current scope. if ($this->isGetDefinedVars($phpcsFile, $stackPtr)) { @@ -265,6 +286,19 @@ public function process(File $phpcsFile, $stackPtr) } } + /** + * Record the boundaries of a for loop. + * + * @param File $phpcsFile + * @param int $stackPtr + * + * @return void + */ + private function recordForLoop($phpcsFile, $stackPtr) + { + $this->forLoops[$stackPtr] = Helpers::makeForLoopInfo($phpcsFile, $stackPtr); + } + /** * Find scopes closed by a token and process their variables. * @@ -279,12 +313,40 @@ private function searchForAndProcessClosingScopesAt($phpcsFile, $stackPtr) { $scopeIndicesThisCloses = $this->scopeManager->getScopesForScopeEnd($phpcsFile->getFilename(), $stackPtr); + $tokens = $phpcsFile->getTokens(); + $token = $tokens[$stackPtr]; + $line = $token['line']; foreach ($scopeIndicesThisCloses as $scopeIndexThisCloses) { - Helpers::debug('found closing scope at index', $stackPtr, 'for scopes starting at:', $scopeIndexThisCloses); + Helpers::debug('found closing scope at index', $stackPtr, 'line', $line, 'for scopes starting at:', $scopeIndexThisCloses->scopeStartIndex); $this->processScopeClose($phpcsFile, $scopeIndexThisCloses->scopeStartIndex); } } + /** + * Scan variables that were postponed because they exist in the increment expression of a for loop. + * + * @param File $phpcsFile + * @param int $stackPtr + * + * @return void + */ + private function processClosingForLoopsAt($phpcsFile, $stackPtr) + { + $forLoopsThisCloses = []; + foreach ($this->forLoops as $forLoop) { + if ($forLoop->blockEnd === $stackPtr) { + $forLoopsThisCloses[] = $forLoop; + } + } + + foreach ($forLoopsThisCloses as $forLoop) { + foreach ($forLoop->incrementVariables as $varIndex => $varInfo) { + Helpers::debug('processing delayed for loop increment variable at', $varIndex, $varInfo); + $this->processVariable($phpcsFile, $varIndex, ['ignore-for-loops' => true]); + } + } + } + /** * Return true if the token is a call to `get_defined_vars()`. * @@ -362,7 +424,7 @@ protected function getOrCreateVariableInfo($varName, $currScope) Helpers::debug("getOrCreateVariableInfo: starting for '{$varName}'"); $scopeInfo = $this->getOrCreateScopeInfo($currScope); if (isset($scopeInfo->variables[$varName])) { - Helpers::debug("getOrCreateVariableInfo: found scope for '{$varName}'", $scopeInfo); + Helpers::debug("getOrCreateVariableInfo: found variable for '{$varName}'", $scopeInfo->variables[$varName]); return $scopeInfo->variables[$varName]; } Helpers::debug("getOrCreateVariableInfo: creating a new variable for '{$varName}' in scope", $scopeInfo); @@ -566,7 +628,7 @@ protected function markVariableRead($varName, $stackPtr, $currScope) protected function isVariableUndefined($varName, $stackPtr, $currScope) { $varInfo = $this->getOrCreateVariableInfo($varName, $currScope); - Helpers::debug('isVariableUndefined', $varInfo); + Helpers::debug('isVariableUndefined', $varInfo, 'at', $stackPtr); if ($varInfo->ignoreUndefined) { return false; } @@ -576,6 +638,31 @@ protected function isVariableUndefined($varName, $stackPtr, $currScope) if (isset($varInfo->firstInitialized) && $varInfo->firstInitialized <= $stackPtr) { return false; } + // If we are inside a for loop increment expression, check to see if the + // variable was defined inside the for loop. + foreach ($this->forLoops as $forLoop) { + if ($stackPtr > $forLoop->incrementStart && $stackPtr < $forLoop->incrementEnd) { + Helpers::debug('isVariableUndefined looking at increment expression for loop', $forLoop); + if ( + isset($varInfo->firstInitialized) + && $varInfo->firstInitialized > $forLoop->blockStart + && $varInfo->firstInitialized < $forLoop->blockEnd + ) { + return false; + } + } + } + // If we are inside a for loop body, check to see if the variable was + // defined in that loop's third expression. + foreach ($this->forLoops as $forLoop) { + if ($stackPtr > $forLoop->blockStart && $stackPtr < $forLoop->blockEnd) { + foreach ($forLoop->incrementVariables as $forLoopVarInfo) { + if ($varInfo === $forLoopVarInfo) { + return false; + } + } + } + } return true; } @@ -1418,12 +1505,17 @@ protected function processVariableAsSymbolicObjectProperty(File $phpcsFile, $sta * 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. + * If the 'ignore-for-loops' option is true, we will ignore the special + * processing for the increment variables of for loops. This will prevent + * infinite loops. + * + * @param File $phpcsFile The PHP_CodeSniffer file where this token was found. + * @param int $stackPtr The position where the token was found. + * @param array $options See above. * * @return void */ - protected function processVariable(File $phpcsFile, $stackPtr) + protected function processVariable(File $phpcsFile, $stackPtr, $options = []) { $tokens = $phpcsFile->getTokens(); $token = $tokens[$stackPtr]; @@ -1460,6 +1552,18 @@ protected function processVariable(File $phpcsFile, $stackPtr) // Assignment via foreach (... as ...) { } // Pass-by-reference to known pass-by-reference function + // Are we inside the third expression of a for loop? Store such variables + // for processing after the loop ends by `processClosingForLoopsAt()`. + if (empty($options['ignore-for-loops'])) { + $forLoop = Helpers::getForLoopForIncrementVariable($stackPtr, $this->forLoops); + if ($forLoop) { + Helpers::debug('found variable inside for loop third expression'); + $varInfo = $this->getOrCreateVariableInfo($varName, $currScope); + $forLoop->incrementVariables[$stackPtr] = $varInfo; + return; + } + } + // Are we a $object->$property type symbolic reference? if ($this->processVariableAsSymbolicObjectProperty($phpcsFile, $stackPtr, $varName, $currScope)) { Helpers::debug('found symbolic object property'); @@ -1519,7 +1623,7 @@ protected function processVariable(File $phpcsFile, $stackPtr) if (Helpers::isTokenInsideAssignmentLHS($phpcsFile, $stackPtr)) { Helpers::debug('found assignment'); $this->processVariableAsAssignment($phpcsFile, $stackPtr, $varName, $currScope); - if (Helpers::isTokenInsideAssignmentRHS($phpcsFile, $stackPtr) || Helpers::isTokenInsideFunctionCall($phpcsFile, $stackPtr)) { + if (Helpers::isTokenInsideAssignmentRHS($phpcsFile, $stackPtr) || Helpers::isTokenInsideFunctionCallArgument($phpcsFile, $stackPtr)) { Helpers::debug("found assignment that's also inside an expression"); $this->markVariableRead($varName, $stackPtr, $currScope); return;