From 91ab7883bf589ee750321207ae9e051efe545c5c Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Mon, 18 Feb 2019 16:19:30 +0200 Subject: [PATCH 01/14] Update to weirdan/codeception-psalm-module 0.2.1 --- composer.json | 2 +- tests/acceptance/Assert.feature | 12 +++++++++++- tests/acceptance/Assert75.feature | 12 +++++++++++- tests/acceptance/TestCase.feature | 15 ++++++++++++++- 4 files changed, 37 insertions(+), 4 deletions(-) diff --git a/composer.json b/composer.json index b3676c8..4048992 100755 --- a/composer.json +++ b/composer.json @@ -18,7 +18,7 @@ "require-dev": { "squizlabs/php_codesniffer": "^3.3.1", "codeception/base": "^2.5", - "weirdan/codeception-psalm-module": "^0.1.0" + "weirdan/codeception-psalm-module": "^0.2.1" }, "extra": { "psalm": { diff --git a/tests/acceptance/Assert.feature b/tests/acceptance/Assert.feature index 9321dcc..ca3b0a8 100644 --- a/tests/acceptance/Assert.feature +++ b/tests/acceptance/Assert.feature @@ -4,7 +4,17 @@ Feature: Assert I need Psalm to typecheck asserts Background: - Given I have the following code preamble + Given I have the following config + """ + + + + + + + + """ + And I have the following code preamble """ + + + + + + + """ + And I have the following code preamble """ + + + + + + + + + + """ + And I have the following code preamble """ Date: Mon, 18 Feb 2019 17:51:21 +0200 Subject: [PATCH 02/14] Check for missing dataProviders --- hooks/TestCaseHandler.php | 83 +++++++++++++++++++++++++------ tests/acceptance/TestCase.feature | 23 +++++++++ 2 files changed, 91 insertions(+), 15 deletions(-) diff --git a/hooks/TestCaseHandler.php b/hooks/TestCaseHandler.php index 65c7e76..761a6dd 100644 --- a/hooks/TestCaseHandler.php +++ b/hooks/TestCaseHandler.php @@ -5,13 +5,18 @@ use PHPUnit\Framework\TestCase; use PhpParser\Comment\Doc; use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; +use Psalm\CodeLocation; use Psalm\Codebase; use Psalm\DocComment; use Psalm\FileSource; +use Psalm\IssueBuffer; +use Psalm\Issue\UndefinedMethod; +use Psalm\Plugin\Hook\AfterClassLikeAnalysisInterface; use Psalm\Plugin\Hook\AfterClassLikeVisitInterface; +use Psalm\StatementsSource; use Psalm\Storage\ClassLikeStorage; -class TestCaseHandler implements AfterClassLikeVisitInterface +class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAnalysisInterface { /** * {@inheritDoc} @@ -23,13 +28,60 @@ class TestCaseHandler implements AfterClassLikeVisitInterface Codebase $codebase, array &$file_replacements = [] ) { - if ($codebase->classExtends($classStorage->name, TestCase::class)) { - if (self::hasInitializers($classStorage, $classNode)) { - $classStorage->suppressed_issues[] = 'MissingConstructor'; + if (!$codebase->classExtends($classStorage->name, TestCase::class)) { + return; + } + + if (self::hasInitializers($classStorage, $classNode)) { + $classStorage->suppressed_issues[] = 'MissingConstructor'; + } + } + + /** + * {@inheritDoc} + */ + public static function afterStatementAnalysis( + ClassLike $classNode, + ClassLikeStorage $classStorage, + StatementsSource $statements_source, + Codebase $codebase, + array &$file_replacements = [] + ) { + foreach ($classStorage->methods as $method_name => $method_storage) { + if (!$method_storage->location) { + continue; + } + + $stmt_method = $classNode->getMethod($method_name); + + if (!$stmt_method) { + throw new \RuntimeException('Failed to find ' . $method_name); + } + + $specials = self::getSpecials($stmt_method); + + if (!isset($specials['dataProvider'])) { + continue; + } + + foreach ($specials['dataProvider'] as $line => $provider) { + $provider_method_id = $classStorage->name . '::' . (string) $provider; + + if (!$codebase->methodExists($provider_method_id)) { + $location = clone $method_storage->location; + $location->setCommentLine($line); + + IssueBuffer::accepts(new UndefinedMethod( + 'Provider method ' . $provider_method_id . ' is not defined', + $location, + $provider_method_id + )); + } } } } + private static function hasInitializers(ClassLikeStorage $storage, ClassLike $stmt): bool { if (isset($storage->methods['setup'])) { @@ -50,20 +102,21 @@ class TestCaseHandler implements AfterClassLikeVisitInterface private static function isBeforeInitializer(ClassMethod $method): bool { - /** @var string[] $comments */ - $comments = $method->getAttribute('comments', []); + $specials = self::getSpecials($method); + return isset($specials['before']); + } - foreach ($comments as $comment) { - if (!$comment instanceof Doc) { - continue; - } + /** @return array> */ + private static function getSpecials(ClassMethod $method): array + { + $docblock = $method->getDocComment(); - $parsed_comment = DocComment::parse((string)$comment->getReformattedText()); - if (isset($parsed_comment['specials']['before'])) { - return true; + if ($docblock) { + $parsed_comment = DocComment::parse((string)$docblock->getReformattedText(), $docblock->getLine()); + if (isset($parsed_comment['specials'])) { + return $parsed_comment['specials']; } } - - return false; + return []; } } diff --git a/tests/acceptance/TestCase.feature b/tests/acceptance/TestCase.feature index d6c1817..c2a4c39 100644 --- a/tests/acceptance/TestCase.feature +++ b/tests/acceptance/TestCase.feature @@ -144,3 +144,26 @@ Feature: TestCase Then I see these errors | Type | Message | | MissingConstructor | NS\MyTestCase has an uninitialized variable $this->i, but no constructor | + + Scenario: Missing data provider is reported + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** + * @param mixed $int + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething($int) { + $this->assertIsInt($int); + } + } + new MyTestCase; + """ + When I run Psalm + Then I see these errors + | Type | Message | + | UndefinedMethod | Provider method NS\MyTestCase::provide is not defined | + And I see no other errors From d9277bb993077c3346f81c8f942bae0907b04edb Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Mon, 18 Feb 2019 18:16:13 +0200 Subject: [PATCH 03/14] Bumped Psalm version to 3.0.10 Breaking change: minimum psalm version is now 3.0.10 (due to the hook interface added in that version) Also fixed test to be compatible with legacy PHPUnit --- composer.json | 2 +- tests/acceptance/TestCase.feature | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 4048992..bd97649 100755 --- a/composer.json +++ b/composer.json @@ -11,7 +11,7 @@ ], "require": { "phpunit/phpunit": "^6.0 || ^7.0 || ^8.0", - "vimeo/psalm": "^3.0.8 || dev-master", + "vimeo/psalm": "^3.0.10 || dev-master", "composer/semver": "^1.4", "muglug/package-versions-56": "^1.2" }, diff --git a/tests/acceptance/TestCase.feature b/tests/acceptance/TestCase.feature index c2a4c39..cf200b0 100644 --- a/tests/acceptance/TestCase.feature +++ b/tests/acceptance/TestCase.feature @@ -157,7 +157,7 @@ Feature: TestCase * @dataProvider provide */ public function testSomething($int) { - $this->assertIsInt($int); + $this->assertEquals(1, $int); } } new MyTestCase; From 8732175020b98bb168dfa56e28251cd3971d2d0c Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Tue, 19 Feb 2019 22:58:56 +0200 Subject: [PATCH 04/14] Implemented provider type checks - Invalid providers (should be iterable with array elements) - Providers returning fewer arguments then test requires - Providers returning datasets that are incompatible with test signatures refs psalm/phpunit-psalm-plugin#13 --- Exception/UnsupportedPsalmVersion.php | 8 + composer.json | 3 +- hooks/TestCaseHandler.php | 245 +++++++++++++++++++++++-- tests/acceptance/TestCase.feature | 246 ++++++++++++++++++++++++++ 4 files changed, 487 insertions(+), 15 deletions(-) create mode 100644 Exception/UnsupportedPsalmVersion.php diff --git a/Exception/UnsupportedPsalmVersion.php b/Exception/UnsupportedPsalmVersion.php new file mode 100644 index 0000000..00c4222 --- /dev/null +++ b/Exception/UnsupportedPsalmVersion.php @@ -0,0 +1,8 @@ +methods as $method_name => $method_storage) { + if (!$codebase->classExtends($class_storage->name, TestCase::class)) { + return null; + } + + /** @var MethodStorage $method_storage */ + foreach ($class_storage->methods as $method_name => $method_storage) { if (!$method_storage->location) { continue; } - $stmt_method = $classNode->getMethod($method_name); + $stmt_method = $class_node->getMethod($method_name); if (!$stmt_method) { throw new \RuntimeException('Failed to find ' . $method_name); @@ -60,27 +67,237 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $specials = self::getSpecials($stmt_method); + $method_id = $class_storage->name . '::' . $method_storage->cased_name; + if (!isset($specials['dataProvider'])) { continue; } foreach ($specials['dataProvider'] as $line => $provider) { - $provider_method_id = $classStorage->name . '::' . (string) $provider; + $provider_method_id = $class_storage->name . '::' . (string) $provider; + + $provider_docblock_location = clone $method_storage->location; + $provider_docblock_location->setCommentLine($line); if (!$codebase->methodExists($provider_method_id)) { - $location = clone $method_storage->location; - $location->setCommentLine($line); - - IssueBuffer::accepts(new UndefinedMethod( + IssueBuffer::accepts(new Issue\UndefinedMethod( 'Provider method ' . $provider_method_id . ' is not defined', - $location, + $provider_docblock_location, $provider_method_id )); + + continue; + } + + $provider_return_type = $codebase->getMethodReturnType($provider_method_id, $classStorage->name); + assert(null !== $provider_return_type); + + $provider_return_type_location = $codebase->getMethodReturnTypeLocation($provider_method_id); + assert(null !== $provider_return_type_location); + + $expected_provider_return_type = new Type\Atomic\TIterable([ + Type::combineUnionTypes(Type::getInt(), Type::getString()), + Type::getArray(), + ]); + + if (!self::isTypeContainedByType( + $codebase, + $provider_return_type, + new Type\Union([$expected_provider_return_type]) + )) { + IssueBuffer::accepts(new Issue\InvalidReturnType( + 'Providers must return ' . $expected_provider_return_type->getId() + . ', ' . $provider_return_type->getId() . ' provided', + $provider_return_type_location + )); + + continue; + } + + foreach ($provider_return_type->getTypes() as $type) { + if (!$type->isIterable($codebase)) { + IssueBuffer::accepts(new Issue\InvalidReturnType( + 'Providers must return ' . $expected_provider_return_type->getId() + . ', ' . $provider_return_type->getId() . ' provided', + $provider_return_type_location + )); + continue; + } + } + + // unionize iterable so that instead of array|Traversable + // we get iterable + + $provider_return_type = self::unionizeIterables($codebase, $provider_return_type); + + // this is basically a repetition of above $expected_provider_return_type check + // but older Psalm versions couldn't check iterable descendants (see vimeo/psalm#1359) + if (!self::isTypeContainedByType( + $codebase, + $provider_return_type->type_params[0], + $expected_provider_return_type->type_params[0] + ) || !self::isTypeContainedByType( + $codebase, + $provider_return_type->type_params[1], + $expected_provider_return_type->type_params[1] + )) { + IssueBuffer::accepts(new Issue\InvalidReturnType( + 'Providers must return ' . $expected_provider_return_type->getId() + . ', ' . $provider_return_type->getId() . ' provided', + $provider_return_type_location + )); + continue; + } + + $checkParam = function ( + Type\Union $potential_argument_type, + FunctionLikeParameter $param, + int $param_offset + ) use ( + $codebase, + $method_id, + $provider_method_id, + $provider_return_type, + $provider_docblock_location + ): void { + assert(null !== $param->type); + if (self::isTypeContainedByType($codebase, $potential_argument_type, $param->type)) { + // ok + } elseif (self::canTypeBeContainedByType($codebase, $potential_argument_type, $param->type)) { + IssueBuffer::accepts(new Issue\PossiblyInvalidArgument( + 'Argument ' . ($param_offset + 1) . ' of ' . $method_id + . ' expects ' . $param->type->getId() . ', ' + . $potential_argument_type->getId() . ' provided' + . ' by ' . $provider_method_id . '():(' . $provider_return_type->getId() . ')', + $provider_docblock_location + )); + } else { + IssueBuffer::accepts(new Issue\InvalidArgument( + 'Argument ' . ($param_offset + 1) . ' of ' . $method_id + . ' expects ' . $param->type->getId() . ', ' + . $potential_argument_type->getId() . ' provided' + . ' by ' . $provider_method_id . '():(' . $provider_return_type->getId() . ')', + $provider_docblock_location + )); + } + }; + + /** @var Type\Atomic\TArray|Type\Atomic\ObjectLike $dataset_type */ + $dataset_type = $provider_return_type->type_params[1]->getTypes()['array']; + + if ($dataset_type instanceof Type\Atomic\TArray) { + // check that all of the required (?) params accept value type + $potential_argument_type = $dataset_type->type_params[1]; + foreach ($method_storage->params as $param_offset => $param) { + $checkParam($potential_argument_type, $param, $param_offset); + } + } else { + // iterate over all params checking if corresponding value type is acceptable + // let's hope properties are sorted in array order + $potential_argument_types = array_values($dataset_type->properties); + + if (count($potential_argument_types) < $method_storage->required_param_count) { + IssueBuffer::accepts(new Issue\TooFewArguments( + 'Too few arguments for ' . $method_id + . ' - expecting ' . $method_storage->required_param_count + . ' but saw ' . count($potential_argument_types) + . ' provided by ' . $provider_method_id . '()' + . ':(' . $provider_return_type->getId() . ')', + $provider_docblock_location, + $method_id + )); + } + + + foreach ($method_storage->params as $param_offset => $param) { + if (!isset($potential_argument_types[$param_offset])) { + break; + } + $potential_argument_type = $potential_argument_types[$param_offset]; + + $checkParam($potential_argument_type, $param, $param_offset); + } } } } } + private static function isTypeContainedByType( + Codebase $codebase, + Type\Union $input_type, + Type\Union $container_type + ): bool { + if (method_exists($codebase, 'isTypeContainedByType')) { + return (bool) $codebase->isTypeContainedByType($input_type, $container_type); + } elseif (class_exists(\Psalm\Internal\Analyzer\TypeAnalyzer::class, true) + && method_exists(\Psalm\Internal\Analyzer\TypeAnalyzer::class, 'isContainedBy')) { + return \Psalm\Internal\Analyzer\TypeAnalyzer::isContainedBy($codebase, $input_type, $container_type); + } else { + throw new UnsupportedPsalmVersion(); + } + } + + private static function canTypeBeContainedByType( + Codebase $codebase, + Type\Union $input_type, + Type\Union $container_type + ): bool { + if (method_exists($codebase, 'canTypeBeContainedByType')) { + return (bool) $codebase->canTypeBeContainedByType($input_type, $container_type); + } elseif (class_exists(\Psalm\Internal\Analyzer\TypeAnalyzer::class, true) + && method_exists(\Psalm\Internal\Analyzer\TypeAnalyzer::class, 'canBeContainedBy')) { + return \Psalm\Internal\Analyzer\TypeAnalyzer::canBeContainedBy($codebase, $input_type, $container_type); + } else { + throw new UnsupportedPsalmVersion(); + } + } + + private static function unionizeIterables(Codebase $codebase, Type\Union $iterables): Type\Atomic\TIterable + { + /** @var Type\Union[] $key_types */ + $key_types = []; + + /** @var Type\Union[] $value_types */ + $value_types = []; + + foreach ($iterables->getTypes() as $type) { + if (!$type->isIterable($codebase)) { + throw new \RuntimeException('should be iterable'); + } + + if ($type instanceof Type\Atomic\TArray) { + $key_types[] = $type->type_params[0] ?? Type::getMixed(); + $value_types[] = $type->type_params[1] ?? Type::getMixed(); + } elseif ($type instanceof Type\Atomic\ObjectLike) { + $key_types[] = $type->getGenericKeyType(); + $value_types[] = $type->getGenericValueType(); + } elseif ($type instanceof Type\Atomic\TNamedObject || $type instanceof Type\Atomic\TIterable) { + $iterable_key_type = null; + $iterable_value_type = null; + + \Psalm\Internal\Analyzer\Statements\Block\ForeachAnalyzer::getKeyValueParamsForTraversableObject( + $type, + $codebase, + $iterable_key_type, + $iterable_value_type + ); + $key_types[] = $iterable_key_type ?? Type::getMixed(); + $value_types[] = $iterable_value_type ?? Type::getMixed(); + } else { + throw new \RuntimeException('unexpected type'); + } + } + + $combine = function (Type\Union $a, Type\Union $b) use ($codebase): Type\Union { + return Type::combineUnionTypes($a, $b, $codebase); + }; + + return new Type\Atomic\TIterable([ + array_reduce($key_types, $combine, new Type\Union([])), + array_reduce($value_types, $combine, new Type\Union([])) + ]); + } + private static function hasInitializers(ClassLikeStorage $storage, ClassLike $stmt): bool { diff --git a/tests/acceptance/TestCase.feature b/tests/acceptance/TestCase.feature index cf200b0..e4c9d39 100644 --- a/tests/acceptance/TestCase.feature +++ b/tests/acceptance/TestCase.feature @@ -167,3 +167,249 @@ Feature: TestCase | Type | Message | | UndefinedMethod | Provider method NS\MyTestCase::provide is not defined | And I see no other errors + + Scenario: Invalid iterable data provider is reported + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return iterable */ + public function provide() { + yield 1; + } + /** + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm + Then I see these errors + | Type | Message | + | InvalidReturnType | Providers must return iterable>, iterable provided | + + Scenario: Valid iterable data provider is allowed + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return iterable> */ + public function provide() { + yield [1]; + } + /** + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm + Then I see no errors + + Scenario: Invalid generator data provider is reported + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return \Generator */ + public function provide() { + yield 1; + } + /** + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm + Then I see these errors + | Type | Message | + | InvalidReturnType | Providers must return iterable>, Generator provided | + + Scenario: Valid generator data provider is allowed + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return \Generator,mixed,void> */ + public function provide() { + yield [1]; + } + /** + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm + Then I see no errors + + Scenario: Invalid array data provider is reported + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return array */ + public function provide() { + return [1 => 1]; + } + /** + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm + Then I see these errors + | Type | Message | + | InvalidReturnType | Providers must return iterable>, array provided | + + Scenario: Valid array data provider is allowed + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return array> */ + public function provide() { + return [ + "data set name" => [1], + ]; + } + /** + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm + Then I see no errors + + Scenario: Valid object data provider is allowed + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return \ArrayObject> */ + public function provide() { + return new \ArrayObject([ + "data set name" => [1], + ]); + } + /** + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm + Then I see no errors + + Scenario: Invalid dataset shape is reported + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return iterable */ + public function provide() { + yield "data set name" => ["str"]; + } + /** + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm + Then I see these errors + | Type | Message | + | InvalidArgument | Argument 1 of NS\MyTestCase::testSomething expects int, string provided by NS\MyTestCase::provide():(iterable) | + + Scenario: Invalid dataset array is reported + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return iterable> */ + public function provide() { + yield "data set name" => ["str"]; + } + /** + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm + Then I see these errors + | Type | Message | + | PossiblyInvalidArgument | Argument 1 of NS\MyTestCase::testSomething expects int, string\|int provided by NS\MyTestCase::provide():(iterable>) | + + Scenario: Shape dataset with missing params is reported + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return iterable */ + public function provide() { + yield "data set name" => [1]; + } + /** + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething(int $int, int $i) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm + Then I see these errors + | Type | Message | + | TooFewArguments | Too few arguments for NS\MyTestCase::testSomething - expecting 2 but saw 1 provided by NS\MyTestCase::provide():(iterable) | From 62d7bcffa4799aa7e6a1e4ca7199f4dbbc069055 Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Wed, 20 Feb 2019 00:03:06 +0200 Subject: [PATCH 05/14] Bump psalm version due to missing functionality Breaking change: plugin now requires psalm 3.0.13+ (TIterable was made generic in that version) --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 327f70d..2a048b9 100755 --- a/composer.json +++ b/composer.json @@ -11,7 +11,7 @@ ], "require": { "phpunit/phpunit": "^6.0 || ^7.0 || ^8.0", - "vimeo/psalm": "^3.0.10 || dev-master", + "vimeo/psalm": "^3.0.13 || dev-master", "composer/semver": "^1.4", "muglug/package-versions-56": "^1.2" }, From 17d8351c4000581d2b85a60a841fe029bd358510 Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Wed, 20 Feb 2019 00:05:32 +0200 Subject: [PATCH 06/14] PHP 7.0 compatibility --- hooks/TestCaseHandler.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/hooks/TestCaseHandler.php b/hooks/TestCaseHandler.php index 38f2430..4a17486 100644 --- a/hooks/TestCaseHandler.php +++ b/hooks/TestCaseHandler.php @@ -149,7 +149,9 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna continue; } - $checkParam = function ( + $checkParam = + /** @return void */ + function ( Type\Union $potential_argument_type, FunctionLikeParameter $param, int $param_offset @@ -159,7 +161,7 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $provider_method_id, $provider_return_type, $provider_docblock_location - ): void { + ) { assert(null !== $param->type); if (self::isTypeContainedByType($codebase, $potential_argument_type, $param->type)) { // ok From 55ee7eabc701eae4e7ac5c3a2fbca831421fa0fe Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Wed, 20 Feb 2019 00:09:27 +0200 Subject: [PATCH 07/14] Add new files/folders to phpcs check --- phpcs.xml.dist | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/phpcs.xml.dist b/phpcs.xml.dist index 3ca3171..729276b 100755 --- a/phpcs.xml.dist +++ b/phpcs.xml.dist @@ -1,11 +1,15 @@ - + Plugin.php + tests/_support/Helper + tests/_support/AcceptanceTester.php + hooks + Exception From 6906f62cfa8c650fdeef214b7fb5fe64f78a9e8b Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Wed, 20 Feb 2019 00:47:51 +0200 Subject: [PATCH 08/14] Fixed reported type for invalid iterables --- hooks/TestCaseHandler.php | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/hooks/TestCaseHandler.php b/hooks/TestCaseHandler.php index 4a17486..82b25cd 100644 --- a/hooks/TestCaseHandler.php +++ b/hooks/TestCaseHandler.php @@ -92,6 +92,8 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $provider_return_type = $codebase->getMethodReturnType($provider_method_id, $classStorage->name); assert(null !== $provider_return_type); + $provider_return_type_string = $provider_return_type->getId(); + $provider_return_type_location = $codebase->getMethodReturnTypeLocation($provider_method_id); assert(null !== $provider_return_type_location); @@ -107,7 +109,7 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna )) { IssueBuffer::accepts(new Issue\InvalidReturnType( 'Providers must return ' . $expected_provider_return_type->getId() - . ', ' . $provider_return_type->getId() . ' provided', + . ', ' . $provider_return_type_string . ' provided', $provider_return_type_location )); @@ -118,7 +120,7 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna if (!$type->isIterable($codebase)) { IssueBuffer::accepts(new Issue\InvalidReturnType( 'Providers must return ' . $expected_provider_return_type->getId() - . ', ' . $provider_return_type->getId() . ' provided', + . ', ' . $provider_return_type_string . ' provided', $provider_return_type_location )); continue; @@ -143,7 +145,7 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna )) { IssueBuffer::accepts(new Issue\InvalidReturnType( 'Providers must return ' . $expected_provider_return_type->getId() - . ', ' . $provider_return_type->getId() . ' provided', + . ', ' . $provider_return_type_string . ' provided', $provider_return_type_location )); continue; @@ -159,7 +161,7 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $codebase, $method_id, $provider_method_id, - $provider_return_type, + $provider_return_type_string, $provider_docblock_location ) { assert(null !== $param->type); @@ -170,7 +172,7 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna 'Argument ' . ($param_offset + 1) . ' of ' . $method_id . ' expects ' . $param->type->getId() . ', ' . $potential_argument_type->getId() . ' provided' - . ' by ' . $provider_method_id . '():(' . $provider_return_type->getId() . ')', + . ' by ' . $provider_method_id . '():(' . $provider_return_type_string . ')', $provider_docblock_location )); } else { @@ -178,7 +180,7 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna 'Argument ' . ($param_offset + 1) . ' of ' . $method_id . ' expects ' . $param->type->getId() . ', ' . $potential_argument_type->getId() . ' provided' - . ' by ' . $provider_method_id . '():(' . $provider_return_type->getId() . ')', + . ' by ' . $provider_method_id . '():(' . $provider_return_type_string . ')', $provider_docblock_location )); } @@ -204,7 +206,7 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna . ' - expecting ' . $method_storage->required_param_count . ' but saw ' . count($potential_argument_types) . ' provided by ' . $provider_method_id . '()' - . ':(' . $provider_return_type->getId() . ')', + . ':(' . $provider_return_type_string . ')', $provider_docblock_location, $method_id )); From 330b3a1ef6c24c0c7fab4d6795dd8671d8c5e21f Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Wed, 20 Feb 2019 01:21:21 +0200 Subject: [PATCH 09/14] Added getKeyValueParamsForTraversableObject proxy --- hooks/TestCaseHandler.php | 48 ++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/hooks/TestCaseHandler.php b/hooks/TestCaseHandler.php index 82b25cd..ce5ec1b 100644 --- a/hooks/TestCaseHandler.php +++ b/hooks/TestCaseHandler.php @@ -256,6 +256,42 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna } } + /** + * @param Type\Atomic\TNamedObject|Type\Atomic\TIterable $type + * @return array{0:Type\Union,1:Type\Union} + */ + private static function getKeyValueParamsForTraversableObject(Codebase $codebase, $type): array + { + if (method_exists($codebase, 'getKeyValueParamsForTraversableObject')) { + $ret = (array) $codebase->getKeyValueParamsForTraversableObject($type); + assert($ret[0] instanceof Type\Union); + assert($ret[1] instanceof Type\Union); + return [$ret[0], $ret[1]]; + } elseif (class_exists(\Psalm\Internal\Analyzer\Statements\Block\ForeachAnalyzer::class, true) + && method_exists( + \Psalm\Internal\Analyzer\Statements\Block\ForeachAnalyzer::class, + 'getKeyValueParamsForTraversableObject' + ) + ) { + $iterable_key_type = null; + $iterable_value_type = null; + + \Psalm\Internal\Analyzer\Statements\Block\ForeachAnalyzer::getKeyValueParamsForTraversableObject( + $type, + $codebase, + $iterable_key_type, + $iterable_value_type + ); + + return [ + $iterable_key_type ?? Type::getMixed(), + $iterable_value_type ?? Type::getMixed(), + ]; + } else { + throw new UnsupportedPsalmVersion(); + } + } + private static function unionizeIterables(Codebase $codebase, Type\Union $iterables): Type\Atomic\TIterable { /** @var Type\Union[] $key_types */ @@ -276,17 +312,7 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $key_types[] = $type->getGenericKeyType(); $value_types[] = $type->getGenericValueType(); } elseif ($type instanceof Type\Atomic\TNamedObject || $type instanceof Type\Atomic\TIterable) { - $iterable_key_type = null; - $iterable_value_type = null; - - \Psalm\Internal\Analyzer\Statements\Block\ForeachAnalyzer::getKeyValueParamsForTraversableObject( - $type, - $codebase, - $iterable_key_type, - $iterable_value_type - ); - $key_types[] = $iterable_key_type ?? Type::getMixed(); - $value_types[] = $iterable_value_type ?? Type::getMixed(); + list($key_types[], $value_types[]) = self::getKeyValueParamsForTraversableObject($codebase, $type); } else { throw new \RuntimeException('unexpected type'); } From 4fe3c88acae8e3c0df22761dc87be0e346fedb00 Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Wed, 20 Feb 2019 13:19:11 +0200 Subject: [PATCH 10/14] Tweak dead code detection on TestCase descendants - Don't report TestCase descendants as unused - Don't report test methods as unused - Don't report providers referenced by test methods as unused Refs psalm/phpunit-psalm-plugin#13 /cc @SignpostMarv --- hooks/TestCaseHandler.php | 22 +++++- tests/acceptance/TestCase.feature | 123 ++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 2 deletions(-) diff --git a/hooks/TestCaseHandler.php b/hooks/TestCaseHandler.php index ce5ec1b..091708c 100644 --- a/hooks/TestCaseHandler.php +++ b/hooks/TestCaseHandler.php @@ -53,7 +53,17 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna return null; } - /** @var MethodStorage $method_storage */ + // add a fake reference to test class to prevent it from being marked as unused + // it would have been easier to add a suppression, but that's only possible + // since 3.0.17 (vimeo/psalm#1353) + // + // This should always pass, we're calling it for the side-effect + // of adding self-reference + + if (!$codebase->classOrInterfaceExists($class_storage->name, $class_storage->location)) { + return null; + } + foreach ($class_storage->methods as $method_name => $method_storage) { if (!$method_storage->location) { continue; @@ -69,6 +79,13 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $method_id = $class_storage->name . '::' . $method_storage->cased_name; + if (0 !== strpos($method_storage->cased_name, 'test') + && !isset($specials['test'])) { + continue; // skip non-test methods + } + + $method_storage->suppressed_issues[] = 'PossiblyUnusedMethod'; + if (!isset($specials['dataProvider'])) { continue; } @@ -79,7 +96,8 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $provider_docblock_location = clone $method_storage->location; $provider_docblock_location->setCommentLine($line); - if (!$codebase->methodExists($provider_method_id)) { + // methodExists also can mark methods as used (weird, but handy) + if (!$codebase->methodExists($provider_method_id, $provider_docblock_location, $method_id)) { IssueBuffer::accepts(new Issue\UndefinedMethod( 'Provider method ' . $provider_method_id . ' is not defined', $provider_docblock_location, diff --git a/tests/acceptance/TestCase.feature b/tests/acceptance/TestCase.feature index e4c9d39..31778ff 100644 --- a/tests/acceptance/TestCase.feature +++ b/tests/acceptance/TestCase.feature @@ -41,6 +41,7 @@ Feature: TestCase Then I see these errors | Type | Message | | InvalidArgument | Argument 1 of PHPUnit\Framework\TestCase::expectException expects class-string, NS\MyTestCase::class provided | + And I see no other errors Scenario: TestCase::expectException() accepts throwables Given I have the following code @@ -144,6 +145,7 @@ Feature: TestCase Then I see these errors | Type | Message | | MissingConstructor | NS\MyTestCase has an uninitialized variable $this->i, but no constructor | + And I see no other errors Scenario: Missing data provider is reported Given I have the following code @@ -192,6 +194,7 @@ Feature: TestCase Then I see these errors | Type | Message | | InvalidReturnType | Providers must return iterable>, iterable provided | + And I see no other errors Scenario: Valid iterable data provider is allowed Given I have the following code @@ -240,6 +243,7 @@ Feature: TestCase Then I see these errors | Type | Message | | InvalidReturnType | Providers must return iterable>, Generator provided | + And I see no other errors Scenario: Valid generator data provider is allowed Given I have the following code @@ -288,6 +292,7 @@ Feature: TestCase Then I see these errors | Type | Message | | InvalidReturnType | Providers must return iterable>, array provided | + And I see no other errors Scenario: Valid array data provider is allowed Given I have the following code @@ -363,6 +368,7 @@ Feature: TestCase Then I see these errors | Type | Message | | InvalidArgument | Argument 1 of NS\MyTestCase::testSomething expects int, string provided by NS\MyTestCase::provide():(iterable) | + And I see no other errors Scenario: Invalid dataset array is reported Given I have the following code @@ -388,6 +394,7 @@ Feature: TestCase Then I see these errors | Type | Message | | PossiblyInvalidArgument | Argument 1 of NS\MyTestCase::testSomething expects int, string\|int provided by NS\MyTestCase::provide():(iterable>) | + And I see no other errors Scenario: Shape dataset with missing params is reported Given I have the following code @@ -413,3 +420,119 @@ Feature: TestCase Then I see these errors | Type | Message | | TooFewArguments | Too few arguments for NS\MyTestCase::testSomething - expecting 2 but saw 1 provided by NS\MyTestCase::provide():(iterable) | + And I see no other errors + + Scenario: Referenced providers are not marked as unused + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return iterable */ + public function provide() { + yield "data set name" => [1]; + } + /** + * @return void + * @psalm-suppress UnusedMethod + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm with dead code detection + Then I see no errors + + Scenario: Unreferenced providers are marked as unused + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return iterable */ + public function provide() { + yield "data set name" => [1]; + } + /** + * @return void + * @psalm-suppress UnusedMethod + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm with dead code detection + Then I see these errors + | Type | Message | + | PossiblyUnusedMethod | Cannot find public calls to method NS\MyTestCase::provide | + And I see no other errors + + Scenario: Test method are never marked as unused + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** + * @return void + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + /** + * @return void + * @test + */ + public function somethingElse(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm with dead code detection + Then I see no errors + + Scenario: Unreferenced non-test methods are marked as unused + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** + * @return void + */ + public function somethingElse(int $int) { + $this->assertEquals(1, $int); + } + } + new MyTestCase; + """ + When I run Psalm with dead code detection + Then I see these errors + | Type | Message | + | PossiblyUnusedMethod | Cannot find public calls to method NS\MyTestCase::somethingElse | + And I see no other errors + + Scenario: Unreferenced TestCase descendants are never marked as unused + Given I have the following code + """ + class MyTestCase extends TestCase + { + } + """ + When I run Psalm with dead code detection + Then I see no errors + + Scenario: Unreferenced non-test classes are marked as unused + Given I have the following code + """ + class UtilityClass + { + } + """ + When I run Psalm with dead code detection + Then I see these errors + | Type | Message | + | UnusedClass | Class NS\UtilityClass is never used | + And I see no other errors From 6025f28933ba4cdeb4b7c2517e5d452bc4eba365 Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Fri, 22 Feb 2019 22:41:23 +0200 Subject: [PATCH 11/14] Fix empty union crash, clarify invalid provider message --- hooks/TestCaseHandler.php | 40 +++++++++++++++++++++------- tests/acceptance/TestCase.feature | 43 ++++++------------------------- 2 files changed, 38 insertions(+), 45 deletions(-) diff --git a/hooks/TestCaseHandler.php b/hooks/TestCaseHandler.php index 091708c..896d652 100644 --- a/hooks/TestCaseHandler.php +++ b/hooks/TestCaseHandler.php @@ -17,6 +17,7 @@ use Psalm\Storage\ClassLikeStorage; use Psalm\Storage\FunctionLikeParameter; use Psalm\Storage\MethodStorage; use Psalm\Type; +use Psalm\Type\Atomic\TIterable; class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAnalysisInterface { @@ -125,12 +126,23 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $provider_return_type, new Type\Union([$expected_provider_return_type]) )) { - IssueBuffer::accepts(new Issue\InvalidReturnType( - 'Providers must return ' . $expected_provider_return_type->getId() - . ', ' . $provider_return_type_string . ' provided', - $provider_return_type_location - )); - + if (self::isTypeContainedByType( + $codebase, + $provider_return_type, + new Type\Union([new TIterable()]) + )) { + IssueBuffer::accepts(new Issue\InvalidReturnType( + 'Providers must return ' . $expected_provider_return_type->getId() + . ', possibly different ' . $provider_return_type_string . ' provided', + $provider_return_type_location + )); + } else { + IssueBuffer::accepts(new Issue\InvalidReturnType( + 'Providers must return ' . $expected_provider_return_type->getId() + . ', ' . $provider_return_type_string . ' provided', + $provider_return_type_location + )); + } continue; } @@ -336,13 +348,21 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna } } - $combine = function (Type\Union $a, Type\Union $b) use ($codebase): Type\Union { - return Type::combineUnionTypes($a, $b, $codebase); + if (empty($key_types)) { + $key_types[] = Type::getMixed(); + } + + if (empty($value_types)) { + $value_types[] = Type::getMixed(); + } + + $combine = function (?Type\Union $a, Type\Union $b) use ($codebase): Type\Union { + return $a ? Type::combineUnionTypes($a, $b, $codebase) : $b; }; return new Type\Atomic\TIterable([ - array_reduce($key_types, $combine, new Type\Union([])), - array_reduce($value_types, $combine, new Type\Union([])) + array_reduce($key_types, $combine), + array_reduce($value_types, $combine) ]); } diff --git a/tests/acceptance/TestCase.feature b/tests/acceptance/TestCase.feature index 31778ff..5066b50 100644 --- a/tests/acceptance/TestCase.feature +++ b/tests/acceptance/TestCase.feature @@ -29,7 +29,7 @@ Feature: TestCase Given I have Psalm newer than "3.0.12" (because of "missing functionality") Given I have the following code """ - class MyTestCase extends TestCase + class MyTestCase extends TestCase { /** @return void */ public function testSomething() { @@ -46,7 +46,7 @@ Feature: TestCase Scenario: TestCase::expectException() accepts throwables Given I have the following code """ - class MyTestCase extends TestCase + class MyTestCase extends TestCase { /** @return void */ public function testSomething() { @@ -123,7 +123,7 @@ Feature: TestCase interface I { public function work(): int; } - class MyTestCase extends TestCase + class MyTestCase extends TestCase { /** @var ObjectProphecy */ private $i; @@ -144,7 +144,7 @@ Feature: TestCase When I run Psalm Then I see these errors | Type | Message | - | MissingConstructor | NS\MyTestCase has an uninitialized variable $this->i, but no constructor | + | MissingConstructor | NS\MyTestCase has an uninitialized variable $this->i, but no constructor | And I see no other errors Scenario: Missing data provider is reported @@ -162,7 +162,6 @@ Feature: TestCase $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm Then I see these errors @@ -181,14 +180,12 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod * @dataProvider provide */ public function testSomething(int $int) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm Then I see these errors @@ -207,14 +204,12 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod * @dataProvider provide */ public function testSomething(int $int) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm Then I see no errors @@ -230,14 +225,12 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod * @dataProvider provide */ public function testSomething(int $int) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm Then I see these errors @@ -256,14 +249,12 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod * @dataProvider provide */ public function testSomething(int $int) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm Then I see no errors @@ -279,19 +270,17 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod * @dataProvider provide */ public function testSomething(int $int) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm Then I see these errors - | Type | Message | - | InvalidReturnType | Providers must return iterable>, array provided | + | Type | Message | + | InvalidReturnType | Providers must return iterable>, possibly different array provided | And I see no other errors Scenario: Valid array data provider is allowed @@ -307,14 +296,12 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod * @dataProvider provide */ public function testSomething(int $int) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm Then I see no errors @@ -332,14 +319,12 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod * @dataProvider provide */ public function testSomething(int $int) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm Then I see no errors @@ -355,14 +340,12 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod * @dataProvider provide */ public function testSomething(int $int) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm Then I see these errors @@ -381,14 +364,12 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod * @dataProvider provide */ public function testSomething(int $int) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm Then I see these errors @@ -407,19 +388,17 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod * @dataProvider provide */ public function testSomething(int $int, int $i) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm Then I see these errors | Type | Message | - | TooFewArguments | Too few arguments for NS\MyTestCase::testSomething - expecting 2 but saw 1 provided by NS\MyTestCase::provide():(iterable) | + | TooFewArguments | Too few arguments for NS\MyTestCase::testSomething - expecting 2 but saw 1 provided by NS\MyTestCase::provide():(iterable) | And I see no other errors Scenario: Referenced providers are not marked as unused @@ -433,14 +412,12 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod * @dataProvider provide */ public function testSomething(int $int) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm with dead code detection Then I see no errors @@ -456,13 +433,11 @@ Feature: TestCase } /** * @return void - * @psalm-suppress UnusedMethod */ public function testSomething(int $int) { $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm with dead code detection Then I see these errors @@ -489,7 +464,6 @@ Feature: TestCase $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm with dead code detection Then I see no errors @@ -506,11 +480,10 @@ Feature: TestCase $this->assertEquals(1, $int); } } - new MyTestCase; """ When I run Psalm with dead code detection Then I see these errors - | Type | Message | + | Type | Message | | PossiblyUnusedMethod | Cannot find public calls to method NS\MyTestCase::somethingElse | And I see no other errors From 3452ea5d76b7f8757038508f29db9851c3856ae3 Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Sat, 23 Feb 2019 16:11:53 +0200 Subject: [PATCH 12/14] Fixes after psalm smoke tests - More possibly-different handling - Handle possibly-undefined offsets for dataset shapes --- hooks/TestCaseHandler.php | 79 ++++++++-------- tests/acceptance/TestCase.feature | 145 +++++++++++++++++++++++++++++- 2 files changed, 179 insertions(+), 45 deletions(-) diff --git a/hooks/TestCaseHandler.php b/hooks/TestCaseHandler.php index 896d652..7b90734 100644 --- a/hooks/TestCaseHandler.php +++ b/hooks/TestCaseHandler.php @@ -121,31 +121,6 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna Type::getArray(), ]); - if (!self::isTypeContainedByType( - $codebase, - $provider_return_type, - new Type\Union([$expected_provider_return_type]) - )) { - if (self::isTypeContainedByType( - $codebase, - $provider_return_type, - new Type\Union([new TIterable()]) - )) { - IssueBuffer::accepts(new Issue\InvalidReturnType( - 'Providers must return ' . $expected_provider_return_type->getId() - . ', possibly different ' . $provider_return_type_string . ' provided', - $provider_return_type_location - )); - } else { - IssueBuffer::accepts(new Issue\InvalidReturnType( - 'Providers must return ' . $expected_provider_return_type->getId() - . ', ' . $provider_return_type_string . ' provided', - $provider_return_type_location - )); - } - continue; - } - foreach ($provider_return_type->getTypes() as $type) { if (!$type->isIterable($codebase)) { IssueBuffer::accepts(new Issue\InvalidReturnType( @@ -162,8 +137,6 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $provider_return_type = self::unionizeIterables($codebase, $provider_return_type); - // this is basically a repetition of above $expected_provider_return_type check - // but older Psalm versions couldn't check iterable descendants (see vimeo/psalm#1359) if (!self::isTypeContainedByType( $codebase, $provider_return_type->type_params[0], @@ -173,11 +146,20 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $provider_return_type->type_params[1], $expected_provider_return_type->type_params[1] )) { - IssueBuffer::accepts(new Issue\InvalidReturnType( - 'Providers must return ' . $expected_provider_return_type->getId() - . ', ' . $provider_return_type_string . ' provided', - $provider_return_type_location - )); + if ($provider_return_type->type_params[0]->hasMixed() + || $provider_return_type->type_params[1]->hasMixed()) { + IssueBuffer::accepts(new Issue\InvalidReturnType( + 'Providers must return ' . $expected_provider_return_type->getId() + . ', possibly different ' . $provider_return_type_string . ' provided', + $provider_return_type_location + )); + } else { + IssueBuffer::accepts(new Issue\InvalidReturnType( + 'Providers must return ' . $expected_provider_return_type->getId() + . ', ' . $provider_return_type_string . ' provided', + $provider_return_type_location + )); + } continue; } @@ -195,12 +177,24 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $provider_docblock_location ) { assert(null !== $param->type); - if (self::isTypeContainedByType($codebase, $potential_argument_type, $param->type)) { + $param_type = clone $param->type; + if ($param->default_type) { + $param_type->possibly_undefined = true; + } + if (self::isTypeContainedByType($codebase, $potential_argument_type, $param_type)) { // ok - } elseif (self::canTypeBeContainedByType($codebase, $potential_argument_type, $param->type)) { + } elseif (self::canTypeBeContainedByType($codebase, $potential_argument_type, $param_type)) { IssueBuffer::accepts(new Issue\PossiblyInvalidArgument( 'Argument ' . ($param_offset + 1) . ' of ' . $method_id - . ' expects ' . $param->type->getId() . ', ' + . ' expects ' . $param_type->getId() . ', ' + . $potential_argument_type->getId() . ' provided' + . ' by ' . $provider_method_id . '():(' . $provider_return_type_string . ')', + $provider_docblock_location + )); + } elseif ($potential_argument_type->possibly_undefined && !$param->default_type) { + IssueBuffer::accepts(new Issue\InvalidArgument( + 'Argument ' . ($param_offset + 1) . ' of ' . $method_id + . ' has no default value, but possibly undefined ' . $potential_argument_type->getId() . ' provided' . ' by ' . $provider_method_id . '():(' . $provider_return_type_string . ')', $provider_docblock_location @@ -208,7 +202,7 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna } else { IssueBuffer::accepts(new Issue\InvalidArgument( 'Argument ' . ($param_offset + 1) . ' of ' . $method_id - . ' expects ' . $param->type->getId() . ', ' + . ' expects ' . $param_type->getId() . ', ' . $potential_argument_type->getId() . ' provided' . ' by ' . $provider_method_id . '():(' . $provider_return_type_string . ')', $provider_docblock_location @@ -348,12 +342,11 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna } } - if (empty($key_types)) { - $key_types[] = Type::getMixed(); - } - - if (empty($value_types)) { - $value_types[] = Type::getMixed(); + if (empty($key_types) || empty($value_types)) { + return new Type\Atomic\TIterable([ + Type::getMixed(), + Type::getMixed(), + ]); } $combine = function (?Type\Union $a, Type\Union $b) use ($codebase): Type\Union { @@ -362,7 +355,7 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna return new Type\Atomic\TIterable([ array_reduce($key_types, $combine), - array_reduce($value_types, $combine) + array_reduce($value_types, $combine), ]); } diff --git a/tests/acceptance/TestCase.feature b/tests/acceptance/TestCase.feature index 5066b50..0c706d0 100644 --- a/tests/acceptance/TestCase.feature +++ b/tests/acceptance/TestCase.feature @@ -279,8 +279,80 @@ Feature: TestCase """ When I run Psalm Then I see these errors - | Type | Message | - | InvalidReturnType | Providers must return iterable>, possibly different array provided | + | Type | Message | + | InvalidReturnType | Providers must return iterable>, array provided | + And I see no other errors + + Scenario: Underspecified array data provider is reported + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return array */ + public function provide() { + return [1 => [1]]; + } + /** + * @return void + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + """ + When I run Psalm + Then I see these errors + | Type | Message | + | InvalidReturnType | Providers must return iterable>, possibly different array provided | + And I see no other errors + + Scenario: Underspecified iterable data provider is reported + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return iterable */ + public function provide() { + return [1 => [1]]; + } + /** + * @return void + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + """ + When I run Psalm + Then I see these errors + | Type | Message | + | InvalidReturnType | Providers must return iterable>, possibly different iterable provided | + And I see no other errors + + Scenario: Underspecified generator data provider is reported + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return \Generator */ + public function provide() { + yield 1 => [1]; + } + /** + * @return void + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + """ + When I run Psalm + Then I see these errors + | Type | Message | + | InvalidReturnType | Providers must return iterable>, possibly different Generator provided | And I see no other errors Scenario: Valid array data provider is allowed @@ -509,3 +581,72 @@ Feature: TestCase | Type | Message | | UnusedClass | Class NS\UtilityClass is never used | And I see no other errors + + Scenario: Provider returning possibly undefined offset is fine when test method has default for that param + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return iterable */ + public function provide() { + yield "data set name" => rand(0,1) ? [1] : []; + } + /** + * @return void + * @dataProvider provide + */ + public function testSomething(int $int = 2) { + $this->assertEquals(1, $int); + } + } + """ + When I run Psalm + Then I see no errors + + Scenario: Provider returning possibly undefined offset with mismatching type is reported even when test method has default for that param + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return iterable */ + public function provide() { + yield "data set name" => rand(0,1) ? ["1"] : []; + } + /** + * @return void + * @dataProvider provide + */ + public function testSomething(int $int = 2) { + $this->assertEquals(1, $int); + } + } + """ + When I run Psalm + Then I see these errors + | Type | Message | + | InvalidArgument | Argument 1 of NS\MyTestCase::testSomething expects int, string provided by NS\MyTestCase::provide():(iterable) | + And I see no other errors + + Scenario: Provider returning possibly undefined offset is marked when test method has no default for that param + Given I have the following code + """ + class MyTestCase extends TestCase + { + /** @return iterable */ + public function provide() { + yield "data set name" => rand(0,1) ? [1] : []; + } + /** + * @return void + * @dataProvider provide + */ + public function testSomething(int $int) { + $this->assertEquals(1, $int); + } + } + """ + When I run Psalm + Then I see these errors + | Type | Message | + | InvalidArgument | Argument 1 of NS\MyTestCase::testSomething has no default value, but possibly undefined int provided by NS\MyTestCase::provide():(iterable) | + And I see no other errors From 586c2ef13318500d858646472826385d98aa79f8 Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Sat, 23 Feb 2019 16:19:57 +0200 Subject: [PATCH 13/14] PHP 7.0 compatibility --- hooks/TestCaseHandler.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/hooks/TestCaseHandler.php b/hooks/TestCaseHandler.php index 7b90734..49bbae3 100644 --- a/hooks/TestCaseHandler.php +++ b/hooks/TestCaseHandler.php @@ -349,9 +349,11 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna ]); } - $combine = function (?Type\Union $a, Type\Union $b) use ($codebase): Type\Union { - return $a ? Type::combineUnionTypes($a, $b, $codebase) : $b; - }; + $combine = + /** @param null|Type\Union $a */ + function ($a, Type\Union $b) use ($codebase): Type\Union { + return $a ? Type::combineUnionTypes($a, $b, $codebase) : $b; + }; return new Type\Atomic\TIterable([ array_reduce($key_types, $combine), From c13cf7b5faa7a791e7de97a5f9e4d8c4a387e00c Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Sat, 23 Feb 2019 20:15:07 +0200 Subject: [PATCH 14/14] Addressed code review comments --- hooks/TestCaseHandler.php | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/hooks/TestCaseHandler.php b/hooks/TestCaseHandler.php index 49bbae3..9cadb37 100644 --- a/hooks/TestCaseHandler.php +++ b/hooks/TestCaseHandler.php @@ -134,7 +134,8 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna // unionize iterable so that instead of array|Traversable // we get iterable - + // + // TODO: this may get implemented in a future Psalm version, remove it then $provider_return_type = self::unionizeIterables($codebase, $provider_return_type); if (!self::isTypeContainedByType( @@ -236,7 +237,6 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna )); } - foreach ($method_storage->params as $param_offset => $param) { if (!isset($potential_argument_types[$param_offset])) { break; @@ -257,12 +257,15 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna ): bool { if (method_exists($codebase, 'isTypeContainedByType')) { return (bool) $codebase->isTypeContainedByType($input_type, $container_type); - } elseif (class_exists(\Psalm\Internal\Analyzer\TypeAnalyzer::class, true) + } + + /** @psalm-suppress RedundantCondition */ + if (class_exists(\Psalm\Internal\Analyzer\TypeAnalyzer::class, true) && method_exists(\Psalm\Internal\Analyzer\TypeAnalyzer::class, 'isContainedBy')) { return \Psalm\Internal\Analyzer\TypeAnalyzer::isContainedBy($codebase, $input_type, $container_type); - } else { - throw new UnsupportedPsalmVersion(); } + + throw new UnsupportedPsalmVersion(); } private static function canTypeBeContainedByType( @@ -272,12 +275,15 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna ): bool { if (method_exists($codebase, 'canTypeBeContainedByType')) { return (bool) $codebase->canTypeBeContainedByType($input_type, $container_type); - } elseif (class_exists(\Psalm\Internal\Analyzer\TypeAnalyzer::class, true) + } + + /** @psalm-suppress RedundantCondition */ + if (class_exists(\Psalm\Internal\Analyzer\TypeAnalyzer::class, true) && method_exists(\Psalm\Internal\Analyzer\TypeAnalyzer::class, 'canBeContainedBy')) { return \Psalm\Internal\Analyzer\TypeAnalyzer::canBeContainedBy($codebase, $input_type, $container_type); - } else { - throw new UnsupportedPsalmVersion(); } + + throw new UnsupportedPsalmVersion(); } /** @@ -291,7 +297,10 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna assert($ret[0] instanceof Type\Union); assert($ret[1] instanceof Type\Union); return [$ret[0], $ret[1]]; - } elseif (class_exists(\Psalm\Internal\Analyzer\Statements\Block\ForeachAnalyzer::class, true) + } + + /** @psalm-suppress RedundantCondition */ + if (class_exists(\Psalm\Internal\Analyzer\Statements\Block\ForeachAnalyzer::class, true) && method_exists( \Psalm\Internal\Analyzer\Statements\Block\ForeachAnalyzer::class, 'getKeyValueParamsForTraversableObject' @@ -311,9 +320,9 @@ class TestCaseHandler implements AfterClassLikeVisitInterface, AfterClassLikeAna $iterable_key_type ?? Type::getMixed(), $iterable_value_type ?? Type::getMixed(), ]; - } else { - throw new UnsupportedPsalmVersion(); } + + throw new UnsupportedPsalmVersion(); } private static function unionizeIterables(Codebase $codebase, Type\Union $iterables): Type\Atomic\TIterable