[DependencyInjection] Optimize autowiring logic by telling it about e…

…xcluded symbols
symfony · nicolas-grekas · May 27, 2022 · May 6, 2022 · May 8, 2022 · 739789f384cca3b06a3b5f35793e116fff337473
commit 739789f384cca3b06a3b5f35793e116fff337473
diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/UnusedTagsPass.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/UnusedTagsPass.php
@@ -33,6 +33,7 @@ class UnusedTagsPass implements CompilerPassInterface
         'console.command',
         'container.env_var_loader',
         'container.env_var_processor',
+        'container.excluded',
         'container.hot_path',
         'container.no_preload',
         'container.preload',

diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/services.php b/src/Symfony/Bundle/FrameworkBundle/Resources/config/services.php
@@ -13,6 +13,7 @@
 
 use Symfony\Bundle\FrameworkBundle\CacheWarmer\ConfigBuilderCacheWarmer;
 use Symfony\Bundle\FrameworkBundle\HttpCache\HttpCache;
+use Symfony\Component\Config\Loader\LoaderInterface;
 use Symfony\Component\Config\Resource\SelfCheckingResourceChecker;
 use Symfony\Component\Config\ResourceCheckerConfigCacheFactory;
 use Symfony\Component\Console\ConsoleEvents;
@@ -26,7 +27,10 @@
 use Symfony\Component\EventDispatcher\EventDispatcherInterface as EventDispatcherInterfaceComponentAlias;
 use Symfony\Component\Filesystem\Filesystem;
 use Symfony\Component\Form\FormEvents;
+use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\RequestStack;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpFoundation\Session\SessionInterface;
 use Symfony\Component\HttpFoundation\UrlHelper;
 use Symfony\Component\HttpKernel\CacheClearer\ChainCacheClearer;
 use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerAggregate;
@@ -218,5 +222,11 @@ class_exists(WorkflowEvents::class) ? WorkflowEvents::ALIASES : []
         ->set('config_builder.warmer', ConfigBuilderCacheWarmer::class)
             ->args([service(KernelInterface::class), service('logger')->nullOnInvalid()])
             ->tag('kernel.cache_warmer')
+
+        // register as abstract and excluded, aka not-autowirable types
+        ->set(LoaderInterface::class)->abstract()->tag('container.excluded')
+        ->set(Request::class)->abstract()->tag('container.excluded')
+        ->set(Response::class)->abstract()->tag('container.excluded')
+        ->set(SessionInterface::class)->abstract()->tag('container.excluded')
     ;
 };
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php b/src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php
@@ -75,6 +75,9 @@ protected function processValue(mixed $value, bool $isRoot = false)
         if (\is_array($value)) {
             foreach ($value as $k => $v) {
                 if ($isRoot) {
+                    if ($v->hasTag('container.excluded')) {
+                        continue;
+                    }
                     $this->currentId = $k;
                 }
                 if ($v !== $processedValue = $this->processValue($v, $isRoot)) {

@@ -503,7 +503,19 @@ private function createTypeNotFoundMessageCallback(TypedReference $reference, st
 
     private function createTypeNotFoundMessage(TypedReference $reference, string $label, string $currentId): string
     {
-        if (!$r = $this->container->getReflectionClass($type = $reference->getType(), false)) {
+        $type = $reference->getType();
+
+        $i = null;
+        $namespace = $type;
+        do {
+            $namespace = substr($namespace, 0, $i);
+
+            if ($this->container->hasDefinition($namespace) && $tag = $this->container->getDefinition($namespace)->getTag('container.excluded')) {
+                return sprintf('Cannot autowire service "%s": %s needs an instance of "%s" but this type has been excluded %s.', $currentId, $label, $type, $tag[0]['source'] ?? 'from autowiring');
+            }
+        } while (false !== $i = strrpos($namespace, '\\'));
+
+        if (!$r = $this->container->getReflectionClass($type, false)) {
             // either $type does not exist or a parent class does not exist
             try {
                 $resource = new ClassExistenceResource($type, false);

diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php
@@ -598,7 +598,11 @@ public function merge(self $container)
             throw new BadMethodCallException('Cannot merge on a compiled container.');
         }
 
-        $this->addDefinitions($container->getDefinitions());
+        foreach ($container->getDefinitions() as $id => $definition) {
+            if (!$definition->hasTag('container.excluded') || !$this->has($id)) {
+                $this->setDefinition($id, $definition);
+            }
+        }
         $this->addAliases($container->getAliases());
         $this->getParameterBag()->add($container->getParameterBag()->all());
 

diff --git a/src/Symfony/Component/DependencyInjection/Loader/Configurator/PrototypeConfigurator.php b/src/Symfony/Component/DependencyInjection/Loader/Configurator/PrototypeConfigurator.php
@@ -41,8 +41,9 @@ class PrototypeConfigurator extends AbstractServiceConfigurator
     private string $resource;
     private ?array $excludes = null;
     private bool $allowParent;
+    private ?string $path;
 
-    public function __construct(ServicesConfigurator $parent, PhpFileLoader $loader, Definition $defaults, string $namespace, string $resource, bool $allowParent)
+    public function __construct(ServicesConfigurator $parent, PhpFileLoader $loader, Definition $defaults, string $namespace, string $resource, bool $allowParent, string $path = null)
     {
         $definition = new Definition();
         if (!$defaults->isPublic() || !$defaults->isPrivate()) {
@@ -57,6 +58,7 @@ public function __construct(ServicesConfigurator $parent, PhpFileLoader $loader,
         $this->loader = $loader;
         $this->resource = $resource;
         $this->allowParent = $allowParent;
+        $this->path = $path;
 
         parent::__construct($parent, $definition, $namespace, $defaults->getTags());
     }
@@ -66,7 +68,7 @@ public function __destruct()
         parent::__destruct();
 
         if (isset($this->loader)) {
-            $this->loader->registerClasses($this->definition, $this->id, $this->resource, $this->excludes);
+            $this->loader->registerClasses($this->definition, $this->id, $this->resource, $this->excludes, $this->path);
         }
         unset($this->loader);
     }

diff --git a/src/Symfony/Component/DependencyInjection/Loader/Configurator/ServicesConfigurator.php b/src/Symfony/Component/DependencyInjection/Loader/Configurator/ServicesConfigurator.php
@@ -129,7 +129,7 @@ final public function alias(string $id, string $referencedId): AliasConfigurator
      */
     final public function load(string $namespace, string $resource): PrototypeConfigurator
     {
-        return new PrototypeConfigurator($this, $this->loader, $this->defaults, $namespace, $resource, true);
+        return new PrototypeConfigurator($this, $this->loader, $this->defaults, $namespace, $resource, true, $this->path);
     }
 
     /**

diff --git a/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
@@ -90,8 +90,9 @@ public function import(mixed $resource, string $type = null, bool|string $ignore
      * @param string               $namespace The namespace prefix of classes in the scanned directory
      * @param string               $resource  The directory to look for classes, glob-patterns allowed
      * @param string|string[]|null $exclude   A globbed path of files to exclude or an array of globbed paths of files to exclude
+     * @param string|null          $source    The path to the file that defines the auto-discovery rule
      */
-    public function registerClasses(Definition $prototype, string $namespace, string $resource, string|array $exclude = null)
+    public function registerClasses(Definition $prototype, string $namespace, string $resource, string|array $exclude = null/*, string $source = null*/)
     {
         if (!str_ends_with($namespace, '\\')) {
             throw new InvalidArgumentException(sprintf('Namespace prefix must end with a "\\": "%s".', $namespace));
@@ -100,9 +101,10 @@ public function registerClasses(Definition $prototype, string $namespace, string
             throw new InvalidArgumentException(sprintf('Namespace is not a valid PSR-4 prefix: "%s".', $namespace));
         }
 
+        $source = \func_num_args() > 4 ? func_get_arg(4) : null;
         $autoconfigureAttributes = new RegisterAutoconfigureAttributesPass();
         $autoconfigureAttributes = $autoconfigureAttributes->accept($prototype) ? $autoconfigureAttributes : null;
-        $classes = $this->findClasses($namespace, $resource, (array) $exclude, $autoconfigureAttributes);
+        $classes = $this->findClasses($namespace, $resource, (array) $exclude, $autoconfigureAttributes, $source);
         // prepare for deep cloning
         $serializedPrototype = serialize($prototype);
 
@@ -169,7 +171,7 @@ protected function setDefinition(string $id, Definition $definition)
         }
     }
 
-    private function findClasses(string $namespace, string $pattern, array $excludePatterns, ?RegisterAutoconfigureAttributesPass $autoconfigureAttributes): array
+    private function findClasses(string $namespace, string $pattern, array $excludePatterns, ?RegisterAutoconfigureAttributesPass $autoconfigureAttributes, ?string $source): array
     {
         $parameterBag = $this->container->getParameterBag();
 
@@ -189,7 +191,6 @@ private function findClasses(string $namespace, string $pattern, array $excludeP
 
         $pattern = $parameterBag->unescapeValue($parameterBag->resolveValue($pattern));
         $classes = [];
-        $extRegexp = '/\\.php$/';
         $prefixLen = null;
         foreach ($this->glob($pattern, true, $resource, false, false, $excludePaths) as $path => $info) {
             if (null === $prefixLen) {
@@ -204,10 +205,10 @@ private function findClasses(string $namespace, string $pattern, array $excludeP
                 continue;
             }
 
-            if (!preg_match($extRegexp, $path, $m) || !$info->isReadable()) {
+            if (!str_ends_with($path, '.php') || !$info->isReadable()) {
                 continue;
             }
-            $class = $namespace.ltrim(str_replace('/', '\\', substr($path, $prefixLen, -\strlen($m[0]))), '\\');
+            $class = $namespace.ltrim(str_replace('/', '\\', substr($path, $prefixLen, -4)), '\\');
 
             if (!preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+(?:\\\\[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+)*+$/', $class)) {
                 continue;
@@ -242,6 +243,19 @@ private function findClasses(string $namespace, string $pattern, array $excludeP
             }
         }
 
+        if (null !== $prefixLen) {
+            $attributes = null !== $source ? ['source' => sprintf('in "%s/%s"', basename(\dirname($source)), basename($source))] : [];
+
+            foreach ($excludePaths as $path => $_) {
+                $class = $namespace.ltrim(str_replace('/', '\\', substr($path, $prefixLen, str_ends_with($path, '.php') ? -4 : null)), '\\');
+                if (!$this->container->has($class)) {
+                    $this->container->register($class)
+                        ->setAbstract(true)
+                        ->addTag('container.excluded', $attributes);
+                }
+            }
+        }
+
         return $classes;
     }
 }
diff --git a/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
@@ -184,7 +184,7 @@ private function parseDefinitions(\DOMDocument $xml, string $file, Definition $d
                         }
                         $excludes = [$service->getAttribute('exclude')];
                     }
-                    $this->registerClasses($definition, (string) $service->getAttribute('namespace'), (string) $service->getAttribute('resource'), $excludes);
+                    $this->registerClasses($definition, (string) $service->getAttribute('namespace'), (string) $service->getAttribute('resource'), $excludes, $file);
                 } else {
                     $this->setDefinition((string) $service->getAttribute('id'), $definition);
                 }

diff --git a/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
@@ -692,7 +692,7 @@ private function parseDefinition(string $id, array|string|null $service, string
             }
             $exclude = $service['exclude'] ?? null;
             $namespace = $service['namespace'] ?? $id;
-            $this->registerClasses($definition, $namespace, $service['resource'], $exclude);
+            $this->registerClasses($definition, $namespace, $service['resource'], $exclude, $file);
         } else {
             $this->setDefinition($id, $definition);
         }

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php
@@ -36,9 +36,6 @@
 
 require_once __DIR__.'/../Fixtures/includes/autowiring_classes.php';
 
-/**
- * @author Kévin Dunglas <dunglas@gmail.com>
- */
 class AutowirePassTest extends TestCase
 {
     public function testProcess()
@@ -1186,4 +1183,38 @@ public function testAsDecoratorAttribute()
         $this->assertSame(AsDecoratorBaz::class.'.inner', (string) $container->getDefinition(AsDecoratorBaz::class)->getArgument(0));
         $this->assertSame(2, $container->getDefinition(AsDecoratorBaz::class)->getArgument(0)->getInvalidBehavior());
     }
+
+    public function testTypeSymbolExcluded()
+    {
+        $container = new ContainerBuilder();
+
+        $container->register(Foo::class)->setAbstract(true)->addTag('container.excluded', ['source' => 'for tests']);
+        $aDefinition = $container->register('a', NotGuessableArgument::class);
+        $aDefinition->setAutowired(true);
+
+        $pass = new AutowirePass();
+        try {
+            $pass->process($container);
+            $this->fail('AutowirePass should have thrown an exception');
+        } catch (AutowiringFailedException $e) {
+            $this->assertSame('Cannot autowire service "a": argument "$k" of method "Symfony\Component\DependencyInjection\Tests\Compiler\NotGuessableArgument::__construct()" needs an instance of "Symfony\Component\DependencyInjection\Tests\Compiler\Foo" but this type has been excluded for tests.', (string) $e->getMessage());
+        }
+    }
+
+    public function testTypeNamespaceExcluded()
+    {
+        $container = new ContainerBuilder();
+
+        $container->register(__NAMESPACE__)->setAbstract(true)->addTag('container.excluded');
+        $aDefinition = $container->register('a', NotGuessableArgument::class);
+        $aDefinition->setAutowired(true);
+
+        $pass = new AutowirePass();
+        try {
+            $pass->process($container);
+            $this->fail('AutowirePass should have thrown an exception');
+        } catch (AutowiringFailedException $e) {
+            $this->assertSame('Cannot autowire service "a": argument "$k" of method "Symfony\Component\DependencyInjection\Tests\Compiler\NotGuessableArgument::__construct()" needs an instance of "Symfony\Component\DependencyInjection\Tests\Compiler\Foo" but this type has been excluded from autowiring.', (string) $e->getMessage());
+        }
+    }
 }
diff --git a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
@@ -633,6 +633,21 @@ public function testMerge()
         $this->assertSame(['AInterface' => $childDefA, 'BInterface' => $childDefB], $container->getAutoconfiguredInstanceof());
     }
 
+    public function testMergeWithExcludedServices()
+    {
+        $container = new ContainerBuilder();
+        $container->setAlias('bar', 'foo');
+        $container->register('foo', 'Bar\FooClass');
+        $config = new ContainerBuilder();
+        $config->register('bar', 'Bar')->addTag('container.excluded');
+        $config->register('foo', 'Bar')->addTag('container.excluded');
+        $config->register('baz', 'Bar')->addTag('container.excluded');
+        $container->merge($config);
+        $this->assertEquals(['service_container', 'foo', 'baz'], array_keys($container->getDefinitions()));
+        $this->assertFalse($container->getDefinition('foo')->hasTag('container.excluded'));
+        $this->assertTrue($container->getDefinition('baz')->hasTag('container.excluded'));
+    }
+
     public function testMergeThrowsExceptionForDuplicateAutomaticInstanceofDefinitions()
     {
         $this->expectException(InvalidArgumentException::class);

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Loader/FileLoaderTest.php b/src/Symfony/Component/DependencyInjection/Tests/Loader/FileLoaderTest.php
@@ -27,7 +27,7 @@
 use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\BadClasses\MissingParent;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Foo;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\FooInterface;
-use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\OtherDir\AnotherSub\DeeperBaz;
+use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\OtherDir\AnotherSub;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\OtherDir\Baz;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\Bar;
 use Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\Sub\BarInterface;
@@ -116,10 +116,15 @@ public function testRegisterClassesWithExclude()
             'Prototype/{%other_dir%/AnotherSub,Foo.php}'
         );
 
-        $this->assertTrue($container->has(Bar::class));
-        $this->assertTrue($container->has(Baz::class));
-        $this->assertFalse($container->has(Foo::class));
-        $this->assertFalse($container->has(DeeperBaz::class));
+        $this->assertFalse($container->getDefinition(Bar::class)->isAbstract());
+        $this->assertFalse($container->getDefinition(Baz::class)->isAbstract());
+        $this->assertTrue($container->getDefinition(Foo::class)->isAbstract());
+        $this->assertTrue($container->getDefinition(AnotherSub::class)->isAbstract());
+
+        $this->assertFalse($container->getDefinition(Bar::class)->hasTag('container.excluded'));
+        $this->assertFalse($container->getDefinition(Baz::class)->hasTag('container.excluded'));
+        $this->assertTrue($container->getDefinition(Foo::class)->hasTag('container.excluded'));
+        $this->assertTrue($container->getDefinition(AnotherSub::class)->hasTag('container.excluded'));
 
         $this->assertEquals([BarInterface::class], array_keys($container->getAliases()));
 

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php b/src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php
@@ -718,7 +718,7 @@ public function testPrototype()
         $loader = new XmlFileLoader($container, new FileLocator(self::$fixturesPath.'/xml'));
         $loader->load('services_prototype.xml');
 
-        $ids = array_keys($container->getDefinitions());
+        $ids = array_keys(array_filter($container->getDefinitions(), fn ($def) => !$def->hasTag('container.excluded')));
         sort($ids);
         $this->assertSame([Prototype\Foo::class, Prototype\Sub\Bar::class, 'service_container'], $ids);
 
@@ -750,7 +750,7 @@ public function testPrototypeExcludeWithArray()
         $loader = new XmlFileLoader($container, new FileLocator(self::$fixturesPath.'/xml'));
         $loader->load('services_prototype_array.xml');
 
-        $ids = array_keys($container->getDefinitions());
+        $ids = array_keys(array_filter($container->getDefinitions(), fn ($def) => !$def->hasTag('container.excluded')));
         sort($ids);
         $this->assertSame([Prototype\Foo::class, Prototype\Sub\Bar::class, 'service_container'], $ids);
 

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php b/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php
@@ -497,7 +497,7 @@ public function testPrototype()
         $loader = new YamlFileLoader($container, new FileLocator(self::$fixturesPath.'/yaml'));
         $loader->load('services_prototype.yml');
 
-        $ids = array_keys($container->getDefinitions());
+        $ids = array_keys(array_filter($container->getDefinitions(), fn ($def) => !$de
729A
f->hasTag('container.excluded')));
         sort($ids);
         $this->assertSame([Prototype\Foo::class, Prototype\Sub\Bar::class, 'service_container'], $ids);
 
@@ -528,7 +528,7 @@ public function testPrototypeWithNamespace()
         $loader = new YamlFileLoader($container, new FileLocator(self::$fixturesPath.'/yaml'));
         $loader->load('services_prototype_namespace.yml');
 
-        $ids = array_keys($container->getDefinitions());
+        $ids = array_keys(array_filter($container->getDefinitions(), fn ($def) => !$def->hasTag('container.excluded')));
         sort($ids);
 
         $this->assertSame([
-Original file line number
+Diff line change
@@ Expand Up @@
          */
         final public function load(string $namespace, string $resource): PrototypeConfigurator
         {
-            return new PrototypeConfigurator($this, $this->loader, $this->defaults, $namespace, $resource, true);
+            return new PrototypeConfigurator($this, $this->loader, $this->defaults, $namespace, $resource, true, $this->path);
         }
         /**
@@ Expand Down @@