Fix: check datasets of providers without a @return type

AGENTS.md

@@ -15,6 +15,10 @@ Layout:
   `AfterMethodCallAnalysis` hook and plugin issues that suggest `assertNull()`, `assertTrue()` /
   `assertFalse()` and `assertCount()` over `assertSame()`, and `assertSame()` over `assertEquals()`
   for two arguments of one scalar type.
+- `src/Hooks/ProviderReturnTypeHandler.php`: the `AfterFunctionLikeAnalysis` hook that keeps the type Psalm
+  infers for each dataset of a static provider with only a native return type (never the merged type of all
+  datasets). `TestCaseHandler` checks the datasets against it only for providers of the analysed class or its
+  traits (other storages may be analysed by another process).
 - `src/Hooks/MockMethodHandler.php`: the `AfterExpressionAnalysis` hook that reports
   `->method('typo')` on mocks and stubs as `UndefinedMethod`.
 - `src/Hooks/IntersectionMockReturnTypeProvider.php`: the `MethodReturnTypeProviderInterface` hook that types

README.md

@@ -35,6 +35,7 @@ vendor/bin/psalm-plugin enable psalm/plugin-phpunit
 
 * Properties initialized in `setUp()` or a `@before` / `#[Before]` method count as initialized, so no false `MissingConstructor`.
 * Data providers (`@dataProvider`, `#[DataProvider]`, `#[DataProviderExternal]`) are validated: the provider exists, is `public static` (`InvalidDataProvider`), returns an `iterable`, and its datasets match the parameters of the test method.
+* A provider with only a native return type (`: iterable`, `: array`, `: \Generator`) and no `@return` has each of its datasets (each `yield`, each element of a returned array literal) checked against the type Psalm infers for it, when the provider is a static method declared in the test case or in a trait it uses. Otherwise (a parent class, an external class, a body with `yield from`, an inferred type that is not an iterable, or datasets of an unknown type) the provider is reported as `MixedReturnStatement`, asking for a `@return` type.
 * The datasets of a literal closure in a `#[DataProviderClosure]` (PHPUnit 13.1 and newer, closures in attributes need PHP 8.5) are validated the same way, against the return type Psalm infers for the closure: `#[DataProviderClosure(static fn (): iterable => [['not an int']])]` on `testFoo(int $a)` is an `InvalidArgument`.
 * Datasets with string keys (`['cents' => 100, 'currency' => 'EUR']`) are matched to the test parameters by name. Unknown, already used and missing names are reported as `InvalidNamedArgument` and `TooFewArguments`, as is a string-keyed dataset on a test with a `@depends` / `#[Depends]` dependency that passes a value, because PHPUnit appends that value as a positional argument after the named ones. A dependency passes no value when its test is declared `void`. Before PHPUnit 12.5.40 and 13.4.3 (and on PHPUnit 11) PHPUnit stops at the first `void` dependency, so only the first dependency counts, and only when it is a method of the same class. Tests with a size group in a method docblock (`@small`, `@group medium` and so on) are then skipped, since PHPUnit may ignore the values of a larger dependency.
 * A positional entry after a named one in a literal dataset (`['b' => 'x', 0 => 1]`, which PHP rejects when PHPUnit unpacks it) is reported as `InvalidNamedArgument`.

src/Hooks/ProviderReturnTypeHandler.php

@@ -0,0 +1,179 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Psalm\PhpUnitPlugin\Hooks;
+
+use PhpParser\Node\Expr\YieldFrom;
+use PhpParser\Node\Expr\Yield_;
+use PhpParser\Node\Stmt\ClassMethod;
+use PhpParser\NodeFinder;
+use Psalm\Internal\Analyzer\FunctionLike\ReturnTypeCollector;
+use Psalm\Internal\Provider\NodeDataProvider;
+use Psalm\Internal\TypeVisitor\TypeVariableResolver;
+use Psalm\Plugin\EventHandler\AfterFunctionLikeAnalysisInterface;
+use Psalm\Plugin\EventHandler\Event\AfterFunctionLikeAnalysisEvent;
+use Psalm\Storage\MethodStorage;
+use Psalm\Type\Atomic\TGenericObject;
+use Psalm\Type\Atomic\TKeyedArray;
+use Psalm\Type\Atomic\TMixed;
+use Psalm\Type\Atomic\TNever;
+use Psalm\Type\Atomic\TNull;
+use Psalm\Type\TypeNode;
+use Psalm\Type\TypeVisitor;
+use Psalm\Type\Union;
+use WeakMap;
+
+/**
+ * A data provider with only a native return type (`iterable`, `array`, `Generator`) has no `@return` type to check
+ * its datasets against, so the datasets Psalm infers from the body of the provider are kept: one type for each
+ * `yield` and for each element of a returned array shape, so that datasets of different shapes are not merged.
+ *
+ * The storage of a method of a trait is shared by all the classes using it, and the types kept are the ones of the
+ * class analysed last. Only read them while a class is analysed that has just analysed the method itself.
+ */
+final class ProviderReturnTypeHandler implements AfterFunctionLikeAnalysisInterface
+{
+    /** @var WeakMap<object, list<Union>>|null */
+    private static ?WeakMap $inferredTypes = null;
+
+    /** Whether the return type of the provider is its native one, with no docblock to override it */
+    public static function hasNativeReturnTypeOnly(MethodStorage $storage): bool
+    {
+        return $storage->signature_return_type !== null
+            && !$storage->has_docblock_return_type
+            && $storage->inherited_return_type !== true;
+    }
+
+    /** @return list<Union>|null the type of each dataset, null when they could not be inferred */
+    public static function inferredTypes(MethodStorage $storage): ?array
+    {
+        return self::$inferredTypes[$storage] ?? null;
+    }
+
+    #[\Override]
+    public static function afterStatementAnalysis(AfterFunctionLikeAnalysisEvent $event): ?bool
+    {
+        $method  = $event->getStmt();
+        $storage = $event->getFunctionlikeStorage();
+        $nodes   = $event->getNodeTypeProvider();
+        if (
+            !$method instanceof ClassMethod
+            || !$method->isStatic()
+            || !$storage instanceof MethodStorage
+            || !$nodes instanceof NodeDataProvider
+            || !self::hasNativeReturnTypeOnly($storage)
+        ) {
+            return null;
+        }
+
+        if (self::$inferredTypes === null) {
+            /** @var WeakMap<object, list<Union>> $types */
+            $types               = new WeakMap();
+            self::$inferredTypes = $types;
+        }
+
+        unset(self::$inferredTypes[$storage]);
+
+        $codebase   = $event->getCodebase();
+        $statements = $method->stmts ?? [];
+
+        $finder = new NodeFinder();
+        // Psalm drops what a `yield from` yields
+        if ($finder->findFirstInstanceOf($statements, YieldFrom::class) !== null) {
+            return null;
+        }
+
+        $yield_types  = [];
+        $return_types = ReturnTypeCollector::getReturnTypes($codebase, $nodes, $statements, $yield_types);
+
+        $datasets = [];
+        if ($finder->findFirstInstanceOf($statements, Yield_::class) !== null) {
+            foreach ($yield_types as $yield_type) {
+                $generator = $yield_type->isSingle() ? $yield_type->getSingleAtomic() : null;
+                if (!$generator instanceof TGenericObject || $generator->value !== 'Generator') {
+                    return null;
+                }
+
+                $datasets[] = $yield_type;
+            }
+        } else {
+            foreach ($return_types as $return_type) {
+                foreach ($return_type->getAtomicTypes() as $atomic) {
+                    // `null` is an empty set of datasets, and a guard that throws or skips the test returns nothing
+                    if ($atomic instanceof TNull || $atomic instanceof TNever) {
+                        continue;
+                    }
+
+                    // Anything else than iterables is reported as a wrong return type, which is not known here
+                    if (!$atomic->isIterable($codebase)) {
+                        return null;
+                    }
+
+                    if (!$atomic instanceof TKeyedArray || $atomic->fallback_params !== null) {
+                        $datasets[] = new Union([$atomic]);
+                        continue;
+                    }
+
+                    foreach ($atomic->properties as $key => $dataset) {
+                        $datasets[] = new Union([
+                            TKeyedArray::make([$key => $dataset->setPossiblyUndefined(false)]),
+                        ]);
+                    }
+                }
+            }
+        }
+
+        if ($datasets === []) {
+            return null;
+        }
+
+        $resolver = new TypeVariableResolver($codebase);
+        foreach ($datasets as $offset => $dataset) {
+            $resolver->traverse($dataset);
+
+            // What is not known about the datasets is not reported, a @return type is asked for instead
+            if (self::hasMixed($dataset)) {
+                return null;
+            }
+
+            $datasets[$offset] = $dataset;
+        }
+
+        self::$inferredTypes[$storage] = $datasets;
+
+        return null;
+    }
+
+    private static function hasMixed(Union $type): bool
+    {
+        $visitor = new class extends TypeVisitor {
+            public bool $found = false;
+
+            /** @psalm-external-mutation-free */
+            #[\Override]
+            protected function enterNode(TypeNode $type): ?int
+            {
+                if ($type instanceof TMixed) {
+                    $this->found = true;
+
+                    return self::STOP_TRAVERSAL;
+                }
+
+                return null;
+            }
+        };
+
+        // The last two parameters of a Generator are not the ones of the datasets
+        foreach ($type->getAtomicTypes() as $atomic) {
+            if ($atomic instanceof TGenericObject && $atomic->value === 'Generator') {
+                $visitor->traverse($atomic->type_params[0]);
+                $visitor->traverse($atomic->type_params[1]);
+            } else {
+                $visitor->traverse($atomic);
+            }
+        }
+
+        return $visitor->found;
+    }
+}

src/Hooks/TestCaseHandler.php

@@ -396,7 +396,8 @@ public static function afterStatementAnalysis(AfterClassLikeAnalysisEvent $event
              *     type: Type\Union,
              *     location: CodeLocation,
              *     return_type_location: CodeLocation,
-             *     return_type_suppressions: array<array-key, string>
+             *     return_type_suppressions: array<array-key, string>,
+             *     hint: string
              * }> $provided_types
              */
             $provided_types = [];
@@ -486,27 +487,48 @@ public static function afterStatementAnalysis(AfterClassLikeAnalysisEvent $event
                     continue;
                 }
 
-                $provider_return_type = self::expandForMethod(
-                    $codebase,
-                    $provider_return_type,
-                    $apparent_provider_method_name,
-                    $provider['class'] ?? $class_storage->name,
-                );
+                $provider_hint  = '';
+                $provider_types = [$provider_return_type];
+                if (ProviderReturnTypeHandler::hasNativeReturnTypeOnly($provider_method_storage)) {
+                    // A trait method is analysed with each class using it, other methods only with their own class
+                    $provider_analysed_here = $provider_class_storage->is_trait
+                        ? isset($class_storage->used_traits[strtolower($provider_class_storage->name)])
+                        : $provider_class_storage->name === $class_storage->name;
+                    $inferred_types = $provider_analysed_here
+                        ? ProviderReturnTypeHandler::inferredTypes($provider_method_storage)
+                        : null;
+
+                    if ($inferred_types !== null) {
+                        $provider_types = $inferred_types;
+                    } else {
+                        $provider_hint = '; add a @return type to ' . $provider_method_id . '()';
+                    }
+                }
 
                 $provider_return_type_location = $codebase->getMethodReturnTypeLocation($provider_method_id);
                 assert(null !== $provider_return_type_location);
-                $provided_types[] = [
-                    'name'                 => $provider_method_id . '()',
-                    'type'                 => $provider_return_type,
-                    'location'             => $provider_docblock_location,
-                    'return_type_location' => $provider_return_type_location,
-                    'return_type_suppressions' => Suppressions::of(
-                        $provider_return_type_location,
-                        $provider_method_storage->suppressed_issues + $provider_class_storage->suppressed_issues,
-                        $class_storage,
-                        $statements_source,
-                    ),
-                ];
+                $return_type_suppressions = Suppressions::of(
+                    $provider_return_type_location,
+                    $provider_method_storage->suppressed_issues + $provider_class_storage->suppressed_issues,
+                    $class_storage,
+                    $statements_source,
+                );
+
+                foreach ($provider_types as $provider_type) {
+                    $provided_types[] = [
+                        'name'                 => $provider_method_id . '()',
+                        'type'                 => self::expandForMethod(
+                            $codebase,
+                            $provider_type,
+                            $apparent_provider_method_name,
+                            $provider['class'] ?? $class_storage->name,
+                        ),
+                        'location'             => $provider_docblock_location,
+                        'return_type_location' => $provider_return_type_location,
+                        'return_type_suppressions' => $return_type_suppressions,
+                        'hint' => $provider_hint,
+                    ];
+                }
             }
 
             foreach ($closure_providers as $closure) {
@@ -532,6 +554,7 @@ public static function afterStatementAnalysis(AfterClassLikeAnalysisEvent $event
                     'location'             => $closure_location,
                     'return_type_location' => $closure_location,
                     'return_type_suppressions' => $test_suppressions,
+                    'hint' => '',
                 ];
             }
 
@@ -548,6 +571,7 @@ public static function afterStatementAnalysis(AfterClassLikeAnalysisEvent $event
                 $provider_docblock_location    = $provided['location'];
                 $provider_return_type_location = $provided['return_type_location'];
                 $return_type_suppressions      = $provided['return_type_suppressions'];
+                $provider_hint                 = $provided['hint'];
                 $provider_return_type_string   = $provider_return_type->getId();
 
                 $expected_provider_return_type = new Type\Atomic\TIterable([
@@ -609,7 +633,7 @@ public static function afterStatementAnalysis(AfterClassLikeAnalysisEvent $event
                     if ($provider_key_type_is_uncertain) {
                         IssueBuffer::accepts(new Issue\MixedReturnStatement(
                             'Providers must return ' . $expected_provider_return_type->getId()
-                            . ', possibly different ' . $provider_return_type_string . ' provided',
+                            . ', possibly different ' . $provider_return_type_string . ' provided' . $provider_hint,
                             $provider_return_type_location
                         ), $return_type_suppressions);
                     } else {
@@ -631,7 +655,7 @@ public static function afterStatementAnalysis(AfterClassLikeAnalysisEvent $event
                     if ($provider_return_type->type_params[1]->hasMixed()) {
                         IssueBuffer::accepts(new Issue\MixedReturnStatement(
                             'Providers must return ' . $expected_provider_return_type->getId()
-                            . ', possibly different ' . $provider_return_type_string . ' provided',
+                            . ', possibly different ' . $provider_return_type_string . ' provided' . $provider_hint,
                             $provider_return_type_location
                         ), $return_type_suppressions);
                     } else {

src/Plugin.php

@@ -40,5 +40,8 @@ class_exists(Hooks\DataProviderNarrowingHandler::class, true);
 
         class_exists(Hooks\DataProviderClosureHandler::class, true);
         $psalm->registerHooksFromClass(Hooks\DataProviderClosureHandler::class);
+
+        class_exists(Hooks\ProviderReturnTypeHandler::class, true);
+        $psalm->registerHooksFromClass(Hooks\ProviderReturnTypeHandler::class);
     }
 }