From a6318ea38bcd6aab6f7a45bc7c471793d13327bb Mon Sep 17 00:00:00 2001
From: Payton Swick <payton@foolord.com>
Date: Thu, 9 Jul 2020 19:35:24 -0400
Subject: [PATCH 1/6] Add tests for if condition blocks

---
 .../VariableAnalysisSniff/IfConditionTest.php |  30 +++++
 .../FunctionWithIfConditionFixture.php        | 120 ++++++++++++++++++
 2 files changed, 150 insertions(+)
 create mode 100644 Tests/VariableAnalysisSniff/IfConditionTest.php
 create mode 100644 Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php

diff --git a/Tests/VariableAnalysisSniff/IfConditionTest.php b/Tests/VariableAnalysisSniff/IfConditionTest.php
new file mode 100644
index 00000000..54b5491b
--- /dev/null
+++ b/Tests/VariableAnalysisSniff/IfConditionTest.php
@@ -0,0 +1,30 @@
+<?php
+namespace VariableAnalysis\Tests\VariableAnalysisSniff;
+
+use VariableAnalysis\Tests\BaseTestCase;
+
+class IfConditionTest extends BaseTestCase {
+  public function testIfConditionWarnings() {
+    $fixtureFile = $this->getFixture('FunctionWithIfConditionFixture.php');
+    $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile);
+    $phpcsFile->ruleset->setSniffProperty(
+      'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff',
+      'allowUnusedParametersBeforeUsed',
+      'true'
+    );
+    $phpcsFile->process();
+    $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
+    $expectedWarnings = [
+      15,
+      27,
+      36,
+      38,
+      47,
+      58,
+      62,
+      70,
+      74,
+    ];
+    $this->assertEquals($expectedWarnings, $lines);
+  }
+}
diff --git a/Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php b/Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php
new file mode 100644
index 00000000..ce5207ee
--- /dev/null
+++ b/Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php
@@ -0,0 +1,120 @@
+<?php
+
+function normalIfCondition($first, $second) {
+  $name = 'human';
+  if ($first) {
+    $words = "hello {$name}";
+  } elseif ($second) {
+    $words = "bye {$name}";
+  }
+  echo $words;
+}
+
+function undefinedInsideIfCondition($second) {
+  $name = 'human';
+  if ($first) { // undefined variable $first
+    $words = "hello {$name}";
+  } elseif ($second) {
+    $words = "bye {$name}";
+  }
+  echo $words;
+}
+
+function undefinedInsideElseCondition($first) {
+  $name = 'human';
+  if ($first) {
+    $words = "hello {$name}";
+  } elseif ($second) { // undefined variable $second
+    $words = "bye {$name}";
+  }
+  echo $words;
+}
+
+function definedInsideFirstBlockUndefinedInsideElseCondition($first) {
+  $name = 'human';
+  if ($first) {
+    $second = true; // unused variable $second
+    $words = "hello {$name}";
+  } elseif ($second) { // undefined variable $second
+    $words = "bye {$name}";
+  }
+  echo $words;
+}
+
+function unusedInsideFirstBlock($first, $second) {
+  $name = 'human';
+  if ($first) {
+    $unused = true; // unused variable $unused
+    $words = "hello {$name}";
+  } elseif ($second) {
+    $words = "bye {$name}";
+  }
+  echo $words;
+}
+
+function definedInsideFirstBlockUndefinedInsideElseIfBlock($first) {
+  $name = 'human';
+  if ($first) {
+    $second = true; // unused variable $second
+    $words = "hello {$name}";
+  } elseif ($name) {
+    $words = "bye {$name}";
+    echo $second; // undefined variable $second
+  }
+  echo $words;
+}
+
+function definedInsideFirstBlockUndefinedInsideElseBlock($first) {
+  $name = 'human';
+  if ($first) {
+    $second = true; // unused variable $second
+    $words = "hello {$name}";
+  } else {
+    $words = "bye {$name}";
+    echo $second; // undefined variable $second
+  }
+  echo $words;
+}
+
+function definedInsideFirstBlockUndefinedInsideUnconnectedElseCondition($first) {
+  $name = 'human';
+  if ($first) {
+    $second = true;
+    $words = "hello {$name}";
+  } elseif ($name) {
+    $words = "bye {$name}";
+  }
+  if ($first) {
+    $second = true;
+    $words = "hello {$name}";
+  } elseif ($second) {
+    $words = "bye {$name}";
+  }
+  echo $words;
+}
+
+function definedInsideFirstBlockUndefinedInsideSecondCondition($first) {
+  $name = 'human';
+  if ($first) {
+    $second = true;
+    $words = "hello {$name}";
+  }
+  if ($second) {
+    $words = "bye {$name}";
+  }
+  echo $words;
+}
+
+function ifConditionWithPossibleDefinition($first) {
+  if ($first) {
+    $name = 'person';
+  }
+  echo $name;
+}
+
+function ifConditionWithPossibleUse($first) {
+  $name = 'person';
+  if ($first) {
+    echo $name;
+  }
+}

From 5df844fa612e6910d9f65e44588a9378e0aa8053 Mon Sep 17 00:00:00 2001
From: Payton Swick <payton@foolord.com>
Date: Thu, 9 Jul 2020 20:30:50 -0400
Subject: [PATCH 2/6] Process else conditions/blocks with more scrutiny

---
 VariableAnalysis/Lib/Helpers.php              | 70 +++++++++++++++++++
 .../CodeAnalysis/VariableAnalysisSniff.php    | 37 ++++++++++
 2 files changed, 107 insertions(+)

diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php
index 574c4538..2db21579 100644
--- a/VariableAnalysis/Lib/Helpers.php
+++ b/VariableAnalysis/Lib/Helpers.php
@@ -560,4 +560,74 @@ public static function splitStringToArray($pattern, $value) {
   public static function isVariableANumericVariable($varName) {
     return is_numeric(substr($varName, 0, 1));
   }
+
+  /**
+   * @param File $phpcsFile
+   * @param int $stackPtr
+   *
+   * @return bool
+   */
+  public static function isVariableInsideElseCondition(File $phpcsFile, $stackPtr) {
+    $tokens = $phpcsFile->getTokens();
+    $nonFunctionTokenTypes = array_values(Tokens::$emptyTokens);
+    $nonFunctionTokenTypes[] = T_OPEN_PARENTHESIS;
+    $nonFunctionTokenTypes[] = T_VARIABLE;
+    $nonFunctionTokenTypes[] = T_ELLIPSIS;
+    $nonFunctionTokenTypes[] = T_COMMA;
+    $nonFunctionTokenTypes[] = T_STRING;
+    $nonFunctionTokenTypes[] = T_BITWISE_AND;
+    $elsePtr = self::getIntOrNull($phpcsFile->findPrevious($nonFunctionTokenTypes, $stackPtr - 1, null, true, null, true));
+    $elseTokenTypes = [
+      T_ELSE,
+      T_ELSEIF,
+    ];
+    if (is_int($elsePtr) && in_array($tokens[$elsePtr]['code'], $elseTokenTypes, true)) {
+      return true;
+    }
+    return false;
+  }
+
+  /**
+   * @param File $phpcsFile
+   * @param int $stackPtr
+   *
+   * @return bool
+   */
+  public static function isVariableInsideElseBody(File $phpcsFile, $stackPtr) {
+    $tokens = $phpcsFile->getTokens();
+    $token = $tokens[$stackPtr];
+    $conditions = isset($token['conditions']) ? $token['conditions'] : [];
+    $elseTokenTypes = [
+      T_ELSE,
+      T_ELSEIF,
+    ];
+    foreach (array_reverse($conditions, true) as $scopeCode) {
+      if (in_array($scopeCode, $elseTokenTypes, true)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  /**
+   * @param File $phpcsFile
+   * @param int $stackPtr
+   *
+   * @return [int, int]
+   */
+  public static function getAttachedIfBlockStartEndForElse(File $phpcsFile, $stackPtr) {
+    // If this is indeed an `else` or `elseif`, we can look backward until we
+    // find the nearest `if` without worrying about scope.
+    $ifPtr = self::getIntOrNull($phpcsFile->findPrevious([T_IF], $stackPtr - 1));
+    if (! is_int($ifPtr)) {
+      throw new \Exception("Cannot find if for else at {$stackPtr}");
+    }
+    $tokens = $phpcsFile->getTokens();
+      $token = $tokens[$ifPtr];
+    self::debug("found if token", $token);
+    return [
+      $token['scope_opener'],
+      $token['scope_closer'],
+    ];
+  }
 }
diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
index 4c2081c3..19d88f1c 100644
--- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
+++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
@@ -1390,11 +1390,48 @@ protected function processVariable(File $phpcsFile, $stackPtr) {
       return;
     }
 
+    if (Helpers::isVariableInsideElseCondition($phpcsFile, $stackPtr) || Helpers::isVariableInsideElseBody($phpcsFile, $stackPtr)) {
+      Helpers::debug('found variable inside else condition or body');
+      $this->processVaribleInsideElse($phpcsFile, $stackPtr, $varName, $currScope);
+      return;
+    }
+
     // OK, we don't appear to be a write to the var, assume we're a read.
     Helpers::debug('looks like a variable read');
     $this->markVariableReadAndWarnIfUndefined($phpcsFile, $varName, $stackPtr, $currScope);
   }
 
+  /**
+   * @param File $phpcsFile
+   * @param int $stackPtr
+   * @param string $varName
+   * @param int $currScope
+   *
+   * @return void
+   */
+  protected function processVaribleInsideElse(File $phpcsFile, $stackPtr, $varName, $currScope) {
+    // Find all assignments to this variable inside the current scope.
+    $varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
+    $allAssignmentIndices = array_unique($varInfo->allAssignments);
+    // Find the attached if-block start and end.
+    list($ifStart, $ifEnd) = Helpers::getAttachedIfBlockStartEndForElse($phpcsFile, $stackPtr);
+    // If all of the assignments are within the attached if-block, then warn about undefined.
+    foreach ($allAssignmentIndices as $index) {
+      if ($index < $ifStart || $index > $ifEnd) {
+        Helpers::debug('looks like a variable read');
+        $this->markVariableReadAndWarnIfUndefined($phpcsFile, $varName, $stackPtr, $currScope);
+        return;
+      }
+    }
+    Helpers::debug("variable $varName inside else looks undefined");
+    $phpcsFile->addWarning(
+      "Variable %s is undefined.",
+      $stackPtr,
+      'UndefinedVariable',
+      ["\${$varName}"]
+    );
+  }
+
   /**
    * Called to process variables found in double quoted strings.
    *

From 979635f8ed27aea16e7f00d071c36cc1e70e6bc1 Mon Sep 17 00:00:00 2001
From: Payton Swick <payton@foolord.com>
Date: Thu, 9 Jul 2020 20:36:14 -0400
Subject: [PATCH 3/6] Add test for if block inside the else

---
 Tests/VariableAnalysisSniff/IfConditionTest.php    |  2 ++
 .../fixtures/FunctionWithIfConditionFixture.php    | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/Tests/VariableAnalysisSniff/IfConditionTest.php b/Tests/VariableAnalysisSniff/IfConditionTest.php
index 54b5491b..31f525fd 100644
--- a/Tests/VariableAnalysisSniff/IfConditionTest.php
+++ b/Tests/VariableAnalysisSniff/IfConditionTest.php
@@ -24,6 +24,8 @@ public function testIfConditionWarnings() {
       62,
       70,
       74,
+      82,
+      87,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
diff --git a/Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php b/Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php
index ce5207ee..7723f16d 100644
--- a/Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php
+++ b/Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php
@@ -76,6 +76,20 @@ function definedInsideFirstBlockUndefinedInsideElseBlock($first) {
   echo $words;
 }
 
+function definedInsideFirstBlockUndefinedInsideElseBlockInsideAnotherIf($first) {
+  $name = 'human';
+  if ($first) {
+    $second = true; // unused variable $second
+    $words = "hello {$name}";
+  } else {
+    if ($name) {
+      $words = "bye {$name}";
+      echo $second; // undefined variable $second
+    }
+  }
+  echo $words;
+}
+
 function definedInsideFirstBlockUndefinedInsideUnconnectedElseCondition($first) {
   $name = 'human';
   if ($first) {

From b47f79e49b7785a6c7d18b1d54b192db01ad149d Mon Sep 17 00:00:00 2001
From: Payton Swick <payton@foolord.com>
Date: Thu, 9 Jul 2020 20:38:55 -0400
Subject: [PATCH 4/6] Add test for definition within previous elseif

---
 Tests/VariableAnalysisSniff/IfConditionTest.php     |  2 ++
 .../fixtures/FunctionWithIfConditionFixture.php     | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/Tests/VariableAnalysisSniff/IfConditionTest.php b/Tests/VariableAnalysisSniff/IfConditionTest.php
index 31f525fd..729bc2a9 100644
--- a/Tests/VariableAnalysisSniff/IfConditionTest.php
+++ b/Tests/VariableAnalysisSniff/IfConditionTest.php
@@ -26,6 +26,8 @@ public function testIfConditionWarnings() {
       74,
       82,
       87,
+      98,
+      101,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
diff --git a/Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php b/Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php
index 7723f16d..0e90f494 100644
--- a/Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php
+++ b/Tests/VariableAnalysisSniff/fixtures/FunctionWithIfConditionFixture.php
@@ -90,6 +90,19 @@ function definedInsideFirstBlockUndefinedInsideElseBlockInsideAnotherIf($first)
   echo $words;
 }
 
+function definedInsideElseIfBlockUndefinedInsideElseBlock($first) {
+  $name = 'human';
+  if ($first) {
+    $words = "hello {$name}";
+  } elseif ($name) {
+    $second = true; // unused variable $second
+  } else {
+    $words = "bye {$name}";
+    echo $second; // undefined variable $second
+  }
+  echo $words;
+}
+
 function definedInsideFirstBlockUndefinedInsideUnconnectedElseCondition($first) {
   $name = 'human';
   if ($first) {

From 7a0df4630dcc73f95fbefac65417ae354d6a2703 Mon Sep 17 00:00:00 2001
From: Payton Swick <payton@foolord.com>
Date: Thu, 9 Jul 2020 21:06:18 -0400
Subject: [PATCH 5/6] Check all previous if/elseif scopes rather than just the
 if

---
 VariableAnalysis/Lib/Helpers.php              | 43 +++++++++++++------
 .../CodeAnalysis/VariableAnalysisSniff.php    | 19 +++++---
 2 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php
index 2db21579..b45c1545 100644
--- a/VariableAnalysis/Lib/Helpers.php
+++ b/VariableAnalysis/Lib/Helpers.php
@@ -613,21 +613,40 @@ public static function isVariableInsideElseBody(File $phpcsFile, $stackPtr) {
    * @param File $phpcsFile
    * @param int $stackPtr
    *
-   * @return [int, int]
+   * @return int[]
    */
-  public static function getAttachedIfBlockStartEndForElse(File $phpcsFile, $stackPtr) {
-    // If this is indeed an `else` or `elseif`, we can look backward until we
-    // find the nearest `if` without worrying about scope.
-    $ifPtr = self::getIntOrNull($phpcsFile->findPrevious([T_IF], $stackPtr - 1));
+  public static function getAttachedBlockIndicesForElse(File $phpcsFile, $stackPtr) {
+    $currentElsePtr = $phpcsFile->findPrevious([T_ELSE, T_ELSEIF], $stackPtr - 1);
+    if (! is_int($currentElsePtr)) {
+      throw new \Exception("Cannot find expected else at {$stackPtr}");
+    }
+
+    $ifPtr = $phpcsFile->findPrevious([T_IF], $currentElsePtr - 1);
     if (! is_int($ifPtr)) {
       throw new \Exception("Cannot find if for else at {$stackPtr}");
     }
-    $tokens = $phpcsFile->getTokens();
-      $token = $tokens[$ifPtr];
-    self::debug("found if token", $token);
-    return [
-      $token['scope_opener'],
-      $token['scope_closer'],
-    ];
+    $blockIndices = [$ifPtr];
+
+    $previousElseIfPtr = $currentElsePtr;
+    do {
+      $elseIfPtr = $phpcsFile->findPrevious([T_ELSEIF], $previousElseIfPtr - 1, $ifPtr);
+      if (is_int($elseIfPtr)) {
+        $blockIndices[] = $elseIfPtr;
+        $previousElseIfPtr = $elseIfPtr;
+      }
+    } while (is_int($elseIfPtr));
+
+    return $blockIndices;
+  }
+
+  /**
+   * @param int $needle
+   * @param int $scopeStart
+   * @param int $scopeEnd
+   *
+   * @return bool
+   */
+  public static function isIndexInsideScope($needle, $scopeStart, $scopeEnd) {
+    return ($needle > $scopeStart && $needle < $scopeEnd);
   }
 }
diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
index 19d88f1c..d7ea88cd 100644
--- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
+++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
@@ -1413,14 +1413,19 @@ protected function processVaribleInsideElse(File $phpcsFile, $stackPtr, $varName
     // Find all assignments to this variable inside the current scope.
     $varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
     $allAssignmentIndices = array_unique($varInfo->allAssignments);
-    // Find the attached if-block start and end.
-    list($ifStart, $ifEnd) = Helpers::getAttachedIfBlockStartEndForElse($phpcsFile, $stackPtr);
-    // If all of the assignments are within the attached if-block, then warn about undefined.
+    // Find the attached 'if' and 'elseif' block start and end indices.
+    $blockIndices = Helpers::getAttachedBlockIndicesForElse($phpcsFile, $stackPtr);
+    Helpers::debug('else block attached indices are', $blockIndices);
+    // If all of the assignments are within the previous attached blocks, then warn about undefined.
+    $tokens = $phpcsFile->getTokens();
     foreach ($allAssignmentIndices as $index) {
-      if ($index < $ifStart || $index > $ifEnd) {
-        Helpers::debug('looks like a variable read');
-        $this->markVariableReadAndWarnIfUndefined($phpcsFile, $varName, $stackPtr, $currScope);
-        return;
+      foreach ($blockIndices as $blockIndex) {
+        Helpers::debug('looking at scope', $index, $tokens[$blockIndex]['scope_opener'], $tokens[$blockIndex]['scope_closer']);
+        if (! Helpers::isIndexInsideScope($index, $tokens[$blockIndex]['scope_opener'], $tokens[$blockIndex]['scope_closer'])) {
+          Helpers::debug('looks like a variable read inside else');
+          $this->markVariableReadAndWarnIfUndefined($phpcsFile, $varName, $stackPtr, $currScope);
+          return;
+        }
       }
     }
     Helpers::debug("variable $varName inside else looks undefined");

From c257e19105c5acc685fd11758ffa5e2d2a6214f5 Mon Sep 17 00:00:00 2001
From: Payton Swick <payton@foolord.com>
Date: Thu, 9 Jul 2020 21:14:04 -0400
Subject: [PATCH 6/6] Make sure all assignments are in attached blocks

---
 .../CodeAnalysis/VariableAnalysisSniff.php    | 32 +++++++++++--------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
index d7ea88cd..2d75e921 100644
--- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
+++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
@@ -1415,26 +1415,32 @@ protected function processVaribleInsideElse(File $phpcsFile, $stackPtr, $varName
     $allAssignmentIndices = array_unique($varInfo->allAssignments);
     // Find the attached 'if' and 'elseif' block start and end indices.
     $blockIndices = Helpers::getAttachedBlockIndicesForElse($phpcsFile, $stackPtr);
-    Helpers::debug('else block attached indices are', $blockIndices);
+
     // If all of the assignments are within the previous attached blocks, then warn about undefined.
     $tokens = $phpcsFile->getTokens();
+    $assignmentsInsideAttachedBlocks = [];
     foreach ($allAssignmentIndices as $index) {
       foreach ($blockIndices as $blockIndex) {
-        Helpers::debug('looking at scope', $index, $tokens[$blockIndex]['scope_opener'], $tokens[$blockIndex]['scope_closer']);
-        if (! Helpers::isIndexInsideScope($index, $tokens[$blockIndex]['scope_opener'], $tokens[$blockIndex]['scope_closer'])) {
-          Helpers::debug('looks like a variable read inside else');
-          $this->markVariableReadAndWarnIfUndefined($phpcsFile, $varName, $stackPtr, $currScope);
-          return;
+        Helpers::debug('looking at scope', $index, 'between', $tokens[$blockIndex]['scope_opener'], 'and', $tokens[$blockIndex]['scope_closer']);
+        if (Helpers::isIndexInsideScope($index, $tokens[$blockIndex]['scope_opener'], $tokens[$blockIndex]['scope_closer'])) {
+          $assignmentsInsideAttachedBlocks[] = $index;
         }
       }
     }
-    Helpers::debug("variable $varName inside else looks undefined");
-    $phpcsFile->addWarning(
-      "Variable %s is undefined.",
-      $stackPtr,
-      'UndefinedVariable',
-      ["\${$varName}"]
-    );
+
+    if (count($assignmentsInsideAttachedBlocks) === count($allAssignmentIndices)) {
+      Helpers::debug("variable $varName inside else looks undefined");
+      $phpcsFile->addWarning(
+        "Variable %s is undefined.",
+        $stackPtr,
+        'UndefinedVariable',
+        ["\${$varName}"]
+      );
+      return;
+    }
+
+    Helpers::debug('looks like a variable read inside else');
+    $this->markVariableReadAndWarnIfUndefined($phpcsFile, $varName, $stackPtr, $currScope);
   }
 
   /**