From a31046c32e95cff4062856a3eb78770dd1bc1e48 Mon Sep 17 00:00:00 2001
From: Payton Swick <payton@foolord.com>
Date: Mon, 22 Apr 2019 17:18:55 -0400
Subject: [PATCH] Allow global var assignment to count as a read (#83)

* Tests: improve tests for global keyword

* Add some additional debugging for undefined vars

* Add docs for some VariableInfo properties

* Inline getStackPtrIfVariableIsUnused

* Allow global var assignment to count as a read

* Tests: add fixture for single unused global
---
 VariableAnalysis/Lib/Helpers.php              | 10 ---------
 VariableAnalysis/Lib/VariableInfo.php         |  6 +++---
 .../CodeAnalysis/VariableAnalysisSniff.php    | 12 +++++++++--
 .../CodeAnalysis/VariableAnalysisTest.php     |  4 +++-
 .../fixtures/FunctionWithGlobalVarFixture.php | 21 +++++++++++++------
 5 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php
index 45c0873b..a1b12b0a 100644
--- a/VariableAnalysis/Lib/Helpers.php
+++ b/VariableAnalysis/Lib/Helpers.php
@@ -228,16 +228,6 @@ public static function findVariableScope(File $phpcsFile, $stackPtr) {
     return 0;
   }
 
-  public static function getStackPtrIfVariableIsUnused(VariableInfo $varInfo) {
-    if (isset($varInfo->firstDeclared)) {
-      return $varInfo->firstDeclared;
-    }
-    if (isset($varInfo->firstInitialized)) {
-      return $varInfo->firstInitialized;
-    }
-    return null;
-  }
-
   public static function debug($message) {
     if (! defined('PHP_CODESNIFFER_VERBOSITY')) {
       return;
diff --git a/VariableAnalysis/Lib/VariableInfo.php b/VariableAnalysis/Lib/VariableInfo.php
index 4fe914cc..95f295b7 100644
--- a/VariableAnalysis/Lib/VariableInfo.php
+++ b/VariableAnalysis/Lib/VariableInfo.php
@@ -13,9 +13,9 @@ class VariableInfo {
   public $scopeType;
   public $typeHint;
   public $passByReference = false;
-  public $firstDeclared;
-  public $firstInitialized;
-  public $firstRead;
+  public $firstDeclared; // stack pointer of first declaration
+  public $firstInitialized; // stack pointer of first initialization
+  public $firstRead; // stack pointer of first read
   public $ignoreUnused = false;
   public $ignoreUndefined = false;
   public $isForeachLoopVar = false;
diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
index 16a5da63..54f1dc88 100644
--- a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
+++ b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
@@ -274,6 +274,7 @@ protected function isVariableUndefined($varName, $stackPtr, $currScope) {
   protected function markVariableReadAndWarnIfUndefined($phpcsFile, $varName, $stackPtr, $currScope) {
     $this->markVariableRead($varName, $stackPtr, $currScope);
     if ($this->isVariableUndefined($varName, $stackPtr, $currScope) === true) {
+      Helpers::debug("variable $varName looks undefined");
       $phpcsFile->addWarning(
         "Variable %s is undefined.",
         $stackPtr,
@@ -319,6 +320,7 @@ protected function checkForFunctionPrototype(File $phpcsFile, $stackPtr, $varNam
       $this->markVariableRead($varName, $stackPtr, $currScope);
       if ($this->isVariableUndefined($varName, $stackPtr, $currScope) === true) {
         // We haven't been defined by this point.
+        Helpers::debug("variable $varName in function prototype looks undefined");
         $phpcsFile->addWarning("Variable %s is undefined.", $stackPtr, 'UndefinedVariable', ["\${$varName}"]);
         return true;
       }
@@ -577,7 +579,6 @@ protected function checkForListAssignment(File $phpcsFile, $stackPtr, $varName,
 
   protected function checkForGlobalDeclaration(File $phpcsFile, $stackPtr, $varName, $currScope) {
     $tokens = $phpcsFile->getTokens();
-    $token  = $tokens[$stackPtr];
 
     // Are we a global declaration?
     // Search backwards for first token that isn't whitespace, comma or variable.
@@ -1052,8 +1053,15 @@ protected function processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo
       // of "unused variable" warnings.
       return;
     }
-    $stackPtr = Helpers::getStackPtrIfVariableIsUnused($varInfo);
+    if ($varInfo->scopeType === 'global' && isset($varInfo->firstInitialized)) {
+      // If we imported this variable from the global scope, any further use of
+      // the variable, including assignment, should count as "variable use" for
+      // the purposes of "unused variable" warnings.
+      return;
+    }
+    $stackPtr = $varInfo->firstDeclared ?? $varInfo->firstInitialized ?? null;
     if ($stackPtr) {
+      Helpers::debug("variable {$varInfo->name} at end of scope looks undefined");
       $phpcsFile->addWarning(
         "Unused %s %s.",
         $stackPtr,
diff --git a/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php b/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php
index 53a44ee3..e426e7d1 100644
--- a/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php
+++ b/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php
@@ -95,7 +95,8 @@ public function testFunctionWithGlobalVarWarnings() {
       8,
       23,
       28,
-      29
+      29,
+      39,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
@@ -734,6 +735,7 @@ public function testValidUndefinedVariableNamesIgnoresVarsInGlobalScope() {
       4,
       7,
       23,
+      39,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
diff --git a/VariableAnalysis/Tests/CodeAnalysis/fixtures/FunctionWithGlobalVarFixture.php b/VariableAnalysis/Tests/CodeAnalysis/fixtures/FunctionWithGlobalVarFixture.php
index 14c00e55..5ac38c22 100644
--- a/VariableAnalysis/Tests/CodeAnalysis/fixtures/FunctionWithGlobalVarFixture.php
+++ b/VariableAnalysis/Tests/CodeAnalysis/fixtures/FunctionWithGlobalVarFixture.php
@@ -1,11 +1,11 @@
 <?php
 
 function function_with_global_var() {
-    global $var, $var2, $unused;
+    global $var, $var2, $unused; // should warn that `unused` is unused
 
     echo $var;
-    echo $var3;
-    echo $ice_cream;
+    echo $var3; // should warn that var is undefined
+    echo $ice_cream; // should warn that var is undefined
     return $var2;
 }
 
@@ -20,12 +20,21 @@ function function_with_superglobals() {
     echo print_r($_REQUEST, true);
     echo print_r($_ENV, true);
     echo "{$GLOBALS['whatever']}";
-    echo "{$GLOBALS['whatever']} $var";
+    echo "{$GLOBALS['whatever']} $var"; // should warn that var is undefined
 }
 
 // Variables within the global scope
 $cherry = 'topping';
-$sunday = $ice_cream . 'and a ' . $cherry;
-if ( $ice_cream ) {
+$sunday = $ice_cream . 'and a ' . $cherry; // should warn that ice_cream is undefined
+if ( $ice_cream ) { // should warn that var is undefined
     echo 'Two scoops please!';
 }
+
+function updateGlobal($newVal) {
+  global $myGlobal;
+  $myGlobal = $newVal;
+}
+
+function unusedGlobal() {
+  global $myGlobal; // should warn that var is unused
+}