Fix unused-before-used detection (#171)

* Add test for unused params after used with global

* Make sure areFollowingArgumentsUsed only examines params

* Remove unused function arguments

Author: sirbrillig

Tests/VariableAnalysisSniff/VariableAnalysisTest.php

@@ -97,6 +97,7 @@ public function testFunctionWithGlobalVarWarnings() {
       28,
       29,
       39,
+      54,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
@@ -762,6 +763,7 @@ public function testValidUndefinedVariableNamesIgnoresVarsInGlobalScope() {
       7,
       23,
       39,
+			54,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
@@ -802,6 +804,8 @@ public function testUnusedArgumentsBeforeUsedArgumentsAreFoundIfFalse() {
       8,
       16,
       19,
+      28,
+      34,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
@@ -814,6 +818,8 @@ public function testUnusedArgumentsBeforeUsedArgumentsAreIgnoredByDefault() {
     $expectedWarnings = [
       8,
       19,
+      28,
+      34,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }

Tests/VariableAnalysisSniff/fixtures/FunctionWithGlobalVarFixture.php

@@ -50,3 +50,8 @@ function updateGlobalWithListShorthand($newVal) {
   [ $myGlobal, $otherVar ] = my_function($newVal);
   echo $otherVar;
 }
+
+function globalWithUnusedFunctionArg($user_type, $text, $testvar) { // should warn that testvar is unused
+	global $config;
+	return $config . $text . $user_type;
+}

Tests/VariableAnalysisSniff/fixtures/UnusedAfterUsedFixture.php

@@ -23,3 +23,17 @@ function inner_function() {
     };
     $foo();
 }
+
+// The following line should report an unused variable (unused after used)
+function function_with_one_unused_param($used, $used_two, $unused_three) {
+    echo $used;
+    echo $used_two;
+}
+
+// The following line should report an unused variable (unused after used)
+function function_with_local_and_unused_params($used, $used_two, $unused_three) {
+    $foobar = 'hello';
+    echo $used;
+    echo $foobar;
+    echo $used_two;
+}

VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php

@@ -283,6 +283,9 @@ protected function areFollowingArgumentsUsed($varInfo, $scopeInfo) {
       if (! $foundVarPosition) {
         continue;
       }
+      if ($variable->scopeType !== 'param') {
+        continue;
+      }
       if ($variable->firstRead) {
         return true;
       }
@@ -522,12 +525,10 @@ protected function checkForFunctionPrototype(File $phpcsFile, $stackPtr, $varNam
   /**
    * @param File $phpcsFile
    * @param int $stackPtr
-   * @param string $varName
-   * @param int $currScope
    *
    * @return bool
    */
-  protected function checkForClassProperty(File $phpcsFile, $stackPtr, $varName, $currScope) {
+  protected function checkForClassProperty(File $phpcsFile, $stackPtr) {
     $propertyDeclarationKeywords = [
       T_PUBLIC,
       T_PRIVATE,
@@ -660,12 +661,10 @@ protected function checkForSuperGlobal(File $phpcsFile, $stackPtr, $varName) {
   /**
    * @param File $phpcsFile
    * @param int $stackPtr
-   * @param string $varName
-   * @param int $currScope
    *
    * @return bool
    */
-  protected function checkForStaticMember(File $phpcsFile, $stackPtr, $varName, $currScope) {
+  protected function checkForStaticMember(File $phpcsFile, $stackPtr) {
     $tokens = $phpcsFile->getTokens();
 
     $doubleColonPtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $stackPtr - 1, null, true);
@@ -696,11 +695,10 @@ protected function checkForStaticMember(File $phpcsFile, $stackPtr, $varName, $c
    * @param File $phpcsFile
    * @param int $stackPtr
    * @param string $varName
-   * @param int $currScope
    *
    * @return bool
    */
-  protected function checkForStaticOutsideClass(File $phpcsFile, $stackPtr, $varName, $currScope) {
+  protected function checkForStaticOutsideClass(File $phpcsFile, $stackPtr, $varName) {
     // Are we refering to self:: outside a class?
 
     $tokens = $phpcsFile->getTokens();
@@ -752,7 +750,7 @@ protected function checkForAssignment(File $phpcsFile, $stackPtr, $varName, $cur
     }
 
     // Is this a variable variable? If so, it's not an assignment to the current variable.
-    if ($this->checkForVariableVariable($phpcsFile, $stackPtr, $varName, $currScope)) {
+    if ($this->checkForVariableVariable($phpcsFile, $stackPtr)) {
       Helpers::debug('found variable variable');
       return false;
     }
@@ -779,12 +777,10 @@ protected function checkForAssignment(File $phpcsFile, $stackPtr, $varName, $cur
   /**
    * @param File $phpcsFile
    * @param int $stackPtr
-   * @param string $varName
-   * @param int $currScope
    *
    * @return bool
    */
-  protected function checkForVariableVariable(File $phpcsFile, $stackPtr, $varName, $currScope) {
+  protected function checkForVariableVariable(File $phpcsFile, $stackPtr) {
     $tokens = $phpcsFile->getTokens();
 
     $prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($stackPtr - 1), null, true);
@@ -1184,18 +1180,18 @@ protected function processVariable(File $phpcsFile, $stackPtr) {
     }
 
     // Check for static members used outside a class
-    if ($this->checkForStaticOutsideClass($phpcsFile, $stackPtr, $varName, $currScope)) {
+    if ($this->checkForStaticOutsideClass($phpcsFile, $stackPtr, $varName)) {
       Helpers::debug('found static usage outside of class');
       return;
     }
 
     // $var part of class::$var static member
-    if ($this->checkForStaticMember($phpcsFile, $stackPtr, $varName, $currScope)) {
+    if ($this->checkForStaticMember($phpcsFile, $stackPtr)) {
       Helpers::debug('found static member');
       return;
     }
 
-    if ($this->checkForClassProperty($phpcsFile, $stackPtr, $varName, $currScope)) {
+    if ($this->checkForClassProperty($phpcsFile, $stackPtr)) {
       Helpers::debug('found class property definition');
       return;
     }
@@ -1403,6 +1399,7 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v
       return;
     }
     if ($this->allowUnusedParametersBeforeUsed && $varInfo->scopeType === 'param' && $this->areFollowingArgumentsUsed($varInfo, $scopeInfo)) {
+      Helpers::debug("variable {$varInfo->name} at end of scope has unused following args");
       return;
     }
     if ($this->allowUnusedForeachVariables && $varInfo->isForeachLoopAssociativeValue) {