Only allow unused values in associative foreach loops with option (#167)

* Fix syntax of ThisWithinNestedClosedScopeFixture

Weird that I never noticed that before.

* Update tests to mark unused keys even with allowUnusedForeachVariables

* Update description of allowUnusedForeachVariables to be accurate

This makes it clear that it only operates on the values of a `key =>
value` expression.

* Adjust more tests which use default allowUnusedForeachVariables

* Only allow unused values in associative foreach loops with option

* Fix unused foreach keys in sniff and helpers

Author: sirbrillig

README.md

@@ -66,7 +66,7 @@ The available options are as follows:
 - `validUnusedVariableNames` (string, default `null`): a space-separated list of names of placeholder variables that you want to ignore from unused variable warnings. For example, to ignore the variables `$junk` and `$unused`, this could be set to `'junk unused'`.
 - `ignoreUnusedRegexp` (string, default `null`): a PHP regexp string (note that this requires explicit delimiters) for variables that you want to ignore from unused variable warnings. For example, to ignore the variables `$_junk` and `$_unused`, this could be set to `'/^_/'`.
 - `validUndefinedVariableNames` (string, default `null`): a space-separated list of names of placeholder variables that you want to ignore from undefined variable warnings. For example, to ignore the variables `$post` and `$undefined`, this could be set to `'post undefined'`.
-- `allowUnusedForeachVariables` (bool, default `true`): if set to true, unused keys or values created by the `as` statement in a `foreach` loop will never be marked as unused.
+- `allowUnusedForeachVariables` (bool, default `true`): if set to true, unused values from the `key => value` syntax in a `foreach` loop will never be marked as unused.
 - `sitePassByRefFunctions` (string, default `null`): a list of custom functions which pass in variables to be initialized by reference (eg `preg_match()`) and therefore should not require those variables to be defined ahead of time. The list is space separated and each entry is of the form `functionName:1,2`. The function name comes first followed by a colon and a comma-separated list of argument numbers (starting from 1) which should be considered variable definitions. The special value `...` in the arguments list will cause all arguments after the last number to be considered variable definitions.
 - `allowWordPressPassByRefFunctions` (bool, default `false`): if set to true, a list of common WordPress pass-by-reference functions will be added to the list of PHP ones so that passing undefined variables to these functions (to be initialized by reference) will be allowed.
 

VariableAnalysis/Lib/Helpers.php

@@ -82,7 +82,7 @@ public static function findParenthesisOwner(File $phpcsFile, $stackPtr) {
    * @return bool
    */
   public static function areAnyConditionsAClass(array $conditions) {
-    foreach (array_reverse($conditions, true) as $scopePtr => $scopeCode) {
+    foreach (array_reverse($conditions, true) as $scopeCode) {
       if ($scopeCode === T_CLASS || $scopeCode === T_ANON_CLASS || $scopeCode === T_TRAIT) {
         return true;
       }
@@ -99,7 +99,7 @@ public static function areConditionsWithinFunctionBeforeClass(array $conditions)
     // Return true if the token conditions are within a function before
     // they are within a class.
     $classTypes = [T_CLASS, T_ANON_CLASS, T_TRAIT];
-    foreach (array_reverse($conditions, true) as $scopePtr => $scopeCode) {
+    foreach (array_reverse($conditions, true) as $scopeCode) {
       if (in_array($scopeCode, $classTypes)) {
         return false;
       }

VariableAnalysis/Lib/VariableInfo.php

@@ -67,7 +67,7 @@ class VariableInfo {
   /**
    * @var bool
    */
-  public $isForeachLoopVar = false;
+  public $isForeachLoopAssociativeValue = false;
 
   /**
    * @var string[]

VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php

@@ -98,7 +98,7 @@ class VariableAnalysisSniff implements Sniff {
   public $allowUnusedParametersBeforeUsed = true;
 
   /**
-   * If set to true, unused keys or values created by the `as` statement
+   * If set to true, unused values from the `key => value` syntax
    * in a `foreach` loop will never be marked as unused.
    *
    *  @var bool
@@ -607,7 +607,7 @@ protected function checkForThisWithinClass(File $phpcsFile, $stackPtr, $varName)
     }
 
     $inFunction = false;
-    foreach (array_reverse($token['conditions'], true) as $scopePtr => $scopeCode) {
+    foreach (array_reverse($token['conditions'], true) as $scopeCode) {
       //  $this within a closure is valid
       if ($scopeCode === T_CLOSURE && $inFunction === false) {
         return true;
@@ -1015,7 +1015,11 @@ protected function checkForForeachLoopVar(File $phpcsFile, $stackPtr, $varName,
     }
     $this->markVariableAssignment($varName, $stackPtr, $currScope);
     $varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
-    $varInfo->isForeachLoopVar = true;
+
+    // Is this the value of a key => value foreach?
+    if ($phpcsFile->findPrevious(T_DOUBLE_ARROW, $stackPtr - 1, $openParenPtr) !== false) {
+      $varInfo->isForeachLoopAssociativeValue = true;
+    }
 
     return true;
   }
@@ -1401,7 +1405,7 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v
     if ($this->allowUnusedParametersBeforeUsed && $varInfo->scopeType === 'param' && $this->areFollowingArgumentsUsed($varInfo, $scopeInfo)) {
       return;
     }
-    if ($this->allowUnusedForeachVariables && $varInfo->isForeachLoopVar) {
+    if ($this->allowUnusedForeachVariables && $varInfo->isForeachLoopAssociativeValue) {
       return;
     }
     if ($varInfo->passByReference && isset($varInfo->firstInitialized)) {

VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php

@@ -124,6 +124,10 @@ public function testFunctionWithForeachWarnings() {
       22,
       24,
       26,
+      48,
+      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
@@ -832,12 +836,15 @@ public function testUnusedForeachVariablesAreIgnoredByDefault() {
     $phpcsFile->process();
     $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
     $expectedWarnings = [
+      5,
       7,
       8,
       16,
       17,
+      23,
       25,
       26,
+      32,
       33,
       34,
     ];
@@ -882,12 +889,15 @@ public function testUnusedForeachVariablesAreIgnoredIfSet() {
     $phpcsFile->process();
     $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
     $expectedWarnings = [
+      5,
       7,
       8,
       16,
       17,
+      23,
       25,
       26,
+      32,
       33,
       34,
     ];

VariableAnalysis/Tests/CodeAnalysis/fixtures/ThisWithinNestedClosedScopeFixture.php

@@ -36,9 +36,9 @@ function nestedFunctionDeclaration() {
             }
         }
     }
-}
+};
 
-$anonClass = class() {
+$anonClass = new class() {
     public function bar() {
         // Using $this here is fine.
         if ($this->something) {
@@ -50,7 +50,7 @@ function nestedFunctionDeclaration() {
             }
         }
     }
-}
+};
 
 trait FooTrait {
     public function bar() {

VariableAnalysis/Tests/CodeAnalysis/fixtures/UnusedForeachFixture.php

@@ -2,7 +2,7 @@
 
 function loopWithUnusedKey() {
     $array = [];
-    foreach ( $array as $key => $value ) { // maybe marked as unused
+    foreach ( $array as $key => $value ) { // key always marked as unused
         echo $value;
         $unused = 'foobar'; // should always be marked as unused
         echo $undefined; // should always be marked as undefined
@@ -11,7 +11,7 @@ function loopWithUnusedKey() {
 
 function loopWithUnusedValue() {
     $array = [];
-    foreach ( $array as $key => $value ) { // maybe marked as unused
+    foreach ( $array as $key => $value ) { // key maybe marked as unused
         echo $key;
         $unused = 'foobar'; // should always be marked as unused
         echo $undefined; // should always be marked as undefined
@@ -20,7 +20,7 @@ function loopWithUnusedValue() {
 
 function loopWithUnusedKeyAndValue() {
     $array = [];
-    foreach ( $array as $key => $value ) { // maybe marked as unused
+    foreach ( $array as $key => $value ) { // key always marked as unused
         echo 'hello';
         $unused = 'foobar'; // should always be marked as unused
         echo $undefined; // should always be marked as undefined
@@ -29,7 +29,7 @@ function loopWithUnusedKeyAndValue() {
 
 function loopWithUnusedValueOnly() {
     $array = [];
-    foreach ( $array as $value ) { // maybe marked as unused
+    foreach ( $array as $value ) { // value always marked as unused
         $unused = 'foobar'; // should always be marked as unused
         echo $undefined; // should always be marked as undefined
     }