Skip to content

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

Merged
TomasVotruba merged 3 commits into
mainfrom
fix-duplicate-tag-registration
Jul 30, 2026
Merged

Register each tagged service once, add PHPStan rule to keep it that way#8228
TomasVotruba merged 3 commits into
mainfrom
fix-duplicate-tag-registration

Conversation

@TomasVotruba

@TomasVotruba TomasVotruba commented Jul 30, 2026

Copy link
Copy Markdown
Member

I went looking at LazyContainerFactory::create() because it is a 320-line method, and found two duplicate registrations hiding in it. The second commit adds a PHPStan rule so they cannot come back.

Every phpdoc node visitor was registered twice

self::BASE_PHP_DOC_NODE_VISITORS is passed to registerTagged() in two separate places — once in the multi-line block, and again 18 lines later in the one-liner block:

$this->registerTagged(
    $rectorConfig,
    self::BASE_PHP_DOC_NODE_VISITORS,
    BasePhpDocNodeVisitorInterface::class
);

// PHP 8.0 attributes
// ...

$this->registerTagged($rectorConfig, self::OUTPUT_FORMATTER_CLASSES, OutputFormatterInterface::class);
$this->registerTagged($rectorConfig, self::BASE_PHP_DOC_NODE_VISITORS, BasePhpDocNodeVisitorInterface::class);

registerTagged() calls $container->tag(), which appends, so the tag collection ends up holding each class twice. PhpDocNodeMapper takes its $phpDocNodeVisitors from that tag:

before: resolved visitors: 10
DUPLICATE x2  ArrayTypePhpDocNodeVisitor
DUPLICATE x2  CallableTypePhpDocNodeVisitor
DUPLICATE x2  IntersectionTypeNodePhpDocNodeVisitor
DUPLICATE x2  TemplatePhpDocNodeVisitor
DUPLICATE x2  UnionTypeNodePhpDocNodeVisitor

after:  resolved visitors: 5

They are the same singleton instances, so this is duplicated traversal work on every doc node of every file rather than corrupted state — the full test suite produces identical results before and after.

DynamicSourceLocatorProvider was tagged on top of autotagging

$rectorConfig->tag(DynamicSourceLocatorProvider::class, ResettableInterface::class);

RectorConfig already autotags ResettableInterface ($autotagInterfaces), and this class is registered via singleton() a few lines below, so the explicit tag is a second registration. It got reset twice on every AbstractRectorTestCase boot. The neighbouring RenamedClassesDataCollector line is not redundant — it is never registered through singleton(), so autotagging never fires for it and the explicit tag is what puts it in the collection. Only the one line is removed.

Runtime test

A test asserts no tag collection holds the same class twice. It fails on both counts without this fix:

 Array &0 [
      0 => 'Rector\PostRector\Application\PostFileProcessor',
      1 => '...\DynamicSourceLocatorProvider',
 -    2 => 'Rector\Configuration\RenamedClassesDataCollector',
 +    2 => '...\DynamicSourceLocatorProvider',
 +    3 => 'Rector\Configuration\RenamedClassesDataCollector',
  ]

PHPStan rule

SingleServiceRegistrationRule catches the same class of mistake at analysis time, in any registration method:

$rectorConfig->singleton(SomeService::class);
$rectorConfig->tag(SomeOtherService::class, SomeTagInterface::class);

$rectorConfig->tag(SomeOtherService::class, SomeTagInterface::class);
// "tag(SomeOtherService, SomeTagInterface)" is already called on line 25; register the service exactly once.

$rectorConfig->tag(SomeService::class, ResettableInterface::class);
// Service "SomeService" is registered as singleton and "ResettableInterface" is autotagged,
// so this tag() call registers it twice.

It tracks singleton(), tag() and registerTagged() calls within one method, keyed by the arguments that identify the service — the container argument itself is excluded, so registerTagged($rectorConfig, X, Y) compares on X, Y.

Checked against the unfixed file: it reports the duplicated registerTagged(self::BASE_PHP_DOC_NODE_VISITORS) on line 613 and the redundant DynamicSourceLocatorProvider tag on line 481. Running it over the whole codebase produces no other errors.

One implementation note worth recording: the Scope handed to a ClassMethod rule is the scope at method entry, where body variables are still mixed. Gating on $scope->getType($rectorConfig) being a Container silently matched almost nothing — it dropped 27 of the 30 registration calls in create() while still passing its own fixtures. The rule matches on call shape instead.

Scope note

The runtime test and the PHPStan rule cover different things and both are kept: the rule sees source-level repetition in any method, the test sees the resulting container, including tags that arrive through autotagging rather than an explicit call.

Two classes were tagged twice in create():

- self::BASE_PHP_DOC_NODE_VISITORS was passed to registerTagged() in two
  separate places, so PhpDocNodeMapper received 10 visitors instead of 5 and
  traversed every doc node with each visitor twice.
- DynamicSourceLocatorProvider was tagged ResettableInterface explicitly, on
  top of the autotagging RectorConfig already applies to that interface.

Both are duplicate registrations of the same singleton, so no test output
changes; the work was simply done twice.

Cover it with a test that asserts tagged collections hold no class twice.
…rations

Guards the two mistakes fixed in this branch:

- the same singleton()/tag()/registerTagged() call repeated in one method
- tag() with an interface RectorConfig already autotags, on a class that is
  also registered via singleton()

Verified against the unfixed LazyContainerFactory: the rule reports both the
duplicated registerTagged(self::BASE_PHP_DOC_NODE_VISITORS) on line 613 and
the redundant DynamicSourceLocatorProvider tag on line 481.

The scope handed to a ClassMethod rule is the method entry, where body
variables are still mixed, so the container is matched on call shape rather
than on its resolved type.
@TomasVotruba TomasVotruba changed the title Register each tagged service once in LazyContainerFactory Register each tagged service once, add PHPStan rule to keep it that way Jul 30, 2026
The two Source fixtures exist to be read as text by RuleTestCase, so they are
never instantiated and have no parent or interface, which is exactly what
class-leak reports. The existing rule-test fixtures dodge this only because
their names end in "Rector", which these are not.
@TomasVotruba
TomasVotruba merged commit a0ad10f into main Jul 30, 2026
65 checks passed
@TomasVotruba
TomasVotruba deleted the fix-duplicate-tag-registration branch July 30, 2026 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant