Emit separate type of issue when foreach value is unused (#5932)

* Emit separate type of issue when foreach value is unused Fixes #5929 * Fixed var name case sensitivity
vimeo · Jun 17, 2021 · e552925 · e552925
1 parent b1b1072
commit e552925
Show file tree

Hide file tree

Showing 8 changed files with 106 additions and 7 deletions.
diff --git a/config.xsd b/config.xsd
@@ -452,6 +452,7 @@
             <xs:element name="UnusedClass" type="ClassIssueHandlerType" minOccurs="0" />
             <xs:element name="UnusedClosureParam" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="UnusedConstructor" type="MethodIssueHandlerType" minOccurs="0" />
+            <xs:element name="UnusedForeachValue" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="UnusedFunctionCall" type="FunctionIssueHandlerType" minOccurs="0" />
             <xs:element name="UnusedMethod" type="MethodIssueHandlerType" minOccurs="0" />
             <xs:element name="UnusedMethodCall" type="MethodIssueHandlerType" minOccurs="0" />

diff --git a/docs/running_psalm/error_levels.md b/docs/running_psalm/error_levels.md
@@ -310,6 +310,7 @@ These issues are treated as errors at level 7 and below.
  - [UnusedClass](issues/UnusedClass.md)
  - [UnusedClosureParam](issues/UnusedClosureParam.md)
  - [UnusedConstructor](issues/UnusedConstructor.md)
+ - [UnusedForeachValue](issues/UnusedForeachValue.md)
  - [UnusedMethod](issues/UnusedMethod.md)
  - [UnusedParam](issues/UnusedParam.md)
  - [UnusedProperty](issues/UnusedProperty.md)

diff --git a/docs/running_psalm/issues/UnusedForeachValue.md b/docs/running_psalm/issues/UnusedForeachValue.md
@@ -0,0 +1,27 @@
+# UnusedForeachValue
+
+Emitted when `--find-dead-code` is turned on and Psalm cannot find any
+references to the foreach value
+
+```php
+<?php
+
+/** @param non-empty-array<string, int> $a */
+function foo(array $a) : void {
+    foreach ($a as $key => $value) { // $value is unused
+        break;
+    }
+    echo $key;
+}
+```
+
+Can be suppressed by prefixing the variable name with an underscore or naming
+it `$_`:
+
+```php
+<?php
+
+foreach ([1, 2, 3] as $key => $_val) {}
+
+foreach ([1, 2, 3] as $key => $_) {}
+```
diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php
@@ -1606,6 +1606,10 @@ public static function getParentIssueType(string $issue_type): ?string
             return 'UndefinedClass';
         }
 
+        if ($issue_type === 'UnusedForeachValue') {
+            return 'UnusedVariable';
+        }
+
         return null;
     }
 

diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/ForeachAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/ForeachAnalyzer.php
@@ -93,6 +93,10 @@ public static function analyze(
 
         if ($stmt->valueVar instanceof PhpParser\Node\Expr\Variable && is_string($stmt->valueVar->name)) {
             $safe_var_ids['$' . $stmt->valueVar->name] = true;
+            $statements_analyzer->foreach_var_locations['$' . $stmt->valueVar->name][] = new CodeLocation(
+                $statements_analyzer,
+                $stmt->valueVar
+            );
         } elseif ($stmt->valueVar instanceof PhpParser\Node\Expr\List_) {
             foreach ($stmt->valueVar->items as $list_item) {
                 if (!$list_item) {

diff --git a/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php b/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php
@@ -38,6 +38,7 @@
 use Psalm\Issue\UndefinedTrace;
 use Psalm\Issue\UnevaluatedCode;
 use Psalm\Issue\UnrecognizedStatement;
+use Psalm\Issue\UnusedForeachValue;
 use Psalm\Issue\UnusedVariable;
 use Psalm\IssueBuffer;
 use Psalm\Plugin\EventHandler\Event\AfterStatementAnalysisEvent;
@@ -129,6 +130,16 @@ class StatementsAnalyzer extends SourceAnalyzer
     /** @var ?DataFlowGraph */
     public $data_flow_graph;
 
+    /**
+     * Locations of foreach values
+     *
+     * Used to discern ordinary UnusedVariables from UnusedForeachValues
+     *
+     * @var array<string, list<CodeLocation>>
+     * @psalm-internal Psalm\Internal\Analyzer
+     */
+    public $foreach_var_locations = [];
+
     public function __construct(SourceAnalyzer $source, \Psalm\Internal\Provider\NodeDataProvider $node_data)
     {
         $this->source = $source;
@@ -769,12 +780,31 @@ public function checkUnreferencedVars(array $stmts): void
                 && $this->data_flow_graph instanceof VariableUseGraph
                 && !$this->data_flow_graph->isVariableUsed($assignment_node)
             ) {
-                $issue = new UnusedVariable(
-                    $var_id . ' is never referenced or the value is not used',
-                    $original_location
-                );
+                $is_foreach_var = false;
+
+                if (isset($this->foreach_var_locations[$var_id])) {
+                    foreach ($this->foreach_var_locations[$var_id] as $location) {
+                        if ($location->raw_file_start === $original_location->raw_file_start) {
+                            $is_foreach_var = true;
+                            break;
+                        }
+                    }
+                }
+
+                if ($is_foreach_var) {
+                    $issue = new UnusedForeachValue(
+                        $var_id . ' is never referenced or the value is not used',
+                        $original_location
+                    );
+                } else {
+                    $issue = new UnusedVariable(
+                        $var_id . ' is never referenced or the value is not used',
+                        $original_location
+                    );
+                }
 
                 if ($codebase->alter_code
+                    && $issue instanceof UnusedVariable
                     && !$unused_var_remover->checkIfVarRemoved($var_id, $original_location)
                     && isset($project_analyzer->getIssuesToFix()['UnusedVariable'])
                     && !IssueBuffer::isSuppressed($issue, $this->getSuppressedIssues())

diff --git a/src/Psalm/Issue/UnusedForeachValue.php b/src/Psalm/Issue/UnusedForeachValue.php
@@ -0,0 +1,9 @@
+<?php
+
+namespace Psalm\Issue;
+
+class UnusedForeachValue extends CodeIssue
+{
+    public const ERROR_LEVEL = -2;
+    public const SHORTCODE = 275;
+}
diff --git a/tests/UnusedVariableTest.php b/tests/UnusedVariableTest.php
@@ -2968,7 +2968,7 @@ function foo() : void {
                             $i = $i;
                         }
                     }',
-                'error_message' => 'UnusedVariable',
+                'error_message' => 'UnusedForeachValue',
             ],
             'detectUnusedVariableInsideLoopAfterAssignmentWithAddition' => [
                 '<?php
@@ -2977,7 +2977,7 @@ function foo() : void {
                             $i = $i + 1;
                         }
                     }',
-                'error_message' => 'UnusedVariable',
+                'error_message' => 'UnusedForeachValue',
             ],
             'detectUnusedVariableInsideLoopCalledInFunction' => [
                 '<?php
@@ -3078,7 +3078,30 @@ function getLastNum(array $a): int {
                         }
                         return 4;
                     }',
-                'error_message' => 'UnusedVariable',
+                'error_message' => 'UnusedForeachValue',
+            ],
+            'conditionalForeachWithUnusedValue' => [
+                '<?php
+                    if (rand(0, 1) > 0) {
+                        foreach ([1, 2, 3] as $val) {}
+                    }
+                ',
+                'error_message' => 'UnusedForeachValue',
+            ],
+            'doubleForeachWithInnerUnusedValue' => [
+                '<?php
+                    /**
+                     * @param non-empty-list<list<int>> $arr
+                     * @return list<int>
+                     */
+                    function f(array $arr): array {
+                        foreach ($arr as $elt) {
+                            foreach ($elt as $subelt) {}
+                        }
+                        return $elt;
+                    }
+                ',
+                'error_message' => 'UnusedForeachValue'
             ],
             'defineInBothBranchesOfConditional' => [
                 '<?php