Skip to content

Commit 5048655

Browse files
sirbrilligsirbrillig
authored
Fix array_walk pass-by-reference in 2.x (#216)
* 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 26f24af commit 5048655

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
@@ -1015,4 +1015,52 @@ public static function isTokenInsideAssignmentRHS(File $phpcsFile, $stackPtr) {
10151015
}
10161016
return false;
10171017
}
1018+
1019+
/**
1020+
* @param File $phpcsFile
1021+
* @param int $stackPtr
1022+
*
1023+
* @return bool
1024+
*/
1025+
public static function isTokenInsideAssignmentLHS(File $phpcsFile, $stackPtr) {
1026+
// Is the next non-whitespace an assignment?
1027+
$assignPtr = self::getNextAssignPointer($phpcsFile, $stackPtr);
1028+
if (! is_int($assignPtr)) {
1029+
return false;
1030+
}
1031+
1032+
// Is this a variable variable? If so, it's not an assignment to the current variable.
1033+
if (self::isTokenVariableVariable($phpcsFile, $stackPtr)) {
1034+
self::debug('found variable variable');
1035+
return false;
1036+
}
1037+
return true;
1038+
}
1039+
1040+
/**
1041+
* @param File $phpcsFile
1042+
* @param int $stackPtr
1043+
*
1044+
* @return bool
1045+
*/
1046+
public static function isTokenVariableVariable(File $phpcsFile, $stackPtr) {
1047+
$tokens = $phpcsFile->getTokens();
1048+
1049+
$prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($stackPtr - 1), null, true);
1050+
if ($prev === false) {
1051+
return false;
1052+
}
1053+
if ($tokens[$prev]['code'] === T_DOLLAR) {
1054+
return true;
1055+
}
1056+
if ($tokens[$prev]['code'] !== T_OPEN_CURLY_BRACKET) {
1057+
return false;
1058+
}
1059+
1060+
$prevPrev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prev - 1), null, true);
1061+
if ($prevPrev !== false && $tokens[$prevPrev]['code'] === T_DOLLAR) {
1062+
return true;
1063+
}
1064+
return false;
1065+
}
10181066
}

‎VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php

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

@@ -406,10 +409,12 @@ protected function getOrCreateVariableInfo($varName, $currScope) {
406409
* @return void
407410
*/
408411
protected function markVariableAssignment($varName, $stackPtr, $currScope) {
412+
Helpers::debug('markVariableAssignment: starting for', $varName);
409413
$this->markVariableAssignmentWithoutInitialization($varName, $stackPtr, $currScope);
414+
Helpers::debug('markVariableAssignment: marked as assigned without initialization', $varName);
410415
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
411416
if (isset($varInfo->firstInitialized) && ($varInfo->firstInitialized <= $stackPtr)) {
412-
Helpers::debug('markVariableAssignment variable is already initialized', $varName);
417+
Helpers::debug('markVariableAssignment: variable is already initialized', $varName);
413418
return;
414419
}
415420
$varInfo->firstInitialized = $stackPtr;
@@ -427,7 +432,11 @@ protected function markVariableAssignmentWithoutInitialization($varName, $stackP
427432

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

433442
if (!isset($varInfo->scopeType)) {
@@ -620,12 +629,14 @@ protected function processVariableAsFunctionDefinitionArgument(File $phpcsFile,
620629
// Are we pass-by-reference?
621630
$referencePtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true, null, true);
622631
if (($referencePtr !== false) && ($tokens[$referencePtr]['code'] === T_BITWISE_AND)) {
632+
Helpers::debug("processVariableAsFunctionDefinitionArgument found pass-by-reference to scope", $outerScope);
623633
$varInfo = $this->getOrCreateVariableInfo($varName, $functionPtr);
624634
$varInfo->referencedVariableScope = $outerScope;
625635
}
626636

627637
// Are we optional with a default?
628638
if (Helpers::getNextAssignPointer($phpcsFile, $stackPtr) !== null) {
639+
Helpers::debug("processVariableAsFunctionDefinitionArgument optional with default");
629640
$this->markVariableAssignment($varName, $stackPtr, $functionPtr);
630641
}
631642
}
@@ -896,19 +907,13 @@ protected function processVariableAsStaticOutsideClass(File $phpcsFile, $stackPt
896907
* @param string $varName
897908
* @param int $currScope
898909
*
899-
* @return bool
910+
* @return void
900911
*/
901912
protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varName, $currScope) {
902-
// Is the next non-whitespace an assignment?
913+
Helpers::debug("processVariableAsAssignment: starting for '${varName}'");
903914
$assignPtr = Helpers::getNextAssignPointer($phpcsFile, $stackPtr);
904915
if (! is_int($assignPtr)) {
905-
return false;
906-
}
907-
908-
// Is this a variable variable? If so, it's not an assignment to the current variable.
909-
if ($this->processVariableAsVariableVariable($phpcsFile, $stackPtr)) {
910-
Helpers::debug('found variable variable');
911-
return false;
916+
return;
912917
}
913918

914919
// If the right-hand-side of the assignment to this variable is a reference
@@ -920,12 +925,12 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN
920925
$tokens = $phpcsFile->getTokens();
921926
$referencePtr = $phpcsFile->findNext(Tokens::$emptyTokens, $assignPtr + 1, null, true, null, true);
922927
if (is_int($referencePtr) && $tokens[$referencePtr]['code'] === T_BITWISE_AND) {
928+
Helpers::debug('processVariableAsAssignment: found reference variable');
923929
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
924930
// If the variable was already declared, but was not yet read, it is
925931
// unused because we're about to change the binding.
926932
$scopeInfo = $this->getOrCreateScopeInfo($currScope);
927933
$this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
928-
Helpers::debug('found reference variable');
929934
// The referenced variable may have a different name, but we don't
930935
// actually need to mark it as used in this case because the act of this
931936
// assignment will mark it used on the next token.
@@ -934,39 +939,11 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN
934939
// An assignment to a reference is a binding and should not count as
935940
// initialization since it doesn't change any values.
936941
$this->markVariableAssignmentWithoutInitialization($varName, $stackPtr, $currScope);
937-
return true;
942+
return;
938943
}
939944

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

972949
/**
@@ -1394,13 +1371,14 @@ protected function processVariable(File $phpcsFile, $stackPtr) {
13941371
}
13951372

13961373
// Is the next non-whitespace an assignment?
1397-
if ($this->processVariableAsAssignment($phpcsFile, $stackPtr, $varName, $currScope)) {
1374+
if (Helpers::isTokenInsideAssignmentLHS($phpcsFile, $stackPtr)) {
1375+
Helpers::debug('found assignment');
1376+
$this->processVariableAsAssignment($phpcsFile, $stackPtr, $varName, $currScope);
13981377
if (Helpers::isTokenInsideAssignmentRHS($phpcsFile, $stackPtr) || Helpers::isTokenInsideFunctionCall($phpcsFile, $stackPtr)) {
13991378
Helpers::debug("found assignment that's also inside an expression");
14001379
$this->markVariableRead($varName, $stackPtr, $currScope);
14011380
return;
14021381
}
1403-
Helpers::debug('found assignment');
14041382
return;
14051383
}
14061384

0 commit comments

Comments
 (0)