Skip to content

Commit c6d3b02

Browse files
authored
Refactor and clean up reference variables (#187)
* Remove unnecessary passByReference and referencedVariable * Make sure we allow 0 scopes * Make AssignmentByReferenceFixture more accurate * Refactor assignment-by-reference to be more clear * Report all declarations of an unused variable * Change UnusedVarWithValueChangeFixture to be more accurate * Check re-bound variables for use at the time of rebinding * Update reference test to cover ignores * Use existing checks for scope close for changing bindings * Make sure scope always exists for re-binding scope close
1 parent 58b0fd8 commit c6d3b02

File tree

6 files changed

+132
-63
lines changed

6 files changed

+132
-63
lines changed

Tests/VariableAnalysisSniff/VariableAnalysisTest.php

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -970,8 +970,11 @@ public function testUnusedVarWithValueChange() {
970970
$lines = $this->getWarningLineNumbersFromFile($phpcsFile);
971971
$expectedWarnings = [
972972
5,
973+
6,
973974
8,
975+
9,
974976
11,
977+
12,
975978
];
976979
$this->assertEquals($expectedWarnings, $lines);
977980
}
@@ -983,7 +986,27 @@ public function testAssignmentByReference() {
983986
$lines = $this->getWarningLineNumbersFromFile($phpcsFile);
984987
$expectedWarnings = [
985988
26,
986-
33,
989+
34,
990+
35,
991+
43,
992+
];
993+
$this->assertEquals($expectedWarnings, $lines);
994+
}
995+
996+
public function testAssignmentByReferenceWithIgnoreUnusedMatch() {
997+
$fixtureFile = $this->getFixture('AssignmentByReferenceFixture.php');
998+
$phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile);
999+
$phpcsFile->ruleset->setSniffProperty(
1000+
'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff',
1001+
'ignoreUnusedRegexp',
1002+
'/^varX$/'
1003+
);
1004+
$phpcsFile->process();
1005+
$lines = $this->getWarningLineNumbersFromFile($phpcsFile);
1006+
$expectedWarnings = [
1007+
26,
1008+
34,
1009+
35,
9871010
];
9881011
$this->assertEquals($expectedWarnings, $lines);
9891012
}

Tests/VariableAnalysisSniff/fixtures/AssignmentByReferenceFixture.php

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,23 +23,34 @@ function usedAssignmentByReference() {
2323
function unusedAssignmentByReference() {
2424
$a = new A();
2525

26-
$var = &$a->getProp();
26+
$var = &$a->getProp(); // unused variable $var
2727
return $a;
2828
}
2929

3030
function doubleUnusedAssignmentByReference() {
3131
$a = new A();
32+
$bee = 'hello';
3233

33-
$var = &$a->getProp();
34-
$var = &$a->getProp();
34+
$var = &$a->getProp(); // unused variable $var
35+
$var = &$bee; // unused variable $var
3536
return $a;
3637
}
3738

3839
function doubleUnusedThenUsedAssignmentByReference() {
3940
$a = new A();
41+
$bee = 'hello';
42+
43+
$varX = &$a->getProp(); // unused variable $varX (because it is actually $a->prop and changes to $bee on the next line)
44+
$varX = &$bee;
45+
return $varX;
46+
}
47+
48+
function doubleUsedThenUsedAssignmentByReference() {
49+
$a = new A();
50+
$bee = 'hello';
4051

41-
// @todo the first one should be marked as unused.
42-
$var = &$a->getProp();
4352
$var = &$a->getProp();
53+
echo $var;
54+
$var = &$bee;
4455
return $var;
4556
}

Tests/VariableAnalysisSniff/fixtures/FunctionWithUseReferenceFixture.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,4 @@
55
$function_with_use_reference = function () use (& /*comment */ $ref) {
66
$ref = 1;
77
};
8+
echo $function_with_use_reference();

Tests/VariableAnalysisSniff/fixtures/UnusedVarWithValueChangeFixture.php

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,21 @@
22

33
// Issue #111.
44
function foo() {
5-
$var = 'abc';
6-
$var = 'def';
5+
$var = 'abc'; // unused variable $var
6+
$var = 'def'; // unused variable $var
77

8-
$var2 = 'def';
9-
$var2 .= 'ghi';
8+
$var2 = 'def'; // unused variable $var2
9+
$var2 .= 'ghi'; // unused variable $var2
1010

11-
$var3 = 10;
12-
$var3 += 20;
11+
$var3 = 10; // unused variable $var3
12+
$var3 += 20; // unused variable $var3
13+
14+
$var4 = 20;
15+
$var4 += 20;
16+
echo $var4;
1317
}
1418

1519
// Safeguard that this change doesn't influence (not) reporting on assignments to parameters passed by reference.
1620
function bar(&$param) {
17-
$param .= 'foo';
21+
$param .= 'foo';
1822
}

VariableAnalysis/Lib/VariableInfo.php

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,29 +25,18 @@ class VariableInfo {
2525
*/
2626
public $typeHint;
2727

28-
/**
29-
* @var bool
30-
*/
31-
public $passByReference = false;
32-
33-
/**
34-
* @var self | null
35-
*/
36-
public $referencedVariable;
37-
3828
/**
3929
* @var int | null
4030
*/
4131
public $referencedVariableScope;
4232

43-
/**
44-
* @var bool
45-
*/
46-
public $isReference = false;
47-
4833
/**
4934
* Stack pointer of first declaration
5035
*
36+
* Declaration is when a variable is created but has no value assigned.
37+
*
38+
* Assignment by reference is also a declaration and not an initialization.
39+
*
5140
* @var int
5241
*/
5342
public $firstDeclared;
@@ -66,6 +55,16 @@ class VariableInfo {
6655
*/
6756
public $firstRead;
6857

58+
/**
59+
* Stack pointers of all assignments
60+
*
61+
* This includes both declarations and initializations and may contain
62+
* duplicates!
63+
*
64+
* @var int[]
65+
*/
66+
public $allAssignments = [];
67+
6968
/**
7069
* @var bool
7170
*/

VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php

Lines changed: 66 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -362,22 +362,34 @@ protected function areFollowingArgumentsUsed($varInfo, $scopeInfo) {
362362
* @return void
363363
*/
364364
protected function markVariableAssignment($varName, $stackPtr, $currScope) {
365+
$this->markVariableAssignmentWithoutInitialization($varName, $stackPtr, $currScope);
366+
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
367+
if (isset($varInfo->firstInitialized) && ($varInfo->firstInitialized <= $stackPtr)) {
368+
Helpers::debug('markVariableAssignment variable is already initialized', $varName);
369+
return;
370+
}
371+
$varInfo->firstInitialized = $stackPtr;
372+
}
373+
374+
/**
375+
* @param string $varName
376+
* @param int $stackPtr
377+
* @param int $currScope
378+
*
379+
* @return void
380+
*/
381+
protected function markVariableAssignmentWithoutInitialization($varName, $stackPtr, $currScope) {
365382
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
366383

367384
// Is the variable referencing another variable? If so, mark that variable used also.
368-
if ($varInfo->referencedVariable && $varInfo->referencedVariableScope) {
385+
if ($varInfo->referencedVariableScope !== null && $varInfo->referencedVariableScope !== $currScope) {
369386
$this->markVariableAssignment($varInfo->name, $stackPtr, $varInfo->referencedVariableScope);
370387
}
371388

372389
if (!isset($varInfo->scopeType)) {
373390
$varInfo->scopeType = ScopeType::LOCAL;
374391
}
375-
if (isset($varInfo->firstInitialized) && ($varInfo->firstInitialized <= $stackPtr)) {
376-
Helpers::debug('markVariableAssignment failed; already initialized', $varName);
377-
return;
378-
}
379-
Helpers::debug('markVariableAssignment', $varName);
380-
$varInfo->firstInitialized = $stackPtr;
392+
$varInfo->allAssignments[] = $stackPtr;
381393
}
382394

383395
/**
@@ -400,6 +412,7 @@ protected function markVariableDeclaration(
400412
) {
401413
Helpers::debug("marking variable '{$varName}' declared in scope starting at token", $currScope);
402414
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
415+
403416
if (isset($varInfo->scopeType)) {
404417
if (($permitMatchingRedeclaration === false) || ($varInfo->scopeType !== $scopeType)) {
405418
// Issue redeclaration/reuse warning
@@ -418,6 +431,7 @@ protected function markVariableDeclaration(
418431
);
419432
}
420433
}
434+
421435
$varInfo->scopeType = $scopeType;
422436
if (isset($typeHint)) {
423437
$varInfo->typeHint = $typeHint;
@@ -427,6 +441,7 @@ protected function markVariableDeclaration(
427441
return;
428442
}
429443
$varInfo->firstDeclared = $stackPtr;
444+
$varInfo->allAssignments[] = $stackPtr;
430445
Helpers::debug("variable '{$varName}' marked declared", $varInfo);
431446
}
432447

@@ -538,7 +553,7 @@ protected function markAllVariablesRead(File $phpcsFile, $stackPtr) {
538553
*
539554
* @return void
540555
*/
541-
protected function processVariableAsFunctionDefinitionArgument(File $phpcsFile, $stackPtr, $varName) {
556+
protected function processVariableAsFunctionDefinitionArgument(File $phpcsFile, $stackPtr, $varName, $outerScope) {
542557
Helpers::debug("processVariableAsFunctionDefinitionArgument", $stackPtr, $varName);
543558
$tokens = $phpcsFile->getTokens();
544559

@@ -554,7 +569,7 @@ protected function processVariableAsFunctionDefinitionArgument(File $phpcsFile,
554569
$referencePtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true, null, true);
555570
if (($referencePtr !== false) && ($tokens[$referencePtr]['code'] === T_BITWISE_AND)) {
556571
$varInfo = $this->getOrCreateVariableInfo($varName, $functionPtr);
557-
$varInfo->passByReference = true;
572+
$varInfo->referencedVariableScope = $outerScope;
558573
}
559574

560575
// Are we optional with a default?
@@ -576,7 +591,7 @@ protected function processVariableAsFunctionDefinitionArgument(File $phpcsFile,
576591
protected function processVariableAsUseImportDefinition(File $phpcsFile, $stackPtr, $varName, $outerScope) {
577592
$tokens = $phpcsFile->getTokens();
578593

579-
Helpers::debug("processVariableAsUseImportDefinition", $stackPtr, $varName);
594+
Helpers::debug("processVariableAsUseImportDefinition", $stackPtr, $varName, $outerScope);
580595

581596
$endOfArgsPtr = $phpcsFile->findPrevious([T_CLOSE_PARENTHESIS], $stackPtr - 1, null);
582597
if (! is_int($endOfArgsPtr)) {
@@ -606,9 +621,6 @@ protected function processVariableAsUseImportDefinition(File $phpcsFile, $stackP
606621
if (is_int($referencePtr) && $tokens[$referencePtr]['code'] === T_BITWISE_AND) {
607622
Helpers::debug("variable '{$varName}' in function definition looks passed by reference");
608623
$varInfo = $this->getOrCreateVariableInfo($varName, $functionPtr);
609-
$varInfo->passByReference = true;
610-
$referencedVariable = $this->getVariableInfo($varName, $outerScope);
611-
$varInfo->referencedVariable = $referencedVariable;
612624
$varInfo->referencedVariableScope = $outerScope;
613625
}
614626
}
@@ -844,22 +856,36 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN
844856
return false;
845857
}
846858

847-
// Plain ol' assignment. Simpl(ish).
848859
$writtenPtr = Helpers::findWhereAssignExecuted($phpcsFile, $assignPtr);
849-
$this->markVariableAssignment($varName, $writtenPtr, $currScope);
850860

851-
// Are we are reference variable?
852-
$tokens = $tokens = $phpcsFile->getTokens();
861+
// If the right-hand-side of the assignment to this variable is a reference
862+
// variable, then this variable is a reference to that one, and as such any
863+
// assignment to this variable (except another assignment by reference,
864+
// which would change the binding) has a side effect of changing the
865+
// referenced variable and therefore should count as both an assignment and
866+
// a read.
867+
$tokens = $phpcsFile->getTokens();
853868
$referencePtr = $phpcsFile->findNext(Tokens::$emptyTokens, $assignPtr + 1, null, true, null, true);
854-
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
855-
if ($referencePtr !== false && $tokens[$referencePtr]['code'] === T_BITWISE_AND) {
856-
$varInfo->isReference = true;
857-
} elseif ($varInfo->isReference) {
858-
// If this is an assigment to a reference variable then that variable is
859-
// used.
860-
$this->markVariableRead($varName, $stackPtr, $currScope);
869+
if (is_int($referencePtr) && $tokens[$referencePtr]['code'] === T_BITWISE_AND) {
870+
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
871+
// If the variable was already declared, but was not yet read, it is
872+
// unused because we're about to change the binding.
873+
$scopeInfo = $this->getOrCreateScopeInfo($currScope);
874+
$this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
875+
Helpers::debug('found reference variable');
876+
// The referenced variable may have a different name, but we don't
877+
// actually need to mark it as used in this case because the act of this
878+
// assignment will mark it used on the next token.
879+
$varInfo->referencedVariableScope = $currScope;
880+
$this->markVariableDeclaration($varName, ScopeType::LOCAL, null, $writtenPtr, $currScope, true);
881+
// An assignment to a reference is a binding and should not count as
882+
// initialization since it doesn't change any values.
883+
$this->markVariableAssignmentWithoutInitialization($varName, $writtenPtr, $currScope);
884+
return true;
861885
}
862886

887+
$this->markVariableAssignment($varName, $writtenPtr, $currScope);
888+
863889
return true;
864890
}
865891

@@ -1233,7 +1259,7 @@ protected function processVariable(File $phpcsFile, $stackPtr) {
12331259
$token = $tokens[$stackPtr];
12341260

12351261
$varName = Helpers::normalizeVarName($token['content']);
1236-
Helpers::debug("examining token for variable '{$varName}'", $token);
1262+
Helpers::debug("examining token for variable '{$varName}' on line {$token['line']}", $token);
12371263
$currScope = Helpers::findVariableScope($phpcsFile, $stackPtr);
12381264
if ($currScope === null) {
12391265
Helpers::debug('no scope found');
@@ -1270,7 +1296,7 @@ protected function processVariable(File $phpcsFile, $stackPtr) {
12701296
// Are we a function or closure parameter?
12711297
if (Helpers::isTokenInsideFunctionDefinitionArgumentList($phpcsFile, $stackPtr)) {
12721298
Helpers::debug('found function definition argument');
1273-
$this->processVariableAsFunctionDefinitionArgument($phpcsFile, $stackPtr, $varName);
1299+
$this->processVariableAsFunctionDefinitionArgument($phpcsFile, $stackPtr, $varName, $currScope);
12741300
return;
12751301
}
12761302

@@ -1527,7 +1553,7 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v
15271553
if ($this->allowUnusedForeachVariables && $varInfo->isForeachLoopAssociativeValue) {
15281554
return;
15291555
}
1530-
if ($varInfo->passByReference && isset($varInfo->firstInitialized)) {
1556+
if ($varInfo->referencedVariableScope !== null && isset($varInfo->firstInitialized)) {
15311557
// If we're pass-by-reference then it's a common pattern to
15321558
// use the variable to return data to the caller, so any
15331559
// assignment also counts as "variable use" for the purposes
@@ -1540,19 +1566,24 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v
15401566
// the purposes of "unused variable" warnings.
15411567
return;
15421568
}
1543-
1544-
$stackPtr = null;
1545-
if (! empty($varInfo->firstDeclared)) {
1546-
$stackPtr = $varInfo->firstDeclared;
1547-
} elseif (! empty($varInfo->firstInitialized)) {
1548-
$stackPtr = $varInfo->firstInitialized;
1569+
if (empty($varInfo->firstDeclared) && empty($varInfo->firstInitialized)) {
1570+
return;
15491571
}
1572+
$this->warnAboutUnusedVariable($phpcsFile, $varInfo);
1573+
}
15501574

1551-
if ($stackPtr) {
1575+
/**
1576+
* @param File $phpcsFile
1577+
* @param VariableInfo $varInfo
1578+
*
1579+
* @return void
1580+
*/
1581+
protected function warnAboutUnusedVariable(File $phpcsFile, VariableInfo $varInfo) {
1582+
foreach (array_unique($varInfo->allAssignments) as $indexForWarning) {
15521583
Helpers::debug("variable {$varInfo->name} at end of scope looks unused");
15531584
$phpcsFile->addWarning(
15541585
"Unused %s %s.",
1555-
$stackPtr,
1586+
$indexForWarning,
15561587
'UnusedVariable',
15571588
[
15581589
VariableInfo::$scopeTypeDescriptions[$varInfo->scopeType],

0 commit comments

Comments
 (0)