vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.55k stars 659 forks source link

MoreSpecificImplementedParamType for subclass of an interface implementation #8846

Open ChristophWurst opened 1 year ago

ChristophWurst commented 1 year ago

https://psalm.dev/r/33a0a9254b

^ This seems to be the minimal example of an issue I ran into today. It appears Psalm doesn't derive the typed Closure down to the subclass. Or like the type is erased for the subclass.

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/33a0a9254b ```php
weirdan commented 1 year ago

I'm not sure it's a bug. I specifies the parameter as Closure():string, then A widens that parameter to Closure (valid according to LSP) and then B narrows it back to Closure():string (invalid).

The question here is whether we expect the docblock to be inherited (leading to potential problems with multiple inheritance) or not.

Arkemlar commented 1 year ago

in my case everything is much simpler - typical case when implementing framework interface. https://psalm.dev/r/605fa7a49b

Getting:

ERROR: [MoreSpecificImplementedParamType](https://psalm.dev/140) - 23:37 - Argument 1 of TaskNormalizer::normalize has the more specific type 'Task', expecting 'mixed' as defined by NormalizerInterface::normalize
psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/605fa7a49b ```php
weirdan commented 1 year ago

in my case everything is much simpler - typical case when implementing framework interface.

And Psalm rightfully complains in your case because a valid code like this:

function f(NormalizerInterface $normalizer): void {
   $normalizer->normalize(new stdClass);
}
f(new TaskNormalizer);

will likely not work as intended.

Arkemlar commented 1 year ago

will likely not work as intended.

It depends on what is expected. This interface is for normalizer realizations, one for Task class, another for DateTime, there are some for stdClass too. It is ok that in every case it will get different object and produce different result like array, string, etc - according to return type of interface method. Actually, it is behind Symfony Serializer system that ensures every normalizer get type it is designed for. It is so because interface defines supports() method to be implemented and it returns type. I cut out that method because it is not relevant to bug case.

weirdan commented 1 year ago

Adherence to LSP is expected. It is for that reason you can't narrow down the native parameter type (and Psalm doesn't allow it for docblocks either). If you need to narrow parameter types, as opposed to widening them, you need a generic interface. Psalm (as well as PHPStan) supports generic interfaces, and Symfony ecosystem is not shy to use them. If some component doesn't (yet?) use generics where it should, then it's the problem with that component, not Psalm.