Skip to content

[DI] Analyze setter-circular deps more precisely #25055

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 20, 2017
Merged

[DI] Analyze setter-circular deps more precisely #25055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Symfony/Component/DependencyInjection/Compiler/CheckCircularReferencesPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ private function checkOutEdges(array $edges)

if (empty($this->checkedNodes[$id])) {
// Don't check circular references for lazy edges
if (!$node->getValue() || !$edge->isLazy()) {
if (!$node->getValue() || (!$edge->isLazy() && !$edge->isWeak())) {
$searchKey = array_search($id, $this->currentPath);
$this->currentPath[] = $id;

Expand Down
66 changes: 54 additions & 12 deletions src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
use Symfony\Component\DependencyInjection\Variable;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Compiler\AnalyzeServiceReferencesPass;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Container;
use Symfony\Component\DependencyInjection\ContainerInterface;
Expand Down Expand Up @@ -66,6 +67,7 @@ class PhpDumper extends Dumper
private $hotPathTag;
private $inlineRequires;
private $inlinedRequires = array();
private $circularReferences = array();

/**
* @var ProxyDumper
Expand Down Expand Up @@ -133,6 +135,14 @@ public function dump(array $options = array())

$this->initializeMethodNamesMap('Container' === $baseClass ? Container::class : $baseClass);

(new AnalyzeServiceReferencesPass())->process($this->container);
$this->circularReferences = array();
$checkedNodes = array();
foreach ($this->container->getCompiler()->getServiceReferenceGraph()->getNodes() as $id => $node) {
$currentPath = array($id => $id);
$this->analyzeCircularReferences($node->getOutEdges(), $checkedNodes, $currentPath);
}

$this->docStar = $options['debug'] ? '*' : '';

if (!empty($options['file']) && is_dir($dir = dirname($options['file']))) {
Expand Down Expand Up @@ -228,6 +238,7 @@ class_alias(\\Container{$hash}\\{$options['class']}::class, {$options['class']}:

$this->targetDirRegex = null;
$this->inlinedRequires = array();
$this->circularReferences = array();

$unusedEnvs = array();
foreach ($this->container->getEnvCounters() as $env => $use) {
Expand Down Expand Up @@ -272,18 +283,18 @@ private function addServiceLocalTempVariables($cId, Definition $definition, arra
array_unshift($inlinedDefinitions, $definition);

$collectLineage = $this->inlineRequires && !$this->isHotPath($definition);
$isNonLazyShared = !$this->getProxyDumper()->isProxyCandidate($definition) && $definition->isShared();
$isNonLazyShared = isset($this->circularReferences[$cId]) && !$this->getProxyDumper()->isProxyCandidate($definition) && $definition->isShared();
$lineage = $calls = $behavior = array();
foreach ($inlinedDefinitions as $iDefinition) {
if ($collectLineage && !$iDefinition->isDeprecated() && $class = is_array($factory = $iDefinition->getFactory()) && is_string($factory[0]) ? $factory[0] : $iDefinition->getClass()) {
$this->collectLineage($class, $lineage);
}
$this->getServiceCallsFromArguments($iDefinition->getArguments(), $calls, $behavior, $isNonLazyShared);
$this->getServiceCallsFromArguments($iDefinition->getArguments(), $calls, $behavior, $isNonLazyShared, $cId);
$isPreInstantiation = $isNonLazyShared && $iDefinition !== $definition && !$this->hasReference($cId, $iDefinition->getMethodCalls(), true) && !$this->hasReference($cId, $iDefinition->getProperties(), true);
$this->getServiceCallsFromArguments($iDefinition->getMethodCalls(), $calls, $behavior, $isPreInstantiation);
$this->getServiceCallsFromArguments($iDefinition->getProperties(), $calls, $behavior, $isPreInstantiation);
$this->getServiceCallsFromArguments(array($iDefinition->getConfigurator()), $calls, $behavior, $isPreInstantiation);
$this->getServiceCallsFromArguments(array($iDefinition->getFactory()), $calls, $behavior, $isNonLazyShared);
$this->getServiceCallsFromArguments($iDefinition->getMethodCalls(), $calls, $behavior, $isPreInstantiation, $cId);
$this->getServiceCallsFromArguments($iDefinition->getProperties(), $calls, $behavior, $isPreInstantiation, $cId);
$this->getServiceCallsFromArguments(array($iDefinition->getConfigurator()), $calls, $behavior, $isPreInstantiation, $cId);
$this->getServiceCallsFromArguments(array($iDefinition->getFactory()), $calls, $behavior, $isNonLazyShared, $cId);
}

$code = '';
Expand Down Expand Up @@ -336,6 +347,33 @@ private function addServiceLocalTempVariables($cId, Definition $definition, arra
return $code;
}

private function analyzeCircularReferences(array $edges, &$checkedNodes, &$currentPath)
{
foreach ($edges as $edge) {
$node = $edge->getDestNode();
$id = $node->getId();

if (isset($checkedNodes[$id])) {
continue;
}

if ($node->getValue() && ($edge->isLazy() || $edge->isWeak())) {
// no-op
} elseif (isset($currentPath[$id])) {
foreach (array_reverse($currentPath) as $parentId) {
$this->circularReferences[$parentId][$id] = $id;
$id = $parentId;
}
} else {
$currentPath[$id] = $id;
$this->analyzeCircularReferences($node->getOutEdges(), $checkedNodes, $currentPath);
}

$checkedNodes[$id] = true;
array_pop($currentPath);
}
}

private function collectLineage($class, array &$lineage)
{
if (isset($lineage[$class])) {
Expand Down Expand Up @@ -562,15 +600,15 @@ private function isTrivialInstance(Definition $definition)
}

foreach ($definition->getArguments() as $arg) {
if (!$arg || ($arg instanceof Reference && 'service_container' === (string) $arg)) {
if (!$arg) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just removes useless logic that is already handled below in the method

continue;
}
if (is_array($arg) && 3 >= count($arg)) {
foreach ($arg as $k => $v) {
if ($this->dumpValue($k) !== $this->dumpValue($k, false)) {
return false;
}
if (!$v || ($v instanceof Reference && 'service_container' === (string) $v)) {
if (!$v) {
continue;
}
if ($v instanceof Reference && $this->container->has($id = (string) $v) && $this->container->findDefinition($id)->isSynthetic()) {
Expand Down Expand Up @@ -1501,16 +1539,16 @@ private function getServiceConditionals($value)
/**
* Builds service calls from arguments.
*/
private function getServiceCallsFromArguments(array $arguments, array &$calls, array &$behavior, $isPreInstantiation)
private function getServiceCallsFromArguments(array $arguments, array &$calls, array &$behavior, $isPreInstantiation, $callerId)
{
foreach ($arguments as $argument) {
if (is_array($argument)) {
$this->getServiceCallsFromArguments($argument, $calls, $behavior, $isPreInstantiation);
$this->getServiceCallsFromArguments($argument, $calls, $behavior, $isPreInstantiation, $callerId);
} elseif ($argument instanceof Reference) {
$id = (string) $argument;

if (!isset($calls[$id])) {
$calls[$id] = (int) ($isPreInstantiation && $this->container->has($id) && !$this->container->findDefinition($id)->isSynthetic());
$calls[$id] = (int) ($isPreInstantiation && isset($this->circularReferences[$callerId][$id]));
}
if (!isset($behavior[$id])) {
$behavior[$id] = $argument->getInvalidBehavior();
Expand Down Expand Up @@ -1582,6 +1620,10 @@ private function getDefinitionsFromArguments(array $arguments)
*/
private function hasReference($id, array $arguments, $deep = false, array &$visited = array())
{
if (!isset($this->circularReferences[$id])) {
return false;
}

foreach ($arguments as $argument) {
if (is_array($argument)) {
if ($this->hasReference($id, $argument, $deep, $visited)) {
Expand All @@ -1595,7 +1637,7 @@ private function hasReference($id, array $arguments, $deep = false, array &$visi
return true;
}

if (!$deep || isset($visited[$argumentId]) || 'service_container' === $argumentId) {
if (!$deep || isset($visited[$argumentId]) || !isset($this->circularReferences[$id][$argumentId])) {
continue;
}

Expand Down
8 changes: 1 addition & 7 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/container_inline_requires.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,7 @@ protected function getC2Service()
require_once $this->targetDirs[1].'/includes/HotPath/C2.php';
require_once $this->targetDirs[1].'/includes/HotPath/C3.php';

$a = ${($_ = isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3']) ? $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3'] : $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3()) && false ?: '_'};

if (isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'])) {
return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'];
}

return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2($a);
return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2(${($_ = isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3']) ? $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3'] : $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3()) && false ?: '_'});
}

/**
Expand Down
32 changes: 3 additions & 29 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,6 @@ protected function getBarService()
{
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};

if (isset($this->services['bar'])) {
return $this->services['bar'];
}

$this->services['bar'] = $instance = new \Bar\FooClass('foo', $a, $this->getParameter('foo_bar'));

$a->configure($instance);
Expand Down Expand Up @@ -186,13 +182,7 @@ protected function getDeprecatedServiceService()
*/
protected function getFactoryServiceService()
{
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};

if (isset($this->services['factory_service'])) {
return $this->services['factory_service'];
}

return $this->services['factory_service'] = $a->getInstance();
return $this->services['factory_service'] = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'}->getInstance();
}

/**
Expand All @@ -202,13 +192,7 @@ protected function getFactoryServiceService()
*/
protected function getFactoryServiceSimpleService()
{
$a = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'};

if (isset($this->services['factory_service_simple'])) {
return $this->services['factory_service_simple'];
}

return $this->services['factory_service_simple'] = $a->getInstance();
return $this->services['factory_service_simple'] = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'}->getInstance();
}

/**
Expand All @@ -220,10 +204,6 @@ protected function getFooService()
{
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};

if (isset($this->services['foo'])) {
return $this->services['foo'];
}

$this->services['foo'] = $instance = \Bar\FooClass::getInstance('foo', $a, array($this->getParameter('foo') => 'foo is '.$this->getParameter('foo').'', 'foobar' => $this->getParameter('foo')), true, $this);

$instance->foo = 'bar';
Expand Down Expand Up @@ -341,13 +321,7 @@ protected function getMethodCall1Service()
*/
protected function getNewFactoryServiceService()
{
$a = ${($_ = isset($this->services['new_factory']) ? $this->services['new_factory'] : $this->getNewFactoryService()) && false ?: '_'};

if (isset($this->services['new_factory_service'])) {
return $this->services['new_factory_service'];
}

$this->services['new_factory_service'] = $instance = $a->getInstance();
$this->services['new_factory_service'] = $instance = ${($_ = isset($this->services['new_factory']) ? $this->services['new_factory'] : $this->getNewFactoryService()) && false ?: '_'}->getInstance();

$instance->foo = 'bar';

Expand Down
36 changes: 5 additions & 31 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_as_files.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,12 @@ use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
// Returns the public 'configured_service' shared service.

$a = ${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->load(__DIR__.'/getBazService.php')) && false ?: '_'};

if (isset($this->services['configured_service'])) {
return $this->services['configured_service'];
}

$b = new \ConfClass();
$b->setFoo($a);
$a = new \ConfClass();
$a->setFoo(${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->load(__DIR__.'/getBazService.php')) && false ?: '_'});

$this->services['configured_service'] = $instance = new \stdClass();

$b->configureStdClass($instance);
$a->configureStdClass($instance);

return $instance;

Expand Down Expand Up @@ -97,13 +91,7 @@ use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
// Returns the public 'factory_service' shared service.

$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->load(__DIR__.'/getFoo_BazService.php')) && false ?: '_'};

if (isset($this->services['factory_service'])) {
return $this->services['factory_service'];
}

return $this->services['factory_service'] = $a->getInstance();
return $this->services['factory_service'] = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->load(__DIR__.'/getFoo_BazService.php')) && false ?: '_'}->getInstance();

[Container%s/getFactoryServiceSimpleService.php] => <?php

Expand All @@ -112,13 +100,7 @@ use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
// Returns the public 'factory_service_simple' shared service.

$a = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->load(__DIR__.'/getFactorySimpleService.php')) && false ?: '_'};

if (isset($this->services['factory_service_simple'])) {
return $this->services['factory_service_simple'];
}

return $this->services['factory_service_simple'] = $a->getInstance();
return $this->services['factory_service_simple'] = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->load(__DIR__.'/getFactorySimpleService.php')) && false ?: '_'}->getInstance();

[Container%s/getFactorySimpleService.php] => <?php

Expand All @@ -140,10 +122,6 @@ use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;

$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->load(__DIR__.'/getFoo_BazService.php')) && false ?: '_'};

if (isset($this->services['foo'])) {
return $this->services['foo'];
}

$this->services['foo'] = $instance = \Bar\FooClass::getInstance('foo', $a, array('bar' => 'foo is bar', 'foobar' => 'bar'), true, $this);

$instance->foo = 'bar';
Expand Down Expand Up @@ -383,10 +361,6 @@ class ProjectServiceContainer extends Container
{
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->load(__DIR__.'/getFoo_BazService.php')) && false ?: '_'};

if (isset($this->services['bar'])) {
return $this->services['bar'];
}

$this->services['bar'] = $instance = new \Bar\FooClass('foo', $a, $this->getParameter('foo_bar'));

$a->configure($instance);
Expand Down
36 changes: 5 additions & 31 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,6 @@ protected function getBarService()
{
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};

if (isset($this->services['bar'])) {
return $this->services['bar'];
}

$this->services['bar'] = $instance = new \Bar\FooClass('foo', $a, $this->getParameter('foo_bar'));

$a->configure($instance);
Expand Down Expand Up @@ -133,18 +129,12 @@ protected function getBazService()
*/
protected function getConfiguredServiceService()
{
$a = ${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->getBazService()) && false ?: '_'};

if (isset($this->services['configured_service'])) {
return $this->services['configured_service'];
}

$b = new \ConfClass();
$b->setFoo($a);
$a = new \ConfClass();
$a->setFoo(${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->getBazService()) && false ?: '_'});

$this->services['configured_service'] = $instance = new \stdClass();

$b->configureStdClass($instance);
$a->configureStdClass($instance);

return $instance;
}
Expand Down Expand Up @@ -204,13 +194,7 @@ protected function getDeprecatedServiceService()
*/
protected function getFactoryServiceService()
{
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};

if (isset($this->services['factory_service'])) {
return $this->services['factory_service'];
}

return $this->services['factory_service'] = $a->getInstance();
return $this->services['factory_service'] = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'}->getInstance();
}

/**
Expand All @@ -220,13 +204,7 @@ protected function getFactoryServiceService()
*/
protected function getFactoryServiceSimpleService()
{
$a = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'};

if (isset($this->services['factory_service_simple'])) {
return $this->services['factory_service_simple'];
}

return $this->services['factory_service_simple'] = $a->getInstance();
return $this->services['factory_service_simple'] = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'}->getInstance();
}

/**
Expand All @@ -238,10 +216,6 @@ protected function getFooService()
{
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};

if (isset($this->services['foo'])) {
return $this->services['foo'];
}

$this->services['foo'] = $instance = \Bar\FooClass::getInstance('foo', $a, array('bar' => 'foo is bar', 'foobar' => 'bar'), true, $this);

$instance->foo = 'bar';
Expand Down
Loading