8000 [DI] Analyze setter-circular deps more precisely · symfony/symfony@9cc4a21 · GitHub
[go: up one dir, main page]
Skip to content

Commit 9cc4a21

Browse files
nicolas-grekasnicolas-grekas
committed
[DI] Analyze setter-circular deps more precisely
1 parent 1b6597d commit 9cc4a21

File tree

9 files changed

+73
-143
lines changed

9 files changed

+73
-143
lines changed

‎src/Symfony/Component/DependencyInjection/Compiler/CheckCircularReferencesPass.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ private function checkOutEdges(array $edges)
5959

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

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

Lines changed: 54 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
1717
use Symfony\Component\DependencyInjection\Variable;
1818
use Symfony\Component\DependencyInjection\Definition;
19+
use Symfony\Component\DependencyInjection\Compiler\AnalyzeServiceReferencesPass;
1920
use Symfony\Component\DependencyInjection\ContainerBuilder;
2021
use Symfony\Component\DependencyInjection\Container;
2122
use Symfony\Component\DependencyInjection\ContainerInterface;
@@ -66,6 +67,7 @@ class PhpDumper extends Dumper
6667
private $hotPathTag;
6768
private $inlineRequires;
6869
private $inlinedRequires = array();
70+
private $circularReferences = array();
6971

7072
/**
7173
* @var ProxyDumper
@@ -133,6 +135,14 @@ public function dump(array $options = array())
133135

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

138+
(new AnalyzeServiceReferencesPass())->process($this->container);
139+
$this->circularReferences = array();
140+
$checkedNodes = array();
141+
foreach ($this->container->getCompiler()->getServiceReferenceGraph()->getNodes() as $id => $node) {
142+
$currentPath = array($id => $id);
143+
$this->analyzeCircularReferences($node->getOutEdges(), $checkedNodes, $currentPath);
144+
}
145+
136146
$this->docStar = $options['debug'] ? '*' : '';
137147

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

229239
$this->targetDirRegex = null;
230240
$this->inlinedRequires = array();
241+
$this->circularReferences = array();
231242

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

274285
$collectLineage = $this->inlineRequires && !$this->isHotPath($definition);
275-
$isNonLazyShared = !$this->getProxyDumper()->isProxyCandidate($definition) && $definition->isShared();
286+
$isNonLazyShared = isset($this->circularReferences[$cId]) && !$this->getProxyDumper()->isProxyCandidate($definition) && $definition->isShared();
276287
$lineage = $calls = $behavior = array();
277288
foreach ($inlinedDefinitions as $iDefinition) {
278289
if ($collectLineage && !$iDefinition->isDeprecated() && $class = is_array($factory = $iDefinition->getFactory()) && is_string($factory[0]) ? $factory[0] : $iDefinition->getClass()) {
279290
$this->collectLineage($class, $lineage);
280291
}
281-
$this->getServiceCallsFromArguments($iDefinition->getArguments(), $calls, $behavior, $isNonLazyShared);
292+
$this->getServiceCallsFromArguments($iDefinition->getArguments(), $calls, $behavior, $isNonLazyShared, $cId);
282293
$isPreInstantiation = $isNonLazyShared && $iDefinition !== $definition && !$this->hasReference($cId, $iDefinition->getMethodCalls(), true) && !$this->hasReference($cId, $iDefinition->getProperties(), true);
283-
$this->getServiceCallsFromArguments($iDefinition->getMethodCalls(), $calls, $behavior, $isPreInstantiation);
284-
$this->getServiceCallsFromArguments($iDefinition->getProperties(), $calls, $behavior, $isPreInstantiation);
285-
$this->getServiceCallsFromArguments(array($iDefinition->getConfigurator()), $calls, $behavior, $isPreInstantiation);
286-
$this->getServiceCallsFromArguments(array($iDefinition->getFactory()), $calls, $behavior, $isNonLazyShared);
294+
$this->getServiceCallsFromArguments($iDefinition->getMethodCalls(), $calls, $behavior, $isPreInstantiation, $cId);
295+
$this->getServiceCallsFromArguments($iDefinition->getProperties(), $calls, $behavior, $isPreInstantiation, $cId);
296+
$this->getServiceCallsFromArguments(array($iDefinition->getConfigurator()), $calls, $behavior, $isPreInstantiation, $cId);
297+
$this->getServiceCallsFromArguments(array($iDefinition->getFactory()), $calls, $behavior, $isNonLazyShared, $cId);
287298
}
288299

289300
$code = '';
@@ -336,6 +347,33 @@ private function addServiceLocalTempVariables($cId, Definition $definition, arra
336347
return $code;
337348
}
338349

350+
private function analyzeCircularReferences(array $edges, &$checkedNodes, &$currentPath)
351+
{
352+
foreach ($edges as $edge) {
353+
$node = $edge->getDestNode();
354+
$id = $node->getId();
355+
356+
if (isset($checkedNodes[$id])) {
357+
continue;
358+
}
359+
360+
if ($node->getValue() && ($edge->isLazy() || $edge->isWeak())) {
361+
// no-op
362+
} elseif (isset($currentPath[$id])) {
363+
foreach (array_reverse($currentPath) as $parentId) {
364+
$this->circularReferences[$parentId][$id] = $id;
365+
$id = $parentId;
366+
}
367+
} else {
368+
$currentPath[$id] = $id;
369+
$this->analyzeCircularReferences($node->getOutEdges(), $checkedNodes, $currentPath);
370+
}
371+
372+
$checkedNodes[$id] = true;
373+
array_pop($currentPath);
374+
}
375+
}
376+
339377
private function collectLineage($class, array &$lineage)
340378
{
341379
if (isset($lineage[$class])) {
@@ -562,15 +600,15 @@ private function isTrivialInstance(Definition $definition)
562600
}
563601

564602
foreach ($definition->getArguments() as $arg) {
565-
if (!$arg || ($arg instanceof Reference && 'service_container' === (string) $arg)) {
603+
if (!$arg) {
566604
continue;
567605
}
568606
if (is_array($arg) && 3 >= count($arg)) {
569607
foreach ($arg as $k => $v) {
570608
if ($this->dumpValue($k) !== $this->dumpValue($k, false)) {
571609
return false;
572610
}
573-
if (!$v || ($v instanceof Reference && 'service_container' === (string) $v)) {
611+
if (!$v) {
574612
continue;
575613
}
576614
if ($v instanceof Reference && $this->container->has($id = (string) $v) && $this->container->findDefinition($id)->isSynthetic()) {
@@ -1501,16 +1539,16 @@ private function getServiceConditionals($value)
15011539
/**
15021540
* Builds service calls from arguments.
15031541
*/
1504-
private function getServiceCallsFromArguments(array $arguments, array &$calls, array &$behavior, $isPreInstantiation)
1542+
private function getServiceCallsFromArguments(array $arguments, array &$calls, array &$behavior, $isPreInstantiation, $callerId)
15051543
{
15061544
foreach ($arguments as $argument) {
15071545
if (is_array($argument)) {
1508-
$this->getServiceCallsFromArguments($argument, $calls, $behavior, $isPreInstantiation);
1546+
$this->getServiceCallsFromArguments($argument, $calls, $behavior, $isPreInstantiation, $callerId);
15091547
} elseif ($argument instanceof Reference) {
15101548
$id = (string) $argument;
15111549

15121550
if (!isset($calls[$id])) {
1513-
$calls[$id] = (int) ($isPreInstantiation && $this->container->has($id) && !$this->container->findDefinition($id)->isSynthetic());
1551+
$calls[$id] = (int) ($isPreInstantiation && isset($this->circularReferences[$callerId][$id]));
15141552
}
15151553
if (!isset($behavior[$id])) {
15161554
$behavior[$id] = $argument->getInvalidBehavior();
@@ -1582,6 +1620,10 @@ private function getDefinitionsFromArguments(array $arguments)
15821620
*/
15831621
private function hasReference($id, array $arguments, $deep = false, array &$visited = array())
15841622
{
1623+
if (!isset($this->circularReferences[$id])) {
1624+
return false;
1625+
}
1626+
15851627
foreach ($arguments as $argument) {
15861628
if (is_array($argument)) {
15871629
if ($this->hasReference($id, $argument, $deep, $visited)) {
@@ -1595,7 +1637,7 @@ private function hasReference($id, array $arguments, $deep = false, array &$visi
15951637
return true;
15961638
}
15971639

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

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/container_inline_requires.php

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,7 @@ protected function getC2Service()
9595
require_once $this->targetDirs[1].'/includes/HotPath/C2.php';
9696
require_once $this->targetDirs[1].'/includes/HotPath/C3.php';
9797

98-
$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 ?: '_'};
99-
100-
if (isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'])) {
101-
return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'];
102-
}
103-
104-
return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2($a);
98+
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 ?: '_'});
10599
}
106100

107101
/**

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

Lines changed: 3 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,6 @@ protected function getBarService()
8282
{
8383
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};
8484

85-
if (isset($this->services['bar'])) {
86-
return $this->services['bar'];
87-
}
88-
8985
$this->services['bar'] = $instance = new \Bar\FooClass('foo', $a, $this->getParameter('foo_bar'));
9086

9187
$a->configure($instance);
@@ -186,13 +182,7 @@ protected function getDeprecatedServiceService()
186182
*/
187183
protected function getFactoryServiceService()
188184
{
189-
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};
190-
191-
if (isset($this->services['factory_service'])) {
192-
return $this->services['factory_service'];
193-
}
194-
195-
return $this->services['factory_service'] = $a->getInstance();
185+
return $this->services['factory_service'] = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'}->getInstance();
196186
}
197187

198188
/**
@@ -202,13 +192,7 @@ protected function getFactoryServiceService()
202192
*/
203193
protected function getFactoryServiceSimpleService()
204194
{
205-
$a = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'};
206-
207-
if (isset($this->services['factory_service_simple'])) {
208-
return $this->services['factory_service_simple'];
209-
}
210-
211-
return $this->services['factory_service_simple'] = $a->getInstance();
195+
return $this->services['factory_service_simple'] = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'}->getInstance();
212196
}
213197

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

223-
if (isset($this->services['foo'])) {
224-
return $this->services['foo'];
225-
}
226-
227207
$this->services['foo'] = $instance = \Bar\FooClass::getInstance('foo', $a, array($this->getParameter('foo') => 'foo is '.$this->getParameter('foo').'', 'foobar' => $this->getParameter('foo')), true, $this);
228208

229209
$instance->foo = 'bar';
@@ -341,13 +321,7 @@ protected function getMethodCall1Service()
341321
*/
342322
protected function getNewFactoryServiceService()
343323
{
344-
$a = ${($_ = isset($this->services['new_factory']) ? $this->services['new_factory'] : $this->getNewFactoryService()) && false ?: '_'};
345-
346-
if (isset($this->services['new_factory_service'])) {
347-
return $this->services['new_factory_service'];
348-
}
349-
350-
$this->services['new_factory_service'] = $instance = $a->getInstance();
324+
$this->services['new_factory_service'] = $instance = ${($_ = isset($this->services['new_factory']) ? $this->services['new_factory'] : $this->getNewFactoryService()) && false ?: '_'}->getInstance();
351325

352326
$instance->foo = 'bar';
353327

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_as_files.txt

Lines changed: 5 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -33,18 +33,12 @@ use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
3333
// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
3434
// Returns the public 'configured_service' shared service.
3535

36-
$a = ${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->load(__DIR__.'/getBazService.php')) && false ?: '_'};
37-
38-
if (isset($this->services['configured_service'])) {
39-
return $this->services['configured_service'];
40-
}
41-
42-
$b = new \ConfClass();
43-
$b->setFoo($a);
36+
$a = new \ConfClass();
37+
$a->setFoo(${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->load(__DIR__.'/getBazService.php')) && false ?: '_'});
4438

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

47-
$b->configureStdClass($instance);
41+
$a->configureStdClass($instance);
4842

4943
return $instance;
5044

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

100-
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->load(__DIR__.'/getFoo_BazService.php')) && false ?: '_'};
101-
102-
if (isset($this->services['factory_service'])) {
103-
return $this->services['factory_service'];
104-
}
105-
106-
return $this->services['factory_service'] = $a->getInstance();
94+
return $this->services['factory_service'] = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->load(__DIR__.'/getFoo_BazService.php')) && false ?: '_'}->getInstance();
10795

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

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

115-
$a = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->load(__DIR__.'/getFactorySimpleService.php')) && false ?: '_'};
116-
117-
if (isset($this->services['factory_service_simple'])) {
118-
return $this->services['factory_service_simple'];
119-
}
120-
121-
return $this->services['factory_service_simple'] = $a->getInstance();
103+
return $this->services['factory_service_simple'] = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->load(__DIR__.'/getFactorySimpleService.php')) && false ?: '_'}->getInstance();
122104

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

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

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

143-
if (isset($this->services['foo'])) {
144-
return $this->services['foo'];
145-
}
146-
147125
$this->services['foo'] = $instance = \Bar\FooClass::getInstance('foo', $a, array('bar' => 'foo is bar', 'foobar' => 'bar'), true, $this);
148126

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

386-
if (isset($this->services['bar'])) {
387-
return $this->services['bar'];
388-
}
389-
390364
$this->services['bar'] = $instance = new \Bar\FooClass('foo', $a, $this->getParameter('foo_bar'));
391365

392366
$a->configure($instance);

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php

Lines changed: 5 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -101,10 +101,6 @@ protected function getBarService()
101101
{
102102
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};
103103

104-
if (isset($this->services['bar'])) {
105-
return $this->services['bar'];
106-
}
107-
108104
$this->services['bar'] = $instance = new \Bar\FooClass('foo', $a, $this->getParameter('foo_bar'));
109105

110106
$a->configure($instance);
@@ -133,18 +129,12 @@ protected function getBazService()
133129
*/
134130
protected function getConfiguredServiceService()
135131
{
136-
$a = ${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->getBazService()) && false ?: '_'};
137-
138-
if (isset($this->services['configured_service'])) {
139-
return $this->services['configured_service'];
140-
}
141-
142-
$b = new \ConfClass();
143-
$b->setFoo($a);
132+
$a = new \ConfClass();
133+
$a->setFoo(${($_ = isset($this->services['baz']) ? $this->services['baz'] : $this->getBazService()) && false ?: '_'});
144134

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

147-
$b->configureStdClass($instance);
137+
$a->configureStdClass($instance);
148138

149139
return $instance;
150140
}
@@ -204,13 +194,7 @@ protected function getDeprecatedServiceService()
204194
*/
205195
protected function getFactoryServiceService()
206196
{
207-
$a = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'};
208-
209-
if (isset($this->services['factory_service'])) {
210-
return $this->services['factory_service'];
211-
}
212-
213-
return $this->services['factory_service'] = $a->getInstance();
197+
return $this->services['factory_service'] = ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->getFoo_BazService()) && false ?: '_'}->getInstance();
214198
}
215199

216200
/**
@@ -220,13 +204,7 @@ protected function getFactoryServiceService()
220204
*/
221205
protected function getFactoryServiceSimpleService()
222206
{
223-
$a = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'};
224-
225-
if (isset($this->services['factory_service_simple'])) {
226-
return $this->services['factory_service_simple'];
227-
}
228-
229-
return $this->services['factory_service_simple'] = $a->getInstance();
207+
return $this->services['factory_service_simple'] = ${($_ = isset($this->services['factory_simple']) ? $this->services['factory_simple'] : $this->getFactorySimpleService()) && false ?: '_'}->getInstance();
230208
}
231209

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

241-
if (isset($this->services['foo'])) {
242-
return $this->services['foo'];
243-
}
244-
245219
$this->services['foo'] = $instance = \Bar\FooClass::getInstance('foo', $a, array('bar' => 'foo is bar', 'foobar' => 'bar'), true, $this);
246220

247221
$instance->foo = 'bar';

0 commit comments

Comments
 (0)
0