Ignore double assignment by reference if second is inside condition (sirbrillig#235)

* Add test for overwriting assignment by reference inside conditional

* Allow overwriting reference assignments within conditions

* Make sure unused reference reassignments still trigger in conditions

* Include new test case in other tests

* Compare position of IF token and last assignment

* Fix return type of new function

Author: sirbrillig

Tests/VariableAnalysisSniff/VariableAnalysisTest.php

@@ -1040,6 +1040,7 @@ public function testAssignmentByReference() {
       34,
       35,
       43,
+      70,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
@@ -1058,6 +1059,7 @@ public function testAssignmentByReferenceWithIgnoreUnusedMatch() {
       26,
       34,
       35,
+      70,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }

Tests/VariableAnalysisSniff/fixtures/AssignmentByReferenceFixture.php

@@ -54,3 +54,21 @@ function doubleUsedThenUsedAssignmentByReference() {
   $var = &$bee;
   return $var;
 }
+
+function somefunc($choice, &$arr1, &$arr_default) {
+  $var = &$arr_default;
+
+  if ($choice) {
+    $var = &$arr1;
+  }
+
+  echo $var;
+}
+
+function somefunc($choice, &$arr1, &$arr_default) {
+  if ($choice) {
+    $var = &$arr_default; // unused variable $var
+    $var = &$arr1;
+    echo $var;
+  }
+}

VariableAnalysis/Lib/Helpers.php

@@ -102,6 +102,25 @@ public static function areConditionsWithinFunctionBeforeClass(array $conditions)
     return false;
   }
 
+  /**
+   * @param (int|string)[] $conditions
+   *
+   * @return int|string|null
+   */
+  public static function getClosestIfPositionIfBeforeOtherConditions(array $conditions) {
+    // Return true if the token conditions are within an if block before
+    // they are within a class or function.
+    $conditionsInsideOut = array_reverse($conditions, true);
+    if (empty($conditions)) {
+      return null;
+    }
+    $scopeCode = reset($conditionsInsideOut);
+    if ($scopeCode === T_IF) {
+      return key($conditionsInsideOut);
+    }
+    return null;
+  }
+
   /**
    * @param File $phpcsFile
    * @param int $stackPtr

VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php

@@ -937,9 +937,18 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN
       Helpers::debug('processVariableAsAssignment: found reference variable');
       $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.
+      // 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);
-      $this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
+      $ifPtr = Helpers::getClosestIfPositionIfBeforeOtherConditions($tokens[$referencePtr]['conditions']);
+      $lastAssignmentPtr = $varInfo->firstDeclared;
+      if (! $ifPtr && $lastAssignmentPtr) {
+        $this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
+      }
+      if ($ifPtr && $lastAssignmentPtr && $ifPtr <= $lastAssignmentPtr) {
+        $this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
+      }
       // 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.