diff --git a/extension.neon b/extension.neon index 863d69ae..2d7230bb 100644 --- a/extension.neon +++ b/extension.neon @@ -169,6 +169,11 @@ services: descendIntoOtherMethods: %doctrine.searchOtherMethodsForQueryBuilderBeginning% parser: @defaultAnalysisParser + - + class: PHPStan\Type\Doctrine\QueryBuilder\ReturnQueryBuilderExpressionTypeResolverExtension + tags: + - phpstan.broker.expressionTypeResolverExtension + - class: PHPStan\Stubs\Doctrine\StubFilesExtensionLoader tags: diff --git a/rules.neon b/rules.neon index 07d41f2d..7545b956 100644 --- a/rules.neon +++ b/rules.neon @@ -35,7 +35,6 @@ services: class: PHPStan\Rules\Doctrine\ORM\QueryBuilderDqlRule arguments: reportDynamicQueryBuilders: %doctrine.reportDynamicQueryBuilders% - searchOtherMethodsForQueryBuilderBeginning: %doctrine.searchOtherMethodsForQueryBuilderBeginning% tags: - phpstan.rules.rule - diff --git a/src/Rules/Doctrine/ORM/QueryBuilderDqlRule.php b/src/Rules/Doctrine/ORM/QueryBuilderDqlRule.php index a3186f60..5e0030cd 100644 --- a/src/Rules/Doctrine/ORM/QueryBuilderDqlRule.php +++ b/src/Rules/Doctrine/ORM/QueryBuilderDqlRule.php @@ -12,7 +12,6 @@ use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\Doctrine\DoctrineTypeUtils; use PHPStan\Type\Doctrine\ObjectMetadataResolver; -use PHPStan\Type\Doctrine\QueryBuilder\OtherMethodQueryBuilderParser; use PHPStan\Type\ObjectType; use PHPStan\Type\TypeUtils; use Throwable; @@ -32,23 +31,13 @@ class QueryBuilderDqlRule implements Rule /** @var bool */ private $reportDynamicQueryBuilders; - /** @var OtherMethodQueryBuilderParser */ - private $otherMethodQueryBuilderParser; - - /** @var bool */ - private $searchOtherMethodsForQueryBuilderBeginning; - public function __construct( ObjectMetadataResolver $objectMetadataResolver, - OtherMethodQueryBuilderParser $otherMethodQueryBuilderParser, - bool $reportDynamicQueryBuilders, - bool $searchOtherMethodsForQueryBuilderBeginning + bool $reportDynamicQueryBuilders ) { $this->objectMetadataResolver = $objectMetadataResolver; - $this->otherMethodQueryBuilderParser = $otherMethodQueryBuilderParser; $this->reportDynamicQueryBuilders = $reportDynamicQueryBuilders; - $this->searchOtherMethodsForQueryBuilderBeginning = $searchOtherMethodsForQueryBuilderBeginning; } public function getNodeType(): string @@ -69,14 +58,6 @@ public function processNode(Node $node, Scope $scope): array $calledOnType = $scope->getType($node->var); $queryBuilderTypes = DoctrineTypeUtils::getQueryBuilderTypes($calledOnType); if (count($queryBuilderTypes) === 0) { - - if ($this->searchOtherMethodsForQueryBuilderBeginning) { - $queryBuilderTypes = $this->otherMethodQueryBuilderParser->getQueryBuilderTypes($scope, $node); - if (count($queryBuilderTypes) !== 0) { - return []; - } - } - if ( $this->reportDynamicQueryBuilders && (new ObjectType('Doctrine\ORM\QueryBuilder'))->isSuperTypeOf($calledOnType)->yes() diff --git a/src/Type/Doctrine/QueryBuilder/OtherMethodQueryBuilderParser.php b/src/Type/Doctrine/QueryBuilder/OtherMethodQueryBuilderParser.php index 9e08de40..fccb8fdd 100644 --- a/src/Type/Doctrine/QueryBuilder/OtherMethodQueryBuilderParser.php +++ b/src/Type/Doctrine/QueryBuilder/OtherMethodQueryBuilderParser.php @@ -3,8 +3,6 @@ namespace PHPStan\Type\Doctrine\QueryBuilder; use PhpParser\Node; -use PhpParser\Node\Expr\MethodCall; -use PhpParser\Node\Identifier; use PhpParser\Node\Stmt; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassMethod; @@ -17,13 +15,12 @@ use PHPStan\Analyser\ScopeFactory; use PHPStan\DependencyInjection\Container; use PHPStan\Parser\Parser; -use PHPStan\Reflection\ReflectionProvider; +use PHPStan\Reflection\MethodReflection; use PHPStan\Type\Generic\TemplateTypeMap; use PHPStan\Type\IntersectionType; use PHPStan\Type\Type; use PHPStan\Type\TypeTraverser; use PHPStan\Type\UnionType; -use function count; use function is_array; class OtherMethodQueryBuilderParser @@ -32,61 +29,28 @@ class OtherMethodQueryBuilderParser /** @var bool */ private $descendIntoOtherMethods; - /** @var ReflectionProvider */ - private $reflectionProvider; - /** @var Parser */ private $parser; /** @var Container */ private $container; - public function __construct(bool $descendIntoOtherMethods, ReflectionProvider $reflectionProvider, Parser $parser, Container $container) + public function __construct(bool $descendIntoOtherMethods, Parser $parser, Container $container) { $this->descendIntoOtherMethods = $descendIntoOtherMethods; - $this->reflectionProvider = $reflectionProvider; $this->parser = $parser; $this->container = $container; } /** - * @return QueryBuilderType[] + * @return list */ - public function getQueryBuilderTypes(Scope $scope, MethodCall $methodCall): array + public function findQueryBuilderTypesInCalledMethod(Scope $scope, MethodReflection $methodReflection): array { - if (!$this->descendIntoOtherMethods || !$methodCall->var instanceof MethodCall) { - return []; - } - - return $this->findQueryBuilderTypesInCalledMethod($scope, $methodCall->var); - } - /** - * @return QueryBuilderType[] - */ - private function findQueryBuilderTypesInCalledMethod(Scope $scope, MethodCall $methodCall): array - { - $methodCalledOnType = $scope->getType($methodCall->var); - if (!$methodCall->name instanceof Identifier) { - return []; - } - - $methodCalledOnTypeClassNames = $methodCalledOnType->getObjectClassNames(); - - if (count($methodCalledOnTypeClassNames) !== 1) { - return []; - } - - if (!$this->reflectionProvider->hasClass($methodCalledOnTypeClassNames[0])) { - return []; - } - - $classReflection = $this->reflectionProvider->getClass($methodCalledOnTypeClassNames[0]); - $methodName = $methodCall->name->toString(); - if (!$classReflection->hasNativeMethod($methodName)) { + if (!$this->descendIntoOtherMethods) { return []; } - $methodReflection = $classReflection->getNativeMethod($methodName); $fileName = $methodReflection->getDeclaringClass()->getFileName(); if ($fileName === null) { return []; diff --git a/src/Type/Doctrine/QueryBuilder/QueryBuilderGetQueryDynamicReturnTypeExtension.php b/src/Type/Doctrine/QueryBuilder/QueryBuilderGetQueryDynamicReturnTypeExtension.php index d2f8fd55..1d0a1809 100644 --- a/src/Type/Doctrine/QueryBuilder/QueryBuilderGetQueryDynamicReturnTypeExtension.php +++ b/src/Type/Doctrine/QueryBuilder/QueryBuilderGetQueryDynamicReturnTypeExtension.php @@ -66,22 +66,17 @@ class QueryBuilderGetQueryDynamicReturnTypeExtension implements DynamicMethodRet /** @var DescriptorRegistry */ private $descriptorRegistry; - /** @var OtherMethodQueryBuilderParser */ - private $otherMethodQueryBuilderParser; - public function __construct( ObjectMetadataResolver $objectMetadataResolver, ArgumentsProcessor $argumentsProcessor, ?string $queryBuilderClass, - DescriptorRegistry $descriptorRegistry, - OtherMethodQueryBuilderParser $otherMethodQueryBuilderParser + DescriptorRegistry $descriptorRegistry ) { $this->objectMetadataResolver = $objectMetadataResolver; $this->argumentsProcessor = $argumentsProcessor; $this->queryBuilderClass = $queryBuilderClass; $this->descriptorRegistry = $descriptorRegistry; - $this->otherMethodQueryBuilderParser = $otherMethodQueryBuilderParser; } public function getClass(): string @@ -108,10 +103,7 @@ public function getTypeFromMethodCall( )->getReturnType(); $queryBuilderTypes = DoctrineTypeUtils::getQueryBuilderTypes($calledOnType); if (count($queryBuilderTypes) === 0) { - $queryBuilderTypes = $this->otherMethodQueryBuilderParser->getQueryBuilderTypes($scope, $methodCall); - if (count($queryBuilderTypes) === 0) { - return $defaultReturnType; - } + return $defaultReturnType; } $objectManager = $this->objectMetadataResolver->getObjectManager(); diff --git a/src/Type/Doctrine/QueryBuilder/QueryBuilderMethodDynamicReturnTypeExtension.php b/src/Type/Doctrine/QueryBuilder/QueryBuilderMethodDynamicReturnTypeExtension.php index 32b437fa..cb76ba29 100644 --- a/src/Type/Doctrine/QueryBuilder/QueryBuilderMethodDynamicReturnTypeExtension.php +++ b/src/Type/Doctrine/QueryBuilder/QueryBuilderMethodDynamicReturnTypeExtension.php @@ -26,16 +26,11 @@ class QueryBuilderMethodDynamicReturnTypeExtension implements DynamicMethodRetur /** @var string|null */ private $queryBuilderClass; - /** @var OtherMethodQueryBuilderParser */ - private $otherMethodQueryBuilderParser; - public function __construct( - ?string $queryBuilderClass, - OtherMethodQueryBuilderParser $otherMethodQueryBuilderParser + ?string $queryBuilderClass ) { $this->queryBuilderClass = $queryBuilderClass; - $this->otherMethodQueryBuilderParser = $otherMethodQueryBuilderParser; } public function getClass(): string @@ -74,10 +69,7 @@ public function getTypeFromMethodCall( $queryBuilderTypes = DoctrineTypeUtils::getQueryBuilderTypes($calledOnType); if (count($queryBuilderTypes) === 0) { - $queryBuilderTypes = $this->otherMethodQueryBuilderParser->getQueryBuilderTypes($scope, $methodCall); - if (count($queryBuilderTypes) === 0) { - return $calledOnType; - } + return $calledOnType; } if (count($queryBuilderTypes) > self::MAX_COMBINATIONS) { diff --git a/src/Type/Doctrine/QueryBuilder/ReturnQueryBuilderExpressionTypeResolverExtension.php b/src/Type/Doctrine/QueryBuilder/ReturnQueryBuilderExpressionTypeResolverExtension.php new file mode 100644 index 00000000..2f780f22 --- /dev/null +++ b/src/Type/Doctrine/QueryBuilder/ReturnQueryBuilderExpressionTypeResolverExtension.php @@ -0,0 +1,103 @@ +otherMethodQueryBuilderParser = $otherMethodQueryBuilderParser; + } + + public function getType(Expr $expr, Scope $scope): ?Type + { + if (!$expr instanceof MethodCall && !$expr instanceof StaticCall) { + return null; + } + + if ($expr->isFirstClassCallable()) { + return null; + } + + $methodReflection = $this->getMethodReflection($expr, $scope); + + if ($methodReflection === null) { + return null; + } + + $returnType = ParametersAcceptorSelector::selectFromArgs($scope, $expr->getArgs(), $methodReflection->getVariants())->getReturnType(); + + $returnsQueryBuilder = (new ObjectType(QueryBuilder::class))->isSuperTypeOf($returnType)->yes(); + + if (!$returnsQueryBuilder) { + return null; + } + + $queryBuilderTypes = $this->otherMethodQueryBuilderParser->findQueryBuilderTypesInCalledMethod($scope, $methodReflection); + if (count($queryBuilderTypes) === 0) { + return null; + } + + return TypeCombinator::union(...$queryBuilderTypes); + } + + /** + * @param StaticCall|MethodCall $call + */ + private function getMethodReflection(CallLike $call, Scope $scope): ?MethodReflection + { + if (!$call->name instanceof Identifier) { + return null; + } + + if ($call instanceof MethodCall) { + $callerType = $scope->getType($call->var); + } else { + if (!$call->class instanceof Name) { + return null; + } + $callerType = $scope->resolveTypeByName($call->class); + } + + $methodName = $call->name->name; + + foreach ($callerType->getObjectClassReflections() as $callerClassReflection) { + if ($callerClassReflection->is(QueryBuilder::class)) { + return null; // covered by QueryBuilderMethodDynamicReturnTypeExtension + } + if ($callerClassReflection->is(EntityRepository::class) && $methodName === 'createQueryBuilder') { + return null; // covered by EntityRepositoryCreateQueryBuilderDynamicReturnTypeExtension + } + if ($callerClassReflection->is(EntityManagerInterface::class) && $methodName === 'createQueryBuilder') { + return null; // no need to dive there + } + } + + return $scope->getMethodReflection($callerType, $methodName); + } + +} diff --git a/tests/Rules/Doctrine/ORM/QueryBuilderDqlRuleSlowTest.php b/tests/Rules/Doctrine/ORM/QueryBuilderDqlRuleSlowTest.php index 655b89c7..7c4f4d40 100644 --- a/tests/Rules/Doctrine/ORM/QueryBuilderDqlRuleSlowTest.php +++ b/tests/Rules/Doctrine/ORM/QueryBuilderDqlRuleSlowTest.php @@ -5,7 +5,6 @@ use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; use PHPStan\Type\Doctrine\ObjectMetadataResolver; -use PHPStan\Type\Doctrine\QueryBuilder\OtherMethodQueryBuilderParser; /** * @extends RuleTestCase @@ -17,8 +16,6 @@ protected function getRule(): Rule { return new QueryBuilderDqlRule( new ObjectMetadataResolver(__DIR__ . '/entity-manager.php', __DIR__ . '/../../../../tmp'), - self::getContainer()->getByType(OtherMethodQueryBuilderParser::class), - true, true ); } diff --git a/tests/Rules/Doctrine/ORM/QueryBuilderDqlRuleTest.php b/tests/Rules/Doctrine/ORM/QueryBuilderDqlRuleTest.php index 67b94c8f..37124635 100644 --- a/tests/Rules/Doctrine/ORM/QueryBuilderDqlRuleTest.php +++ b/tests/Rules/Doctrine/ORM/QueryBuilderDqlRuleTest.php @@ -5,7 +5,6 @@ use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; use PHPStan\Type\Doctrine\ObjectMetadataResolver; -use PHPStan\Type\Doctrine\QueryBuilder\OtherMethodQueryBuilderParser; /** * @extends RuleTestCase @@ -17,8 +16,6 @@ protected function getRule(): Rule { return new QueryBuilderDqlRule( new ObjectMetadataResolver(__DIR__ . '/entity-manager.php', __DIR__ . '/../../../../tmp'), - self::getContainer()->getByType(OtherMethodQueryBuilderParser::class), - true, true ); } diff --git a/tests/Rules/Doctrine/ORM/data/query-builder-dql.php b/tests/Rules/Doctrine/ORM/data/query-builder-dql.php index 2f6f5953..49aa95f2 100644 --- a/tests/Rules/Doctrine/ORM/data/query-builder-dql.php +++ b/tests/Rules/Doctrine/ORM/data/query-builder-dql.php @@ -84,14 +84,14 @@ public function selectArray(): void ->getQuery(); } - public function analyseQueryBuilderUnknownBeginning(): void + public function analyseQueryBuilderOtherMethodBeginning(): void { $this->createQb()->getQuery(); } private function createQb(): \Doctrine\ORM\QueryBuilder { - return $this->entityManager->createQueryBuilder(); + return $this->entityManager->createQueryBuilder()->select('e')->from(MyEntity::class, 'e'); } public function analyseQueryBuilderDynamicArgs(string $entity): void diff --git a/tests/Type/Doctrine/QueryBuilder/ReturnQueryBuilderExpressionTypeResolverExtensionTest.php b/tests/Type/Doctrine/QueryBuilder/ReturnQueryBuilderExpressionTypeResolverExtensionTest.php new file mode 100644 index 00000000..e62fd054 --- /dev/null +++ b/tests/Type/Doctrine/QueryBuilder/ReturnQueryBuilderExpressionTypeResolverExtensionTest.php @@ -0,0 +1,35 @@ + */ + public function dataFileAsserts(): iterable + { + yield from $this->gatherAssertTypes(__DIR__ . '/../data/QueryResult/queryBuilderExpressionTypeResolver.php'); + } + + /** + * @dataProvider dataFileAsserts + * @param mixed ...$args + */ + public function testFileAsserts( + string $assertType, + string $file, + ...$args + ): void + { + $this->assertFileAsserts($assertType, $file, ...$args); + } + + /** @return string[] */ + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/../data/QueryResult/config.neon']; + } + +} diff --git a/tests/Type/Doctrine/data/QueryResult/queryBuilderExpressionTypeResolver.php b/tests/Type/Doctrine/data/QueryResult/queryBuilderExpressionTypeResolver.php new file mode 100644 index 00000000..9fe7bcd1 --- /dev/null +++ b/tests/Type/Doctrine/data/QueryResult/queryBuilderExpressionTypeResolver.php @@ -0,0 +1,110 @@ += 8.1 + +namespace QueryResult\CreateQuery; + +use Doctrine\Common\Collections\Criteria; +use Doctrine\ORM\AbstractQuery; +use Doctrine\ORM\EntityManagerInterface; +use Doctrine\ORM\EntityRepository; +use Doctrine\ORM\Query\Expr\Andx; +use Doctrine\ORM\Query\Expr\From; +use Doctrine\ORM\QueryBuilder; +use QueryResult\Entities\Many; +use function PHPStan\Testing\assertType; + +class QueryBuilderExpressionTypeResolverTest +{ + + /** + * @var MyRepository + */ + private $myRepository; + + public function testQueryTypeIsInferredOnAcrossMethods(EntityManagerInterface $em): void + { + $query = $this->getQueryBuilder($em)->getQuery(); + $branchingQuery = $this->getBranchingQueryBuilder($em)->getQuery(); + + assertType('Doctrine\ORM\Query', $query); + assertType('Doctrine\ORM\Query', $branchingQuery); + } + + public function testQueryTypeIsInferredOnAcrossMethodsEvenWhenVariableAssignmentIsUsed(EntityManagerInterface $em): void + { + $queryBuilder = $this->getQueryBuilder($em); + + assertType('Doctrine\ORM\Query', $queryBuilder->getQuery()); + } + + public function testQueryBuilderPassedElsewhereNotTracked(EntityManagerInterface $em): void + { + $queryBuilder = $this->getQueryBuilder($em); + $queryBuilder->indexBy('m', 'm.stringColumn'); + + $this->adjustQueryBuilderToIndexByInt($queryBuilder); + + assertType('Doctrine\ORM\Query', $queryBuilder->getQuery()); + } + + public function testDiveIntoCustomEntityRepository(EntityManagerInterface $em): void + { + $queryBuilder = $this->myRepository->getCustomQueryBuilder($em); + + assertType('Doctrine\ORM\Query', $queryBuilder->getQuery()); + } + + + public function testStaticCallWorksToo(EntityManagerInterface $em): void + { + $queryBuilder = self::getStaticQueryBuilder($em); + + assertType('Doctrine\ORM\Query', $queryBuilder->getQuery()); + } + + public function testFirstClassCallableDoesNotFail(EntityManagerInterface $em): void + { + $this->getQueryBuilder(...); + } + + private function adjustQueryBuilderToIndexByInt(QueryBuilder $qb): void + { + $qb->indexBy('m', 'm.intColumn'); + } + + private function getQueryBuilder(EntityManagerInterface $em): QueryBuilder + { + return $em->createQueryBuilder() + ->select('m') + ->from(Many::class, 'm'); + } + + private static function getStaticQueryBuilder(EntityManagerInterface $em): QueryBuilder + { + return $em->createQueryBuilder() + ->select('m') + ->from(Many::class, 'm'); + } + + private function getBranchingQueryBuilder(EntityManagerInterface $em): QueryBuilder + { + $queryBuilder = $em->createQueryBuilder() + ->select('m') + ->from(Many::class, 'm'); + + if (random_int(0, 1) === 1) { + $queryBuilder->andWhere('m.intColumn = 1'); + } + + return $queryBuilder; + } +} + +class MyRepository extends EntityRepository { + + private function getCustomQueryBuilder(EntityManagerInterface $em): QueryBuilder + { + return $em->createQueryBuilder() + ->select('m') + ->from(Many::class, 'm'); + } +} diff --git a/tests/Type/Doctrine/data/QueryResult/queryBuilderGetQuery.php b/tests/Type/Doctrine/data/QueryResult/queryBuilderGetQuery.php index e9c63efb..d4357def 100644 --- a/tests/Type/Doctrine/data/QueryResult/queryBuilderGetQuery.php +++ b/tests/Type/Doctrine/data/QueryResult/queryBuilderGetQuery.php @@ -233,6 +233,7 @@ public function testDynamicMethodCall( assertType('mixed', $result); } + /** * @param class-string $many */ @@ -276,33 +277,4 @@ public function testNonEntityClassString(EntityManagerInterface $em, string $cla assertType('mixed', $result); } - public function testQueryTypeIsInferredOnAcrossMethods(EntityManagerInterface $em): void - { - $query = $this->getQueryBuilder($em)->getQuery(); - $branchingQuery = $this->getBranchingQueryBuilder($em)->getQuery(); - - assertType('Doctrine\ORM\Query', $query); - assertType('Doctrine\ORM\Query', $branchingQuery); - } - - private function getQueryBuilder(EntityManagerInterface $em): QueryBuilder - { - return $em->createQueryBuilder() - ->select('m') - ->from(Many::class, 'm'); - } - - private function getBranchingQueryBuilder(EntityManagerInterface $em): QueryBuilder - { - $queryBuilder = $em->createQueryBuilder() - ->select('m') - ->from(Many::class, 'm'); - - if (random_int(0, 1) === 1) { - $queryBuilder->andWhere('m.intColumn = 1'); - } - - return $queryBuilder; - } - }