From 1129ab14756bd2f26f4767ffb36578d26bbb90f8 Mon Sep 17 00:00:00 2001 From: Daniil Gentili Date: Fri, 26 Nov 2021 10:23:01 +0100 Subject: [PATCH 1/6] Raise RedundantCast when using array_values on a list --- .../Call/NamedFunctionCallHandler.php | 33 +++++++++++++++++++ tests/ArrayAssignmentTest.php | 12 +++++++ 2 files changed, 45 insertions(+) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php index 25637330586..ec8295ae81f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php @@ -364,6 +364,39 @@ function (array $_) : bool { return; } + if ($first_arg && $function_id === 'array_values') { + $first_arg_type = $statements_analyzer->node_data->getType($first_arg->value); + + if ($first_arg_type + && UnionTypeComparator::isContainedBy( + $codebase, + $first_arg_type, + Type::getList() + ) + ) { + if ($first_arg_type->from_docblock) { + if (IssueBuffer::accepts( + new \Psalm\Issue\RedundantCastGivenDocblockType( + 'The call to array_values is unnecessary given the docblock type', + new CodeLocation($statements_analyzer, $function_name) + ), + $statements_analyzer->getSuppressedIssues() + )) { + // fall through + } + } else { + if (IssueBuffer::accepts( + new \Psalm\Issue\RedundantCast( + 'The call to array_values is unnecessary', + new CodeLocation($statements_analyzer, $function_name) + ), + $statements_analyzer->getSuppressedIssues() + )) { + // fall through + } + } + } + } if ($first_arg && $function_id === 'strtolower') { $first_arg_type = $statements_analyzer->node_data->getType($first_arg->value); diff --git a/tests/ArrayAssignmentTest.php b/tests/ArrayAssignmentTest.php index 513505f5ed2..6812f748ea9 100644 --- a/tests/ArrayAssignmentTest.php +++ b/tests/ArrayAssignmentTest.php @@ -831,6 +831,7 @@ public function offsetSet($offset, $value): void 'keyedIntOffsetArrayValues' => [ ' [ @@ -2007,6 +2008,17 @@ function baz(array $bar) : void { }', 'error_message' => 'RedundantCast', ], + 'arrayValuesOnList' => [ + ' $a + * @return list + */ + function foo(array $a) : array { + return array_values($a); + }', + 'error_message' => 'RedundantCast', + ], 'assignToListWithUpdatedForeachKey' => [ ' Date: Fri, 26 Nov 2021 10:35:09 +0100 Subject: [PATCH 2/6] Fix tests --- tests/Template/ClassTemplateTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Template/ClassTemplateTest.php b/tests/Template/ClassTemplateTest.php index 0e8acc866c3..dbd7b9cd17b 100644 --- a/tests/Template/ClassTemplateTest.php +++ b/tests/Template/ClassTemplateTest.php @@ -2287,6 +2287,7 @@ public function __construct(array $elements) { * @return static */ public function map(callable $callback) { + /** @psalm-suppress RedundantCast */ return new static(array_values(array_map($callback, $this->elements))); } } From ee8c5c9c35f31fc552ce41fdcf2f342cc72823ed Mon Sep 17 00:00:00 2001 From: Daniil Gentili Date: Fri, 26 Nov 2021 13:48:38 +0100 Subject: [PATCH 3/6] Remove some array_values --- .../Statements/Block/IfElseAnalyzer.php | 26 +++++++++---------- .../Call/NamedFunctionCallHandler.php | 4 +-- .../Statements/Expression/MatchAnalyzer.php | 2 +- .../Statements/Expression/TernaryAnalyzer.php | 26 +++++++++---------- src/Psalm/Internal/Fork/Pool.php | 2 +- tests/ReportOutputTest.php | 3 +-- 6 files changed, 29 insertions(+), 34 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php index 26de808b925..a2daa8a6fe6 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php @@ -130,28 +130,26 @@ public static function analyze( $if_clauses = []; } - $if_clauses = array_values( - array_map( + $if_clauses = array_map( /** * @return Clause */ - function (Clause $c) use ($mixed_var_ids, $cond_object_id): Clause { - $keys = array_keys($c->possibilities); + function (Clause $c) use ($mixed_var_ids, $cond_object_id): Clause { + $keys = array_keys($c->possibilities); - $mixed_var_ids = \array_diff($mixed_var_ids, $keys); + $mixed_var_ids = \array_diff($mixed_var_ids, $keys); - foreach ($keys as $key) { - foreach ($mixed_var_ids as $mixed_var_id) { - if (preg_match('/^' . preg_quote($mixed_var_id, '/') . '(\[|-)/', $key)) { - return new Clause([], $cond_object_id, $cond_object_id, true); - } + foreach ($keys as $key) { + foreach ($mixed_var_ids as $mixed_var_id) { + if (preg_match('/^' . preg_quote($mixed_var_id, '/') . '(\[|-)/', $key)) { + return new Clause([], $cond_object_id, $cond_object_id, true); } } + } - return $c; - }, - $if_clauses - ) + return $c; + }, + $if_clauses ); $entry_clauses = $context->clauses; diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php index ec8295ae81f..c2e663a4a52 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php @@ -377,7 +377,7 @@ function (array $_) : bool { if ($first_arg_type->from_docblock) { if (IssueBuffer::accepts( new \Psalm\Issue\RedundantCastGivenDocblockType( - 'The call to array_values is unnecessary given the docblock type', + 'The call to array_values is unnecessary given the list docblock type '.$first_arg_type, new CodeLocation($statements_analyzer, $function_name) ), $statements_analyzer->getSuppressedIssues() @@ -387,7 +387,7 @@ function (array $_) : bool { } else { if (IssueBuffer::accepts( new \Psalm\Issue\RedundantCast( - 'The call to array_values is unnecessary', + 'The call to array_values is unnecessary, '.$first_arg_type.' is already a list', new CodeLocation($statements_analyzer, $function_name) ), $statements_analyzer->getSuppressedIssues() diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/MatchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/MatchAnalyzer.php index f1f18b5851d..74fccfe9c0b 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/MatchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/MatchAnalyzer.php @@ -209,7 +209,7 @@ public static function analyze( } $all_match_condition = self::convertCondsToConditional( - \array_values($all_conds), + $all_conds, $match_condition, $match_condition->getAttributes() ); diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/TernaryAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/TernaryAnalyzer.php index 0145c48d85c..6f735b7628b 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/TernaryAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/TernaryAnalyzer.php @@ -83,28 +83,26 @@ public static function analyze( } } - $if_clauses = array_values( - array_map( + $if_clauses = array_map( /** * @return \Psalm\Internal\Clause */ - function (\Psalm\Internal\Clause $c) use ($mixed_var_ids, $cond_id): \Psalm\Internal\Clause { - $keys = array_keys($c->possibilities); + function (\Psalm\Internal\Clause $c) use ($mixed_var_ids, $cond_id): \Psalm\Internal\Clause { + $keys = array_keys($c->possibilities); - $mixed_var_ids = \array_diff($mixed_var_ids, $keys); + $mixed_var_ids = \array_diff($mixed_var_ids, $keys); - foreach ($keys as $key) { - foreach ($mixed_var_ids as $mixed_var_id) { - if (preg_match('/^' . preg_quote($mixed_var_id, '/') . '(\[|-)/', $key)) { - return new \Psalm\Internal\Clause([], $cond_id, $cond_id, true); - } + foreach ($keys as $key) { + foreach ($mixed_var_ids as $mixed_var_id) { + if (preg_match('/^' . preg_quote($mixed_var_id, '/') . '(\[|-)/', $key)) { + return new \Psalm\Internal\Clause([], $cond_id, $cond_id, true); } } + } - return $c; - }, - $if_clauses - ) + return $c; + }, + $if_clauses ); // this will see whether any of the clauses in set A conflict with the clauses in set B diff --git a/src/Psalm/Internal/Fork/Pool.php b/src/Psalm/Internal/Fork/Pool.php index 70fac92cf46..e1eb0ca7476 100644 --- a/src/Psalm/Internal/Fork/Pool.php +++ b/src/Psalm/Internal/Fork/Pool.php @@ -387,7 +387,7 @@ private function readResultsFromChildren(): array } } - return array_values($terminationMessages); + return $terminationMessages; } /** diff --git a/tests/ReportOutputTest.php b/tests/ReportOutputTest.php index 232ef2b3f16..39f6192d42f 100644 --- a/tests/ReportOutputTest.php +++ b/tests/ReportOutputTest.php @@ -13,7 +13,6 @@ use Psalm\Report\ReportOptions; use Psalm\Tests\Internal\Provider; -use function array_values; use function file_get_contents; use function json_decode; use function ob_end_clean; @@ -815,7 +814,7 @@ public function testJsonReport(): void $json_report_options = ProjectAnalyzer::getFileReportOptions([__DIR__ . '/test-report.json'])[0]; $this->assertSame( - array_values($issue_data), + $issue_data, json_decode(IssueBuffer::getOutput(IssueBuffer::getIssuesData(), $json_report_options), true) ); } From 00ab986c06979f35063699d28c11f947219ef6ba Mon Sep 17 00:00:00 2001 From: Daniil Gentili Date: Fri, 26 Nov 2021 17:22:22 +0100 Subject: [PATCH 4/6] Update IfElseAnalyzer.php --- .../Internal/Analyzer/Statements/Block/IfElseAnalyzer.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php index a2daa8a6fe6..40d8385b5ae 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php @@ -130,10 +130,10 @@ public static function analyze( $if_clauses = []; } - $if_clauses = array_map( - /** - * @return Clause - */ + $if_clauses = array_map( + /** + * @return Clause + */ function (Clause $c) use ($mixed_var_ids, $cond_object_id): Clause { $keys = array_keys($c->possibilities); From 2145a2cbbd3d9e9409e4f03e2b5365464e1fcd69 Mon Sep 17 00:00:00 2001 From: Daniil Gentili Date: Fri, 26 Nov 2021 21:52:34 +0100 Subject: [PATCH 5/6] Make sure that the keyed array is actually a list after unsetting --- composer.json | 1 + src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 09b100a31c5..2a0c17671cc 100644 --- a/composer.json +++ b/composer.json @@ -35,6 +35,7 @@ "openlss/lib-array2xml": "^1.0", "sebastian/diff": "^3.0 || ^4.0", "symfony/console": "^3.4.17 || ^4.1.6 || ^5.0 || ^6.0", + "symfony/polyfill-php81": "^1.23", "webmozart/path-util": "^2.3" }, "provide": { diff --git a/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php index febeb353ee6..bea728b15ff 100644 --- a/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php @@ -7,6 +7,8 @@ use Psalm\Internal\Analyzer\StatementsAnalyzer; use Psalm\Type; +use function array_is_list; + class UnsetAnalyzer { public static function analyze( @@ -50,8 +52,8 @@ public static function analyze( || $var->dim instanceof PhpParser\Node\Scalar\LNumber ) { if (isset($atomic_root_type->properties[$var->dim->value])) { - $atomic_root_type->is_list = false; unset($atomic_root_type->properties[$var->dim->value]); + $atomic_root_type->is_list = array_is_list($atomic_root_type->properties); $root_type->bustCache(); //remove id cache } From 925df22052a6716e353eb189b2c02639a05a0e58 Mon Sep 17 00:00:00 2001 From: Daniil Gentili Date: Sun, 28 Nov 2021 13:42:30 +0100 Subject: [PATCH 6/6] Use simplified, proper logic --- composer.json | 1 - src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php | 8 ++++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/composer.json b/composer.json index 2a0c17671cc..09b100a31c5 100644 --- a/composer.json +++ b/composer.json @@ -35,7 +35,6 @@ "openlss/lib-array2xml": "^1.0", "sebastian/diff": "^3.0 || ^4.0", "symfony/console": "^3.4.17 || ^4.1.6 || ^5.0 || ^6.0", - "symfony/polyfill-php81": "^1.23", "webmozart/path-util": "^2.3" }, "provide": { diff --git a/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php index bea728b15ff..63f9a946362 100644 --- a/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php @@ -7,7 +7,7 @@ use Psalm\Internal\Analyzer\StatementsAnalyzer; use Psalm\Type; -use function array_is_list; +use function count; class UnsetAnalyzer { @@ -52,8 +52,12 @@ public static function analyze( || $var->dim instanceof PhpParser\Node\Scalar\LNumber ) { if (isset($atomic_root_type->properties[$var->dim->value])) { + if ($atomic_root_type->is_list + && $var->dim->value !== count($atomic_root_type->properties)-1 + ) { + $atomic_root_type->is_list = false; + } unset($atomic_root_type->properties[$var->dim->value]); - $atomic_root_type->is_list = array_is_list($atomic_root_type->properties); $root_type->bustCache(); //remove id cache }