Skip to content

Commit

Permalink
Fix performance issues around scope close detection (#200)
Browse files Browse the repository at this point in the history 
* Store scope open/close rather than fetching it every process

* Only look for containing arrow function once per findVariableScope

* Only test likely candidates when looking for arrow functions

* Only scan for closing scope for closing scope indices

* Make sure to add global scope to scopeEndIndices

* Remove debug line

* Store scopeStartEndPairs with filename

Temporarily remove scopeEndIndices

* Add scopeEndIndexCache
  • Loading branch information
@sirbrillig
sirbrillig authored Sep 2, 2020
1 parent 9ef0985 commit 81da0cf
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 42 deletions.
21 changes: 7 additions & 14 deletions VariableAnalysis/Lib/Helpers.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,11 +295,12 @@ public static function findVariableScope(File $phpcsFile, $stackPtr) {
$tokens = $phpcsFile->getTokens();
$token = $tokens[$stackPtr];

if (self::isTokenInsideArrowFunctionBody($phpcsFile, $stackPtr)) {
$arrowFunctionIndex = self::getContainingArrowFunctionIndex($phpcsFile, $stackPtr);
$isTokenInsideArrowFunctionBody = (bool) $arrowFunctionIndex;
if ($isTokenInsideArrowFunctionBody) {
// 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)) {
Expand Down Expand Up @@ -432,16 +433,6 @@ public static function isTokenInsideArrowFunctionDefinition(File $phpcsFile, $st
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
Expand Down Expand Up @@ -472,9 +463,11 @@ public static function getContainingArrowFunctionIndex(File $phpcsFile, $stackPt
* @return ?int
*/
private static function getPreviousArrowFunctionIndex(File $phpcsFile, $stackPtr) {
$tokens = $phpcsFile->getTokens();
$enclosingScopeIndex = self::findVariableScopeExceptArrowFunctions($phpcsFile, $stackPtr);
for ($index = $stackPtr - 1; $index > $enclosingScopeIndex; $index--) {
if (FunctionDeclarations::isArrowFunction($phpcsFile, $index)) {
$token = $tokens[$index];
if ($token['content'] === 'fn' && FunctionDeclarations::isArrowFunction($phpcsFile, $index)) {
return $index;
}
}
Expand Down Expand Up @@ -727,7 +720,7 @@ public static function isRequireInScopeAfter(File $phpcsFile, VariableInfo $varI
$indexToStartSearch = $varInfo->firstInitialized;
}
$tokens = $phpcsFile->getTokens();
$indexToStopSearch = isset($tokens[$scopeInfo->owner]['scope_closer']) ? $tokens[$scopeInfo->owner]['scope_closer'] : null;
$indexToStopSearch = isset($tokens[$scopeInfo->scopeStartIndex]['scope_closer']) ? $tokens[$scopeInfo->scopeStartIndex]['scope_closer'] : null;
if (! is_int($indexToStartSearch) || ! is_int($indexToStopSearch)) {
return false;
}
Expand Down
14 changes: 11 additions & 3 deletions VariableAnalysis/Lib/ScopeInfo.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,14 @@ class ScopeInfo {
*
* @var int
*/
public $owner;
public $scopeStartIndex;

/**
* The token index of the end of this scope, if important.
*
* @var int|null
*/
public $scopeEndIndex;

/**
* The variables defined in this scope.
Expand All @@ -20,7 +27,8 @@ class ScopeInfo {
*/
public $variables = [];

public function __construct($scopeStartIndex) {
$this->owner = $scopeStartIndex;
public function __construct($scopeStartIndex, $scopeEndIndex = null) {
$this->scopeStartIndex = $scopeStartIndex;
$this->scopeEndIndex = $scopeEndIndex;
}
}
104 changes: 79 additions & 25 deletions VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,29 @@ class VariableAnalysisSniff implements Sniff {
protected $currentFile = null;

/**
* A list of scopes encountered so far and the variables within them.
* An associative array of scopes for variables encountered so far and the variables within them.
*
* Each scope is keyed by a string of the form `filename:scopeStartIndex` (see `getScopeKey`).
*
* @var ScopeInfo[]
*/
private $scopes = [];

/**
* A list of token indices which start scopes and will be used to check for unused variables.
* An associative array of a list of token index pairs which start and end scopes and will be used to check for unused variables.
*
* Each array of scopes is keyed by a string containing the filename (see `getFilename`).
*
* @var ScopeInfo[][]
*/
private $scopeStartEndPairs = [];

/**
* A cache of scope end indices in the current file to improve performance.
*
* @var int[]
*/
private $scopeStartIndices = [0];
private $scopeEndIndexCache = [];

/**
* A list of custom functions which pass in variables to be initialized by
Expand Down Expand Up @@ -196,30 +207,20 @@ public function process(File $phpcsFile, $stackPtr) {
T_CLOSURE,
];

$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', $scopeStartIndex);
}

if ($stackPtr === $scopeCloserIndex) {
$found[] = $scopeStartIndex;
}
return $found;
}, []);

foreach ($scopeIndicesThisCloses as $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;
$this->scopeEndIndexCache = [];
}

// Add the global scope
if (empty($this->scopeStartEndPairs[$this->getFilename()])) {
$this->recordScopeStartAndEnd($phpcsFile, 0);
}

$this->searchForAndProcessClosingScopesAt($phpcsFile, $stackPtr);

if ($token['code'] === T_VARIABLE) {
$this->processVariable($phpcsFile, $stackPtr);
return;
Expand All @@ -241,11 +242,57 @@ public function process(File $phpcsFile, $stackPtr) {
|| FunctionDeclarations::isArrowFunction($phpcsFile, $stackPtr)
) {
Helpers::debug('found scope condition', $token);
$this->scopeStartIndices[] = $stackPtr;
$this->recordScopeStartAndEnd($phpcsFile, $stackPtr);
return;
}
}

/**
* @param File $phpcsFile
* @param int $scopeStartIndex
*
* @return void
*/
private function recordScopeStartAndEnd($phpcsFile, $scopeStartIndex) {
$scopeEndIndex = Helpers::getScopeCloseForScopeOpen($phpcsFile, $scopeStartIndex);
$filename = $this->getFilename();
if (! isset($this->scopeStartEndPairs[$filename])) {
$this->scopeStartEndPairs[$filename] = [];
}
Helpers::debug('recording scope for file', $filename, 'start/end', $scopeStartIndex, $scopeEndIndex);
$this->scopeStartEndPairs[$filename][] = new ScopeInfo($scopeStartIndex, $scopeEndIndex);
$this->scopeEndIndexCache[] = $scopeEndIndex;
}

/**
* @param File $phpcsFile
* @param int $stackPtr
*
* @return void
*/
private function searchForAndProcessClosingScopesAt($phpcsFile, $stackPtr) {
if (! in_array($stackPtr, $this->scopeEndIndexCache, true)) {
return;
}
$scopePairsForFile = isset($this->scopeStartEndPairs[$this->getFilename()]) ? $this->scopeStartEndPairs[$this->getFilename()] : [];
$scopeIndicesThisCloses = array_reduce($scopePairsForFile, function ($found, $scope) use ($stackPtr) {
if (! is_int($scope->scopeEndIndex)) {
Helpers::debug('No scope closer found for scope start', $scope->scopeStartIndex);
return $found;
}

if ($stackPtr === $scope->scopeEndIndex) {
$found[] = $scope;
}
return $found;
}, []);

foreach ($scopeIndicesThisCloses as $scopeIndexThisCloses) {
Helpers::debug('found closing scope at index', $stackPtr, 'for scopes starting at:', $scopeIndexThisCloses);
$this->processScopeClose($phpcsFile, $scopeIndexThisCloses->scopeStartIndex);
}
}

/**
* @param File $phpcsFile
* @param int $stackPtr
Expand All @@ -272,7 +319,14 @@ protected function isGetDefinedVars(File $phpcsFile, $stackPtr) {
* @return string
*/
protected function getScopeKey($currScope) {
return ($this->currentFile ? $this->currentFile->getFilename() : 'unknown file') . ':' . $currScope;
return $this->getFilename() . ':' . $currScope;
}

/**
* @return string
*/
protected function getFilename() {
return $this->currentFile ? $this->currentFile->getFilename() : 'unknown file';
}

/**
Expand Down Expand Up @@ -332,7 +386,7 @@ protected function getOrCreateVariableInfo($varName, $currScope) {
if (isset($this->ignoreUnusedRegexp) && preg_match($this->ignoreUnusedRegexp, $varName) === 1) {
$scopeInfo->variables[$varName]->ignoreUnused = true;
}
if ($scopeInfo->owner === 0 && $this->allowUndefinedVariablesInFileScope) {
if ($scopeInfo->scopeStartIndex === 0 && $this->allowUndefinedVariablesInFileScope) {
$scopeInfo->variables[$varName]->ignoreUndefined = true;
}
if (in_array($varName, $validUndefinedVariableNames)) {
Expand Down Expand Up @@ -530,7 +584,7 @@ protected function markAllVariablesRead(File $phpcsFile, $stackPtr) {
$count = count($scopeInfo->variables);
Helpers::debug("marking all $count variables in scope as read");
foreach ($scopeInfo->variables as $varInfo) {
$this->markVariableRead($varInfo->name, $stackPtr, $scopeInfo->owner);
$this->markVariableRead($varInfo->name, $stackPtr, $scopeInfo->scopeStartIndex);
}
}

Expand Down

0 comments on commit 81da0cf

Please sign in to comment.