[DependencyInjection] Defer check for circular references instead of skipping them.

Author: biozshock

Compiler/CheckCircularReferencesPass.php

@@ -28,6 +28,7 @@ class CheckCircularReferencesPass implements CompilerPassInterface
 {
     private array $currentPath;
     private array $checkedNodes;
+    private array $checkedLazyNodes;
 
     /**
      * Checks the ContainerBuilder object for circular references.
@@ -59,22 +60,36 @@ private function checkOutEdges(array $edges): void
             $node = $edge->getDestNode();
             $id = $node->getId();
 
-            if (empty($this->checkedNodes[$id])) {
-                // Don't check circular references for lazy edges
-                if (!$node->getValue() || (!$edge->isLazy() && !$edge->isWeak())) {
-                    $searchKey = array_search($id, $this->currentPath);
-                    $this->currentPath[] = $id;
+            if (!empty($this->checkedNodes[$id])) {
+                continue;
+            }
+
+            $isLeaf = !!$node->getValue();
+            $isConcrete = !$edge->isLazy() && !$edge->isWeak();
+
+            // Skip already checked lazy services if they are still lazy. Will not gain any new information.
+            if (!empty($this->checkedLazyNodes[$id]) && (!$isLeaf || !$isConcrete)) {
+                continue;
+            }
 
-                    if (false !== $searchKey) {
-                        throw new ServiceCircularReferenceException($id, \array_slice($this->currentPath, $searchKey));
-                    }
+            // Process concrete references, otherwise defer check circular references for lazy edges.
+            if (!$isLeaf || $isConcrete) {
+                $searchKey = array_search($id, $this->currentPath);
+                $this->currentPath[] = $id;
 
-                    $this->checkOutEdges($node->getOutEdges());
+                if (false !== $searchKey) {
+                    throw new ServiceCircularReferenceException($id, \array_slice($this->currentPath, $searchKey));
                 }
 
+                $this->checkOutEdges($node->getOutEdges());
+
                 $this->checkedNodes[$id] = true;
-                array_pop($this->currentPath);
+                unset($this->checkedLazyNodes[$id]);
+            } else {
+                $this->checkedLazyNodes[$id] = true;
             }
+
+            array_pop($this->currentPath);
         }
     }
 }

Tests/Compiler/CheckCircularReferencesPassTest.php

@@ -13,9 +13,12 @@
 
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
+use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
+use Symfony\Component\DependencyInjection\Argument\TaggedIteratorArgument;
 use Symfony\Component\DependencyInjection\Compiler\AnalyzeServiceReferencesPass;
 use Symfony\Component\DependencyInjection\Compiler\CheckCircularReferencesPass;
 use Symfony\Component\DependencyInjection\Compiler\Compiler;
+use Symfony\Component\DependencyInjection\Compiler\ServiceLocatorTagPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException;
 use Symfony\Component\DependencyInjection\Reference;
@@ -126,6 +129,21 @@ public function testProcessIgnoresLazyServices()
         $this->addToAssertionCount(1);
     }
 
+    public function testProcessDefersLazyServices()
+    {
+        $container = new ContainerBuilder();
+
+        $container->register('a')->addArgument(new ServiceLocatorArgument(new TaggedIteratorArgument('tag', needsIndexes: true)));
+        $container->register('b')->addArgument(new Reference('c'))->addTag('tag');
+        $container->register('c')->addArgument(new Reference('b'));
+
+        (new ServiceLocatorTagPass())->process($container);
+
+        $this->expectException(ServiceCircularReferenceException::class);
+
+        $this->process($container);
+    }
+
     public function testProcessIgnoresIteratorArguments()
     {
         $container = new ContainerBuilder();