[DI] fix reporting bindings on overriden services as unused

[nicolas-grekas]

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

[GuilhemN]

I'm afraid this won't work in most cases: we deep copy bindings in the loaders (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php#L523). So instead we need to store bindings id and check against this list in the pass.

[nicolas-grekas]

@GuilhemN thanks for the review! Updated.

[GuilhemN]

Nice improvement 👍 thank you!

[przemyslaw-bogusz]

If I am not mistaken, this fix has created a new problem in ResolveBindingsPass.

The newly added puts all bindings that are set within a file into the usedBindings array. Before that, usedBindings array was empty as set in the property's declaration.

symfony/src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php

Line 37 in 1d3ce9b

$this->usedBindings = $container->getRemovedBindingIds();

This means that aftewards, when processValue is called, the unusedBindings array remains empty, because the following condition is not met.

symfony/src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php

Lines 84 to 86 in 1d3ce9b

	} elseif (!isset($this->usedBindings[$bindingId])) {
	$this->unusedBindings[$bindingId] = array($key, $this->currentId);
	}

As a result, InvalidArgumentException (informing about an unused binding) is not thrown, even if a binding is actually not used. I have come upon this, because I was just about to make a PR solving #27828, but after I fetched the symfony code to make sure it is up to date my fix stopped working. I believe I can find a new solution, solving both problems.

[nicolas-grekas]

The current code prefers a false negative to the previous false positive.
Being more accurate could mean resolving bindings in the loaders, where there is full context about them.
Or maybe you have another idea. I'd be happy to review it!