Support global scope (#190)

* Fix indentation in VariableAnalysisTest * Add global scope tests * Add global scope to processScopeClose * Fix AnonymousClassWithPropertiesFixture test * Add getScopeCloseForScopeOpen helper * Remove findWhereAssignExecuted * Fix ThisWithinNestedClosedScopeFixture test * Enable test for foreach in global scope * Disable allowUnusedForeachVariables in FunctionWithForeachFixture test * Allow a token to close more than one scope * Fix FunctionWithGlobalVarFixture test
sirbrillig · Jul 11, 2020 · 5620b43 · 5620b43
1 parent 275cb81
commit 5620b43
Show file tree

Hide file tree

Showing 7 changed files with 104 additions and 73 deletions.
diff --git a/Tests/VariableAnalysisSniff/GlobalScopeTest.php b/Tests/VariableAnalysisSniff/GlobalScopeTest.php
@@ -0,0 +1,18 @@
+<?php
+namespace VariableAnalysis\Tests\VariableAnalysisSniff;
+
+use VariableAnalysis\Tests\BaseTestCase;
+
+class GlobalScopeTest extends BaseTestCase {
+  public function testGlobalScopeWarnings() {
+    $fixtureFile = $this->getFixture('GlobalScopeFixture.php');
+    $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile);
+    $phpcsFile->process();
+    $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
+    $expectedErrors = [
+      4,
+      7,
+    ];
+    $this->assertEquals($expectedErrors, $lines);
+  }
+}
diff --git a/Tests/VariableAnalysisSniff/VariableAnalysisTest.php b/Tests/VariableAnalysisSniff/VariableAnalysisTest.php
@@ -114,6 +114,11 @@ public function testFunctionWithForeachErrors() {
   public function testFunctionWithForeachWarnings() {
     $fixtureFile = $this->getFixture('FunctionWithForeachFixture.php');
     $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile);
+    $phpcsFile->ruleset->setSniffProperty(
+      'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff',
+      'allowUnusedForeachVariables',
+      'false'
+    );
     $phpcsFile->process();
     $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
     $expectedWarnings = [
@@ -129,10 +134,7 @@ public function testFunctionWithForeachWarnings() {
       50,
       52,
       54,
-      // FIXME: this is an unused variable that needs to be fixed but for now
-      // we will ignore it. See
-      // https://github.com/sirbrillig/phpcs-variable-analysis/pull/36
-      // 67,
+      67,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
@@ -585,6 +587,7 @@ public function testAnonymousClassAllowPropertyDefinitions() {
     $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
     $expectedWarnings = [
       17,
+      26,
       38,
     ];
     $this->assertEquals($expectedWarnings, $lines);
@@ -767,7 +770,7 @@ public function testValidUndefinedVariableNamesIgnoresVarsInGlobalScope() {
       7,
       23,
       39,
-			54,
+      54,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
@@ -939,10 +942,10 @@ public function testGetDefinedVarsCountsAsRead() {
     $phpcsFile->process();
     $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
     $expectedWarnings = [
-			6,
-			18,
-			22,
-			29,
+      6,
+      18,
+      22,
+      29,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
@@ -953,12 +956,14 @@ public function testThisWithinNestedClosedScope() {
     $phpcsFile->process();
     $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
     $expectedWarnings = [
-            5,
-            8,
-            20,
-            33,
-            47,
-            61,
+      5,
+      8,
+      15,
+      20,
+      33,
+      41,
+      47,
+      61,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
@@ -969,12 +974,12 @@ public function testUnusedVarWithValueChange() {
     $phpcsFile->process();
     $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
     $expectedWarnings = [
-            5,
-            6,
-            8,
-            9,
-            11,
-            12,
+      5,
+      6,
+      8,
+      9,
+      11,
+      12,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }

diff --git a/Tests/VariableAnalysisSniff/fixtures/AnonymousClassWithPropertiesFixture.php b/Tests/VariableAnalysisSniff/fixtures/AnonymousClassWithPropertiesFixture.php
@@ -23,7 +23,7 @@ public function methodWithStaticVar() {
     }
 }
 
-$anonClass = new class() {
+$anonClass = new class() { // should trigger unused warning
     protected $storedHello;
     private static $storedHello2;
     private $storedHello3;

diff --git a/Tests/VariableAnalysisSniff/fixtures/FunctionWithGlobalVarFixture.php b/Tests/VariableAnalysisSniff/fixtures/FunctionWithGlobalVarFixture.php
@@ -55,3 +55,5 @@ function globalWithUnusedFunctionArg($user_type, $text, $testvar) { // should wa
 	global $config;
 	return $config . $text . $user_type;
 }
+
+echo $sunday;
diff --git a/Tests/VariableAnalysisSniff/fixtures/GlobalScopeFixture.php b/Tests/VariableAnalysisSniff/fixtures/GlobalScopeFixture.php
@@ -0,0 +1,7 @@
+<?php
+
+$name = 'friend';
+$place = 'faerie'; // unused variable $place
+
+echo $name;
+echo $activity; // undefined variable $activity
diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php
@@ -237,41 +237,6 @@ public static function findFunctionCallArguments(File $phpcsFile, $stackPtr) {
     return $argPtrs;
   }
 
-  /**
-   * @param File $phpcsFile
-   * @param int $stackPtr
-   *
-   * @return int
-   */
-  public static function findWhereAssignExecuted(File $phpcsFile, $stackPtr) {
-    $tokens = $phpcsFile->getTokens();
-
-    //  Write should be recorded at the next statement to ensure we treat the
-    //  assign as happening after the RHS execution.
-    //  eg: $var = $var + 1; -> RHS could still be undef.
-    //  However, if we're within a bracketed expression, we take place at the
-    //  closing bracket, if that's first.
-    //  eg: echo (($var = 12) && ($var == 12));
-    $semicolonPtr = $phpcsFile->findNext([T_SEMICOLON], $stackPtr + 1, null, false, null, true);
-    $commaPtr = $phpcsFile->findNext([T_COMMA], $stackPtr + 1, null, false, null, true);
-    $closePtr = false;
-    $openPtr = Helpers::findContainingOpeningBracket($phpcsFile, $stackPtr);
-    if ($openPtr !== null) {
-      if (isset($tokens[$openPtr]['parenthesis_closer'])) {
-        $closePtr = $tokens[$openPtr]['parenthesis_closer'];
-      }
-    }
-
-    // Return the first thing: comma, semicolon, close-bracket, or stackPtr if nothing else
-    $assignEndTokens = [$commaPtr, $semicolonPtr, $closePtr];
-    $assignEndTokens = array_filter($assignEndTokens); // remove false values
-    sort($assignEndTokens);
-    if (empty($assignEndTokens)) {
-      return $stackPtr;
-    }
-    return $assignEndTokens[0];
-  }
-
   /**
    * @param File $phpcsFile
    * @param int $stackPtr
@@ -649,4 +614,41 @@ public static function getAttachedBlockIndicesForElse(File $phpcsFile, $stackPtr
   public static function isIndexInsideScope($needle, $scopeStart, $scopeEnd) {
     return ($needle > $scopeStart && $needle < $scopeEnd);
   }
+
+  /**
+   * @param File $phpcsFile
+   * @param int $scopeStartIndex
+   *
+   * @return int
+   */
+  public static function getScopeCloseForScopeOpen(File $phpcsFile, $scopeStartIndex) {
+    $tokens = $phpcsFile->getTokens();
+    $scopeCloserIndex = isset($tokens[$scopeStartIndex]['scope_closer']) ? $tokens[$scopeStartIndex]['scope_closer'] : null;
+
+    if (FunctionDeclarations::isArrowFunction($phpcsFile, $scopeStartIndex)) {
+      $arrowFunctionInfo = FunctionDeclarations::getArrowFunctionOpenClose($phpcsFile, $scopeStartIndex);
+      $scopeCloserIndex = $arrowFunctionInfo ? $arrowFunctionInfo['scope_closer'] : $scopeCloserIndex;
+    }
+
+    if ($scopeStartIndex === 0) {
+      $scopeCloserIndex = Helpers::getLastNonEmptyTokenIndexInFile($phpcsFile);
+    }
+    return $scopeCloserIndex;
+  }
+
+  /**
+   * @param File $phpcsFile
+   *
+   * @return int
+   */
+  public static function getLastNonEmptyTokenIndexInFile(File $phpcsFile) {
+    $tokens = $phpcsFile->getTokens();
+    foreach (array_reverse($tokens, true) as $index => $token) {
+      if (! in_array($token['code'], Tokens::$emptyTokens, true)) {
+        return $index;
+      }
+    }
+    self::debug('no non-empty token found for end of file');
+    return 0;
+  }
 }
diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
@@ -33,7 +33,7 @@ class VariableAnalysisSniff implements Sniff {
    *
    * @var int[]
    */
-  private $scopeStartIndices = [];
+  private $scopeStartIndices = [0];
 
   /**
    * A list of custom functions which pass in variables to be initialized by
@@ -180,21 +180,20 @@ public function process(File $phpcsFile, $stackPtr) {
       T_CLOSURE,
     ];
 
-    $scopeIndexThisCloses = array_reduce($this->scopeStartIndices, function ($found, $index) use ($phpcsFile, $stackPtr, $tokens) {
-      $scopeCloserIndex = isset($tokens[$index]['scope_closer']) ? $tokens[$index]['scope_closer'] : null;
-      if (FunctionDeclarations::isArrowFunction($phpcsFile, $index)) {
-        $arrowFunctionInfo = FunctionDeclarations::getArrowFunctionOpenClose($phpcsFile, $index);
-        $scopeCloserIndex = $arrowFunctionInfo ? $arrowFunctionInfo['scope_closer'] : $scopeCloserIndex;
-      }
+    $scopeIndicesThisCloses = array_reduce($this->scopeStartIndices, function ($found, $scopeStartIndex) use ($phpcsFile, $stackPtr) {
+      $scopeCloserIndex = Helpers::getScopeCloseForScopeOpen($phpcsFile, $scopeStartIndex);
+
       if (!$scopeCloserIndex) {
-        Helpers::debug('No scope closer found for scope start', $index);
+        Helpers::debug('No scope closer found for scope start', $scopeStartIndex);
       }
+
       if ($stackPtr === $scopeCloserIndex) {
-        return $index;
+        $found[] = $scopeStartIndex;
       }
       return $found;
-    }, null);
-    if ($scopeIndexThisCloses) {
+    }, []);
+
+    foreach ($scopeIndicesThisCloses as $scopeIndexThisCloses) {
       Helpers::debug('found closing scope at', $stackPtr, 'for scope', $scopeIndexThisCloses);
       $this->processScopeClose($phpcsFile, $scopeIndexThisCloses);
     }
@@ -856,8 +855,6 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN
       return false;
     }
 
-    $writtenPtr = Helpers::findWhereAssignExecuted($phpcsFile, $assignPtr);
-
     // 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,
@@ -877,14 +874,14 @@ protected function processVariableAsAssignment(File $phpcsFile, $stackPtr, $varN
       // 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);
+      $this->markVariableDeclaration($varName, ScopeType::LOCAL, null, $stackPtr, $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);
+      $this->markVariableAssignmentWithoutInitialization($varName, $stackPtr, $currScope);
       return true;
     }
 
-    $this->markVariableAssignment($varName, $writtenPtr, $currScope);
+    $this->markVariableAssignment($varName, $stackPtr, $currScope);
 
     return true;
   }