Implement DataProviderDataRule #238

staabm · 2025-10-13T13:39:38Z

Proof of concept

<?php

use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\TestCase;

class test extends TestCase
{

	#[DataProvider('aProvider')]
	public function testTrim(string $expectedResult, string $input): void
	{
	}

	public function aProvider(): array /** @phpstan-ignore missingType.iterableValue */
	{
		return [
			[
				'Hello World',
				" Hello World \n",
			],
			[
				'Hello World',
				123,
			],
			[
				'Hello World',
				false,
			],
		];
	}
}

leads to

 ------ ----------------------------------------------------------------------------- 
  Line   test.php                                                                     
 ------ ----------------------------------------------------------------------------- 
  21     Parameter #2 $input of method test::testTrim() expects string, int given.    
         🪪  argument.type                                                            
         at test.php:21                                                               
  25     Parameter #2 $input of method test::testTrim() expects string, false given.  
         🪪  argument.type                                                            
         at test.php:25                                                               
 ------ -----------------------------------------------------------------------------

Closes #70

utilizes phpstan/phpstan-src#4429 phpstan/phpstan-src#4438

Scope

same class dataprovider only for now

Todos

tests/Rules/PHPUnit/data-provider-data.neon

staabm · 2025-10-14T06:06:10Z

Still work in progress. Will separate into more PRs after its mostly done

src/Rules/PHPUnit/DataProviderDataRule.php

…e precise error lines

ondrejmirtes

Also I pushed a bunch of failing tests 😊

ondrejmirtes · 2025-10-17T08:31:19Z

src/Rules/PHPUnit/TestMethodsHelper.php

+	}
+
+	/**
+	 * @return array<ReflectionMethod>


This is a weird type. I'd prefer our own ExtendedMethodReflection to be returned. You can get ClassReflection::getNativeMethod() once you make sure this is a test method from the current $reflectionMethod (to save time from instantiating all methods which is expensive).

I noticed this because I was looking for how you're handling variadic methods and the $testMethod->getNumberOfParameters() method call seemed unfamiliar to me.

from ExtendedMethodReflection I cannot getStartLine, which I need in DataProviderHelper

You mean there isn't getStartLine on AttributeReflection? I don't see getStartLine being called on ReflectionMethod.

see

phpstan-phpunit/src/Rules/PHPUnit/DataProviderHelper.php

Line 292 in 39950c7

$startLine = $node->getStartLine();

tests/Rules/PHPUnit/DataProviderDataRuleTest.php

ondrejmirtes · 2025-10-17T08:55:54Z

src/Rules/PHPUnit/DataProviderDataRule.php

+		}
+
+		$maxNumberOfParameters = 0;
+		$trimArgs = count($testsWithProvider) > 1;


I think in correct implementation, we don't need this condition at all. A foreach over methods that just iterates once should be enough to set everything correctly.

VincentLanglet

What about code like

    #[DataProvider('aProvider')]
	public function testTrim(string $expectedResult, string $input): void
	{
	}

    /** @return array<array{string, string|int|false}> */
	public function aProvider(): array
	{
		return [
			[
				'Hello World',
				" Hello World \n",
			],
			[
				'Hello World',
				123,
			],
			[
				'Hello World',
				false,
			],
		];
	}

@staabm ?

I would expect:

To not parse the implementation of the provider because I added a return type (which improves performance of the rule).
/** @return array<array{string, string|int|false}> */ to be reported because it's not compatible with the testTrim signature.

This way:

I fix the phpdoc to /** @return array<array{string, string}> */
And then, the 123 and false will be reported because they don't respect the @return type.

staabm · 2025-10-17T12:24:03Z

I think with the new rule, you no longer need a return type on the provider (which is now redundant). having to define the signature at the test and another time at the provider feels like unnecessary work

IMO at best we would stop reporting "return type has no value type specified in iterable type array." for data-providers which can be analyzed by this PR.

VincentLanglet · 2025-10-17T12:33:19Z

I think with the new rule, you no longer need a return type on the provider (which is now redundant). having to define the signature at the test and another time at the provider fells like unnecessary work

IMO at best we would stop reporting "return type has no value type specified in iterable type array." for data-providers which can be analyzed by this PR.

I feel like it's a really opinionated decision, and might conflict with some other static analysis tool.

I think psalm use the phpdoc from the dataprovider for his "similar" DataProviderDataRule.
DataProvider might be extended/overriden and then I'll want to add phpdoc to restrict/extends the return type.
Team could have a 100% typed policy (like we do)

Couldn't the DataProviderDataRule either

Skip provider with phpdoc (and another rule will do the check)
Or rely only on the phpdoc

This rule will

Cause some code like

    #[DataProvider('aProvider')]
	public function testTrim(string $expectedResult, string $input): void
	{
	}

    /** @return array<array{string, string|int}> */
	public function aProvider(): array
	{
		return [
			[
				'Hello World',
				" Hello World \n",
			],
		];
	}

Is not reported by the rule currently, but technically wrong (and reported by Psalm)

ondrejmirtes · 2025-10-17T12:50:21Z

@VincentLanglet I disagree with you but I'm on a phone and might write a proper response latee.

VincentLanglet · 2025-10-17T12:52:31Z

src/Rules/PHPUnit/DataProviderDataRule.php

+		$method = $scope->getFunction();
+		$classReflection = $scope->getClassReflection();
+		if (
+			$classReflection === null
+			|| !$classReflection->is(TestCase::class)
+		) {
+			return [];
+		}


Shouldn't this condition be done before buildArrayTypesFromNode to be faster ?

We might also prefer to check

if (count($testsWithProvider) === 0) { return []; }

before the buildArrayTypesFromNode call, it will avoid to inspect methods which are not dataprovider and improves performances.

I am not sure whether AST traversing + type retrieval is faster or slower then runtime reflection.
do you have a test at hand which to prove your thesis about what is faster?

I am not sure whether AST traversing + type retrieval is faster or slower then runtime reflection.
do you have a cast at hand which to prove your thesis about what is faster?

Not at all, it might be a wrong intuition.
I thought type manipulation was something slow, with possible union/intersection etc.

src/Rules/PHPUnit/DataProviderDataRule.php

VincentLanglet · 2025-10-17T13:13:30Z

src/Rules/PHPUnit/DataProviderDataRule.php

+		$testsWithProvider = [];
+		$testMethods = $this->testMethodsHelper->getTestMethods($classReflection, $scope);
+		foreach ($testMethods as $testMethod) {
+			foreach ($this->dataProviderHelper->getDataProviderMethods($scope, $testMethod, $classReflection) as [, $providerMethodName]) {


Do you think it would be worth having a cache on getDataProviderMethods (and maybe getTestMethods ?) ?

Cause we will compute this for every possible return of a TestCase class, and the result will always be the same

I am fine/willing to add a cache in case we find a case where it turns into a measurable improvement

ondrejmirtes · 2025-10-17T14:52:51Z

@VincentLanglet To reply to your earlier comment about this code:

    #[DataProvider('aProvider')]
	public function testTrim(string $expectedResult, string $input): void
	{
	}

    /** @return array<array{string, string|int}> */
	public function aProvider(): array
	{
		return [
			[
				'Hello World',
				" Hello World \n",
			],
		];
	}

Currently this code will be reported with return.nestedUnusedType (https://phpstan.org/r/3b73fe7f-361e-4f8e-a752-0cf8827db30e). Provided it's in a final class or you turn on checkTooWideReturnTypesInProtectedAndPublicMethods.

staabm · 2025-10-18T07:35:51Z

Also I pushed a bunch of failing tests 😊

fixed, thank you 🙏

ondrejmirtes · 2025-10-18T08:42:03Z

I played as Infection for a bit and found some code that could be removed from the algorithm 😊 I still think there's something fishy about it but I didn't prove it yet. In my gut I think we should be interesting in the min number of parameters when deciding how to trim, but maybe I'm wrong, or maybe I will find another failing test :) We'll see. Thank you so far!

staabm · 2025-10-18T10:18:30Z

In my gut I think we should be interesting in the min number of parameters when deciding how to trim

thats what you deleted with f083e63 ...? :)

staabm · 2025-10-18T10:40:05Z

I played as Infection for a bit

understood. finished the infection package, so we can setup it :)

phpstan/build-infection#1

staabm commented Oct 13, 2025

View reviewed changes

tests/Rules/PHPUnit/data-provider-data.neon Outdated Show resolved Hide resolved

staabm and others added 22 commits October 14, 2025 12:29

Implement DataProviderDataRule

c4597bf

report correct line

ccff992

Added DataProviderDataRuleTest

0975030

implement CompositeRule

5c6445a

cleanup

ff6843e

Implement reflection based dataprovider detection

1dd180c

cs

cff1f79

simplify

0ab211d

fix

7f7b38b

Refactor

d74aeec

test annotations

09b226b

support yield

f9c8af7

support yield from

d3a2b9b

fix

f42bd8a

Support test annotation and attribute

3318b73

extract PHPUnitVersionDetector

f715fbf

fix php 7.4

67a40d6

cs

6f4dcc9

bleeding edge only

20d1408

test too many/few arguments

5cb8c63

report too many/too few with when provider re-used

72d5655

cleanup

1453c20

clxmstaab force-pushed the data-provider branch from 4402d12 to 1453c20 Compare October 14, 2025 10:31

ondrejmirtes requested changes Oct 14, 2025

View reviewed changes

src/Rules/PHPUnit/DataProviderDataRule.php Outdated Show resolved Hide resolved

src/Rules/PHPUnit/DataProviderDataRule.php Outdated Show resolved Hide resolved

src/Rules/PHPUnit/DataProviderDataRule.php Outdated Show resolved Hide resolved

staabm added 4 commits October 14, 2025 13:46

Update extension.neon

1506a06

use types

9b69121

support named args

459eb1d

fix invalid call on null

5fcf01d

staabm added 3 commits October 17, 2025 07:20

factor out buildArrayTypesFromNode()

a1e5a19

test more const array variants

7f9dbf2

special case for providers only containing static data, so we get mor…

c689efe

…e precise error lines

staabm force-pushed the data-provider branch from c936d39 to c689efe Compare October 17, 2025 06:11

staabm and others added 4 commits October 17, 2025 08:14

apply static data only special case also for "yield from"

d6c11be

moved cheapest check to the front

21740cb

Failing test - args trimming logic needs some work

41acc04

Not a failing test, just checking :)

807a35f

ondrejmirtes requested changes Oct 17, 2025

View reviewed changes

VincentLanglet reviewed Oct 17, 2025

View reviewed changes

staabm added 3 commits October 17, 2025 16:03

separated php8 only tests

3fc4e0e

separate named args expectations

9a0d039

separated more named args tests

daf978e

staabm added 3 commits October 18, 2025 09:06

fix arg trimming

534694d

more fixes

58461d7

cs

b36a404

ondrejmirtes added 3 commits October 18, 2025 10:35

Manual mutation testing ep. 1

f083e63

Manual mutation testing ep. 2

42ad04e

break

0731026

Implement DataProviderDataRule #238

Are you sure you want to change the base?

Implement DataProviderDataRule #238

Uh oh!

Conversation

staabm commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

staabm commented Oct 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VincentLanglet left a comment

Choose a reason for hiding this comment

Uh oh!

staabm commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VincentLanglet commented Oct 17, 2025

Uh oh!

ondrejmirtes commented Oct 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Oct 17, 2025

Uh oh!

staabm commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Oct 18, 2025

Uh oh!

staabm commented Oct 18, 2025

Uh oh!

staabm commented Oct 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

staabm commented Oct 13, 2025 •

edited

Loading

staabm commented Oct 17, 2025 •

edited

Loading

staabm Oct 17, 2025 •

edited

Loading

staabm commented Oct 18, 2025 •

edited

Loading