Skip to content

Infer QueryBuilderType for any method returning QueryBuilder: revive with cache #518

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

Merged
merged 2 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions extension.neon
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
1 change: 0 additions & 1 deletion rules.neon
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ services:
class: PHPStan\Rules\Doctrine\ORM\QueryBuilderDqlRule
arguments:
reportDynamicQueryBuilders: %doctrine.reportDynamicQueryBuilders%
searchOtherMethodsForQueryBuilderBeginning: %doctrine.searchOtherMethodsForQueryBuilderBeginning%
tags:
- phpstan.rules.rule
-
Expand Down
21 changes: 1 addition & 20 deletions src/Rules/Doctrine/ORM/QueryBuilderDqlRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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()
Expand Down
58 changes: 16 additions & 42 deletions src/Type/Doctrine/QueryBuilder/OtherMethodQueryBuilderParser.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -32,73 +29,48 @@ 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)
/** @var array<string, array<string, list<QueryBuilderType>>> */
private $cache = [];

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<QueryBuilderType>
*/
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])) {
if (!$this->descendIntoOtherMethods) {
return [];
}

$classReflection = $this->reflectionProvider->getClass($methodCalledOnTypeClassNames[0]);
$methodName = $methodCall->name->toString();
if (!$classReflection->hasNativeMethod($methodName)) {
return [];
}

$methodReflection = $classReflection->getNativeMethod($methodName);
$methodName = $methodReflection->getName();
$fileName = $methodReflection->getDeclaringClass()->getFileName();
if ($fileName === null) {
return [];
}

if (isset($this->cache[$fileName][$methodName])) {
return $this->cache[$fileName][$methodName];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: if this gets proven to help, we should improve the cache key to support multiple classes with the same method name in the same file.

}

$nodes = $this->parser->parseFile($fileName);
$classNode = $this->findClassNode($methodReflection->getDeclaringClass()->getName(), $nodes);
if ($classNode === null) {
return [];
}

$methodNode = $this->findMethodNode($methodReflection->getName(), $classNode->stmts);
$methodNode = $this->findMethodNode($methodName, $classNode->stmts);
if ($methodNode === null || $methodNode->stmts === null) {
return [];
}
Expand Down Expand Up @@ -136,6 +108,8 @@ private function findQueryBuilderTypesInCalledMethod(Scope $scope, MethodCall $m
});
});

$this->cache[$fileName][$methodName] = $queryBuilderTypes;

return $queryBuilderTypes;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
<?php declare(strict_types = 1);

namespace PHPStan\Type\Doctrine\QueryBuilder;

use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\EntityRepository;
use Doctrine\ORM\QueryBuilder;
use PhpParser\Node\Expr;
use PhpParser\Node\Expr\CallLike;
use PhpParser\Node\Expr\MethodCall;
use PhpParser\Node\Expr\StaticCall;
use PhpParser\Node\Identifier;
use PhpParser\Node\Name;
use PHPStan\Analyser\Scope;
use PHPStan\Reflection\MethodReflection;
use PHPStan\Reflection\ParametersAcceptorSelector;
use PHPStan\Type\ExpressionTypeResolverExtension;
use PHPStan\Type\ObjectType;
use PHPStan\Type\Type;
use PHPStan\Type\TypeCombinator;
use function count;

class ReturnQueryBuilderExpressionTypeResolverExtension implements ExpressionTypeResolverExtension
{

/** @var OtherMethodQueryBuilderParser */
private $otherMethodQueryBuilderParser;

public function __construct(
OtherMethodQueryBuilderParser $otherMethodQueryBuilderParser
)
{
$this->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);
}

}
3 changes: 0 additions & 3 deletions tests/Rules/Doctrine/ORM/QueryBuilderDqlRuleSlowTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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<QueryBuilderDqlRule>
Expand All @@ -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
);
}
Expand Down
3 changes: 0 additions & 3 deletions tests/Rules/Doctrine/ORM/QueryBuilderDqlRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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<QueryBuilderDqlRule>
Expand All @@ -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
);
}
Expand Down
Loading