Skip to content

Commit

Permalink
Do not close scope of ref reassignment when inside else (#306)
Browse files Browse the repository at this point in the history 
* Add test for unused variable

* Add more debug lines to reference processing

* Do not close scope of ref reassignment when inside else

* Verify all regexps are non-empty strings

This will please psalm, I hope.

* Add guard for empty preg_match_all in processVariableInString
  • Loading branch information
@sirbrillig
sirbrillig authored Aug 5, 2023
1 parent 002ae3a commit 3b71162
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 16 deletions.
14 changes: 12 additions & 2 deletions Tests/VariableAnalysisSniff/fixtures/AssignmentByReferenceFixture.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ function doubleUsedThenUsedAssignmentByReference() {
return $var;
}

function somefunc($choice, &$arr1, &$arr_default) {
function somefunc1($choice, &$arr1, &$arr_default) {
$var = &$arr_default;

if ($choice) {
Expand All @@ -65,10 +65,20 @@ function somefunc($choice, &$arr1, &$arr_default) {
echo $var;
}

function somefunc($choice, &$arr1, &$arr_default) {
function somefunc2($choice, &$arr1, &$arr_default) {
if ($choice) {
$var = &$arr_default; // unused variable $var
$var = &$arr1;
echo $var;
}
}

function assignByRef2($field_id, $form) {
$wrapper_id = $field_id . '_wrapper';
if (!isset($form[$field_id]) && isset($form[$wrapper_id])) {
$element = &$form[$wrapper_id][$field_id];
} else {
$element = &$form[$field_id];
}
$element['test'] = 'test';
}
16 changes: 12 additions & 4 deletions VariableAnalysis/Lib/Helpers.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,21 +128,26 @@ public static function areConditionsWithinFunctionBeforeClass(array $token)
}

/**
* Return true if the token conditions are within an if block before they are
* within a class or function.
* Return true if the token conditions are within an IF/ELSE/ELSEIF block
* before they are within a class or function.
*
* @param (int|string)[] $conditions
*
* @return int|string|null
*/
public static function getClosestIfPositionIfBeforeOtherConditions(array $conditions)
public static function getClosestConditionPositionIfBeforeOtherConditions(array $conditions)
{
$conditionsInsideOut = array_reverse($conditions, true);
if (empty($conditions)) {
return null;
}
$scopeCode = reset($conditionsInsideOut);
if ($scopeCode === T_IF) {
$conditionalCodes = [
T_IF,
T_ELSE,
T_ELSEIF,
];
if (in_array($scopeCode, $conditionalCodes, true)) {
return key($conditionsInsideOut);
}
return null;
Expand Down Expand Up @@ -923,6 +928,9 @@ public static function debug()
*/
public static function splitStringToArray($pattern, $value)
{
if (empty($pattern)) {
return [];
}
$result = preg_split($pattern, $value);
return is_array($result) ? $result : [];
}
Expand Down
41 changes: 31 additions & 10 deletions VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ protected function getOrCreateVariableInfo($varName, $currScope)
if (in_array($varName, $validUnusedVariableNames)) {
$scopeInfo->variables[$varName]->ignoreUnused = true;
}
if (isset($this->ignoreUnusedRegexp) && preg_match($this->ignoreUnusedRegexp, $varName) === 1) {
if (! empty($this->ignoreUnusedRegexp) && preg_match($this->ignoreUnusedRegexp, $varName) === 1) {
$scopeInfo->variables[$varName]->ignoreUnused = true;
}
if ($scopeInfo->scopeStartIndex === 0 && $this->allowUndefinedVariablesInFileScope) {
Expand All @@ -469,7 +469,7 @@ protected function getOrCreateVariableInfo($varName, $currScope)
if (in_array($varName, $validUndefinedVariableNames)) {
$scopeInfo->variables[$varName]->ignoreUndefined = true;
}
if (isset($this->validUndefinedVariableRegexp) && preg_match($this->validUndefinedVariableRegexp, $varName) === 1) {
if (! empty($this->validUndefinedVariableRegexp) && preg_match($this->validUndefinedVariableRegexp, $varName) === 1) {
$scopeInfo->variables[$varName]->ignoreUndefined = true;
}
Helpers::debug("getOrCreateVariableInfo: scope for '{$varName}' is now", $scopeInfo);
Expand Down Expand Up @@ -527,11 +527,16 @@ protected function markVariableAssignmentWithoutInitialization($varName, $stackP

// Is the variable referencing another variable? If so, mark that variable used also.
if ($varInfo->referencedVariableScope !== null && $varInfo->referencedVariableScope !== $currScope) {
Helpers::debug('markVariableAssignmentWithoutInitialization: considering marking referenced variable assigned', $varName);
// Don't do this if the referenced variable does not exist; eg: if it's going to be bound at runtime like in array_walk
if ($this->getVariableInfo($varInfo->name, $varInfo->referencedVariableScope)) {
Helpers::debug('markVariableAssignmentWithoutInitialization: marking referenced variable as assigned also', $varName);
$this->markVariableAssignment($varInfo->name, $stackPtr, $varInfo->referencedVariableScope);
} else {
Helpers::debug('markVariableAssignmentWithoutInitialization: not marking referenced variable assigned', $varName);
}
} else {
Helpers::debug('markVariableAssignmentWithoutInitialization: not considering referenced variable', $varName);
}

if (empty($varInfo->scopeType)) {
Expand Down Expand Up @@ -1142,21 +1147,27 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN
$tokens = $phpcsFile->getTokens();
$referencePtr = $phpcsFile->findNext(Tokens::$emptyTokens, $assignPtr + 1, null, true, null, true);
if (is_int($referencePtr) && $tokens[$referencePtr]['code'] === T_BITWISE_AND) {
Helpers::debug('processVariableAsAssignment: found reference variable');
Helpers::debug("processVariableAsAssignment: found reference variable for '{$varName}'");
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
// If the variable was already declared, but was not yet read, it is
// unused because we're about to change the binding; that is, unless we
// are inside a conditional block because in that case the condition may
// never activate.
$scopeInfo = $this->getOrCreateScopeInfo($currScope);
$ifPtr = Helpers::getClosestIfPositionIfBeforeOtherConditions($tokens[$referencePtr]['conditions']);
$conditionPointer = Helpers::getClosestConditionPositionIfBeforeOtherConditions($tokens[$referencePtr]['conditions']);
$lastAssignmentPtr = $varInfo->firstDeclared;
if (! $ifPtr && $lastAssignmentPtr) {
if (! $conditionPointer && $lastAssignmentPtr) {
Helpers::debug("processVariableAsAssignment: considering close of scope for '{$varName}' due to reference reassignment");
$this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
}
if ($ifPtr && $lastAssignmentPtr && $ifPtr <= $lastAssignmentPtr) {
if ($conditionPointer && $lastAssignmentPtr && $conditionPointer < $lastAssignmentPtr) {
// We may be inside a condition but the last assignment was also inside this condition.
Helpers::debug("processVariableAsAssignment: considering close of scope for '{$varName}' due to reference reassignment ignoring recent condition");
$this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
}
if ($conditionPointer && $lastAssignmentPtr && $conditionPointer > $lastAssignmentPtr) {
Helpers::debug("processVariableAsAssignment: not considering close of scope for '{$varName}' due to reference reassignment because it is conditional");
}
// The referenced variable may have a different name, but we don't
// actually need to mark it as used in this case because the act of this
// assignment will mark it used on the next token.
Expand Down Expand Up @@ -1839,11 +1850,17 @@ protected function processVariableInString(File $phpcsFile, $stackPtr)
$tokens = $phpcsFile->getTokens();
$token = $tokens[$stackPtr];

if (!preg_match_all(Constants::getDoubleQuotedVarRegexp(), $token['content'], $matches)) {
$regexp = Constants::getDoubleQuotedVarRegexp();
if (! empty($regexp) && !preg_match_all($regexp, $token['content'], $matches)) {
Helpers::debug('processVariableInString: no variables found', $token);
return;
}
Helpers::debug('examining token for variable in string', $token);

if (empty($matches)) {
Helpers::debug('processVariableInString: no variables found after search', $token);
return;
}
foreach ($matches[1] as $varName) {
$varName = Helpers::normalizeVarName($varName);

Expand Down Expand Up @@ -1912,7 +1929,8 @@ protected function processCompactArguments(File $phpcsFile, $stackPtr, $argument
}
if ($argumentFirstToken['code'] === T_DOUBLE_QUOTED_STRING) {
// Double-quoted string literal.
if (preg_match(Constants::getDoubleQuotedVarRegexp(), $argumentFirstToken['content'])) {
$regexp = Constants::getDoubleQuotedVarRegexp();
if (! empty($regexp) && preg_match($regexp, $argumentFirstToken['content'])) {
// Bail if the string needs variable expansion, that's runtime stuff.
continue;
}
Expand Down Expand Up @@ -1956,6 +1974,7 @@ protected function processCompact(File $phpcsFile, $stackPtr)
*/
protected function processScopeClose(File $phpcsFile, $stackPtr)
{
Helpers::debug("processScopeClose at {$stackPtr}");
$scopeInfo = $this->scopeManager->getScopeForScopeStart($phpcsFile->getFilename(), $stackPtr);
if (is_null($scopeInfo)) {
return;
Expand Down Expand Up @@ -1984,20 +2003,22 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v
return;
}
if ($this->allowUnusedParametersBeforeUsed && $varInfo->scopeType === ScopeType::PARAM && Helpers::areFollowingArgumentsUsed($varInfo, $scopeInfo)) {
Helpers::debug("variable {$varInfo->name} at end of scope has unused following args");
Helpers::debug("variable '{$varInfo->name}' at end of scope has unused following args");
return;
}
if ($this->allowUnusedForeachVariables && $varInfo->isForeachLoopAssociativeValue) {
return;
}
if ($varInfo->referencedVariableScope !== null && isset($varInfo->firstInitialized)) {
Helpers::debug("variable '{$varInfo->name}' at end of scope is a reference and so counts as used");
// If we're pass-by-reference then it's a common pattern to
// use the variable to return data to the caller, so any
// assignment also counts as "variable use" for the purposes
// of "unused variable" warnings.
return;
}
if ($varInfo->scopeType === ScopeType::GLOBALSCOPE && isset($varInfo->firstInitialized)) {
Helpers::debug("variable '{$varInfo->name}' at end of scope is a global and so counts as used");
// 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.
Expand Down Expand Up @@ -2045,7 +2066,7 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v
protected function warnAboutUnusedVariable(File $phpcsFile, VariableInfo $varInfo)
{
foreach (array_unique($varInfo->allAssignments) as $indexForWarning) {
Helpers::debug("variable {$varInfo->name} at end of scope looks unused");
Helpers::debug("variable '{$varInfo->name}' at end of scope looks unused");
$phpcsFile->addWarning(
'Unused %s %s.',
$indexForWarning,
Expand Down

0 comments on commit 3b71162

Please sign in to comment.