Add allowUnusedParametersBeforeUsed option (#58)

sirbrillig · web-flow · commit 1b8ee46b3b42 · 2018-12-21T15:02:29.000-05:00
* Tests: add test for ignoreUnusedArgsBeforeUsed

* Add ignoreUnusedArgsBeforeUsed property

* Dev: Add ImportDetection to phpcs sniffs

* Clean up imports in VariableAnalysisTest

* Remove old TODOs

* Dev: Ignore LineLength.TooLong

* Fix typo in validUndefinedVariableNames declaration

* Rename property to allowUnusedParametersBeforeUsed
diff --git a/VariableAnalysis/Lib/Helpers.php b/VariableAnalysis/Lib/Helpers.php
@@ -3,6 +3,7 @@
 namespace VariableAnalysis\Lib;
 
 use PHP_CodeSniffer\Files\File;
+use VariableAnalysis\Lib\VariableInfo;
 
 class Helpers {
   public static function findContainingOpeningSquareBracket(File $phpcsFile, $stackPtr) {
@@ -86,7 +87,6 @@ public static function findFunctionCallArguments(File $phpcsFile, $stackPtr) {
     $tokens = $phpcsFile->getTokens();
 
     // Slight hack: also allow this to find args for array constructor.
-    // TODO: probably should refactor into three functions: arg-finding and bracket-finding
     if (($tokens[$stackPtr]['code'] !== T_STRING) && ($tokens[$stackPtr]['code'] !== T_ARRAY)) {
       // Assume $stackPtr is something within the brackets, find our function call
       $stackPtr = Helpers::findFunctionCall($phpcsFile, $stackPtr);
diff --git a/VariableAnalysis/Lib/ScopeInfo.php b/VariableAnalysis/Lib/ScopeInfo.php
@@ -13,6 +13,5 @@ class ScopeInfo {
 
   public function __construct($currScope) {
     $this->owner = $currScope;
-    // TODO: extract opener/closer
   }
 }
diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
@@ -59,7 +59,13 @@ class VariableAnalysisSniff implements Sniff {
    *  ignore from undefined variable warnings. For example, to ignore the variables
    *  `$post` and `$undefined`, this could be set to `'post undefined'`.
    */
-  public $validUdefinedVariableNames = null;
+  public $validUndefinedVariableNames = null;
+
+  /**
+   * Allows unused arguments in a function definition if they are
+   * followed by an argument which is used.
+   */
+  public $allowUnusedParametersBeforeUsed = false;
 
   public function register() {
     return [
@@ -153,6 +159,23 @@ protected function getOrCreateVariableInfo($varName, $currScope) {
     return $scopeInfo->variables[$varName];
   }
 
+  protected function areFollowingArgumentsUsed($varInfo, $scopeInfo) {
+    $foundVarPosition = false;
+    foreach ($scopeInfo->variables as $variable) {
+      if ($variable === $varInfo) {
+        $foundVarPosition = true;
+        continue;
+      }
+      if (! $foundVarPosition) {
+        continue;
+      }
+      if ($variable->firstRead) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   protected function markVariableAssignment($varName, $stackPtr, $currScope) {
     $varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
     if (!isset($varInfo->scopeType)) {
@@ -223,7 +246,6 @@ protected function isVariableUndefined($varName, $stackPtr, $currScope) {
       return false;
     }
     if (isset($varInfo->firstDeclared) && $varInfo->firstDeclared <= $stackPtr) {
-      // TODO: do we want to check scopeType here?
       return false;
     }
     if (isset($varInfo->firstInitialized) && $varInfo->firstInitialized <= $stackPtr) {
@@ -261,7 +283,6 @@ protected function checkForFunctionPrototype(File $phpcsFile, $stackPtr, $varNam
     if (($functionPtr !== false)
       && (($tokens[$functionPtr]['code'] === T_FUNCTION)
       || ($tokens[$functionPtr]['code'] === T_CLOSURE))) {
-      // TODO: typeHint
       $this->markVariableDeclaration($varName, 'param', null, $stackPtr, $functionPtr);
       // Are we pass-by-reference?
       $referencePtr = $phpcsFile->findPrevious(T_WHITESPACE, $stackPtr - 1, null, true, null, true);
@@ -287,7 +308,6 @@ protected function checkForFunctionPrototype(File $phpcsFile, $stackPtr, $varNam
       // $functionPtr is at the use, we need the function keyword for start of scope.
       $functionPtr = $phpcsFile->findPrevious(T_CLOSURE, $functionPtr - 1, $currScope + 1, false, null, true);
       if ($functionPtr !== false) {
-        // TODO: typeHints in use?
         $this->markVariableDeclaration($varName, 'bound', null, $stackPtr, $functionPtr);
         $this->markVariableAssignment($varName, $stackPtr, $functionPtr);
 
@@ -317,7 +337,6 @@ protected function checkForCatchBlock(File $phpcsFile, $stackPtr, $varName, $cur
     $catchPtr = $phpcsFile->findPrevious(T_WHITESPACE, $openPtr - 1, null, true, null, true);
     if (($catchPtr !== false) && ($tokens[$catchPtr]['code'] === T_CATCH)) {
       // Scope of the exception var is actually the function, not just the catch block.
-      // TODO: typeHint
       $this->markVariableDeclaration($varName, 'local', null, $stackPtr, $currScope, true);
       $this->markVariableAssignment($varName, $stackPtr, $currScope);
       if ($this->allowUnusedCaughtExceptions) {
@@ -405,7 +424,6 @@ protected function checkForStaticMember(File $phpcsFile, $stackPtr, $varName, $c
 
   protected function checkForStaticOutsideClass(File $phpcsFile, $stackPtr, $varName, $currScope) {
     // Are we refering to self:: outside a class?
-    // TODO: not sure this is our business or should be some other sniff.
 
     $tokens = $phpcsFile->getTokens();
     $token  = $tokens[$stackPtr];
@@ -925,17 +943,20 @@ protected function processScopeClose(File $phpcsFile, $stackPtr) {
       return;
     }
     foreach ($scopeInfo->variables as $varInfo) {
-      $this->processScopeCloseForVariable($phpcsFile, $varInfo);
+      $this->processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo);
     }
   }
 
-  protected function processScopeCloseForVariable($phpcsFile, $varInfo) {
+  protected function processScopeCloseForVariable($phpcsFile, $varInfo, $scopeInfo) {
     if ($varInfo->ignoreUnused || isset($varInfo->firstRead)) {
       return;
     }
     if ($this->allowUnusedFunctionParameters && $varInfo->scopeType === 'param') {
       return;
     }
+    if ($this->allowUnusedParametersBeforeUsed && $varInfo->scopeType === 'param' && $this->areFollowingArgumentsUsed($varInfo, $scopeInfo)) {
+      return;
+    }
     if ($varInfo->passByReference && isset($varInfo->firstInitialized)) {
       // If we're pass-by-reference then it's a common pattern to
       // use the variable to return data to the caller, so any
diff --git a/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php b/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php
@@ -1,9 +1,7 @@
 <?php
 namespace VariableAnalysis\Tests;
 
-use PHP_CodeSniffer\Files\LocalFile;
-use PHP_CodeSniffer\Ruleset;
-use PHP_CodeSniffer\Config;
+use VariableAnalysis\Tests\BaseTestCase;
 
 class VariableAnalysisTest extends BaseTestCase {
   public function testFunctionWithoutParamsErrors() {
@@ -599,4 +597,32 @@ public function testValidUndefinedVariableNamesIgnoresUndefinedProperties() {
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
+
+  public function testUnusedArgumentsBeforeUsedArgumentsAreFound() {
+    $fixtureFile = $this->getFixture('UnusedAfterUsedFixture.php');
+    $phpcsFile = $this->prepareLocalFileForSniffs($this->getSniffFiles(), $fixtureFile);
+    $phpcsFile->process();
+    $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
+    $expectedWarnings = [
+      5,
+      8,
+    ];
+    $this->assertEquals($expectedWarnings, $lines);
+  }
+
+  public function testUnusedArgumentsBeforeUsedArgumentsAreIgnoredIfSet() {
+    $fixtureFile = $this->getFixture('UnusedAfterUsedFixture.php');
+    $phpcsFile = $this->prepareLocalFileForSniffs($this->getSniffFiles(), $fixtureFile);
+    $phpcsFile->ruleset->setSniffProperty(
+      'VariableAnalysis\Sniffs\CodeAnalysis\VariableAnalysisSniff',
+      'allowUnusedParametersBeforeUsed',
+      'true'
+    );
+    $phpcsFile->process();
+    $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
+    $expectedWarnings = [
+      8,
+    ];
+    $this->assertEquals($expectedWarnings, $lines);
+  }
 }
diff --git a/VariableAnalysis/Tests/CodeAnalysis/fixtures/UnusedAfterUsedFixture.php b/VariableAnalysis/Tests/CodeAnalysis/fixtures/UnusedAfterUsedFixture.php
@@ -0,0 +1,12 @@
+<?php
+
+add_action( 'delete_post_meta', 'check_thumbnail_updated_post_meta', -1000, 3 );
+function check_thumbnail_updated_post_meta(
+    $meta_id,
+    $post_id,
+    $meta_key,
+    $foobar
+) {
+    echo $post_id;
+    echo $meta_key;
+}
diff --git a/composer.json b/composer.json
@@ -38,6 +38,7 @@
     "require-dev": {
         "dealerdirect/phpcodesniffer-composer-installer": "^0.4.4",
         "phpunit/phpunit": "^6.5",
+        "sirbrillig/phpcs-import-detection": "^1.1",
         "squizlabs/php_codesniffer": "^3.1",
         "limedeck/phpunit-detailed-printer": "^3.1"
     }
diff --git a/phpcs.xml.dist b/phpcs.xml.dist
@@ -10,6 +10,7 @@
         <exclude name="Squiz.Functions.MultiLineFunctionDeclaration.BraceOnSameLine" />
         <exclude name="PSR2.Classes.ClassDeclaration.OpenBraceNewLine" />
         <exclude name="Generic.WhiteSpace.DisallowTabIndent"/>
+        <exclude name="Generic.Files.LineLength.TooLong" />
     </rule>
     <arg name="tab-width" value="2"/>
     <rule ref="Generic.WhiteSpace.ScopeIndent">
@@ -29,4 +30,5 @@
     </rule>
     <rule ref="Generic.Functions.OpeningFunctionBraceKernighanRitchie" />
     <rule ref="Generic.Classes.OpeningBraceSameLine"/>
+    <rule ref="ImportDetection" />
 </ruleset>

Original file line number	Diff line number	Diff line change
`@@ -13,6 +13,5 @@ class ScopeInfo {`
`13`	`13`
`14`	`14`	`public function __construct($currScope) {`
`15`	`15`	`$this->owner = $currScope;`
`16`		`- // TODO: extract opener/closer`
`17`	`16`	`}`
`18`	`17`	`}`
Original file line number	Diff line number	Diff line change
`@@ -38,6 +38,7 @@`
`38`	`38`	`"require-dev": {`
`39`	`39`	`"dealerdirect/phpcodesniffer-composer-installer": "^0.4.4",`
`40`	`40`	`"phpunit/phpunit": "^6.5",`
	`41`	`+ "sirbrillig/phpcs-import-detection": "^1.1",`
`41`	`42`	`"squizlabs/php_codesniffer": "^3.1",`
`42`	`43`	`"limedeck/phpunit-detailed-printer": "^3.1"`
`43`	`44`	`}`