diff --git a/.github/workflows/code_analysis.yaml b/.github/workflows/code_analysis.yaml index 217dfb74d99..6573a5e4443 100644 --- a/.github/workflows/code_analysis.yaml +++ b/.github/workflows/code_analysis.yaml @@ -53,7 +53,7 @@ jobs: - name: 'Active Classes' run: | - vendor/bin/class-leak check bin config src rules utils --skip-suffix "Rector" --skip-type="Rector\\Utils\\Compiler\\Unprefixer" --skip-type="Rector\\NodeCollector\\BinaryOpConditionsCollector" --skip-type="Rector\\Set\\Contract\\SetListInterface" --skip-type="Rector\\DependencyInjection\\PHPStan\\RichParserFactory" + vendor/bin/class-leak check bin config src rules utils --skip-suffix "Rector" --skip-type="Rector\\Utils\\Compiler\\Unprefixer" --skip-type="Rector\\NodeCollector\\BinaryOpConditionsCollector" --skip-type="Rector\\Set\\Contract\\SetListInterface" --skip-type="Rector\\DependencyInjection\\PHPStan\\RichParserFactory" --skip-type="Rector\\Utils\\PHPStan\\Tests\\Rule\\SingleServiceRegistrationRule\\Source\\DuplicateRegistrationFactory" --skip-type="Rector\\Utils\\PHPStan\\Tests\\Rule\\SingleServiceRegistrationRule\\Source\\SingleRegistrationFactory" - name: 'Compatible PHPStan versions' diff --git a/phpstan.neon b/phpstan.neon index 5c29f57580e..0539471c0aa 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -15,6 +15,11 @@ services: tags: - phpstan.rules.rule + - + class: Rector\Utils\PHPStan\Rule\SingleServiceRegistrationRule + tags: + - phpstan.rules.rule + parameters: level: 8 diff --git a/src/DependencyInjection/LazyContainerFactory.php b/src/DependencyInjection/LazyContainerFactory.php index 6c780f20e70..9ee6280ca2a 100644 --- a/src/DependencyInjection/LazyContainerFactory.php +++ b/src/DependencyInjection/LazyContainerFactory.php @@ -478,7 +478,8 @@ static function (Container $container): DynamicSourceLocatorProvider { ); // resettable - $rectorConfig->tag(DynamicSourceLocatorProvider::class, ResettableInterface::class); + // DynamicSourceLocatorProvider is autotagged on its singleton() call above, + // as RectorConfig autotags ResettableInterface $rectorConfig->tag(RenamedClassesDataCollector::class, ResettableInterface::class); // caching @@ -610,7 +611,6 @@ static function (AbstractRector $rector, Container $container): void { $this->registerTagged($rectorConfig, self::NODE_NAME_RESOLVER_CLASSES, NodeNameResolverInterface::class); $this->registerTagged($rectorConfig, self::NODE_TYPE_RESOLVER_CLASSES, NodeTypeResolverInterface::class); $this->registerTagged($rectorConfig, self::OUTPUT_FORMATTER_CLASSES, OutputFormatterInterface::class); - $this->registerTagged($rectorConfig, self::BASE_PHP_DOC_NODE_VISITORS, BasePhpDocNodeVisitorInterface::class); $this->registerTagged( $rectorConfig, self::CLASS_NAME_IMPORT_SKIPPER_CLASSES, diff --git a/tests/DependencyInjection/TaggedServicesTest.php b/tests/DependencyInjection/TaggedServicesTest.php new file mode 100644 index 00000000000..26ee6f0b840 --- /dev/null +++ b/tests/DependencyInjection/TaggedServicesTest.php @@ -0,0 +1,39 @@ +tagged($tagInterface)); + + $classNames = array_map(static fn (object $service): string => $service::class, $taggedServices); + + $this->assertSame(array_values(array_unique($classNames)), $classNames); + } + + /** + * @return Iterator + */ + public static function provideTagInterfaces(): Iterator + { + yield [BasePhpDocNodeVisitorInterface::class]; + yield [ResettableInterface::class]; + } +} diff --git a/utils/phpstan/src/Rule/SingleServiceRegistrationRule.php b/utils/phpstan/src/Rule/SingleServiceRegistrationRule.php new file mode 100644 index 00000000000..508074b426f --- /dev/null +++ b/utils/phpstan/src/Rule/SingleServiceRegistrationRule.php @@ -0,0 +1,222 @@ + + * @see \Rector\Utils\PHPStan\Tests\Rule\SingleServiceRegistrationRule\SingleServiceRegistrationRuleTest + */ +final class SingleServiceRegistrationRule implements Rule +{ + private const string DUPLICATE_ERROR_MESSAGE = '"%s(%s)" is already called on line %d; register the service exactly once.'; + + private const string AUTOTAGGED_ERROR_MESSAGE = 'Service "%s" is registered as singleton and "%s" is autotagged, so this tag() call registers it twice.'; + + /** + * Interfaces RectorConfig tags on its own, see RectorConfig::$autotagInterfaces + * + * @var string[] + */ + private const array AUTOTAG_INTERFACES = [ + 'Symfony\Component\Console\Command\Command', + 'Rector\Contract\DependencyInjection\ResettableInterface', + ]; + + /** + * Registration calls on the container, and the arguments that identify what gets registered. + * The container itself is never part of the identity, so registerTagged() skips its first argument. + * + * @var array + */ + private const array REGISTRATION_METHOD_TO_ARG_POSITIONS = [ + 'singleton' => [0], + 'tag' => [0, 1], + 'registerTagged' => [1, 2], + ]; + + public function getNodeType(): string + { + return ClassMethod::class; + } + + /** + * @param ClassMethod $node + * @return list + */ + public function processNode(Node $node, Scope $scope): array + { + if ($node->stmts === null) { + return []; + } + + $ruleErrors = []; + $firstLineByRegistration = []; + $singletonClasses = []; + + $nodeFinder = new NodeFinder(); + + /** @var MethodCall[] $methodCalls */ + $methodCalls = $nodeFinder->findInstanceOf($node->stmts, MethodCall::class); + + foreach ($methodCalls as $methodCall) { + $methodName = $this->resolveRegistrationMethodName($methodCall); + if ($methodName === null) { + continue; + } + + $argumentKeys = $this->resolveArgumentKeys( + $methodCall, + self::REGISTRATION_METHOD_TO_ARG_POSITIONS[$methodName] + ); + + if ($argumentKeys === null) { + continue; + } + + if ($methodName === 'singleton') { + $singletonClasses[] = $argumentKeys[0]; + } + + $registration = $methodName . '(' . implode(', ', $argumentKeys) . ')'; + + if (isset($firstLineByRegistration[$registration])) { + $ruleErrors[] = RuleErrorBuilder::message(sprintf( + self::DUPLICATE_ERROR_MESSAGE, + $methodName, + implode(', ', $argumentKeys), + $firstLineByRegistration[$registration] + )) + ->identifier('rector.singleServiceRegistration') + ->line($methodCall->getStartLine()) + ->build(); + + continue; + } + + $firstLineByRegistration[$registration] = $methodCall->getStartLine(); + + if ($methodName !== 'tag') { + continue; + } + + if (! in_array($argumentKeys[1], self::AUTOTAG_INTERFACES, true)) { + continue; + } + + if (! in_array($argumentKeys[0], $singletonClasses, true)) { + continue; + } + + $ruleErrors[] = RuleErrorBuilder::message( + sprintf(self::AUTOTAGGED_ERROR_MESSAGE, $argumentKeys[0], $argumentKeys[1]) + ) + ->identifier('rector.singleServiceRegistration') + ->line($methodCall->getStartLine()) + ->build(); + } + + return $ruleErrors; + } + + /** + * The scope of a ClassMethod node is the method entry, where body variables are still mixed, + * so the container is matched on call shape instead of on its resolved type. + */ + private function resolveRegistrationMethodName(MethodCall $methodCall): ?string + { + if (! $methodCall->name instanceof Identifier) { + return null; + } + + $methodName = $methodCall->name->toString(); + if (! isset(self::REGISTRATION_METHOD_TO_ARG_POSITIONS[$methodName])) { + return null; + } + + // registerTagged() takes the container as its first argument, the rest are called on it + if ($methodName === 'registerTagged') { + $firstArg = $methodCall->getArgs()[0] ?? null; + if (! $firstArg instanceof Arg || ! $firstArg->value instanceof Variable) { + return null; + } + + return $methodName; + } + + if (! $methodCall->var instanceof Variable) { + return null; + } + + return $methodName; + } + + /** + * @param int[] $argPositions + * @return string[]|null + */ + private function resolveArgumentKeys(MethodCall $methodCall, array $argPositions): ?array + { + $args = $methodCall->getArgs(); + + $argumentKeys = []; + foreach ($argPositions as $argPosition) { + $arg = $args[$argPosition] ?? null; + if (! $arg instanceof Arg) { + return null; + } + + $argumentKey = $this->resolveArgumentKey($arg); + if ($argumentKey === null) { + return null; + } + + $argumentKeys[] = $argumentKey; + } + + return $argumentKeys; + } + + private function resolveArgumentKey(Arg $arg): ?string + { + if ($arg->value instanceof String_) { + return $arg->value->value; + } + + if (! $arg->value instanceof ClassConstFetch) { + return null; + } + + $classConstFetch = $arg->value; + if (! $classConstFetch->class instanceof Name || ! $classConstFetch->name instanceof Identifier) { + return null; + } + + $constantName = $classConstFetch->name->toString(); + if ($constantName === 'class') { + return $classConstFetch->class->toString(); + } + + return $classConstFetch->class->toString() . '::' . $constantName; + } +} diff --git a/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/SingleServiceRegistrationRuleTest.php b/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/SingleServiceRegistrationRuleTest.php new file mode 100644 index 00000000000..1c0a7846a97 --- /dev/null +++ b/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/SingleServiceRegistrationRuleTest.php @@ -0,0 +1,43 @@ + + */ +final class SingleServiceRegistrationRuleTest extends RuleTestCase +{ + public function testDuplicateRegistration(): void + { + $this->analyse([__DIR__ . '/Source/DuplicateRegistrationFactory.php'], [ + [ + '"registerTagged(self::SOME_VISITOR_CLASSES, Rector\Utils\PHPStan\Tests\Rule\SingleServiceRegistrationRule\Source\SomeTagInterface)" is already called on line 22; register the service exactly once.', + 27, + ], + [ + '"tag(Rector\Utils\PHPStan\Tests\Rule\SingleServiceRegistrationRule\Source\SomeOtherService, Rector\Utils\PHPStan\Tests\Rule\SingleServiceRegistrationRule\Source\SomeTagInterface)" is already called on line 25; register the service exactly once.', + 29, + ], + [ + 'Service "Rector\Utils\PHPStan\Tests\Rule\SingleServiceRegistrationRule\Source\SomeResettableService" is registered as singleton and "Rector\Contract\DependencyInjection\ResettableInterface" is autotagged, so this tag() call registers it twice.', + 31, + ], + ]); + } + + public function testSingleRegistration(): void + { + $this->analyse([__DIR__ . '/Source/SingleRegistrationFactory.php'], []); + } + + protected function getRule(): Rule + { + return new SingleServiceRegistrationRule(); + } +} diff --git a/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/Source/DuplicateRegistrationFactory.php b/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/Source/DuplicateRegistrationFactory.php new file mode 100644 index 00000000000..9cfa2014c63 --- /dev/null +++ b/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/Source/DuplicateRegistrationFactory.php @@ -0,0 +1,47 @@ + + */ + private const array SOME_VISITOR_CLASSES = []; + + public function create(): RectorConfig + { + $rectorConfig = new RectorConfig(); + + $this->registerTagged($rectorConfig, self::SOME_VISITOR_CLASSES, SomeTagInterface::class); + + $rectorConfig->singleton(SomeResettableService::class); + $rectorConfig->tag(SomeOtherService::class, SomeTagInterface::class); + + $this->registerTagged($rectorConfig, self::SOME_VISITOR_CLASSES, SomeTagInterface::class); + + $rectorConfig->tag(SomeOtherService::class, SomeTagInterface::class); + + $rectorConfig->tag(SomeResettableService::class, ResettableInterface::class); + + return $rectorConfig; + } + + /** + * @param array $classes + * @param class-string $tagInterface + */ + private function registerTagged(Container $container, array $classes, string $tagInterface): void + { + foreach ($classes as $class) { + $container->singleton($class); + $container->tag($class, $tagInterface); + } + } +} diff --git a/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/Source/SingleRegistrationFactory.php b/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/Source/SingleRegistrationFactory.php new file mode 100644 index 00000000000..3f320fa7317 --- /dev/null +++ b/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/Source/SingleRegistrationFactory.php @@ -0,0 +1,50 @@ + + */ + private const array SOME_VISITOR_CLASSES = []; + + /** + * @var array + */ + private const array SOME_OTHER_CLASSES = []; + + public function create(): RectorConfig + { + $rectorConfig = new RectorConfig(); + + $this->registerTagged($rectorConfig, self::SOME_VISITOR_CLASSES, SomeTagInterface::class); + $this->registerTagged($rectorConfig, self::SOME_OTHER_CLASSES, SomeTagInterface::class); + + $rectorConfig->singleton(SomeOtherService::class); + $rectorConfig->tag(SomeOtherService::class, SomeTagInterface::class); + + // not a singleton here, so the autotagging never fires and this tag is what registers it + $rectorConfig->tag(SomeResettableService::class, ResettableInterface::class); + + return $rectorConfig; + } + + /** + * @param array $classes + * @param class-string $tagInterface + */ + private function registerTagged(Container $container, array $classes, string $tagInterface): void + { + foreach ($classes as $class) { + $container->singleton($class); + $container->tag($class, $tagInterface); + } + } +} diff --git a/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/Source/SomeOtherService.php b/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/Source/SomeOtherService.php new file mode 100644 index 00000000000..c432665f693 --- /dev/null +++ b/utils/phpstan/tests/Rule/SingleServiceRegistrationRule/Source/SomeOtherService.php @@ -0,0 +1,9 @@ +