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

Author: sirbrillig

Tests/VariableAnalysisSniff/VariableAnalysisTest.php

@@ -970,8 +970,11 @@ public function testUnusedVarWithValueChange() {
     $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
     $expectedWarnings = [
             5,
+            6,
             8,
+            9,
             11,
+            12,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
@@ -983,7 +986,27 @@ public function testAssignmentByReference() {
     $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
     $expectedWarnings = [
       26,
-      33,
+      34,
+      35,
+      43,
+    ];
+    $this->assertEquals($expectedWarnings, $lines);
+  }
+
+  public function testAssignmentByReferenceWithIgnoreUnusedMatch() {
+    $fixtureFile = $this->getFixture('AssignmentByReferenceFixture.php');
+    $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile);
+    $phpcsFile->ruleset->setSniffProperty(
+      'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff',
+      'ignoreUnusedRegexp',
+      '/^varX$/'
+    );
+    $phpcsFile->process();
+    $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
+    $expectedWarnings = [
+      26,
+      34,
+      35,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }

Tests/VariableAnalysisSniff/fixtures/AssignmentByReferenceFixture.php

@@ -23,23 +23,34 @@ function usedAssignmentByReference() {
 function unusedAssignmentByReference() {
   $a = new A();
 
-  $var = &$a->getProp();
+  $var = &$a->getProp(); // unused variable $var
   return $a;
 }
 
 function doubleUnusedAssignmentByReference() {
   $a = new A();
+  $bee = 'hello';
 
-  $var = &$a->getProp();
-  $var = &$a->getProp();
+  $var = &$a->getProp(); // unused variable $var
+  $var = &$bee; // unused variable $var
   return $a;
 }
 
 function doubleUnusedThenUsedAssignmentByReference() {
   $a = new A();
+  $bee = 'hello';
+
+  $varX = &$a->getProp(); // unused variable $varX (because it is actually $a->prop and changes to $bee on the next line)
+  $varX = &$bee;
+  return $varX;
+}
+
+function doubleUsedThenUsedAssignmentByReference() {
+  $a = new A();
+  $bee = 'hello';
 
-  // @todo the first one should be marked as unused.
-  $var = &$a->getProp();
   $var = &$a->getProp();
+  echo $var;
+  $var = &$bee;
   return $var;
 }

Tests/VariableAnalysisSniff/fixtures/FunctionWithUseReferenceFixture.php

@@ -5,3 +5,4 @@
 $function_with_use_reference = function () use (& /*comment */ $ref) {
     $ref = 1;
 };
+echo $function_with_use_reference();

Tests/VariableAnalysisSniff/fixtures/UnusedVarWithValueChangeFixture.php

@@ -2,17 +2,21 @@
 
 // Issue #111.
 function foo() {
-    $var = 'abc';
-    $var = 'def';
+    $var = 'abc'; // unused variable $var
+    $var = 'def'; // unused variable $var
 
-    $var2  = 'def';
-    $var2 .= 'ghi';
+    $var2  = 'def'; // unused variable $var2
+    $var2 .= 'ghi'; // unused variable $var2
 
-    $var3  = 10;
-    $var3 += 20;
+    $var3  = 10; // unused variable $var3
+    $var3 += 20; // unused variable $var3
+
+    $var4 = 20;
+    $var4 += 20;
+    echo $var4;
 }
 
 // Safeguard that this change doesn't influence (not) reporting on assignments to parameters passed by reference.
 function bar(&$param) {
-	$param .= 'foo';
+  $param .= 'foo';
 }

VariableAnalysis/Lib/VariableInfo.php

@@ -25,29 +25,18 @@ class VariableInfo {
    */
   public $typeHint;
 
-  /**
-   * @var bool
-   */
-  public $passByReference = false;
-
-  /**
-   * @var self | null
-   */
-  public $referencedVariable;
-
   /**
    * @var int | null
    */
   public $referencedVariableScope;
 
-  /**
-   * @var bool
-   */
-  public $isReference = false;
-
   /**
    * Stack pointer of first declaration
    *
+   * Declaration is when a variable is created but has no value assigned.
+   *
+   * Assignment by reference is also a declaration and not an initialization.
+   *
    * @var int
    */
   public $firstDeclared;
@@ -66,6 +55,16 @@ class VariableInfo {
    */
   public $firstRead;
 
+  /**
+   * Stack pointers of all assignments
+   *
+   * This includes both declarations and initializations and may contain
+   * duplicates!
+   *
+   * @var int[]
+   */
+  public $allAssignments = [];
+
   /**
    * @var bool
    */

VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php

@@ -362,22 +362,34 @@ protected function areFollowingArgumentsUsed($varInfo, $scopeInfo) {
    * @return void
    */
   protected function markVariableAssignment($varName, $stackPtr, $currScope) {
+    $this->markVariableAssignmentWithoutInitialization($varName, $stackPtr, $currScope);
+    $varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
+    if (isset($varInfo->firstInitialized) && ($varInfo->firstInitialized <= $stackPtr)) {
+      Helpers::debug('markVariableAssignment variable is already initialized', $varName);
+      return;
+    }
+    $varInfo->firstInitialized = $stackPtr;
+  }
+
+  /**
+   * @param string $varName
+   * @param int $stackPtr
+   * @param int $currScope
+   *
+   * @return void
+   */
+  protected function markVariableAssignmentWithoutInitialization($varName, $stackPtr, $currScope) {
     $varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
 
     // Is the variable referencing another variable? If so, mark that variable used also.
-    if ($varInfo->referencedVariable && $varInfo->referencedVariableScope) {
+    if ($varInfo->referencedVariableScope !== null && $varInfo->referencedVariableScope !== $currScope) {
       $this->markVariableAssignment($varInfo->name, $stackPtr, $varInfo->referencedVariableScope);
     }
 
     if (!isset($varInfo->scopeType)) {
       $varInfo->scopeType = ScopeType::LOCAL;
     }
-    if (isset($varInfo->firstInitialized) && ($varInfo->firstInitialized <= $stackPtr)) {
-      Helpers::debug('markVariableAssignment failed; already initialized', $varName);
-      return;
-    }
-    Helpers::debug('markVariableAssignment', $varName);
-    $varInfo->firstInitialized = $stackPtr;
+    $varInfo->allAssignments[] = $stackPtr;
   }
 
   /**
@@ -400,6 +412,7 @@ protected function markVariableDeclaration(
   ) {
     Helpers::debug("marking variable '{$varName}' declared in scope starting at token", $currScope);
     $varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
+
     if (isset($varInfo->scopeType)) {
       if (($permitMatchingRedeclaration === false) || ($varInfo->scopeType !== $scopeType)) {
         //  Issue redeclaration/reuse warning
@@ -418,6 +431,7 @@ protected function markVariableDeclaration(
         );
       }
     }
+
     $varInfo->scopeType = $scopeType;
     if (isset($typeHint)) {
       $varInfo->typeHint = $typeHint;
@@ -427,6 +441,7 @@ protected function markVariableDeclaration(
       return;
     }
     $varInfo->firstDeclared = $stackPtr;
+    $varInfo->allAssignments[] = $stackPtr;
     Helpers::debug("variable '{$varName}' marked declared", $varInfo);
   }
 
@@ -538,7 +553,7 @@ protected function markAllVariablesRead(File $phpcsFile, $stackPtr) {
    *
    * @return void
    */
-  protected function processVariableAsFunctionDefinitionArgument(File $phpcsFile, $stackPtr, $varName) {
+  protected function processVariableAsFunctionDefinitionArgument(File $phpcsFile, $stackPtr, $varName, $outerScope) {
     Helpers::debug("processVariableAsFunctionDefinitionArgument", $stackPtr, $varName);
     $tokens = $phpcsFile->getTokens();
 
@@ -554,7 +569,7 @@ protected function processVariableAsFunctionDefinitionArgument(File $phpcsFile,
     $referencePtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true, null, true);
     if (($referencePtr !== false) && ($tokens[$referencePtr]['code'] === T_BITWISE_AND)) {
       $varInfo = $this->getOrCreateVariableInfo($varName, $functionPtr);
-      $varInfo->passByReference = true;
+      $varInfo->referencedVariableScope = $outerScope;
     }
 
     //  Are we optional with a default?
@@ -576,7 +591,7 @@ protected function processVariableAsFunctionDefinitionArgument(File $phpcsFile,
   protected function processVariableAsUseImportDefinition(File $phpcsFile, $stackPtr, $varName, $outerScope) {
     $tokens = $phpcsFile->getTokens();
 
-    Helpers::debug("processVariableAsUseImportDefinition", $stackPtr, $varName);
+    Helpers::debug("processVariableAsUseImportDefinition", $stackPtr, $varName, $outerScope);
 
     $endOfArgsPtr = $phpcsFile->findPrevious([T_CLOSE_PARENTHESIS], $stackPtr - 1, null);
     if (! is_int($endOfArgsPtr)) {
@@ -606,9 +621,6 @@ protected function processVariableAsUseImportDefinition(File $phpcsFile, $stackP
     if (is_int($referencePtr) && $tokens[$referencePtr]['code'] === T_BITWISE_AND) {
       Helpers::debug("variable '{$varName}' in function definition looks passed by reference");
       $varInfo = $this->getOrCreateVariableInfo($varName, $functionPtr);
-      $varInfo->passByReference = true;
-      $referencedVariable = $this->getVariableInfo($varName, $outerScope);
-      $varInfo->referencedVariable = $referencedVariable;
       $varInfo->referencedVariableScope = $outerScope;
     }
   }
@@ -844,22 +856,36 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN
       return false;
     }
 
-    // Plain ol' assignment. Simpl(ish).
     $writtenPtr = Helpers::findWhereAssignExecuted($phpcsFile, $assignPtr);
-    $this->markVariableAssignment($varName, $writtenPtr, $currScope);
 
-    // Are we are reference variable?
-    $tokens = $tokens = $phpcsFile->getTokens();
+    // If the right-hand-side of the assignment to this variable is a reference
+    // variable, then this variable is a reference to that one, and as such any
+    // assignment to this variable (except another assignment by reference,
+    // which would change the binding) has a side effect of changing the
+    // referenced variable and therefore should count as both an assignment and
+    // a read.
+    $tokens = $phpcsFile->getTokens();
     $referencePtr = $phpcsFile->findNext(Tokens::$emptyTokens, $assignPtr + 1, null, true, null, true);
-    $varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
-    if ($referencePtr !== false && $tokens[$referencePtr]['code'] === T_BITWISE_AND) {
-      $varInfo->isReference = true;
-    } elseif ($varInfo->isReference) {
-      // If this is an assigment to a reference variable then that variable is
-      // used.
-      $this->markVariableRead($varName, $stackPtr, $currScope);
+    if (is_int($referencePtr) && $tokens[$referencePtr]['code'] === T_BITWISE_AND) {
+      $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.
+      $scopeInfo = $this->getOrCreateScopeInfo($currScope);
+      $this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
+      Helpers::debug('found reference variable');
+      // 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.
+      $varInfo->referencedVariableScope = $currScope;
+      $this->markVariableDeclaration($varName, ScopeType::LOCAL, null, $writtenPtr, $currScope, true);
+      // An assignment to a reference is a binding and should not count as
+      // initialization since it doesn't change any values.
+      $this->markVariableAssignmentWithoutInitialization($varName, $writtenPtr, $currScope);
+      return true;
     }
 
+    $this->markVariableAssignment($varName, $writtenPtr, $currScope);
+
     return true;
   }
 
@@ -1233,7 +1259,7 @@ protected function processVariable(File $phpcsFile, $stackPtr) {
     $token  = $tokens[$stackPtr];
 
     $varName = Helpers::normalizeVarName($token['content']);
-    Helpers::debug("examining token for variable '{$varName}'", $token);
+    Helpers::debug("examining token for variable '{$varName}' on line {$token['line']}", $token);
     $currScope = Helpers::findVariableScope($phpcsFile, $stackPtr);
     if ($currScope === null) {
       Helpers::debug('no scope found');
@@ -1270,7 +1296,7 @@ protected function processVariable(File $phpcsFile, $stackPtr) {
     // Are we a function or closure parameter?
     if (Helpers::isTokenInsideFunctionDefinitionArgumentList($phpcsFile, $stackPtr)) {
       Helpers::debug('found function definition argument');
-      $this->processVariableAsFunctionDefinitionArgument($phpcsFile, $stackPtr, $varName);
+      $this->processVariableAsFunctionDefinitionArgument($phpcsFile, $stackPtr, $varName, $currScope);
       return;
     }
 
@@ -1527,7 +1553,7 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v
     if ($this->allowUnusedForeachVariables && $varInfo->isForeachLoopAssociativeValue) {
       return;
     }
-    if ($varInfo->passByReference && isset($varInfo->firstInitialized)) {
+    if ($varInfo->referencedVariableScope !== null && isset($varInfo->firstInitialized)) {
       // 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
@@ -1540,19 +1566,24 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v
       // the purposes of "unused variable" warnings.
       return;
     }
-
-    $stackPtr = null;
-    if (! empty($varInfo->firstDeclared)) {
-      $stackPtr = $varInfo->firstDeclared;
-    } elseif (! empty($varInfo->firstInitialized)) {
-      $stackPtr = $varInfo->firstInitialized;
+    if (empty($varInfo->firstDeclared) && empty($varInfo->firstInitialized)) {
+      return;
     }
+    $this->warnAboutUnusedVariable($phpcsFile, $varInfo);
+  }
 
-    if ($stackPtr) {
+  /**
+   * @param File $phpcsFile
+   * @param VariableInfo $varInfo
+   *
+   * @return void
+   */
+  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");
       $phpcsFile->addWarning(
         "Unused %s %s.",
-        $stackPtr,
+        $indexForWarning,
         'UnusedVariable',
         [
           VariableInfo::$scopeTypeDescriptions[$varInfo->scopeType],