bug #22496 [DI] Fix inlining conflict by restricting IteratorArgument…

… to Reference[] (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Fix inlining conflict by restricting IteratorArgument to Reference[]

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

`Reference` found in `ArgumentInterface::getValue()` are currently not inlined.
While trying to do so (hint: I failed), I noticed that the current code is broken for `IteratorArgument` which can contain anonymous `Definition` for now, which are then not inlined correctly.

This PR restricts `IteratorArgument` to arrays of `Reference`, and improves a few related things found while doing it.

(fabbot failure is false positive)

Commits
-------

4d3dce1 [DI] Fix inlining conflict by restricting IteratorArgument to Reference[]

src/Symfony/Component/DependencyInjection/Argument/IteratorArgument.php

@@ -11,6 +11,9 @@
 
 namespace Symfony\Component\DependencyInjection\Argument;
 
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+use Symfony\Component\DependencyInjection\Reference;
+
 /**
  * Represents a collection of values to lazily iterate over.
  *
@@ -20,9 +23,12 @@ class IteratorArgument implements ArgumentInterface
 {
     private $values;
 
+    /**
+     * @param Reference[] $values
+     */
     public function __construct(array $values)
     {
-        $this->values = $values;
+        $this->setValues($values);
     }
 
     /**
@@ -34,10 +40,16 @@ public function getValues()
     }
 
     /**
-     * @param array $values The values to lazily iterate over
+     * @param Reference[] $values The service references to lazily iterate over
      */
     public function setValues(array $values)
     {
+        foreach ($values as $k => $v) {
+            if (null !== $v && !$v instanceof Reference) {
+                throw new InvalidArgumentException(sprintf('An IteratorArgument must hold only Reference instances, "%s" given.', is_object($v) ? get_class($v) : gettype($v)));
+            }
+        }
+
         $this->values = $values;
     }
 }

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

@@ -38,8 +38,7 @@ public function setRepeatedPass(RepeatedPass $repeatedPass)
     protected function processValue($value, $isRoot = false)
     {
         if ($value instanceof ArgumentInterface) {
-            $this->processValue($value->getValues());
-
+            // Reference found in ArgumentInterface::getValues() are not inlineable
             return $value;
         }
         if ($value instanceof Reference && $this->container->hasDefinition($id = (string) $value)) {

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -1135,16 +1135,15 @@ public function resolveServices($value)
                 return $this->resolveServices($reference);
             };
         } elseif ($value instanceof IteratorArgument) {
-            $parameterBag = $this->getParameterBag();
-            $value = new RewindableGenerator(function () use ($value, $parameterBag) {
+            $value = new RewindableGenerator(function () use ($value) {
                 foreach ($value->getValues() as $k => $v) {
                     foreach (self::getServiceConditionals($v) as $s) {
                         if (!$this->has($s)) {
                             continue 2;
                         }
                     }
 
-                    yield $k => $this->resolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($v)));
+                    yield $k => $this->resolveServices($v);
                 }
             }, function () use ($value) {
                 $count = 0;
@@ -1444,7 +1443,7 @@ private function callMethod($service, $call)
      * Shares a given service in the container.
      *
      * @param Definition  $definition
-     * @param mixed       $service
+     * @param object      $service
      * @param string|null $id
      */
     private function shareService(Definition $definition, $service, $id)

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

@@ -202,22 +202,20 @@ private function getProxyDumper()
     /**
      * Generates Service local temp variables.
      *
-     * @param string $cId
-     * @param string $definition
+     * @param string     $cId
+     * @param Definition $definition
+     * @param array      $inlinedDefinitions
      *
      * @return string
      */
-    private function addServiceLocalTempVariables($cId, $definition)
+    private function addServiceLocalTempVariables($cId, Definition $definition, array $inlinedDefinitions)
     {
         static $template = "        \$%s = %s;\n";
 
-        $localDefinitions = array_merge(
-            array($definition),
-            $this->getInlinedDefinitions($definition)
-        );
+        array_unshift($inlinedDefinitions, $definition);
 
         $calls = $behavior = array();
-        foreach ($localDefinitions as $iDefinition) {
+        foreach ($inlinedDefinitions as $iDefinition) {
             $this->getServiceCallsFromArguments($iDefinition->getArguments(), $calls, $behavior);
             $this->getServiceCallsFromArguments($iDefinition->getMethodCalls(), $calls, $behavior);
             $this->getServiceCallsFromArguments($iDefinition->getProperties(), $calls, $behavior);
@@ -280,12 +278,13 @@ private function addProxyClasses()
     /**
      * Generates the require_once statement for service includes.
      *
-     * @param string     $id         The service id
+     * @param string     $id
      * @param Definition $definition
+     * @param array      $inlinedDefinitions
      *
      * @return string
      */
-    private function addServiceInclude($id, $definition)
+    private function addServiceInclude($id, Definition $definition, array $inlinedDefinitions)
     {
         $template = "        require_once %s;\n";
         $code = '';
@@ -294,7 +293,7 @@ private function addServiceInclude($id, $definition)
             $code .= sprintf($template, $this->dumpValue($file));
         }
 
-        foreach ($this->getInlinedDefinitions($definition) as $definition) {
+        foreach ($inlinedDefinitions as $definition) {
             if (null !== $file = $definition->getFile()) {
                 $code .= sprintf($template, $this->dumpValue($file));
             }
@@ -310,21 +309,20 @@ private function addServiceInclude($id, $definition)
     /**
      * Generates the inline definition of a service.
      *
-     * @param string     $id
-     * @param Definition $definition
+     * @param string $id
+     * @param array  $inlinedDefinitions
      *
      * @return string
      *
      * @throws RuntimeException                  When the factory definition is incomplete
      * @throws ServiceCircularReferenceException When a circular reference is detected
      */
-    private function addServiceInlinedDefinitions($id, $definition)
+    private function addServiceInlinedDefinitions($id, array $inlinedDefinitions)
     {
         $code = '';
         $variableMap = $this->definitionVariables;
         $nbOccurrences = new \SplObjectStorage();
         $processed = new \SplObjectStorage();
-        $inlinedDefinitions = $this->getInlinedDefinitions($definition);
 
         foreach ($inlinedDefinitions as $definition) {
             if (false === $nbOccurrences->contains($definition)) {
@@ -375,14 +373,14 @@ private function addServiceInlinedDefinitions($id, $definition)
     /**
      * Adds the service return statement.
      *
-     * @param string     $id         Service id
-     * @param Definition $definition
+     * @param string $id
+     * @param bool   $isSimpleInstance
      *
      * @return string
      */
-    private function addServiceReturn($id, $definition)
+    private function addServiceReturn($id, $isSimpleInstance)
     {
-        if ($this->isSimpleInstance($id, $definition)) {
+        if ($isSimpleInstance) {
             return "    }\n";
         }
 
@@ -394,13 +392,14 @@ private function addServiceReturn($id, $definition)
      *
      * @param string     $id
      * @param Definition $definition
+     * @param bool       $isSimpleInstance
      *
      * @return string
      *
      * @throws InvalidArgumentException
      * @throws RuntimeException
      */
-    private function addServiceInstance($id, Definition $definition)
+    private function addServiceInstance($id, Definition $definition, $isSimpleInstance)
     {
         $class = $definition->getClass();
 
@@ -414,26 +413,25 @@ private function addServiceInstance($id, Definition $definition)
             throw new InvalidArgumentException(sprintf('"%s" is not a valid class name for the "%s" service.', $class, $id));
         }
 
-        $simple = $this->isSimpleInstance($id, $definition);
         $isProxyCandidate = $this->getProxyDumper()->isProxyCandidate($definition);
         $instantiation = '';
 
         if (!$isProxyCandidate && $definition->isShared()) {
-            $instantiation = "\$this->services['$id'] = ".($simple ? '' : '$instance');
-        } elseif (!$simple) {
+            $instantiation = "\$this->services['$id'] = ".($isSimpleInstance ? '' : '$instance');
+        } elseif (!$isSimpleInstance) {
             $instantiation = '$instance';
         }
 
         $return = '';
-        if ($simple) {
+        if ($isSimpleInstance) {
             $return = 'return ';
         } else {
             $instantiation .= ' = ';
         }
 
         $code = $this->addNewInstance($definition, $return, $instantiation, $id);
 
-        if (!$simple) {
+        if (!$isSimpleInstance) {
             $code .= "\n";
         }
 
@@ -500,20 +498,21 @@ private function addServiceProperties($id, Definition $definition, $variableName
     /**
      * Generates the inline definition setup.
      *
-     * @param string     $id
-     * @param Definition $definition
+     * @param string $id
+     * @param array  $inlinedDefinitions
+     * @param bool   $isSimpleInstance
      *
      * @return string
      *
      * @throws ServiceCircularReferenceException when the container contains a circular reference
      */
-    private function addServiceInlinedDefinitionsSetup($id, Definition $definition)
+    private function addServiceInlinedDefinitionsSetup($id, array $inlinedDefinitions, $isSimpleInstance)
     {
         $this->referenceVariables[$id] = new Variable('instance');
 
         $code = '';
         $processed = new \SplObjectStorage();
-        foreach ($this->getInlinedDefinitions($definition) as $iDefinition) {
+        foreach ($inlinedDefinitions as $iDefinition) {
             if ($processed->contains($iDefinition)) {
                 continue;
             }
@@ -525,7 +524,7 @@ private function addServiceInlinedDefinitionsSetup($id, Definition $definition)
 
             // if the instance is simple, the return statement has already been generated
             // so, the only possible way to get there is because of a circular reference
-            if ($this->isSimpleInstance($id, $definition)) {
+            if ($isSimpleInstance) {
                 throw new ServiceCircularReferenceException($id, array($id));
             }
 
@@ -683,16 +682,19 @@ private function addService($id, Definition $definition)
             $code .= sprintf("        @trigger_error(%s, E_USER_DEPRECATED);\n\n", $this->export($definition->getDeprecationMessage($id)));
         }
 
+        $inlinedDefinitions = $this->getInlinedDefinitions($definition);
+        $isSimpleInstance = $this->isSimpleInstance($id, $definition, $inlinedDefinitions);
+
         $code .=
-            $this->addServiceInclude($id, $definition).
-            $this->addServiceLocalTempVariables($id, $definition).
-            $this->addServiceInlinedDefinitions($id, $definition).
-            $this->addServiceInstance($id, $definition).
-            $this->addServiceInlinedDefinitionsSetup($id, $definition).
+            $this->addServiceInclude($id, $definition, $inlinedDefinitions).
+            $this->addServiceLocalTempVariables($id, $definition, $inlinedDefinitions).
+            $this->addServiceInlinedDefinitions($id, $inlinedDefinitions).
+            $this->addServiceInstance($id, $definition, $isSimpleInstance).
+            $this->addServiceInlinedDefinitionsSetup($id, $inlinedDefinitions, $isSimpleInstance).
             $this->addServiceProperties($id, $definition).
             $this->addServiceMethodCalls($id, $definition).
             $this->addServiceConfigurator($id, $definition).
-            $this->addServiceReturn($id, $definition)
+            $this->addServiceReturn($id, $isSimpleInstance)
         ;
 
         $this->definitionVariables = null;
@@ -1331,8 +1333,6 @@ private function getDefinitionsFromArguments(array $arguments)
         foreach ($arguments as $argument) {
             if (is_array($argument)) {
                 $definitions = array_merge($definitions, $this->getDefinitionsFromArguments($argument));
-            } elseif ($argument instanceof ArgumentInterface) {
-                $definitions = array_merge($definitions, $this->getDefinitionsFromArguments($argument->getValues()));
             } elseif ($argument instanceof Definition) {
                 $definitions = array_merge(
                     $definitions,

src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

@@ -490,7 +490,12 @@ private function getArgumentsAsPhp(\DOMElement $node, $name, $lowercase = true,
                     $arguments[$key] = $this->getArgumentsAsPhp($arg, $name, false);
                     break;
                 case 'iterator':
-                    $arguments[$key] = new IteratorArgument($this->getArgumentsAsPhp($arg, $name, false));
+                    $arg = $this->getArgumentsAsPhp($arg, $name, false);
+                    try {
+                        $arguments[$key] = new IteratorArgument($arg);
+                    } catch (InvalidArgumentException $e) {
+                        throw new InvalidArgumentException(sprintf('Tag "<%s>" with type="iterator" only accepts collections of type="service" references.', $name));
+                    }
                     break;
                 case 'string':
                     $arguments[$key] = $arg->nodeValue;

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

@@ -665,14 +665,18 @@ private function resolveServices($value, $file, $isParameter = false)
             $argument = $value->getValue();
             if ('iterator' === $value->getTag()) {
                 if (!is_array($argument)) {
-                    throw new InvalidArgumentException('"!iterator" tag only accepts sequences.');
+                    throw new InvalidArgumentException(sprintf('"!iterator" tag only accepts sequences in "%s".', $file));
+                }
+                $argument = $this->resolveServices($argument, $file, $isParameter);
+                try {
+                    return new IteratorArgument($argument);
+                } catch (InvalidArgumentException $e) {
+                    throw new InvalidArgumentException(sprintf('"!iterator" tag only accepts arrays of "@service" references in "%s".', $file));
                 }
-
-                return new IteratorArgument($this->resolveServices($argument, $file, $isParameter));
             }
             if ('closure_proxy' === $value->getTag()) {
                 if (!is_array($argument) || array(0, 1) !== array_keys($argument) || !is_string($argument[0]) || !is_string($argument[1]) || 0 !== strpos($argument[0], '@') || 0 === strpos($argument[0], '@@')) {
-                    throw new InvalidArgumentException('"!closure_proxy" tagged values must be arrays of [@service, method].');
+                    throw new InvalidArgumentException(sprintf('"!closure_proxy" tagged values must be arrays of [@service, method] in "%s".', $file));
                 }
 
                 if (0 === strpos($argument[0], '@?')) {

src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php

@@ -434,7 +434,7 @@ public function testLazyArgumentProvideGenerator()
         $container->register('lazy_referenced', 'stdClass');
         $container
             ->register('lazy_context', 'LazyContext')
-            ->setArguments(array(new IteratorArgument(array('foo', new Reference('lazy_referenced'), 'k1' => array('foo' => 'bar'), true, 'k2' => new Reference('service_container')))))
+            ->setArguments(array(new IteratorArgument(array('k1' => new Reference('lazy_referenced'), 'k2' => new Reference('service_container')))))
         ;
         $container->compile();
 
@@ -450,24 +450,12 @@ public function testLazyArgumentProvideGenerator()
         foreach ($lazyContext->lazyValues as $k => $v) {
             switch (++$i) {
                 case 0:
-                    $this->assertEquals(0, $k);
-                    $this->assertEquals('foo', $v);
-                    break;
-                case 1:
-                    $this->assertEquals(1, $k);
-                    $this->assertInstanceOf('stdClass', $v);
-                    break;
-                case 2:
                     $this->assertEquals('k1', $k);
-                    $this->assertEquals(array('foo' => 'bar'), $v);
+                    $this->assertInstanceOf('stdCLass', $v);
                     break;
-                case 3:
-                    $this->assertEquals(2, $k);
-                    $this->assertTrue($v);
-                    break;
-                case 4:
+                case 1:
                     $this->assertEquals('k2', $k);
-                    $this->assertInstanceOf('\Symfony_DI_PhpDumper_Test_Lazy_Argument_Provide_Generator', $v);
+                    $this->assertInstanceOf('Symfony_DI_PhpDumper_Test_Lazy_Argument_Provide_Generator', $v);
                     break;
             }
         }

src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container9.php

@@ -134,7 +134,7 @@
 ;
 $container
     ->register('lazy_context', 'LazyContext')
-    ->setArguments(array(new IteratorArgument(array('foo', new Reference('foo.baz'), array('%foo%' => 'foo is %foo%', 'foobar' => '%foo%'), true, new Reference('service_container')))))
+    ->setArguments(array(new IteratorArgument(array('k1' => new Reference('foo.baz'), 'k2' => new Reference('service_container')))))
 ;
 $container
     ->register('lazy_context_ignore_invalid_ref', 'LazyContext')

src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9.php

@@ -321,12 +321,9 @@ protected function getFooWithInlineService()
     protected function getLazyContextService()
     {
         return $this->services['lazy_context'] = new \LazyContext(new RewindableGenerator(function () {
-            yield 0 => 'foo';
-            yield 1 => ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->get('foo.baz')) && false ?: '_'};
-            yield 2 => array($this->getParameter('foo') => 'foo is '.$this->getParameter('foo').'', 'foobar' => $this->getParameter('foo'));
-            yield 3 => true;
-            yield 4 => $this;
-        }, 5));
+            yield 'k1' => ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->get('foo.baz')) && false ?: '_'};
+            yield 'k2' => $this;
+        }, 2));
     }
 
     /**