Register each tagged service once, add PHPStan rule to keep it that way

.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'

phpstan.neon

@@ -15,6 +15,11 @@ services:
         tags:
             - phpstan.rules.rule
 
+    -
+        class: Rector\Utils\PHPStan\Rule\SingleServiceRegistrationRule
+        tags:
+            - phpstan.rules.rule
+
 parameters:
     level: 8
 

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,

tests/DependencyInjection/TaggedServicesTest.php

@@ -0,0 +1,39 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\DependencyInjection;
+
+use Iterator;
+use PHPUnit\Framework\Attributes\DataProvider;
+use Rector\BetterPhpDocParser\Contract\BasePhpDocNodeVisitorInterface;
+use Rector\Contract\DependencyInjection\ResettableInterface;
+use Rector\Testing\PHPUnit\AbstractLazyTestCase;
+
+final class TaggedServicesTest extends AbstractLazyTestCase
+{
+    /**
+     * Registering the same class under the same tag twice makes every consumer of that tag
+     * run it twice, e.g. each phpdoc node visitor traversing every doc node twice
+     *
+     * @param class-string $tagInterface
+     */
+    #[DataProvider('provideTagInterfaces')]
+    public function testServiceIsTaggedOnce(string $tagInterface): void
+    {
+        $taggedServices = iterator_to_array(self::getContainer()->tagged($tagInterface));
+
+        $classNames = array_map(static fn (object $service): string => $service::class, $taggedServices);
+
+        $this->assertSame(array_values(array_unique($classNames)), $classNames);
+    }
+
+    /**
+     * @return Iterator<array{class-string}>
+     */
+    public static function provideTagInterfaces(): Iterator
+    {
+        yield [BasePhpDocNodeVisitorInterface::class];
+        yield [ResettableInterface::class];
+    }
+}

utils/phpstan/src/Rule/SingleServiceRegistrationRule.php

@@ -0,0 +1,222 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Utils\PHPStan\Rule;
+
+use PhpParser\Node;
+use PhpParser\Node\Arg;
+use PhpParser\Node\Expr\ClassConstFetch;
+use PhpParser\Node\Expr\MethodCall;
+use PhpParser\Node\Expr\Variable;
+use PhpParser\Node\Identifier;
+use PhpParser\Node\Name;
+use PhpParser\Node\Scalar\String_;
+use PhpParser\Node\Stmt\ClassMethod;
+use PhpParser\NodeFinder;
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\IdentifierRuleError;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleErrorBuilder;
+
+/**
+ * A container service must be registered exactly once. Registering the same class under the same
+ * tag twice makes every consumer of that tag run it twice, e.g. each phpdoc node visitor
+ * traversing every doc node twice.
+ *
+ * @implements Rule<ClassMethod>
+ * @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<string, int[]>
+     */
+    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<IdentifierRuleError>
+     */
+    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;
+    }
+}

utils/phpstan/tests/Rule/SingleServiceRegistrationRule/SingleServiceRegistrationRuleTest.php

@@ -0,0 +1,43 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Utils\PHPStan\Tests\Rule\SingleServiceRegistrationRule;
+
+use PHPStan\Rules\Rule;
+use PHPStan\Testing\RuleTestCase;
+use Rector\Utils\PHPStan\Rule\SingleServiceRegistrationRule;
+
+/**
+ * @extends RuleTestCase<SingleServiceRegistrationRule>
+ */
+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();
+    }
+}

utils/phpstan/tests/Rule/SingleServiceRegistrationRule/Source/DuplicateRegistrationFactory.php

@@ -0,0 +1,47 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Utils\PHPStan\Tests\Rule\SingleServiceRegistrationRule\Source;
+
+use Illuminate\Container\Container;
+use Rector\Config\RectorConfig;
+use Rector\Contract\DependencyInjection\ResettableInterface;
+
+final class DuplicateRegistrationFactory
+{
+    /**
+     * @var array<class-string>
+     */
+    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<class-string> $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);
+        }
+    }
+}