feature #29108 [DI] compute autowiring error messages lazily (nicolas…

…-grekas)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[DI] compute autowiring error messages lazily

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29019
| License       | MIT
| Doc PR        | -

As suggested in the linked issue:

> the error message may ultimately be "hidden" because the definition in question is removed... and so we're doing work unnecessarily.

Commits
-------

3b3a1bd [DI] compute autowiring error messages lazily

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TestServiceContainerWeakRefPass.php

@@ -31,7 +31,7 @@ public function process(ContainerBuilder $container)
         $definitions = $container->getDefinitions();
 
         foreach ($definitions as $id => $definition) {
-            if ($id && '.' !== $id[0] && (!$definition->isPublic() || $definition->isPrivate()) && !$definition->getErrors() && !$definition->isAbstract()) {
+            if ($id && '.' !== $id[0] && (!$definition->isPublic() || $definition->isPrivate()) && !$definition->hasErrors() && !$definition->isAbstract()) {
                 $privateServices[$id] = new Reference($id, ContainerBuilder::IGNORE_ON_UNINITIALIZED_REFERENCE);
             }
         }
@@ -43,7 +43,7 @@ public function process(ContainerBuilder $container)
                 while (isset($aliases[$target = (string) $alias])) {
                     $alias = $aliases[$target];
                 }
-                if (isset($definitions[$target]) && !$definitions[$target]->getErrors() && !$definitions[$target]->isAbstract()) {
+                if (isset($definitions[$target]) && !$definitions[$target]->hasErrors() && !$definitions[$target]->isAbstract()) {
                     $privateServices[$id] = new Reference($target, ContainerBuilder::IGNORE_ON_UNINITIALIZED_REFERENCE);
                 }
             }

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

@@ -74,7 +74,7 @@ protected function processValue($value, $isRoot = false)
                 throw $e;
             }
 
-            $this->container->getDefinition($this->currentId)->addError($e->getMessage());
+            $this->container->getDefinition($this->currentId)->addError($e->getMessageCallback() ?? $e->getMessage());
 
             return parent::processValue($value, $isRoot);
         }
@@ -86,16 +86,15 @@ private function doProcessValue($value, $isRoot = false)
             if ($ref = $this->getAutowiredReference($value)) {
                 return $ref;
             }
-            $message = $this->createTypeNotFoundMessage($value, 'it');
-
             if (ContainerBuilder::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE === $value->getInvalidBehavior()) {
+                $message = $this->createTypeNotFoundMessageCallback($value, 'it');
+
                 // since the error message varies by referenced id and $this->currentId, so should the id of the dummy errored definition
                 $this->container->register($id = sprintf('.errored.%s.%s', $this->currentId, (string) $value), $value->getType())
                     ->addError($message);
 
                 return new TypedReference($id, $value->getType(), $value->getInvalidBehavior(), $value->getName());
             }
-            $this->container->log($this, $message);
         }
         $value = parent::processValue($value, $isRoot);
 
@@ -222,14 +221,13 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a
 
             $getValue = function () use ($type, $parameter, $class, $method) {
                 if (!$value = $this->getAutowiredReference($ref = new TypedReference($type, $type, ContainerBuilder::EXCEPTION_ON_INVALID_REFERENCE, $parameter->name))) {
-                    $failureMessage = $this->createTypeNotFoundMessage($ref, sprintf('argument "$%s" of method "%s()"', $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));
+                    $failureMessage = $this->createTypeNotFoundMessageCallback($ref, sprintf('argument "$%s" of method "%s()"', $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));
 
                     if ($parameter->isDefaultValueAvailable()) {
                         $value = $parameter->getDefaultValue();
                     } elseif (!$parameter->allowsNull()) {
                         throw new AutowiringFailedException($this->currentId, $failureMessage);
                     }
-                    $this->container->log($this, $failureMessage);
                 }
 
                 return $value;
@@ -307,27 +305,27 @@ private function getAutowiredReference(TypedReference $reference)
     /**
      * Populates the list of available types.
      */
-    private function populateAvailableTypes()
+    private function populateAvailableTypes(ContainerBuilder $container)
     {
         $this->types = array();
         $this->ambiguousServiceTypes = array();
 
-        foreach ($this->container->getDefinitions() as $id => $definition) {
-            $this->populateAvailableType($id, $definition);
+        foreach ($container->getDefinitions() as $id => $definition) {
+            $this->populateAvailableType($container, $id, $definition);
         }
     }
 
     /**
      * Populates the list of available types for a given definition.
      */
-    private function populateAvailableType(string $id, Definition $definition)
+    private function populateAvailableType(ContainerBuilder $container, string $id, Definition $definition)
     {
         // Never use abstract services
         if ($definition->isAbstract()) {
             return;
         }
 
-        if ('' === $id || '.' === $id[0] || $definition->isDeprecated() || !$reflectionClass = $this->container->getReflectionClass($definition->getClass(), false)) {
+        if ('' === $id || '.' === $id[0] || $definition->isDeprecated() || !$reflectionClass = $container->getReflectionClass($definition->getClass(), false)) {
             return;
         }
 
@@ -367,19 +365,21 @@ private function set(string $type, string $id)
         $this->ambiguousServiceTypes[$type][] = $id;
     }
 
-    private function createTypeNotFoundMessage(TypedReference $reference, $label)
+    private function createTypeNotFoundMessageCallback(TypedReference $reference, $label)
     {
-        $trackResources = $this->container->isTrackingResources();
-        $this->container->setResourceTracking(false);
-        try {
-            if ($r = $this->container->getReflectionClass($type = $reference->getType(), false)) {
-                $alternatives = $this->createTypeAlternatives($reference);
-            }
-        } finally {
-            $this->container->setResourceTracking($trackResources);
-        }
+        $container = new ContainerBuilder($this->container->getParameterBag());
+        $container->setAliases($this->container->getAliases());
+        $container->setDefinitions($this->container->getDefinitions());
+        $container->setResourceTracking(false);
+
+        return function () use ($container, $reference, $label) {
+            return $this->createTypeNotFoundMessage($container, $reference, $label);
+        };
+    }
 
-        if (!$r) {
+    private function createTypeNotFoundMessage(ContainerBuilder $container, TypedReference $reference, $label)
+    {
+        if (!$r = $container->getReflectionClass($type = $reference->getType(), false)) {
             // either $type does not exist or a parent class does not exist
             try {
                 $resource = new ClassExistenceResource($type, false);
@@ -392,7 +392,8 @@ private function createTypeNotFoundMessage(TypedReference $reference, $label)
 
             $message = sprintf('has type "%s" but this class %s.', $type, $parentMsg ? sprintf('is missing a parent class (%s)', $parentMsg) : 'was not found');
         } else {
-            $message = $this->container->has($type) ? 'this service is abstract' : 'no such service exists';
+            $alternatives = $this->createTypeAlternatives($container, $reference);
+            $message = $container->has($type) ? 'this service is abstract' : 'no such service exists';
             $message = sprintf('references %s "%s" but %s.%s', $r->isInterface() ? 'interface' : 'class', $type, $message, $alternatives);
 
             if ($r->isInterface() && !$alternatives) {
@@ -410,18 +411,18 @@ private function createTypeNotFoundMessage(TypedReference $reference, $label)
         return $message;
     }
 
-    private function createTypeAlternatives(TypedReference $reference)
+    private function createTypeAlternatives(ContainerBuilder $container, TypedReference $reference)
     {
         // try suggesting available aliases first
-        if ($message = $this->getAliasesSuggestionForType($type = $reference->getType())) {
+        if ($message = $this->getAliasesSuggestionForType($container, $type = $reference->getType())) {
             return ' '.$message;
         }
         if (null === $this->ambiguousServiceTypes) {
-            $this->populateAvailableTypes();
+            $this->populateAvailableTypes($container);
         }
 
-        $servicesAndAliases = $this->container->getServiceIds();
-        if (!$this->container->has($type) && false !== $key = array_search(strtolower($type), array_map('strtolower', $servicesAndAliases))) {
+        $servicesAndAliases = $container->getServiceIds();
+        if (!$container->has($type) && false !== $key = array_search(strtolower($type), array_map('strtolower', $servicesAndAliases))) {
             return sprintf(' Did you mean "%s"?', $servicesAndAliases[$key]);
         } elseif (isset($this->ambiguousServiceTypes[$type])) {
             $message = sprintf('one of these existing services: "%s"', implode('", "', $this->ambiguousServiceTypes[$type]));
@@ -434,11 +435,11 @@ private function createTypeAlternatives(TypedReference $reference)
         return sprintf(' You should maybe alias this %s to %s.', class_exists($type, false) ? 'class' : 'interface', $message);
     }
 
-    private function getAliasesSuggestionForType($type, $extraContext = null)
+    private function getAliasesSuggestionForType(ContainerBuilder $container, $type, $extraContext = null)
     {
         $aliases = array();
         foreach (class_parents($type) + class_implements($type) as $parent) {
-            if ($this->container->has($parent) && !$this->container->findDefinition($parent)->isAbstract()) {
+            if ($container->has($parent) && !$container->findDefinition($parent)->isAbstract()) {
                 $aliases[] = $parent;
             }
         }

src/Symfony/Component/DependencyInjection/Compiler/DefinitionErrorExceptionPass.php

@@ -28,7 +28,7 @@ class DefinitionErrorExceptionPass extends AbstractRecursivePass
      */
     protected function processValue($value, $isRoot = false)
     {
-        if (!$value instanceof Definition || empty($value->getErrors())) {
+        if (!$value instanceof Definition || !$value->hasErrors()) {
             return parent::processValue($value, $isRoot);
         }
 

src/Symfony/Component/DependencyInjection/Compiler/InlineServiceDefinitionsPass.php

@@ -160,7 +160,7 @@ protected function processValue($value, $isRoot = false)
      */
     private function isInlineableDefinition($id, Definition $definition)
     {
-        if ($definition->getErrors() || $definition->isDeprecated() || $definition->isLazy() || $definition->isSynthetic()) {
+        if ($definition->hasErrors() || $definition->isDeprecated() || $definition->isLazy() || $definition->isSynthetic()) {
             return false;
         }
 

src/Symfony/Component/DependencyInjection/Compiler/ResolveChildDefinitionsPass.php

@@ -176,9 +176,8 @@ private function doResolveDefinition(ChildDefinition $definition)
             $def->setMethodCalls(array_merge($def->getMethodCalls(), $calls));
         }
 
-        foreach (array_merge($parentDef->getErrors(), $definition->getErrors()) as $v) {
-            $def->addError($v);
-        }
+        $def->addError($parentDef);
+        $def->addError($definition);
 
         // these attributes are always taken from the child
         $def->setAbstract($definition->isAbstract());

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -592,7 +592,7 @@ private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_
             throw $e;
         }
 
-        if ($e = $definition->getErrors()) {
+        if ($definition->hasErrors() && $e = $definition->getErrors()) {
             throw new RuntimeException(reset($e));
         }
 

src/Symfony/Component/DependencyInjection/Definition.php

@@ -876,13 +876,17 @@ public function setBindings(array $bindings)
     /**
      * Add an error that occurred when building this Definition.
      *
-     * @param string $error
+     * @param string|\Closure|self $error
      *
      * @return $this
      */
     public function addError($error)
     {
-        $this->errors[] = $error;
+        if ($error instanceof self) {
+            $this->errors = array_merge($this->errors, $error->errors);
+        } else {
+            $this->errors[] = $error;
+        }
 
         return $this;
     }
@@ -894,6 +898,19 @@ public function addError($error)
      */
     public function getErrors()
     {
+        foreach ($this->errors as $i => $error) {
+            if ($error instanceof \Closure) {
+                $this->errors[$i] = (string) $error();
+            } elseif (!\is_string($error)) {
+                $this->errors[$i] = (string) $error;
+            }
+        }
+
         return $this->errors;
     }
+
+    public function hasErrors(): bool
+    {
+        return (bool) $this->errors;
+    }
 }

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

@@ -523,7 +523,7 @@ private function addServiceInstance(string $id, Definition $definition, bool $is
 
     private function isTrivialInstance(Definition $definition): bool
     {
-        if ($definition->getErrors()) {
+        if ($definition->hasErrors()) {
             return true;
         }
         if ($definition->isSynthetic() || $definition->getFile() || $definition->getMethodCalls() || $definition->getProperties() || $definition->getConfigurator()) {
@@ -1501,7 +1501,7 @@ private function dumpValue($value, bool $interpolate = true): string
                             continue;
                         }
                         $definition = $this->container->findDefinition($id = (string) $v);
-                        $load = !($e = $definition->getErrors()) ? $this->asFiles && !$this->isHotPath($definition) : reset($e);
+                        $load = !($definition->hasErrors() && $e = $definition->getErrors()) ? $this->asFiles && !$this->isHotPath($definition) : reset($e);
                         $serviceMap .= sprintf("\n            %s => array(%s, %s, %s, %s),",
                             $this->export($k),
                             $this->export($definition->isShared() ? ($definition->isPublic() ? 'services' : 'privates') : false),
@@ -1519,7 +1519,7 @@ private function dumpValue($value, bool $interpolate = true): string
                 list($this->definitionVariables, $this->referenceVariables) = $scope;
             }
         } elseif ($value instanceof Definition) {
-            if ($e = $value->getErrors()) {
+            if ($value->hasErrors() && $e = $value->getErrors()) {
                 $this->addThrow = true;
 
                 return sprintf('$this->throw(%s)', $this->export(reset($e)));
@@ -1628,7 +1628,7 @@ private function getServiceCall(string $id, Reference $reference = null): string
                     return $code;
                 }
             } elseif ($this->isTrivialInstance($definition)) {
-                if ($e = $definition->getErrors()) {
+                if ($definition->hasErrors() && $e = $definition->getErrors()) {
                     $this->addThrow = true;
 
                     return sprintf('$this->throw(%s)', $this->export(reset($e)));

src/Symfony/Component/DependencyInjection/Exception/AutowiringFailedException.php

@@ -17,12 +17,44 @@
 class AutowiringFailedException extends RuntimeException
 {
     private $serviceId;
+    private $messageCallback;
 
-    public function __construct(string $serviceId, string $message = '', int $code = 0, \Exception $previous = null)
+    public function __construct(string $serviceId, $message = '', int $code = 0, \Exception $previous = null)
     {
         $this->serviceId = $serviceId;
 
-        parent::__construct($message, $code, $previous);
+        if (!$message instanceof \Closure) {
+            parent::__construct($message, $code, $previous);
+
+            return;
+        }
+
+        $this->messageCallback = $message;
+        parent::__construct('', $code, $previous);
+
+        $this->message = new class($this->message, $this->messageCallback) {
+            private $message;
+            private $messageCallback;
+
+            public function __construct(&$message, &$messageCallback)
+            {
+                $this->message = &$message;
+                $this->messageCallback = &$messageCallback;
+            }
+
+            public function __toString(): string
+            {
+                $messageCallback = $this->messageCallback;
+                $this->messageCallback = null;
+
+                return $this->message = $messageCallback();
+            }
+        };
+    }
+
+    public function getMessageCallback(): ?\Closure
+    {
+        return $this->messageCallback;
     }
 
     public function getServiceId()

src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php

@@ -20,7 +20,6 @@
 use Symfony\Component\DependencyInjection\Compiler\DecoratorServicePass;
 use Symfony\Component\DependencyInjection\Compiler\ResolveClassPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
-use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Exception\AutowiringFailedException;
 use Symfony\Component\DependencyInjection\Exception\RuntimeException;
 use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
@@ -620,7 +619,7 @@ public function testSetterInjectionCollisionThrowsException()
         }
 
         $this->assertNotNull($e);
-        $this->assertSame('Cannot autowire service "setter_injection_collision": argument "$collision" of method "Symfony\Component\DependencyInjection\Tests\Compiler\SetterInjectionCollision::setMultipleInstancesForOneArg()" references interface "Symfony\Component\DependencyInjection\Tests\Compiler\CollisionInterface" but no such service exists. You should maybe alias this interface to one of these existing services: "c1", "c2".', $e->getMessage());
+        $this->assertSame('Cannot autowire service "setter_injection_collision": argument "$collision" of method "Symfony\Component\DependencyInjection\Tests\Compiler\SetterInjectionCollision::setMultipleInstancesForOneArg()" references interface "Symfony\Component\DependencyInjection\Tests\Compiler\CollisionInterface" but no such service exists. You should maybe alias this interface to one of these existing services: "c1", "c2".', (string) $e->getMessage());
     }
 
     /**
@@ -903,9 +902,7 @@ public function testErroredServiceLocator()
 
         (new AutowirePass())->process($container);
 
-        $erroredDefinition = new Definition(MissingClass::class);
-
-        $this->assertEquals($erroredDefinition->addError('Cannot autowire service "some_locator": it has type "Symfony\Component\DependencyInjection\Tests\Compiler\MissingClass" but this class was not found.'), $container->getDefinition('.errored.some_locator.'.MissingClass::class));
+        $this->assertSame(array('Cannot autowire service "some_locator": it has type "Symfony\Component\DependencyInjection\Tests\Compiler\MissingClass" but this class was not found.'), $container->getDefinition('.errored.some_locator.'.MissingClass::class)->getErrors());
     }
 
     public function testNamedArgumentAliasResolveCollisions()

src/Symfony/Component/DependencyInjection/Tests/DefinitionTest.php

@@ -375,7 +375,7 @@ public function testShouldAutoconfigure()
     public function testAddError()
     {
         $def = new Definition('stdClass');
-        $this->assertEmpty($def->getErrors());
+        $this->assertFalse($def->hasErrors());
         $def->addError('First error');
         $def->addError('Second error');
         $this->assertSame(array('First error', 'Second error'), $def->getErrors());