Merge pull request #9629 from boesing/bugfix/unused-closure-param-for…

…-required-argument Do not report required closure arguments as unused
vimeo · Apr 10, 2023 · 2aaa577 · 2aaa577
2 parents 9faf811 + e5ae26d
commit 2aaa577
Show file tree

Hide file tree

Showing 4 changed files with 197 additions and 160 deletions.
diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php
@@ -72,6 +72,7 @@
 use function end;
 use function in_array;
 use function is_string;
+use function krsort;
 use function mb_strpos;
 use function md5;
 use function microtime;
@@ -80,6 +81,8 @@
 use function strtolower;
 use function substr;
 
+use const SORT_NUMERIC;
+
 /**
  * @internal
  * @template-covariant TFunction as Closure|Function_|ClassMethod|ArrowFunction
@@ -873,96 +876,55 @@ private function checkParamReferences(
     ): void {
         $codebase = $statements_analyzer->getCodebase();
 
-        $unused_params = [];
-
-        foreach ($statements_analyzer->getUnusedVarLocations() as [$var_name, $original_location]) {
-            if (!array_key_exists(substr($var_name, 1), $storage->param_lookup)) {
-                continue;
-            }
-
-            if (strpos($var_name, '$_') === 0 || (strpos($var_name, '$unused') === 0 && $var_name !== '$unused')) {
-                continue;
-            }
+        $unused_params = $this->detectUnusedParameters($statements_analyzer, $storage, $context);
 
-            $position = array_search(substr($var_name, 1), array_keys($storage->param_lookup), true);
+        if (!$storage instanceof MethodStorage
+            || !$storage->cased_name
+            || $storage->visibility === ClassLikeAnalyzer::VISIBILITY_PRIVATE
+        ) {
+            $last_unused_argument_position = $this->detectPreviousUnusedArgumentPosition(
+                $storage,
+                count($storage->params) - 1,
+            );
 
-            if ($position === false) {
-                throw new UnexpectedValueException('$position should not be false here');
-            }
+            // Sort parameters in reverse order so that we can start from the end of parameters
+            krsort($unused_params, SORT_NUMERIC);
 
-            if ($storage->params[$position]->promoted_property) {
-                continue;
-            }
+            foreach ($unused_params as $unused_param_position => $unused_param_code_location) {
+                $unused_param_var_name = $storage->params[$unused_param_position]->name;
+                $unused_param_message = 'Param ' . $unused_param_var_name . ' is never referenced in this method';
 
-            $did_match_param = false;
+                // Remove the key as we already report the issue
+                unset($unused_params[$unused_param_position]);
 
-            foreach ($this->function->params as $param) {
-                if ($param->var->getAttribute('endFilePos') === $original_location->raw_file_end) {
-                    $did_match_param = true;
+                // Do not report unused required parameters
+                if ($unused_param_position !== $last_unused_argument_position) {
                     break;
                 }
-            }
-
-            if (!$did_match_param) {
-                continue;
-            }
 
-            $assignment_node = DataFlowNode::getForAssignment($var_name, $original_location);
-
-            if ($statements_analyzer->data_flow_graph instanceof VariableUseGraph
-                && $statements_analyzer->data_flow_graph->isVariableUsed($assignment_node)
-            ) {
-                continue;
-            }
+                $last_unused_argument_position = $this->detectPreviousUnusedArgumentPosition(
+                    $storage,
+                    $unused_param_position - 1,
+                );
 
-            if (!$storage instanceof MethodStorage
-                || !$storage->cased_name
-                || $storage->visibility === ClassLikeAnalyzer::VISIBILITY_PRIVATE
-            ) {
                 if ($this instanceof ClosureAnalyzer) {
                     IssueBuffer::maybeAdd(
                         new UnusedClosureParam(
-                            'Param ' . $var_name . ' is never referenced in this method',
-                            $original_location,
-                        ),
-                        $this->getSuppressedIssues(),
-                    );
-                } else {
-                    IssueBuffer::maybeAdd(
-                        new UnusedParam(
-                            'Param ' . $var_name . ' is never referenced in this method',
-                            $original_location,
+                            $unused_param_message,
+                            $unused_param_code_location,
                         ),
                         $this->getSuppressedIssues(),
                     );
-                }
-            } else {
-                $fq_class_name = (string)$context->self;
-
-                $class_storage = $codebase->classlike_storage_provider->get($fq_class_name);
-
-                $method_name_lc = strtolower($storage->cased_name);
-
-                if ($storage->abstract) {
                     continue;
                 }
 
-                if (isset($class_storage->overridden_method_ids[$method_name_lc])) {
-                    $parent_method_id = end($class_storage->overridden_method_ids[$method_name_lc]);
-
-                    if ($parent_method_id) {
-                        $parent_method_storage = $codebase->methods->getStorage($parent_method_id);
-
-                        // if the parent method has a param at that position and isn't abstract
-                        if (!$parent_method_storage->abstract
-                            && isset($parent_method_storage->params[$position])
-                        ) {
-                            continue;
-                        }
-                    }
-                }
-
-                $unused_params[$position] = $original_location;
+                IssueBuffer::maybeAdd(
+                    new UnusedParam(
+                        $unused_param_message,
+                        $unused_param_code_location,
+                    ),
+                    $this->getSuppressedIssues(),
+                );
             }
         }
 
@@ -2079,4 +2041,120 @@ private function getFunctionInformation(
             $overridden_method_ids,
         ];
     }
+
+    /**
+     * @return array<int,CodeLocation>
+     */
+    private function detectUnusedParameters(
+        StatementsAnalyzer $statements_analyzer,
+        FunctionLikeStorage $storage,
+        Context $context
+    ): array {
+        $codebase = $statements_analyzer->getCodebase();
+
+        $unused_params = [];
+
+        foreach ($statements_analyzer->getUnusedVarLocations() as [$var_name, $original_location]) {
+            if (!array_key_exists(substr($var_name, 1), $storage->param_lookup)) {
+                continue;
+            }
+
+            if ($this->isIgnoredForUnusedParam($var_name)) {
+                continue;
+            }
+
+            $position = array_search(substr($var_name, 1), array_keys($storage->param_lookup), true);
+
+            if ($position === false) {
+                throw new UnexpectedValueException('$position should not be false here');
+            }
+
+            if ($storage->params[$position]->promoted_property) {
+                continue;
+            }
+
+            $did_match_param = false;
+
+            foreach ($this->function->params as $param) {
+                if ($param->var->getAttribute('endFilePos') === $original_location->raw_file_end) {
+                    $did_match_param = true;
+                    break;
+                }
+            }
+
+            if (!$did_match_param) {
+                continue;
+            }
+
+            $assignment_node = DataFlowNode::getForAssignment($var_name, $original_location);
+
+            if ($statements_analyzer->data_flow_graph instanceof VariableUseGraph
+                && $statements_analyzer->data_flow_graph->isVariableUsed($assignment_node)
+            ) {
+                continue;
+            }
+
+            if (!$storage instanceof MethodStorage
+                || !$storage->cased_name
+                || $storage->visibility === ClassLikeAnalyzer::VISIBILITY_PRIVATE
+            ) {
+                $unused_params[$position] = $original_location;
+                continue;
+            }
+
+            $fq_class_name = (string)$context->self;
+
+            $class_storage = $codebase->classlike_storage_provider->get($fq_class_name);
+
+            $method_name_lc = strtolower($storage->cased_name);
+
+            if ($storage->abstract) {
+                continue;
+            }
+
+            if (isset($class_storage->overridden_method_ids[$method_name_lc])) {
+                $parent_method_id = end($class_storage->overridden_method_ids[$method_name_lc]);
+
+                if ($parent_method_id) {
+                    $parent_method_storage = $codebase->methods->getStorage($parent_method_id);
+
+                    // if the parent method has a param at that position and isn't abstract
+                    if (!$parent_method_storage->abstract
+                        && isset($parent_method_storage->params[$position])
+                    ) {
+                        continue;
+                    }
+                }
+            }
+
+            $unused_params[$position] = $original_location;
+        }
+
+        return $unused_params;
+    }
+
+    private function detectPreviousUnusedArgumentPosition(FunctionLikeStorage $function, int $position): int
+    {
+        $params = $function->params;
+        krsort($params, SORT_NUMERIC);
+
+        foreach ($params as $index => $param) {
+            if ($index > $position) {
+                continue;
+            }
+
+            if ($this->isIgnoredForUnusedParam($param->name)) {
+                continue;
+            }
+
+            return $index;
+        }
+
+        return 0;
+    }
+
+    private function isIgnoredForUnusedParam(string $var_name): bool
+    {
+        return strpos($var_name, '$_') === 0 || (strpos($var_name, '$unused') === 0 && $var_name !== '$unused');
+    }
 }
diff --git a/tests/Traits/InvalidCodeAnalysisTestTrait.php b/tests/Traits/InvalidCodeAnalysisTestTrait.php
@@ -49,7 +49,7 @@ public function testInvalidCode(
         string $code,
         string $error_message,
         array  $error_levels = [],
-        string $php_version = '7.3'
+        string $php_version = '7.4'
     ): void {
         $test_name = $this->getTestName();
         if (strpos($test_name, 'PHP80-') !== false) {

diff --git a/tests/Traits/ValidCodeAnalysisTestTrait.php b/tests/Traits/ValidCodeAnalysisTestTrait.php
@@ -40,7 +40,7 @@ public function testValidCode(
         string $code,
         array $assertions = [],
         array $ignored_issues = [],
-        string $php_version = '7.3'
+        string $php_version = '7.4'
     ): void {
         $test_name = $this->getTestName();
         if (strpos($test_name, 'PHP80-') !== false) {