Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not report required closure arguments as unused #9629

Merged
merged 5 commits into from
Apr 10, 2023
Merged

Do not report required closure arguments as unused #9629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
803786e
qa: restructure `UnusedVariableTest`
boesing Apr 9, 2023
3a9817a
qa: introduce failing test for #8095
boesing Apr 9, 2023
707a0ba
qa: add another test to verify required arguments are not being reported
boesing Apr 9, 2023
6d5f999
bugfix: do not report required closure arguments as unused
boesing Apr 9, 2023
e5ae26d
qa: raise `php_version` in `ValidCodeAnalysisTestTrait` and
boesing Apr 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
222 changes: 150 additions & 72 deletions src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -80,6 +81,8 @@
use function strtolower;
use function substr;

use const SORT_NUMERIC;

/**
* @internal
* @template-covariant TFunction as Closure|Function_|ClassMethod|ArrowFunction
Expand Down Expand Up @@ -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(),
);
}
}

Expand Down Expand Up @@ -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');
}
}
2 changes: 1 addition & 1 deletion tests/Traits/InvalidCodeAnalysisTestTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion tests/Traits/ValidCodeAnalysisTestTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down