Skip to content

Commit b77e69e

Browse files
sirbrilligsirbrillig
authored
Fix array_walk pass-by-reference in 3.x (#217)
* Add test for array_walk pass-by-reference * Move isTokenInsideAssignmentLHS, isTokenVariableVariable to helpers * Add additional debug for variable creation * Prevent creating vars in outside scope when reference is not bound
1 parent d7bfb29 commit b77e69e

File tree

3 files changed

+106
-70
lines changed

3 files changed

+106
-70
lines changed

‎Tests/VariableAnalysisSniff/fixtures/FunctionWithReferenceFixture.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,3 +63,13 @@ function function_with_ignored_reference_call() {
6363
function function_with_wordpress_reference_calls() {
6464
wp_parse_str('foo=bar', $vars);
6565
}
66+
67+
function function_with_array_walk($userNameParts) {
68+
array_walk($userNameParts, function (string &$value): void {
69+
if (strlen($value) <= 3) {
70+
return;
71+
}
72+
73+
$value = ucfirst($value);
74+
});
75+
}

‎VariableAnalysis/Lib/Helpers.php‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -879,4 +879,52 @@ public static function isTokenInsideAssignmentRHS(File $phpcsFile, $stackPtr) {
879879
}
880880
return false;
881881
}
882+
883+
/**
884+
* @param File $phpcsFile
885+
* @param int $stackPtr
886+
*
887+
* @return bool
888+
*/
889+
public static function isTokenInsideAssignmentLHS(File $phpcsFile, $stackPtr) {
890+
// Is the next non-whitespace an assignment?
891+
$assignPtr = self::getNextAssignPointer($phpcsFile, $stackPtr);
892+
if (! is_int($assignPtr)) {
893+
return false;
894+
}
895+
896+
// Is this a variable variable? If so, it's not an assignment to the current variable.
897+
if (self::isTokenVariableVariable($phpcsFile, $stackPtr)) {
898+
self::debug('found variable variable');
899+
return false;
900+
}
901+
return true;
902+
}
903+
904+
/**
905+
* @param File $phpcsFile
906+
* @param int $stackPtr
907+
*
908+
* @return bool
909+
*/
910+
public static function isTokenVariableVariable(File $phpcsFile, $stackPtr) {
911+
$tokens = $phpcsFile->getTokens();
912+
913+
$prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($stackPtr - 1), null, true);
914+
if ($prev === false) {
915+
return false;
916+
}
917+
if ($tokens[$prev]['code'] === T_DOLLAR) {
918+
return true;
919+
}
920+
if ($tokens[$prev]['code'] !== T_OPEN_CURLY_BRACKET) {
921+
return false;
922+
}
923+
924+
$prevPrev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prev - 1), null, true);
925+
if ($prevPrev !== false && $tokens[$prevPrev]['code'] === T_DOLLAR) {
926+
return true;
927+
}
928+
return false;
929+
}
882930
}

‎VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php‎

Lines changed: 48 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -370,33 +370,36 @@ protected function getVariableInfo($varName, $currScope) {
370370
* @return VariableInfo
371371
*/
372372
protected function getOrCreateVariableInfo($varName, $currScope) {
373+
Helpers::debug("getOrCreateVariableInfo: starting for '{$varName}'");
373374
$scopeInfo = $this->getOrCreateScopeInfo($currScope);
374-
if (!isset($scopeInfo->variables[$varName])) {
375-
Helpers::debug("creating a new variable for '{$varName}' in scope", $scopeInfo);
376-
$scopeInfo->variables[$varName] = new VariableInfo($varName);
377-
$validUnusedVariableNames = (empty($this->validUnusedVariableNames))
378-
? []
379-
: Helpers::splitStringToArray('/\s+/', trim($this->validUnusedVariableNames));
380-
$validUndefinedVariableNames = (empty($this->validUndefinedVariableNames))
381-
? []
382-
: Helpers::splitStringToArray('/\s+/', trim($this->validUndefinedVariableNames));
383-
if (in_array($varName, $validUnusedVariableNames)) {
384-
$scopeInfo->variables[$varName]->ignoreUnused = true;
385-
}
386-
if (isset($this->ignoreUnusedRegexp) && preg_match($this->ignoreUnusedRegexp, $varName) === 1) {
387-
$scopeInfo->variables[$varName]->ignoreUnused = true;
388-
}
389-
if ($scopeInfo->scopeStartIndex === 0 && $this->allowUndefinedVariablesInFileScope) {
390-
$scopeInfo->variables[$varName]->ignoreUndefined = true;
391-
}
392-
if (in_array($varName, $validUndefinedVariableNames)) {
393-
$scopeInfo->variables[$varName]->ignoreUndefined = true;
394-
}
395-
if (isset($this->validUndefinedVariableRegexp) && preg_match($this->validUndefinedVariableRegexp, $varName) === 1) {
396-
$scopeInfo->variables[$varName]->ignoreUndefined = true;
397-
}
398-
}
399-
Helpers::debug("scope for '{$varName}' is now", $scopeInfo);
375+
if (isset($scopeInfo->variables[$varName])) {
376+
Helpers::debug("getOrCreateVariableInfo: found scope for '{$varName}'", $scopeInfo);
377+
return $scopeInfo->variables[$varName];
378+
}
379+
Helpers::debug("getOrCreateVariableInfo: creating a new variable for '{$varName}' in scope", $scopeInfo);
380+
$scopeInfo->variables[$varName] = new VariableInfo($varName);
381+
$validUnusedVariableNames = (empty($this->validUnusedVariableNames))
382+
? []
383+
: Helpers::splitStringToArray('/\s+/', trim($this->validUnusedVariableNames));
384+
$validUndefinedVariableNames = (empty($this->validUndefinedVariableNames))
385+
? []
386+
: Helpers::splitStringToArray('/\s+/', trim($this->validUndefinedVariableNames));
387+
if (in_array($varName, $validUnusedVariableNames)) {
388+
$scopeInfo->variables[$varName]->ignoreUnused = true;
389+
}
390+
if (isset($this->ignoreUnusedRegexp) && preg_match($this->ignoreUnusedRegexp, $varName) === 1) {
391+
$scopeInfo->variables[$varName]->ignoreUnused = true;
392+
}
393+
if ($scopeInfo->scopeStartIndex === 0 && $this->allowUndefinedVariablesInFileScope) {
394+
$scopeInfo->variables[$varName]->ignoreUndefined = true;
395+
}
396+
if (in_array($varName, $validUndefinedVariableNames)) {
397+
$scopeInfo->variables[$varName]->ignoreUndefined = true;
398+
}
399+
if (isset($this->validUndefinedVariableRegexp) && preg_match($this->validUndefinedVariableRegexp, $varName) === 1) {
400+
$scopeInfo->variables[$varName]->ignoreUndefined = true;
401+
}
402+
Helpers::debug("getOrCreateVariableInfo: scope for '{$varName}' is now", $scopeInfo);
400403
return $scopeInfo->variables[$varName];
401404
}
402405

@@ -408,10 +411,12 @@ protected function getOrCreateVariableInfo($varName, $currScope) {
408411
* @return void
409412
*/
410413
protected function markVariableAssignment($varName, $stackPtr, $currScope) {
414+
Helpers::debug('markVariableAssignment: starting for', $varName);
411415
$this->markVariableAssignmentWithoutInitialization($varName, $stackPtr, $currScope);
416+
Helpers::debug('markVariableAssignment: marked as assigned without initialization', $varName);
412417
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
413418
if (isset($varInfo->firstInitialized) && ($varInfo->firstInitialized <= $stackPtr)) {
414-
Helpers::debug('markVariableAssignment variable is already initialized', $varName);
419+
Helpers::debug('markVariableAssignment: variable is already initialized', $varName);
415420
return;
416421
}
417422
$varInfo->firstInitialized = $stackPtr;
@@ -429,7 +434,11 @@ protected function markVariableAssignmentWithoutInitialization($varName, $stackP
429434

430435
// Is the variable referencing another variable? If so, mark that variable used also.
431436
if ($varInfo->referencedVariableScope !== null && $varInfo->referencedVariableScope !== $currScope) {
432-
$this->markVariableAssignment($varInfo->name, $stackPtr, $varInfo->referencedVariableScope);
437+
// 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
438+
if ($this->getVariableInfo($varInfo->name, $varInfo->referencedVariableScope)) {
439+
Helpers::debug('markVariableAssignmentWithoutInitialization: marking referenced variable as assigned also', $varName);
440+
$this->markVariableAssignment($varInfo->name, $stackPtr, $varInfo->referencedVariableScope);
441+
}
433442
}
434443

435444
if (!isset($varInfo->scopeType)) {
@@ -622,12 +631,14 @@ protected function processVariableAsFunctionDefinitionArgument(File $phpcsFile,
622631
// Are we pass-by-reference?
623632
$referencePtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true, null, true);
624633
if (($referencePtr !== false) && ($tokens[$referencePtr]['code'] === T_BITWISE_AND)) {
634+
Helpers::debug("processVariableAsFunctionDefinitionArgument found pass-by-reference to scope", $outerScope);
625635
$varInfo = $this->getOrCreateVariableInfo($varName, $functionPtr);
626636
$varInfo->referencedVariableScope = $outerScope;
627637
}
628638

629639
// Are we optional with a default?
630640
if (Helpers::getNextAssignPointer($phpcsFile, $stackPtr) !== null) {
641+
Helpers::debug("processVariableAsFunctionDefinitionArgument optional with default");
631642
$this->markVariableAssignment($varName, $stackPtr, $functionPtr);
632643
}
633644
}
@@ -898,19 +909,13 @@ protected function processVariableAsStaticOutsideClass(File $phpcsFile, $stackPt
898909
* @param string $varName
899910
* @param int $currScope
900911
*
901-
* @return bool
912+
* @return void
902913
*/
903914
protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varName, $currScope) {
904-
// Is the next non-whitespace an assignment?
915+
Helpers::debug("processVariableAsAssignment: starting for '${varName}'");
905916
$assignPtr = Helpers::getNextAssignPointer($phpcsFile, $stackPtr);
906917
if (! is_int($assignPtr)) {
907-
return false;
908-
}
909-
910-
// Is this a variable variable? If so, it's not an assignment to the current variable.
911-
if ($this->processVariableAsVariableVariable($phpcsFile, $stackPtr)) {
912-
Helpers::debug('found variable variable');
913-
return false;
918+
return;
914919
}
915920

916921
// If the right-hand-side of the assignment to this variable is a reference
@@ -922,12 +927,12 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN
922927
$tokens = $phpcsFile->getTokens();
923928
$referencePtr = $phpcsFile->findNext(Tokens::$emptyTokens, $assignPtr + 1, null, true, null, true);
924929
if (is_int($referencePtr) && $tokens[$referencePtr]['code'] === T_BITWISE_AND) {
930+
Helpers::debug('processVariableAsAssignment: found reference variable');
925931
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
926932
// If the variable was already declared, but was not yet read, it is
927933
// unused because we're about to change the binding.
928934
$scopeInfo = $this->getOrCreateScopeInfo($currScope);
929935
$this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
930-
Helpers::debug('found reference variable');
931936
// The referenced variable may have a different name, but we don't
932937
// actually need to mark it as used in this case because the act of this
933938
// assignment will mark it used on the next token.
@@ -936,39 +941,11 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN
936941
// An assignment to a reference is a binding and should not count as
937942
// initialization since it doesn't change any values.
938943
$this->markVariableAssignmentWithoutInitialization($varName, $stackPtr, $currScope);
939-
return true;
944+
return;
940945
}
941946

947+
Helpers::debug('processVariableAsAssignment: marking as assignment in scope', $currScope);
942948
$this->markVariableAssignment($varName, $stackPtr, $currScope);
943-
944-
return true;
945-
}
946-
947-
/**
948-
* @param File $phpcsFile
949-
* @param int $stackPtr
950-
*
951-
* @return bool
952-
*/
953-
protected function processVariableAsVariableVariable(File $phpcsFile, $stackPtr) {
954-
$tokens = $phpcsFile->getTokens();
955-
956-
$prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($stackPtr - 1), null, true);
957-
if ($prev === false) {
958-
return false;
959-
}
960-
if ($tokens[$prev]['code'] === T_DOLLAR) {
961-
return true;
962-
}
963-
if ($tokens[$prev]['code'] !== T_OPEN_CURLY_BRACKET) {
964-
return false;
965-
}
966-
967-
$prevPrev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prev - 1), null, true);
968-
if ($prevPrev !== false && $tokens[$prevPrev]['code'] === T_DOLLAR) {
969-
return true;
970-
}
971-
return false;
972949
}
973950

974951
/**
@@ -1398,13 +1375,14 @@ protected function processVariable(File $phpcsFile, $stackPtr) {
13981375
}
13991376

14001377
// Is the next non-whitespace an assignment?
1401-
if ($this->processVariableAsAssignment($phpcsFile, $stackPtr, $varName, $currScope)) {
1378+
if (Helpers::isTokenInsideAssignmentLHS($phpcsFile, $stackPtr)) {
1379+
Helpers::debug('found assignment');
1380+
$this->processVariableAsAssignment($phpcsFile, $stackPtr, $varName, $currScope);
14021381
if (Helpers::isTokenInsideAssignmentRHS($phpcsFile, $stackPtr) || Helpers::isTokenInsideFunctionCall($phpcsFile, $stackPtr)) {
14031382
Helpers::debug("found assignment that's also inside an expression");
14041383
$this->markVariableRead($varName, $stackPtr, $currScope);
14051384
return;
14061385
}
1407-
Helpers::debug('found assignment');
14081386
return;
14091387
}
14101388

0 commit comments

Comments
 (0)