-
Notifications
You must be signed in to change notification settings - Fork 50
Implement DataProviderDataRule #238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.0.x
Are you sure you want to change the base?
Changes from 54 commits
c4597bf
ccff992
0975030
5c6445a
ff6843e
1dd180c
cff1f79
0ab211d
7f7b38b
d74aeec
09b226b
f9c8af7
d3a2b9b
f42bd8a
3318b73
f715fbf
67a40d6
6f4dcc9
20d1408
5cb8c63
72d5655
1453c20
1506a06
9b69121
459eb1d
5fcf01d
a978aa8
a1dad84
1d3db54
7417fbc
74b67cf
5cb7312
9ee26b1
4cb9404
34a0a0e
bb855f0
a56169b
e31a234
1532c26
68e65e9
30e27db
aafaf8f
b6f11fe
72dc9e0
ccb7b04
8491dd7
aea7e42
a1e5a19
7f9dbf2
c689efe
d6c11be
21740cb
41acc04
807a35f
3fc4e0e
9a0d039
daf978e
534694d
58461d7
b36a404
f083e63
42ad04e
0731026
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,237 @@ | ||
<?php declare(strict_types = 1); | ||
|
||
namespace PHPStan\Rules\PHPUnit; | ||
|
||
use PhpParser\Node; | ||
use PHPStan\Analyser\Scope; | ||
use PHPStan\Node\Expr\TypeExpr; | ||
use PHPStan\Rules\Rule; | ||
use PHPStan\Type\ObjectType; | ||
use PHPStan\Type\Type; | ||
use PHPUnit\Framework\TestCase; | ||
use function array_slice; | ||
use function count; | ||
use function max; | ||
use function min; | ||
|
||
/** | ||
* @implements Rule<Node> | ||
*/ | ||
class DataProviderDataRule implements Rule | ||
{ | ||
|
||
private TestMethodsHelper $testMethodsHelper; | ||
|
||
private DataProviderHelper $dataProviderHelper; | ||
|
||
public function __construct( | ||
TestMethodsHelper $testMethodsHelper, | ||
DataProviderHelper $dataProviderHelper | ||
) | ||
{ | ||
$this->testMethodsHelper = $testMethodsHelper; | ||
$this->dataProviderHelper = $dataProviderHelper; | ||
} | ||
|
||
public function getNodeType(): string | ||
{ | ||
return Node::class; | ||
} | ||
|
||
public function processNode(Node $node, Scope $scope): array | ||
{ | ||
if ( | ||
!$node instanceof Node\Stmt\Return_ | ||
&& !$node instanceof Node\Expr\Yield_ | ||
&& !$node instanceof Node\Expr\YieldFrom | ||
) { | ||
return []; | ||
} | ||
|
||
if ($scope->getFunction() === null) { | ||
return []; | ||
} | ||
if ($scope->isInAnonymousFunction()) { | ||
return []; | ||
} | ||
|
||
$arraysTypes = $this->buildArrayTypesFromNode($node, $scope); | ||
if ($arraysTypes === []) { | ||
return []; | ||
} | ||
|
||
$method = $scope->getFunction(); | ||
$classReflection = $scope->getClassReflection(); | ||
if ( | ||
$classReflection === null | ||
|| !$classReflection->is(TestCase::class) | ||
) { | ||
return []; | ||
} | ||
Comment on lines
+63
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this condition be done before buildArrayTypesFromNode to be faster ? We might also prefer to check
before the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure whether AST traversing + type retrieval is faster or slower then runtime reflection. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not at all, it might be a wrong intuition. |
||
|
||
$testsWithProvider = []; | ||
$testMethods = $this->testMethodsHelper->getTestMethods($classReflection, $scope); | ||
foreach ($testMethods as $testMethod) { | ||
foreach ($this->dataProviderHelper->getDataProviderMethods($scope, $testMethod, $classReflection) as [, $providerMethodName]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it would be worth having a cache on Cause we will compute this for every possible return of a TestCase class, and the result will always be the same There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am fine/willing to add a cache in case we find a case where it turns into a measurable improvement |
||
if ($providerMethodName === $method->getName()) { | ||
$testsWithProvider[] = $testMethod; | ||
continue 2; | ||
} | ||
} | ||
} | ||
|
||
if (count($testsWithProvider) === 0) { | ||
return []; | ||
} | ||
|
||
$maxNumberOfParameters = 0; | ||
$trimArgs = count($testsWithProvider) > 1; | ||
|
||
foreach ($testsWithProvider as $testMethod) { | ||
$maxNumberOfParameters = max($maxNumberOfParameters, $testMethod->getNumberOfParameters()); | ||
} | ||
|
||
foreach ($testsWithProvider as $testMethod) { | ||
$numberOfParameters = $testMethod->getNumberOfParameters(); | ||
|
||
foreach ($arraysTypes as [$startLine, $arraysType]) { | ||
$args = $this->arrayItemsToArgs($arraysType, $numberOfParameters); | ||
if ($args === null) { | ||
continue; | ||
} | ||
|
||
if ($trimArgs && $maxNumberOfParameters !== $numberOfParameters) { | ||
$args = array_slice($args, 0, min($numberOfParameters, $maxNumberOfParameters)); | ||
} | ||
|
||
$scope->invokeNodeCallback(new Node\Expr\MethodCall( | ||
new TypeExpr(new ObjectType($classReflection->getName())), | ||
$testMethod->getName(), | ||
$args, | ||
['startLine' => $startLine], | ||
)); | ||
} | ||
} | ||
|
||
return []; | ||
} | ||
|
||
/** | ||
* @return array<Node\Arg> | ||
*/ | ||
private function arrayItemsToArgs(Type $array, int $numberOfParameters): ?array | ||
{ | ||
$args = []; | ||
|
||
$constArrays = $array->getConstantArrays(); | ||
if ($constArrays !== [] && count($constArrays) === 1) { | ||
$keyTypes = $constArrays[0]->getKeyTypes(); | ||
$valueTypes = $constArrays[0]->getValueTypes(); | ||
} elseif ($array->isArray()->yes()) { | ||
$keyTypes = []; | ||
$valueTypes = []; | ||
for ($i = 0; $i < $numberOfParameters; ++$i) { | ||
$keyTypes[$i] = $array->getIterableKeyType(); | ||
$valueTypes[$i] = $array->getIterableValueType(); | ||
} | ||
} else { | ||
return null; | ||
} | ||
|
||
foreach ($valueTypes as $i => $valueType) { | ||
$key = $keyTypes[$i]->getConstantStrings(); | ||
if (count($key) > 1) { | ||
return null; | ||
} | ||
|
||
if (count($key) === 0) { | ||
$arg = new Node\Arg(new TypeExpr($valueType)); | ||
$args[] = $arg; | ||
continue; | ||
|
||
} | ||
|
||
$arg = new Node\Arg( | ||
new TypeExpr($valueType), | ||
false, | ||
false, | ||
[], | ||
new Node\Identifier($key[0]->getValue()), | ||
); | ||
$args[] = $arg; | ||
} | ||
|
||
return $args; | ||
} | ||
|
||
/** | ||
* @param Node\Stmt\Return_|Node\Expr\Yield_|Node\Expr\YieldFrom $node | ||
* | ||
* @return list<list{int, Type}> | ||
*/ | ||
private function buildArrayTypesFromNode(Node $node, Scope $scope): array | ||
{ | ||
$arraysTypes = []; | ||
|
||
// special case for providers only containing static data, so we get more precise error lines | ||
if ( | ||
($node instanceof Node\Stmt\Return_ && $node->expr instanceof Node\Expr\Array_) | ||
|| ($node instanceof Node\Expr\YieldFrom && $node->expr instanceof Node\Expr\Array_) | ||
) { | ||
foreach ($node->expr->items as $item) { | ||
if (!$item->value instanceof Node\Expr\Array_) { | ||
$arraysTypes = []; | ||
break; | ||
} | ||
|
||
$constArrays = $scope->getType($item->value)->getConstantArrays(); | ||
if ($constArrays === []) { | ||
$arraysTypes = []; | ||
break; | ||
} | ||
|
||
foreach ($constArrays as $constArray) { | ||
$arraysTypes[] = [$item->value->getStartLine(), $constArray]; | ||
} | ||
} | ||
|
||
if ($arraysTypes !== []) { | ||
return $arraysTypes; | ||
} | ||
} | ||
|
||
// general case with less precise error message lines | ||
if ($node instanceof Node\Stmt\Return_ || $node instanceof Node\Expr\YieldFrom) { | ||
if ($node->expr === null) { | ||
return []; | ||
} | ||
|
||
$exprType = $scope->getType($node->expr); | ||
$exprConstArrays = $exprType->getConstantArrays(); | ||
foreach ($exprConstArrays as $constArray) { | ||
foreach ($constArray->getValueTypes() as $valueType) { | ||
foreach ($valueType->getConstantArrays() as $constValueArray) { | ||
$arraysTypes[] = [$node->getStartLine(), $constValueArray]; | ||
} | ||
} | ||
} | ||
|
||
if ($arraysTypes === []) { | ||
foreach ($exprType->getIterableValueType()->getArrays() as $arrayType) { | ||
$arraysTypes[] = [$node->getStartLine(), $arrayType]; | ||
} | ||
} | ||
} elseif ($node instanceof Node\Expr\Yield_) { | ||
if ($node->value === null) { | ||
return []; | ||
} | ||
|
||
$exprType = $scope->getType($node->value); | ||
foreach ($exprType->getConstantArrays() as $constValueArray) { | ||
$arraysTypes[] = [$node->getStartLine(), $constValueArray]; | ||
} | ||
} | ||
|
||
return $arraysTypes; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,94 @@ | ||||
<?php declare(strict_types = 1); | ||||
|
||||
namespace PHPStan\Rules\PHPUnit; | ||||
|
||||
use PHPStan\Analyser\Scope; | ||||
use PHPStan\PhpDoc\ResolvedPhpDocBlock; | ||||
use PHPStan\Reflection\ClassReflection; | ||||
use PHPStan\Type\FileTypeMapper; | ||||
use ReflectionMethod; | ||||
use function str_starts_with; | ||||
use function strtolower; | ||||
|
||||
final class TestMethodsHelper | ||||
{ | ||||
|
||||
private FileTypeMapper $fileTypeMapper; | ||||
|
||||
private bool $phpunit10OrNewer; | ||||
|
||||
public function __construct( | ||||
FileTypeMapper $fileTypeMapper, | ||||
bool $phpunit10OrNewer | ||||
) | ||||
{ | ||||
$this->fileTypeMapper = $fileTypeMapper; | ||||
$this->phpunit10OrNewer = $phpunit10OrNewer; | ||||
} | ||||
|
||||
/** | ||||
* @return array<ReflectionMethod> | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a weird type. I'd prefer our own ExtendedMethodReflection to be returned. You can get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed this because I was looking for how you're handling variadic methods and the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean there isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see
|
||||
*/ | ||||
public function getTestMethods(ClassReflection $classReflection, Scope $scope): array | ||||
{ | ||||
$testMethods = []; | ||||
foreach ($classReflection->getNativeReflection()->getMethods() as $reflectionMethod) { | ||||
if (!$reflectionMethod->isPublic()) { | ||||
continue; | ||||
} | ||||
|
||||
if (str_starts_with(strtolower($reflectionMethod->getName()), 'test')) { | ||||
$testMethods[] = $reflectionMethod; | ||||
continue; | ||||
} | ||||
|
||||
$docComment = $reflectionMethod->getDocComment(); | ||||
if ($docComment !== false) { | ||||
$methodPhpDoc = $this->fileTypeMapper->getResolvedPhpDoc( | ||||
$scope->getFile(), | ||||
$classReflection->getName(), | ||||
$scope->isInTrait() ? $scope->getTraitReflection()->getName() : null, | ||||
$reflectionMethod->getName(), | ||||
$docComment, | ||||
); | ||||
|
||||
if ($this->hasTestAnnotation($methodPhpDoc)) { | ||||
$testMethods[] = $reflectionMethod; | ||||
continue; | ||||
} | ||||
} | ||||
|
||||
if (!$this->phpunit10OrNewer) { | ||||
continue; | ||||
} | ||||
|
||||
$testAttributes = $reflectionMethod->getAttributes('PHPUnit\Framework\Attributes\Test'); // @phpstan-ignore argument.type | ||||
if ($testAttributes === []) { | ||||
continue; | ||||
} | ||||
|
||||
$testMethods[] = $reflectionMethod; | ||||
} | ||||
|
||||
return $testMethods; | ||||
} | ||||
|
||||
private function hasTestAnnotation(?ResolvedPhpDocBlock $phpDoc): bool | ||||
{ | ||||
if ($phpDoc === null) { | ||||
return false; | ||||
} | ||||
|
||||
$phpDocNodes = $phpDoc->getPhpDocNodes(); | ||||
|
||||
foreach ($phpDocNodes as $docNode) { | ||||
$tags = $docNode->getTagsByName('@test'); | ||||
if ($tags !== []) { | ||||
return true; | ||||
} | ||||
} | ||||
|
||||
return false; | ||||
} | ||||
|
||||
} |
Uh oh!
There was an error while loading. Please reload this page.