bug #22254 [DI] Don't use auto-registered services to populate type-candidates (nicolas-grekas)

This PR was merged into the 2.8 branch.

Discussion
----------

[DI] Don't use auto-registered services to populate type-candidates

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no (every bug fix is a bc break, isn't it?)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22162, ~~#21658~~
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

Alternative to #22170 and ~~#21665~~.

The core issue fixed here is that auto-registered services should *not* be used as type-candidates.
The culprit is this line:
`$this->populateAvailableType($argumentId, $argumentDefinition);`

Doing so creates a series of wtf-issues (the linked ones), with no simple fixes (the linked PRs are just dealing with the simplest cases, but the real fix would require a reboot of autowiring for every newly discovered types.)

The other changes accommodate for the removal of the population of the `types` property, and fix a few other issues found along the way:
- variadic constructors
- empty strings injection
- tail args removal when they match the existing defaults

Commits
-------

992c677534 [DI] Don't use auto-registered services to populate type-candidates

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

@@ -28,7 +28,7 @@ class AutowirePass implements CompilerPassInterface
     private $definedTypes = array();
     private $types;
     private $notGuessableTypes = array();
-    private $usedTypes = array();
+    private $autowired = array();
 
     /**
      * {@inheritdoc}
@@ -45,15 +45,6 @@ class AutowirePass implements CompilerPassInterface
                     $this->completeDefinition($id, $definition);
                 }
             }
-
-            foreach ($this->usedTypes as $type => $id) {
-                if (isset($this->usedTypes[$type]) && isset($this->notGuessableTypes[$type])) {
-                    $classOrInterface = class_exists($type) ? 'class' : 'interface';
-                    $matchingServices = implode(', ', $this->types[$type]);
-
-                    throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". Multiple services exist for this %s (%s).', $type, $id, $classOrInterface, $matchingServices));
-                }
-            }
         } catch (\Exception $e) {
         } catch (\Throwable $e) {
         }
@@ -66,7 +57,7 @@ class AutowirePass implements CompilerPassInterface
         $this->definedTypes = array();
         $this->types = null;
         $this->notGuessableTypes = array();
-        $this->usedTypes = array();
+        $this->autowired = array();
 
         if (isset($e)) {
             throw $e;
@@ -92,47 +83,56 @@ class AutowirePass implements CompilerPassInterface
         if (!$constructor = $reflectionClass->getConstructor()) {
             return;
         }
+        $parameters = $constructor->getParameters();
+        if (method_exists('ReflectionMethod', 'isVariadic') && $constructor->isVariadic()) {
+            array_pop($parameters);
+        }
 
         $arguments = $definition->getArguments();
-        foreach ($constructor->getParameters() as $index => $parameter) {
+        foreach ($parameters as $index => $parameter) {
             if (array_key_exists($index, $arguments) && '' !== $arguments[$index]) {
                 continue;
             }
 
             try {
                 if (!$typeHint = $parameter->getClass()) {
+                    if (isset($arguments[$index])) {
+                        continue;
+                    }
+
                     // no default value? Then fail
                     if (!$parameter->isOptional()) {
                         throw new RuntimeException(sprintf('Unable to autowire argument index %d ($%s) for the service "%s". If this is an object, give it a type-hint. Otherwise, specify this argument\'s value explicitly.', $index, $parameter->name, $id));
                     }
 
-                    if (!array_key_exists($index, $arguments)) {
+                    // specifically pass the default value
-                        // specifically pass the default value
+                    $arguments[$index] = $parameter->getDefaultValue();
-                        $arguments[$index] = $parameter->getDefaultValue();
-                    }
 
                     continue;
                 }
 
+                if (isset($this->autowired[$typeHint->name])) {
+                    return $this->autowired[$typeHint->name] ? new Reference($this->autowired[$typeHint->name]) : null;
+                }
+
                 if (null === $this->types) {
                     $this->populateAvailableTypes();
                 }
 
                 if (isset($this->types[$typeHint->name]) && !isset($this->notGuessableTypes[$typeHint->name])) {
                     $value = new Reference($this->types[$typeHint->name]);
-                    $this->usedTypes[$typeHint->name] = $id;
                 } else {
                     try {
                         $value = $this->createAutowiredDefinition($typeHint, $id);
-                        $this->usedTypes[$typeHint->name] = $id;
                     } catch (RuntimeException $e) {
-                        if ($parameter->allowsNull()) {
+                        if ($parameter->isDefaultValueAvailable()) {
-                            $value = null;
-                        } elseif ($parameter->isDefaultValueAvailable()) {
                             $value = $parameter->getDefaultValue();
+                        } elseif ($parameter->allowsNull()) {
+                            $value = null;
                         } else {
                             throw $e;
                         }
+                        $this->autowired[$typeHint->name] = false;
                     }
                 }
             } catch (\ReflectionException $e) {
@@ -148,6 +148,16 @@ class AutowirePass implements CompilerPassInterface
             $arguments[$index] = $value;
         }
 
+        if ($parameters && !isset($arguments[++$index])) {
+            while (0 <= --$index) {
+                $parameter = $parameters[$index];
+                if (!$parameter->isDefaultValueAvailable() || $parameter->getDefaultValue() !== $arguments[$index]) {
+                    break;
+                }
+                unset($arguments[$index]);
+            }
+        }
+
         // it's possible index 1 was set, then index 0, then 2, etc
         // make sure that we re-order so they're injected as expected
         ksort($arguments);
@@ -252,13 +262,11 @@ class AutowirePass implements CompilerPassInterface
             throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". No services were found matching this %s and it cannot be auto-registered.', $typeHint->name, $id, $classOrInterface));
         }
 
-        $argumentId = sprintf('autowired.%s', $typeHint->name);
+        $this->autowired[$typeHint->name] = $argumentId = sprintf('autowired.%s', $typeHint->name);
 
         $argumentDefinition = $this->container->register($argumentId, $typeHint->name);
         $argumentDefinition->setPublic(false);
 
-        $this->populateAvailableType($argumentId, $argumentDefinition);
-
         try {
             $this->completeDefinition($argumentId, $argumentDefinition);
         } catch (RuntimeException $e) {

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

@@ -422,10 +422,6 @@ class AutowirePassTest extends TestCase
             array(
                 new Reference('a'),
                 new Reference('lille'),
-                // third arg shouldn't *need* to be passed
-                // but that's hard to "pull of" with autowiring, so
-                // this assumes passing the default val is ok
-                'some_val',
             ),
             $definition->getArguments()
         );
@@ -462,23 +458,6 @@ class AutowirePassTest extends TestCase
         $this->assertEquals(array(new Reference('a'), '', new Reference('lille')), $container->getDefinition('foo')->getArguments());
     }
 
-    /**
-     * @dataProvider provideAutodiscoveredAutowiringOrder
-     *
-     * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
-     * @expectedExceptionMEssage Unable to autowire argument of type "Symfony\Component\DependencyInjection\Tests\Compiler\CollisionInterface" for the service "a". Multiple services exist for this interface (autowired.Symfony\Component\DependencyInjection\Tests\Compiler\CollisionA, autowired.Symfony\Component\DependencyInjection\Tests\Compiler\CollisionB).
-     */
-    public function testAutodiscoveredAutowiringOrder($class)
-    {
-        $container = new ContainerBuilder();
-
-        $container->register('a', __NAMESPACE__.'\\'.$class)
-            ->setAutowired(true);
-
-        $pass = new AutowirePass();
-        $pass->process($container);
-    }
-
     public function provideAutodiscoveredAutowiringOrder()
     {
         return array(