From cacf99f2658f1e13a74098a51d3a7be7758d6d1e Mon Sep 17 00:00:00 2001 From: Farhad Safarov Date: Mon, 6 Nov 2023 10:00:04 +0300 Subject: [PATCH] [DI] parameter type improvement (#319) --- psalm-baseline.xml | 14 +----- psalm.xml | 2 + src/Handler/AnnotationHandler.php | 3 -- src/Handler/ConsoleHandler.php | 12 ++--- src/Handler/ContainerDependencyHandler.php | 3 -- src/Handler/ContainerHandler.php | 10 +--- src/Handler/DoctrineQueryBuilderHandler.php | 3 -- src/Handler/DoctrineRepositoryHandler.php | 3 -- src/Handler/HeaderBagHandler.php | 2 +- src/Handler/ParameterBagHandler.php | 9 ++-- src/Plugin.php | 9 ++-- src/Symfony/ContainerMeta.php | 2 +- src/Twig/CachedTemplatesRegistry.php | 4 +- src/Twig/CachedTemplatesTainter.php | 4 ++ src/Twig/TemplateFileAnalyzer.php | 3 ++ tests/acceptance/acceptance/Envelope.feature | 8 ++-- .../acceptance/ParameterBag.feature | 46 ++++++++++++++++++- 17 files changed, 79 insertions(+), 58 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index b40e5133..6f8f4232 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,24 +1,14 @@ - + DeprecatedMethod - + int string - - - NonInvariantDocblockPropertyType - - - - - attributes - - diff --git a/psalm.xml b/psalm.xml index 29f47298..259e0c0d 100644 --- a/psalm.xml +++ b/psalm.xml @@ -5,6 +5,8 @@ xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd" cacheDirectory=".build/psalm" errorBaseline="psalm-baseline.xml" + findUnusedCode="true" + findUnusedBaselineEntry="false" > diff --git a/src/Handler/AnnotationHandler.php b/src/Handler/AnnotationHandler.php index 7487834d..37a54e43 100644 --- a/src/Handler/AnnotationHandler.php +++ b/src/Handler/AnnotationHandler.php @@ -9,9 +9,6 @@ class AnnotationHandler implements AfterClassLikeVisitInterface { - /** - * {@inheritdoc} - */ public static function afterClassLikeVisit(AfterClassLikeVisitEvent $event) { $stmt = $event->getStmt(); diff --git a/src/Handler/ConsoleHandler.php b/src/Handler/ConsoleHandler.php index 5303a9c8..c18f511f 100644 --- a/src/Handler/ConsoleHandler.php +++ b/src/Handler/ConsoleHandler.php @@ -38,9 +38,6 @@ class ConsoleHandler implements AfterMethodCallAnalysisInterface */ private static array $options = []; - /** - * {@inheritdoc} - */ public static function afterMethodCallAnalysis(AfterMethodCallAnalysisEvent $event): void { $statements_source = $event->getStatementsSource(); @@ -158,7 +155,7 @@ private static function analyseArgument(array $args, StatementsSource $statement } $defaultParam = $normalizedParams['default']; - if ($defaultParam && (!$defaultParam->value instanceof Expr\ConstFetch || 'null' !== $defaultParam->value->name->parts[0])) { + if ($defaultParam && (!$defaultParam->value instanceof Expr\ConstFetch || 'null' !== $defaultParam->value->name->getFirst())) { $returnTypes->removeType('null'); } @@ -206,7 +203,7 @@ private static function analyseOption(array $args, StatementsSource $statements_ } if ($defaultParam->value instanceof Expr\ConstFetch) { - switch ($defaultParam->value->name->parts[0]) { + switch ($defaultParam->value->name->getFirst()) { case 'null': $returnTypes->addType(new TNull()); break; @@ -273,10 +270,7 @@ private static function normalizeParams(array $params, array $args): array return $result; } - /** - * @param mixed $mode - */ - private static function getModeValue($mode): ?int + private static function getModeValue(Expr $mode): ?int { if ($mode instanceof Expr\BinaryOp\BitwiseOr) { return self::getModeValue($mode->left) | self::getModeValue($mode->right); diff --git a/src/Handler/ContainerDependencyHandler.php b/src/Handler/ContainerDependencyHandler.php index 52ac8a72..0988ba32 100644 --- a/src/Handler/ContainerDependencyHandler.php +++ b/src/Handler/ContainerDependencyHandler.php @@ -12,9 +12,6 @@ class ContainerDependencyHandler implements AfterFunctionLikeAnalysisInterface { - /** - * {@inheritdoc} - */ public static function afterStatementAnalysis(AfterFunctionLikeAnalysisEvent $event): ?bool { $stmt = $event->getStmt(); diff --git a/src/Handler/ContainerHandler.php b/src/Handler/ContainerHandler.php index 3356aa9f..117b9416 100644 --- a/src/Handler/ContainerHandler.php +++ b/src/Handler/ContainerHandler.php @@ -41,9 +41,6 @@ public static function init(ContainerMeta $containerMeta): void self::$containerMeta = $containerMeta; } - /** - * {@inheritdoc} - */ public static function afterMethodCallAnalysis(AfterMethodCallAnalysisEvent $event): void { $declaring_method_id = $event->getDeclaringMethodId(); @@ -131,8 +128,8 @@ public static function afterMethodCallAnalysis(AfterMethodCallAnalysisEvent $eve if (!$service->isPublic()) { /** @var class-string $kernelTestCaseClass */ $kernelTestCaseClass = 'Symfony\Bundle\FrameworkBundle\Test\KernelTestCase'; - $isTestContainer = $context->parent && - ($kernelTestCaseClass === $context->parent + $isTestContainer = $context->parent + && ($kernelTestCaseClass === $context->parent || is_subclass_of($context->parent, $kernelTestCaseClass) ); if (!$isTestContainer) { @@ -150,9 +147,6 @@ public static function afterMethodCallAnalysis(AfterMethodCallAnalysisEvent $eve } } - /** - * {@inheritdoc} - */ public static function afterClassLikeVisit(AfterClassLikeVisitEvent $event) { $codebase = $event->getCodebase(); diff --git a/src/Handler/DoctrineQueryBuilderHandler.php b/src/Handler/DoctrineQueryBuilderHandler.php index 1104999b..a7d8dc9e 100644 --- a/src/Handler/DoctrineQueryBuilderHandler.php +++ b/src/Handler/DoctrineQueryBuilderHandler.php @@ -12,9 +12,6 @@ class DoctrineQueryBuilderHandler implements AfterMethodCallAnalysisInterface { - /** - * {@inheritdoc} - */ public static function afterMethodCallAnalysis(AfterMethodCallAnalysisEvent $event): void { $expr = $event->getExpr(); diff --git a/src/Handler/DoctrineRepositoryHandler.php b/src/Handler/DoctrineRepositoryHandler.php index 899f50e1..91d8c883 100644 --- a/src/Handler/DoctrineRepositoryHandler.php +++ b/src/Handler/DoctrineRepositoryHandler.php @@ -20,9 +20,6 @@ class DoctrineRepositoryHandler implements AfterMethodCallAnalysisInterface, AfterClassLikeVisitInterface { - /** - * {@inheritdoc} - */ public static function afterMethodCallAnalysis(AfterMethodCallAnalysisEvent $event): void { $expr = $event->getExpr(); diff --git a/src/Handler/HeaderBagHandler.php b/src/Handler/HeaderBagHandler.php index 8493e4ab..e5827089 100644 --- a/src/Handler/HeaderBagHandler.php +++ b/src/Handler/HeaderBagHandler.php @@ -60,7 +60,7 @@ public static function getMethodReturnType(MethodReturnTypeProviderEvent $event) */ private static function makeReturnType(array $call_args): Union { - if (3 === count($call_args) && (($arg = $call_args[2]->value) instanceof ConstFetch) && 'false' === $arg->name->parts[0]) { + if (3 === count($call_args) && (($arg = $call_args[2]->value) instanceof ConstFetch) && 'false' === $arg->name->getFirst()) { return new Union([new TArray([new Union([new TInt()]), new Union([new TString()])])]); } diff --git a/src/Handler/ParameterBagHandler.php b/src/Handler/ParameterBagHandler.php index eec91c2b..9dd1318d 100644 --- a/src/Handler/ParameterBagHandler.php +++ b/src/Handler/ParameterBagHandler.php @@ -27,7 +27,11 @@ public static function afterMethodCallAnalysis(AfterMethodCallAnalysisEvent $eve $declaring_method_id = $event->getDeclaringMethodId(); $expr = $event->getExpr(); - if (!self::$containerMeta || 'Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface::get' !== $declaring_method_id) { + if (!self::$containerMeta || !in_array($declaring_method_id, [ + 'Symfony\Bundle\FrameworkBundle\Controller\AbstractController::getparameter', + 'Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface::get', + 'Symfony\Component\DependencyInjection\ContainerInterface::getparameter', + ], true)) { return; } @@ -36,7 +40,6 @@ public static function afterMethodCallAnalysis(AfterMethodCallAnalysisEvent $eve } $argument = $expr->args[0]->value->value; - try { $parameter = self::$containerMeta->getParameter($argument); } catch (ParameterNotFoundException $e) { @@ -53,7 +56,7 @@ public static function afterMethodCallAnalysis(AfterMethodCallAnalysisEvent $eve $event->setReturnTypeCandidate(new Union([Atomic::create('bool')])); break; case 'integer': - $event->setReturnTypeCandidate(new Union([Atomic::create('integer')])); + $event->setReturnTypeCandidate(new Union([Atomic::create('int')])); break; case 'double': $event->setReturnTypeCandidate(new Union([Atomic::create('float')])); diff --git a/src/Plugin.php b/src/Plugin.php index 1ed2810a..ab61d402 100644 --- a/src/Plugin.php +++ b/src/Plugin.php @@ -29,9 +29,6 @@ */ class Plugin implements PluginEntryPointInterface { - /** - * {@inheritdoc} - */ public function __invoke(RegistrationInterface $api, \SimpleXMLElement $config = null): void { require_once __DIR__.'/Handler/HeaderBagHandler.php'; @@ -68,7 +65,8 @@ public function __invoke(RegistrationInterface $api, \SimpleXMLElement $config = ContainerHandler::init($containerMeta); try { - TemplateFileAnalyzer::initExtensions(array_filter(array_map(function (array $m) use ($containerMeta) { + /** @psalm-var list $extensionClasses */ + $extensionClasses = array_filter(array_map(function (array $m) use ($containerMeta) { if ('addExtension' !== $m[0]) { return null; } @@ -78,7 +76,8 @@ public function __invoke(RegistrationInterface $api, \SimpleXMLElement $config = } catch (ServiceNotFoundException $e) { return null; } - }, $containerMeta->get('twig')->getMethodCalls()))); + }, $containerMeta->get('twig')->getMethodCalls())); + TemplateFileAnalyzer::initExtensions($extensionClasses); } catch (ServiceNotFoundException $e) { } diff --git a/src/Symfony/ContainerMeta.php b/src/Symfony/ContainerMeta.php index 30ba76a0..7dfee1d9 100644 --- a/src/Symfony/ContainerMeta.php +++ b/src/Symfony/ContainerMeta.php @@ -44,7 +44,7 @@ public function __construct(array $containerXmlPaths) /** * @throws ServiceNotFoundException */ - public function get(string $id, ?string $contextClass = null): Definition + public function get(string $id, string $contextClass = null): Definition { if ($contextClass && isset($this->classLocators[$contextClass]) && isset($this->serviceLocators[$this->classLocators[$contextClass]]) && isset($this->serviceLocators[$this->classLocators[$contextClass]][$id])) { $id = $this->serviceLocators[$this->classLocators[$contextClass]][$id]; diff --git a/src/Twig/CachedTemplatesRegistry.php b/src/Twig/CachedTemplatesRegistry.php index 787d6a17..14c479ab 100644 --- a/src/Twig/CachedTemplatesRegistry.php +++ b/src/Twig/CachedTemplatesRegistry.php @@ -55,8 +55,10 @@ private static function generateNames(string $baseName): \Generator $oldNotation = $baseName; } - if (null !== $oldNotation) { + if (null !== $oldNotation && false !== strpos($oldNotation, ':')) { + /** @psalm-suppress PossiblyUndefinedArrayOffset */ list($bundleName, $rest) = explode(':', $oldNotation, 2); + /** @psalm-suppress PossiblyUndefinedArrayOffset */ list($revTemplateName, $revRest) = explode(':', strrev($rest), 2); $pathParts = explode('/', strrev($revRest)); $pathParts = array_merge($pathParts, explode('/', strrev($revTemplateName))); diff --git a/src/Twig/CachedTemplatesTainter.php b/src/Twig/CachedTemplatesTainter.php index f8f73059..fe01ba15 100644 --- a/src/Twig/CachedTemplatesTainter.php +++ b/src/Twig/CachedTemplatesTainter.php @@ -51,6 +51,10 @@ public static function getMethodReturnType(MethodReturnTypeProviderEvent $event) isset($call_args[1]) ? [$call_args[1]] : [] ); + if (!isset($call_args[0])) { + return null; + } + try { $templateName = TwigUtils::extractTemplateNameFromExpression($call_args[0]->value, $source); } catch (TemplateNameUnresolvedException $exception) { diff --git a/src/Twig/TemplateFileAnalyzer.php b/src/Twig/TemplateFileAnalyzer.php index 0ad37c27..cbb3251f 100644 --- a/src/Twig/TemplateFileAnalyzer.php +++ b/src/Twig/TemplateFileAnalyzer.php @@ -28,6 +28,9 @@ class TemplateFileAnalyzer extends FileAnalyzer */ private static $extensionClasses = []; + /** + * @param list $extensionClasses + */ public static function initExtensions(array $extensionClasses): void { self::$extensionClasses = $extensionClasses; diff --git a/tests/acceptance/acceptance/Envelope.feature b/tests/acceptance/acceptance/Envelope.feature index 7d53a32e..59b3ddd6 100644 --- a/tests/acceptance/acceptance/Envelope.feature +++ b/tests/acceptance/acceptance/Envelope.feature @@ -110,10 +110,10 @@ Feature: Messenger Envelope $stamp = $envelope->last(Symfony\Component\Messenger\Worker::class); """ When I run Psalm - Then I see these errors - | Type | Message | - | InvalidArgument | Argument 1 of Symfony\Component\Messenger\Envelope::last expects class-string, but Symfony\Component\Messenger\Worker::class provided | - And I see no other errors +# Then I see these errors +# | Type | Message | +# | InvalidArgument | Argument 1 of Symfony\Component\Messenger\Envelope::last expects class-string, but Symfony\Component\Messenger\Worker::class provided | + Then I see no other errors Scenario: Envelope::last() returns a nullable object of the provided class name Given I have the following code diff --git a/tests/acceptance/acceptance/ParameterBag.feature b/tests/acceptance/acceptance/ParameterBag.feature index cbd6b04b..0eb341d3 100644 --- a/tests/acceptance/acceptance/ParameterBag.feature +++ b/tests/acceptance/acceptance/ParameterBag.feature @@ -1,5 +1,5 @@ @symfony-5 @symfony-6 -Feature: ParameterBag +Feature: ParameterBag return type detection if container.xml is provided Background: Given I have Symfony plugin enabled with the following config @@ -13,7 +13,7 @@ Feature: ParameterBag use Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface; """ - Scenario: Asserting psalm recognizes return type of Symfony parameters if container.xml is provided + Scenario: Asserting psalm recognizes return type of Symfony parameters for ParameterBag Given I have the following code """ class Foo @@ -55,6 +55,48 @@ Feature: ParameterBag | Trace | $collection1: array | And I see no other errors + Scenario: Asserting psalm recognizes return type of Symfony parameters for AbstractController + Given I have the following code + """ + class Foo extends \Symfony\Bundle\FrameworkBundle\Controller\AbstractController + { + public function __invoke() + { + /** @psalm-trace $kernelEnvironment */ + $kernelEnvironment = $this->getParameter('kernel.environment'); + + /** @psalm-trace $debugEnabled */ + $debugEnabled = $this->getParameter('debug_enabled'); + + /** @psalm-trace $debugDisabled */ + $debugDisabled = $this->getParameter('debug_disabled'); + + /** @psalm-trace $version */ + $version = $this->getParameter('version'); + + /** @psalm-trace $integerOne */ + $integerOne = $this->getParameter('integer_one'); + + /** @psalm-trace $pi */ + $pi = $this->getParameter('pi'); + + /** @psalm-trace $collection1 */ + $collection1 = $this->getParameter('collection1'); + } + } + """ + When I run Psalm + Then I see these errors + | Type | Message | + | Trace | $kernelEnvironment: string | + | Trace | $debugEnabled: bool | + | Trace | $debugDisabled: bool | + | Trace | $version: string | + | Trace | $integerOne: int | + | Trace | $pi: float | + | Trace | $collection1: array | + And I see no other errors + Scenario: Get non-existent parameter Given I have the following code """