From 3b71162a6bf0cde2bff1752e40a1788d8273d049 Mon Sep 17 00:00:00 2001 From: Payton Swick Date: Sat, 5 Aug 2023 19:46:11 -0400 Subject: [PATCH] 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 --- .../fixtures/AssignmentByReferenceFixture.php | 14 ++++++- VariableAnalysis/Lib/Helpers.php | 16 ++++++-- .../CodeAnalysis/VariableAnalysisSniff.php | 41 ++++++++++++++----- 3 files changed, 55 insertions(+), 16 deletions(-) diff --git a/Tests/VariableAnalysisSniff/fixtures/AssignmentByReferenceFixture.php b/Tests/VariableAnalysisSniff/fixtures/AssignmentByReferenceFixture.php index 2561c94..5ffba22 100644 --- a/Tests/VariableAnalysisSniff/fixtures/AssignmentByReferenceFixture.php +++ b/Tests/VariableAnalysisSniff/fixtures/AssignmentByReferenceFixture.php @@ -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) { @@ -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'; +} diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php index f0cc6d5..1050de9 100644 --- a/VariableAnalysis/Lib/Helpers.php +++ b/VariableAnalysis/Lib/Helpers.php @@ -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; @@ -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 : []; } diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index 1429562..0aee22a 100644 --- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -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) { @@ -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); @@ -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)) { @@ -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. @@ -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); @@ -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; } @@ -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; @@ -1984,13 +2003,14 @@ 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 @@ -1998,6 +2018,7 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v 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. @@ -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,