Skip to content

Do not close scope of ref reassignment when inside else #306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 5, 2023
Merged

Do not close scope of ref reassignment when inside else #306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a24ecd9
Add test for unused variable
sirbrillig Aug 2, 2023
4e9abde
Add more debug lines to reference processing
sirbrillig Aug 5, 2023
ca891cc
Do not close scope of ref reassignment when inside else
sirbrillig Aug 5, 2023
c91a502
Verify all regexps are non-empty strings
sirbrillig Aug 5, 2023
1048f0c
Add guard for empty preg_match_all in processVariableInString
sirbrillig Aug 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions Tests/VariableAnalysisSniff/fixtures/AssignmentByReferenceFixture.php
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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