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

Author: sirbrillig

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;

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;

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,

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);
   }

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
+}