False positive UnusedParam when parameter to the right is used #10530
Comments
|
psalm dev uses lower error level or does not report unused code so does not report unused params. |
Sure does, but you have to enable both checkboxes in settings: https://psalm.dev/r/a353628763
And Psalm reports that.
Why? Psalm cannot be aware of some implied contract, if there is any. You can uncomment |
|
I found these snippets: https://psalm.dev/r/a353628763<?php declare(strict_types=1);
namespace Laminas\Cache\Storage\Adapter\BlackHole;
use function assert;
interface ContainerInterface {}
class BlackHole {}
interface InvokableFactory {
// public function __invoke(ContainerInterface $container, string $name, callable $callback): AdapterPluginManager;
}
interface AdapterPluginManager {
public function configure(array $_): void;
}
final class AdapterPluginManagerDelegatorFactory implements InvokableFactory
{
public function __invoke(ContainerInterface $container, string $name, callable $callback): AdapterPluginManager
{
$pluginManager = $callback();
assert($pluginManager instanceof AdapterPluginManager);
$pluginManager->configure([
'factories' => [
BlackHole::class => InvokableFactory::class,
],
'aliases' => [
'black_hole' => BlackHole::class,
'blackhole' => BlackHole::class,
'blackHole' => BlackHole::class,
'BlackHole' => BlackHole::class,
],
]);
return $pluginManager;
}
}
new AdapterPluginManagerDelegatorFactory; |
I was not getting any errors with all checkboxes enabled. May be I did something wrong.
If this code is incorrect, how could it be fixed? How is this case different from the linked issue? |
|
I found these snippets: https://psalm.dev/r/a353628763<?php declare(strict_types=1);
namespace Laminas\Cache\Storage\Adapter\BlackHole;
use function assert;
interface ContainerInterface {}
class BlackHole {}
interface InvokableFactory {
// public function __invoke(ContainerInterface $container, string $name, callable $callback): AdapterPluginManager;
}
interface AdapterPluginManager {
public function configure(array $_): void;
}
final class AdapterPluginManagerDelegatorFactory implements InvokableFactory
{
public function __invoke(ContainerInterface $container, string $name, callable $callback): AdapterPluginManager
{
$pluginManager = $callback();
assert($pluginManager instanceof AdapterPluginManager);
$pluginManager->configure([
'factories' => [
BlackHole::class => InvokableFactory::class,
],
'aliases' => [
'black_hole' => BlackHole::class,
'blackhole' => BlackHole::class,
'blackHole' => BlackHole::class,
'BlackHole' => BlackHole::class,
],
]);
return $pluginManager;
}
}
new AdapterPluginManagerDelegatorFactory; |
It's not incorrect, per se. It just can be improved - by removing unused parameters. As it stands, there's nothing in the code that mandates factories must necessarily have three arguments (except BC, but that's outside of Psalm's scope), the first being a container, the second being a name and so on. So improvement (fix) is easy - remove unused parameters and remove superfluous arguments at call sites. But it may happen (I speculate here) that there are other kinds of factories, and those kinds actually utilize those additional parameters, and those arguments are passed into a factory by an entity unaware of the specific type of the factory being used. Meaning the factories are instances of a common polymorphic type. In this case, you just need to specify that type as an interface and implement that interface in your factory: https://psalm.dev/r/5ea7fc70ec |
|
I found these snippets: https://psalm.dev/r/5ea7fc70ec<?php declare(strict_types=1);
namespace Laminas\Cache\Storage\Adapter\BlackHole;
use function assert;
interface ContainerInterface {}
class BlackHole {}
interface InvokableFactory {
public function __invoke(ContainerInterface $container, string $name, callable $callback): AdapterPluginManager;
}
interface AdapterPluginManager {
public function configure(array $_): void;
}
final class AdapterPluginManagerDelegatorFactory implements InvokableFactory
{
public function __invoke(ContainerInterface $container, string $name, callable $callback): AdapterPluginManager
{
$pluginManager = $callback();
assert($pluginManager instanceof AdapterPluginManager);
$pluginManager->configure([
'factories' => [
BlackHole::class => InvokableFactory::class,
],
'aliases' => [
'black_hole' => BlackHole::class,
'blackhole' => BlackHole::class,
'blackHole' => BlackHole::class,
'BlackHole' => BlackHole::class,
],
]);
return $pluginManager;
}
}
new AdapterPluginManagerDelegatorFactory; |
|
@weirdan I think you misunderstood what is going here. First and second parameter are unused in this specific factory body but third parameter is used. Because third parameter is used first and second are indirectly used too as they hold positions for third parameter to be accessible. They can not be removed. |
|
And I believe you're mistaken in thinking that. So to put our conversation into a more practical context, you need to implement |
|
Factory in service manager is simply callable. Factory interfaces are a convenience for users nothing more. But it is irrelevant. Implementing interface does not resolve the issue for when there is no fake interface to implement and that is quite common with |
|
That's a matter of perspective. One can say the ability to use callables as factories is just a convenience for users who can't be bothered to correctly implement factory interface, nothing more. The interface in question isn't in any way fake; it's a tangible manifestation of the de facto contract between the caller (service manager) and the callee (factory). As such, it allows static analysis tools like Psalm to verify implementation correctness (both at call and definition sites) to a greater extent and, as a bonus, to understand that those unused parameters are there for a reason. And for times when an interface cannot be used (or was never explicitly defined, for whatever reason), there's another way to suppress |
|
I found these snippets: https://psalm.dev/r/803ab5bc4f<?php declare(strict_types=1);
namespace Laminas\Cache\Storage\Adapter\BlackHole;
use function assert;
interface ContainerInterface {}
class BlackHole {}
interface InvokableFactory {
}
interface AdapterPluginManager {
public function configure(array $_): void;
}
final class AdapterPluginManagerDelegatorFactory
{
/** @no-named-arguments */
public function __invoke(ContainerInterface $_container, string $_name, callable $callback): AdapterPluginManager
{
$pluginManager = $callback();
assert($pluginManager instanceof AdapterPluginManager);
$pluginManager->configure([
'factories' => [
BlackHole::class => InvokableFactory::class,
],
'aliases' => [
'black_hole' => BlackHole::class,
'blackhole' => BlackHole::class,
'blackHole' => BlackHole::class,
'BlackHole' => BlackHole::class,
],
]);
return $pluginManager;
}
}
new AdapterPluginManagerDelegatorFactory; |
Similar to the case with closures #8095 unused parameters to the left of used parameter are reported as an error.
With error level 1 this class produces false positive UnusedParam:
https://github.com/laminas/laminas-cache-storage-adapter-blackhole/blob/2.5.x/src/BlackHole/AdapterPluginManagerDelegatorFactory.php#L16
Method utilizes third parameter
$callbackand first and second parameters are indeed unused however they can not be removed. They are required because parameter to the right is used.The text was updated successfully, but these errors were encountered: