From 0252bbbd7cb93e36be1d087854d31eea2da0f1c2 Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Wed, 15 Nov 2023 09:45:56 +0100 Subject: [PATCH] Fixed https://psalm.dev/r/7f112fd745 - MethodComparator only reported an error for this if the parent class was user defined (= not in stubs), which is wrong, since this will cause a fatal error when running the code --- .../Internal/Analyzer/MethodComparator.php | 96 ++++++++++++++----- .../Event/FunctionParamsProviderEvent.php | 4 +- tests/ArrayAccessTest.php | 14 +-- tests/ArrayAssignmentTest.php | 18 ++-- tests/MethodSignatureTest.php | 8 +- tests/StubTest.php | 2 +- tests/TestCase.php | 13 +++ 7 files changed, 111 insertions(+), 44 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/MethodComparator.php b/src/Psalm/Internal/Analyzer/MethodComparator.php index 3bfd4de65..c18fe47f8 100644 --- a/src/Psalm/Internal/Analyzer/MethodComparator.php +++ b/src/Psalm/Internal/Analyzer/MethodComparator.php @@ -37,6 +37,7 @@ use Psalm\Type\Atomic\TTemplateParam; use Psalm\Type\Union; use function array_filter; +use function count; use function in_array; use function strpos; use function strtolower; @@ -130,7 +131,7 @@ final class MethodComparator ) { IssueBuffer::maybeAdd( new MethodSignatureMustProvideReturnType( - 'Method ' . $cased_implementer_method_id . ' must have a return type signature!', + 'Method ' . $cased_implementer_method_id . ' must have a return type signature', $implementer_method_storage->location ?: $code_location, ), $suppressed_issues + $implementer_classlike_storage->suppressed_issues, @@ -199,10 +200,9 @@ final class MethodComparator ); } - if ($guide_classlike_storage->user_defined - && ($guide_classlike_storage->is_interface - || $guide_classlike_storage->preserve_constructor_signature - || $implementer_method_storage->cased_name !== '__construct') + if (($guide_classlike_storage->is_interface + || $guide_classlike_storage->preserve_constructor_signature + || $implementer_method_storage->cased_name !== '__construct') && $implementer_method_storage->required_param_count > $guide_method_storage->required_param_count ) { if ($implementer_method_storage->cased_name !== '__construct') { @@ -361,10 +361,20 @@ final class MethodComparator CodeLocation $code_location, array $suppressed_issues ): void { + // ignore errors from stubbed/out of project files + $config = Config::getInstance(); + if (!$implementer_classlike_storage->user_defined + && (!$implementer_param->location + || !$config->isInProjectDirs( + $implementer_param->location->file_path, + ) + )) { + return; + } + if ($prevent_method_signature_mismatch) { if (!$guide_classlike_storage->user_defined - && $guide_param->type - ) { + && $guide_param->type) { $implementer_param_type = $implementer_param->signature_type; $guide_param_signature_type = $guide_param->type; @@ -386,8 +396,6 @@ final class MethodComparator && !$guide_param->type->from_docblock && ($implementer_param_type || $guide_param_signature_type) ) { - $config = Config::getInstance(); - if ($implementer_param_type && (!$guide_param_signature_type || strtolower($implementer_param_type->getId()) @@ -438,11 +446,8 @@ final class MethodComparator } } - $config = Config::getInstance(); - if ($guide_param->name !== $implementer_param->name && $guide_method_storage->allow_named_arg_calls - && $guide_classlike_storage->user_defined && $implementer_classlike_storage->user_defined && $implementer_param->location && $guide_method_storage->cased_name @@ -451,7 +456,10 @@ final class MethodComparator $implementer_param->location->file_path, ) ) { - if ($config->allow_named_arg_calls + if (!$guide_classlike_storage->user_defined && $i === 0 && count($guide_method_storage->params) < 2) { + // if it's third party defined and a single arg, renaming is unnecessary + // if we still want to psalter it, move this if and change the else below to elseif + } elseif ($config->allow_named_arg_calls || ($guide_classlike_storage->location && !$config->isInProjectDirs($guide_classlike_storage->location->file_path) ) @@ -491,9 +499,7 @@ final class MethodComparator } } - if ($guide_classlike_storage->user_defined - && $implementer_param->signature_type - ) { + if ($implementer_param->signature_type) { self::compareMethodSignatureParams( $codebase, $i, @@ -532,9 +538,7 @@ final class MethodComparator ); } - if ($guide_classlike_storage->user_defined && $implementer_param->by_ref !== $guide_param->by_ref) { - $config = Config::getInstance(); - + if ($implementer_param->by_ref !== $guide_param->by_ref) { IssueBuffer::maybeAdd( new MethodSignatureMismatch( 'Argument ' . ($i + 1) . ' of ' . $cased_implementer_method_id . ' is' . @@ -585,6 +589,50 @@ final class MethodComparator ) : null; + // CallMapHandler needed due to https://github.com/vimeo/psalm/issues/10378 + if (!$guide_param->signature_type + && $guide_param->type + && InternalCallMapHandler::inCallMap($cased_guide_method_id)) { + $guide_method_storage_param_type = TypeExpander::expandUnion( + $codebase, + $guide_param->type, + $guide_classlike_storage->is_trait && $guide_method_storage->abstract + ? $implementer_classlike_storage->name + : $guide_classlike_storage->name, + $guide_classlike_storage->is_trait && $guide_method_storage->abstract + ? $implementer_classlike_storage->name + : $guide_classlike_storage->name, + $guide_classlike_storage->is_trait && $guide_method_storage->abstract + ? $implementer_classlike_storage->parent_class + : $guide_classlike_storage->parent_class, + ); + + $builder = $guide_method_storage_param_type->getBuilder(); + foreach ($builder->getAtomicTypes() as $k => $t) { + if ($t instanceof TTemplateParam) { + $builder->removeType($k); + + foreach ($t->as->getAtomicTypes() as $as_t) { + $builder->addType($as_t); + } + } + } + + if ($builder->hasMixed()) { + foreach ($builder->getAtomicTypes() as $k => $_) { + if ($k !== 'mixed') { + $builder->removeType($k); + } + } + } + $guide_method_storage_param_type = $builder->freeze(); + unset($builder); + + if (!$guide_method_storage_param_type->hasMixed() || $codebase->analysis_php_version_id >= 8_00_00) { + $guide_param_signature_type = $guide_method_storage_param_type; + } + } + $implementer_param_signature_type = TypeExpander::expandUnion( $codebase, $implementer_param_signature_type, @@ -897,11 +945,11 @@ final class MethodComparator if (!$is_contained_by) { if ($codebase->analysis_php_version_id >= 8_00_00 - || $guide_classlike_storage->is_trait === $implementer_classlike_storage->is_trait - || !in_array($guide_classlike_storage->name, $implementer_classlike_storage->used_traits) - || $implementer_method_storage->defining_fqcln !== $implementer_classlike_storage->name - || (!$implementer_method_storage->abstract - && !$guide_method_storage->abstract) + || $guide_classlike_storage->is_trait === $implementer_classlike_storage->is_trait + || !in_array($guide_classlike_storage->name, $implementer_classlike_storage->used_traits) + || $implementer_method_storage->defining_fqcln !== $implementer_classlike_storage->name + || (!$implementer_method_storage->abstract + && !$guide_method_storage->abstract) ) { IssueBuffer::maybeAdd( new MethodSignatureMismatch( diff --git a/src/Psalm/Plugin/EventHandler/Event/FunctionParamsProviderEvent.php b/src/Psalm/Plugin/EventHandler/Event/FunctionParamsProviderEvent.php index 23f3e05ff..6dd717c64 100644 --- a/src/Psalm/Plugin/EventHandler/Event/FunctionParamsProviderEvent.php +++ b/src/Psalm/Plugin/EventHandler/Event/FunctionParamsProviderEvent.php @@ -12,7 +12,7 @@ final class FunctionParamsProviderEvent private StatementsSource $statements_source; private string $function_id; /** - * @var PhpParser\Node\Arg[] + * @var list */ private array $call_args; private ?Context $context; @@ -47,7 +47,7 @@ final class FunctionParamsProviderEvent } /** - * @return PhpParser\Node\Arg[] + * @return list */ public function getCallArgs(): array { diff --git a/tests/ArrayAccessTest.php b/tests/ArrayAccessTest.php index 2302b2661..8bc2a488f 100644 --- a/tests/ArrayAccessTest.php +++ b/tests/ArrayAccessTest.php @@ -907,20 +907,20 @@ class ArrayAccessTest extends TestCase } /** - * @param ?string $name + * @param ?string $offset * @param scalar|array $value * @psalm-suppress MixedArgumentTypeCoercion */ - public function offsetSet($name, $value) : void + public function offsetSet($offset, $value) : void { if (is_array($value)) { $value = new static($value); } - if (null === $name) { + if (null === $offset) { $this->data[] = $value; } else { - $this->data[$name] = $value; + $this->data[$offset] = $value; } } @@ -1053,12 +1053,12 @@ class ArrayAccessTest extends TestCase } /** - * @param string $name + * @param string $offset * @param mixed $value */ - public function offsetSet($name, $value) : void + public function offsetSet($offset, $value) : void { - $this->data[$name] = $value; + $this->data[$offset] = $value; } public function __isset(string $name) : bool diff --git a/tests/ArrayAssignmentTest.php b/tests/ArrayAssignmentTest.php index 11f308676..38f98d70d 100644 --- a/tests/ArrayAssignmentTest.php +++ b/tests/ArrayAssignmentTest.php @@ -1043,17 +1043,20 @@ class ArrayAssignmentTest extends TestCase * @template-implements ArrayAccess */ class C implements ArrayAccess { - public function offsetExists(int $offset) : bool { return true; } + public function offsetExists(mixed $offset) : bool { return true; } public function offsetGet($offset) : string { return "";} - public function offsetSet(?int $offset, string $value) : void {} + public function offsetSet(mixed $offset, mixed $value) : void {} - public function offsetUnset(int $offset) : void { } + public function offsetUnset(mixed $offset) : void { } } $c = new C(); $c[] = "hello";', + 'assertions' => [], + 'ignored_issues' => [], + 'php_version' => '8.0', ], 'checkEmptinessAfterConditionalArrayAdjustment' => [ 'code' => ' */ class C implements ArrayAccess { - public function offsetExists(int $offset) : bool { return true; } + public function offsetExists(mixed $offset) : bool { return true; } public function offsetGet($offset) : string { return "";} - public function offsetSet(int $offset, string $value) : void {} + public function offsetSet(mixed $offset, mixed $value) : void {} - public function offsetUnset(int $offset) : void { } + public function offsetUnset(mixed $offset) : void { } } $c = new C(); $c[] = "hello";', + 'assertions' => [], + 'ignored_issues' => [], + 'php_version' => '8.0', ], 'conditionalRestrictedDocblockKeyAssignment' => [ 'code' => 'expectException(CodeException::class); - $this->expectExceptionMessage('ImplementedParamTypeMismatch'); + $this->expectExceptionMessage('MethodSignatureMismatch'); $this->project_analyzer = $this->getProjectAnalyzerWithConfig( TestConfig::loadFromXML( dirname(__DIR__), diff --git a/tests/TestCase.php b/tests/TestCase.php index d0d285eb9..2da89b355 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -32,10 +32,23 @@ class TestCase extends BaseTestCase { protected static string $src_dir_path; + /** + * caused by phpunit using setUp() instead of __construct + * could perhaps use psalm-plugin-phpunit once https://github.com/psalm/psalm-plugin-phpunit/issues/129 + * to remove this suppression + * + * @psalm-suppress PropertyNotSetInConstructor + */ protected ProjectAnalyzer $project_analyzer; + /** + * @psalm-suppress PropertyNotSetInConstructor + */ protected FakeFileProvider $file_provider; + /** + * @psalm-suppress PropertyNotSetInConstructor + */ protected Config $testConfig; public static function setUpBeforeClass(): void