Skip to content

Commit 3b71162

Browse files
sirbrilligsirbrillig
authored
Do not close scope of ref reassignment when inside else (#306)
* 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
1 parent 002ae3a commit 3b71162

File tree

3 files changed

+55
-16
lines changed

3 files changed

+55
-16
lines changed

‎Tests/VariableAnalysisSniff/fixtures/AssignmentByReferenceFixture.php

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ function doubleUsedThenUsedAssignmentByReference() {
5555
return $var;
5656
}
5757

58-
function somefunc($choice, &$arr1, &$arr_default) {
58+
function somefunc1($choice, &$arr1, &$arr_default) {
5959
$var = &$arr_default;
6060

6161
if ($choice) {
@@ -65,10 +65,20 @@ function somefunc($choice, &$arr1, &$arr_default) {
6565
echo $var;
6666
}
6767

68-
function somefunc($choice, &$arr1, &$arr_default) {
68+
function somefunc2($choice, &$arr1, &$arr_default) {
6969
if ($choice) {
7070
$var = &$arr_default; // unused variable $var
7171
$var = &$arr1;
7272
echo $var;
7373
}
7474
}
75+
76+
function assignByRef2($field_id, $form) {
77+
$wrapper_id = $field_id . '_wrapper';
78+
if (!isset($form[$field_id]) && isset($form[$wrapper_id])) {
79+
$element = &$form[$wrapper_id][$field_id];
80+
} else {
81+
$element = &$form[$field_id];
82+
}
83+
$element['test'] = 'test';
84+
}

‎VariableAnalysis/Lib/Helpers.php

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -128,21 +128,26 @@ public static function areConditionsWithinFunctionBeforeClass(array $token)
128128
}
129129

130130
/**
131-
* Return true if the token conditions are within an if block before they are
132-
* within a class or function.
131+
* Return true if the token conditions are within an IF/ELSE/ELSEIF block
132+
* before they are within a class or function.
133133
*
134134
* @param (int|string)[] $conditions
135135
*
136136
* @return int|string|null
137137
*/
138-
public static function getClosestIfPositionIfBeforeOtherConditions(array $conditions)
138+
public static function getClosestConditionPositionIfBeforeOtherConditions(array $conditions)
139139
{
140140
$conditionsInsideOut = array_reverse($conditions, true);
141141
if (empty($conditions)) {
142142
return null;
143143
}
144144
$scopeCode = reset($conditionsInsideOut);
145-
if ($scopeCode === T_IF) {
145+
$conditionalCodes = [
146+
T_IF,
147+
T_ELSE,
148+
T_ELSEIF,
149+
];
150+
if (in_array($scopeCode, $conditionalCodes, true)) {
146151
return key($conditionsInsideOut);
147152
}
148153
return null;
@@ -923,6 +928,9 @@ public static function debug()
923928
*/
924929
public static function splitStringToArray($pattern, $value)
925930
{
931+
if (empty($pattern)) {
932+
return [];
933+
}
926934
$result = preg_split($pattern, $value);
927935
return is_array($result) ? $result : [];
928936
}

‎VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -460,7 +460,7 @@ protected function getOrCreateVariableInfo($varName, $currScope)
460460
if (in_array($varName, $validUnusedVariableNames)) {
461461
$scopeInfo->variables[$varName]->ignoreUnused = true;
462462
}
463-
if (isset($this->ignoreUnusedRegexp) && preg_match($this->ignoreUnusedRegexp, $varName) === 1) {
463+
if (! empty($this->ignoreUnusedRegexp) && preg_match($this->ignoreUnusedRegexp, $varName) === 1) {
464464
$scopeInfo->variables[$varName]->ignoreUnused = true;
465465
}
466466
if ($scopeInfo->scopeStartIndex === 0 && $this->allowUndefinedVariablesInFileScope) {
@@ -469,7 +469,7 @@ protected function getOrCreateVariableInfo($varName, $currScope)
469469
if (in_array($varName, $validUndefinedVariableNames)) {
470470
$scopeInfo->variables[$varName]->ignoreUndefined = true;
471471
}
472-
if (isset($this->validUndefinedVariableRegexp) && preg_match($this->validUndefinedVariableRegexp, $varName) === 1) {
472+
if (! empty($this->validUndefinedVariableRegexp) && preg_match($this->validUndefinedVariableRegexp, $varName) === 1) {
473473
$scopeInfo->variables[$varName]->ignoreUndefined = true;
474474
}
475475
Helpers::debug("getOrCreateVariableInfo: scope for '{$varName}' is now", $scopeInfo);
@@ -527,11 +527,16 @@ protected function markVariableAssignmentWithoutInitialization($varName, $stackP
527527

528528
// Is the variable referencing another variable? If so, mark that variable used also.
529529
if ($varInfo->referencedVariableScope !== null && $varInfo->referencedVariableScope !== $currScope) {
530+
Helpers::debug('markVariableAssignmentWithoutInitialization: considering marking referenced variable assigned', $varName);
530531
// 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
531532
if ($this->getVariableInfo($varInfo->name, $varInfo->referencedVariableScope)) {
532533
Helpers::debug('markVariableAssignmentWithoutInitialization: marking referenced variable as assigned also', $varName);
533534
$this->markVariableAssignment($varInfo->name, $stackPtr, $varInfo->referencedVariableScope);
535+
} else {
536+
Helpers::debug('markVariableAssignmentWithoutInitialization: not marking referenced variable assigned', $varName);
534537
}
538+
} else {
539+
Helpers::debug('markVariableAssignmentWithoutInitialization: not considering referenced variable', $varName);
535540
}
536541

537542
if (empty($varInfo->scopeType)) {
@@ -1142,21 +1147,27 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN
11421147
$tokens = $phpcsFile->getTokens();
11431148
$referencePtr = $phpcsFile->findNext(Tokens::$emptyTokens, $assignPtr + 1, null, true, null, true);
11441149
if (is_int($referencePtr) && $tokens[$referencePtr]['code'] === T_BITWISE_AND) {
1145-
Helpers::debug('processVariableAsAssignment: found reference variable');
1150+
Helpers::debug("processVariableAsAssignment: found reference variable for '{$varName}'");
11461151
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
11471152
// If the variable was already declared, but was not yet read, it is
11481153
// unused because we're about to change the binding; that is, unless we
11491154
// are inside a conditional block because in that case the condition may
11501155
// never activate.
11511156
$scopeInfo = $this->getOrCreateScopeInfo($currScope);
1152-
$ifPtr = Helpers::getClosestIfPositionIfBeforeOtherConditions($tokens[$referencePtr]['conditions']);
1157+
$conditionPointer = Helpers::getClosestConditionPositionIfBeforeOtherConditions($tokens[$referencePtr]['conditions']);
11531158
$lastAssignmentPtr = $varInfo->firstDeclared;
1154-
if (! $ifPtr && $lastAssignmentPtr) {
1159+
if (! $conditionPointer && $lastAssignmentPtr) {
1160+
Helpers::debug("processVariableAsAssignment: considering close of scope for '{$varName}' due to reference reassignment");
11551161
$this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
11561162
}
1157-
if ($ifPtr && $lastAssignmentPtr && $ifPtr <= $lastAssignmentPtr) {
1163+
if ($conditionPointer && $lastAssignmentPtr && $conditionPointer < $lastAssignmentPtr) {
1164+
// We may be inside a condition but the last assignment was also inside this condition.
1165+
Helpers::debug("processVariableAsAssignment: considering close of scope for '{$varName}' due to reference reassignment ignoring recent condition");
11581166
$this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
11591167
}
1168+
if ($conditionPointer && $lastAssignmentPtr && $conditionPointer > $lastAssignmentPtr) {
1169+
Helpers::debug("processVariableAsAssignment: not considering close of scope for '{$varName}' due to reference reassignment because it is conditional");
1170+
}
11601171
// The referenced variable may have a different name, but we don't
11611172
// actually need to mark it as used in this case because the act of this
11621173
// assignment will mark it used on the next token.
@@ -1839,11 +1850,17 @@ protected function processVariableInString(File $phpcsFile, $stackPtr)
18391850
$tokens = $phpcsFile->getTokens();
18401851
$token = $tokens[$stackPtr];
18411852

1842-
if (!preg_match_all(Constants::getDoubleQuotedVarRegexp(), $token['content'], $matches)) {
1853+
$regexp = Constants::getDoubleQuotedVarRegexp();
1854+
if (! empty($regexp) && !preg_match_all($regexp, $token['content'], $matches)) {
1855+
Helpers::debug('processVariableInString: no variables found', $token);
18431856
return;
18441857
}
18451858
Helpers::debug('examining token for variable in string', $token);
18461859

1860+
if (empty($matches)) {
1861+
Helpers::debug('processVariableInString: no variables found after search', $token);
1862+
return;
1863+
}
18471864
foreach ($matches[1] as $varName) {
18481865
$varName = Helpers::normalizeVarName($varName);
18491866

@@ -1912,7 +1929,8 @@ protected function processCompactArguments(File $phpcsFile, $stackPtr, $argument
19121929
}
19131930
if ($argumentFirstToken['code'] === T_DOUBLE_QUOTED_STRING) {
19141931
// Double-quoted string literal.
1915-
if (preg_match(Constants::getDoubleQuotedVarRegexp(), $argumentFirstToken['content'])) {
1932+
$regexp = Constants::getDoubleQuotedVarRegexp();
1933+
if (! empty($regexp) && preg_match($regexp, $argumentFirstToken['content'])) {
19161934
// Bail if the string needs variable expansion, that's runtime stuff.
19171935
continue;
19181936
}
@@ -1956,6 +1974,7 @@ protected function processCompact(File $phpcsFile, $stackPtr)
19561974
*/
19571975
protected function processScopeClose(File $phpcsFile, $stackPtr)
19581976
{
1977+
Helpers::debug("processScopeClose at {$stackPtr}");
19591978
$scopeInfo = $this->scopeManager->getScopeForScopeStart($phpcsFile->getFilename(), $stackPtr);
19601979
if (is_null($scopeInfo)) {
19611980
return;
@@ -1984,20 +2003,22 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v
19842003
return;
19852004
}
19862005
if ($this->allowUnusedParametersBeforeUsed && $varInfo->scopeType === ScopeType::PARAM && Helpers::areFollowingArgumentsUsed($varInfo, $scopeInfo)) {
1987-
Helpers::debug("variable {$varInfo->name} at end of scope has unused following args");
2006+
Helpers::debug("variable '{$varInfo->name}' at end of scope has unused following args");
19882007
return;
19892008
}
19902009
if ($this->allowUnusedForeachVariables && $varInfo->isForeachLoopAssociativeValue) {
19912010
return;
19922011
}
19932012
if ($varInfo->referencedVariableScope !== null && isset($varInfo->firstInitialized)) {
2013+
Helpers::debug("variable '{$varInfo->name}' at end of scope is a reference and so counts as used");
19942014
// If we're pass-by-reference then it's a common pattern to
19952015
// use the variable to return data to the caller, so any
19962016
// assignment also counts as "variable use" for the purposes
19972017
// of "unused variable" warnings.
19982018
return;
19992019
}
20002020
if ($varInfo->scopeType === ScopeType::GLOBALSCOPE && isset($varInfo->firstInitialized)) {
2021+
Helpers::debug("variable '{$varInfo->name}' at end of scope is a global and so counts as used");
20012022
// If we imported this variable from the global scope, any further use of
20022023
// the variable, including assignment, should count as "variable use" for
20032024
// the purposes of "unused variable" warnings.
@@ -2045,7 +2066,7 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v
20452066
protected function warnAboutUnusedVariable(File $phpcsFile, VariableInfo $varInfo)
20462067
{
20472068
foreach (array_unique($varInfo->allAssignments) as $indexForWarning) {
2048-
Helpers::debug("variable {$varInfo->name} at end of scope looks unused");
2069+
Helpers::debug("variable '{$varInfo->name}' at end of scope looks unused");
20492070
$phpcsFile->addWarning(
20502071
'Unused %s %s.',
20512072
$indexForWarning,

0 commit comments

Comments
 (0)