vimeo / psalm

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

NonInvariantPropertyType with php_user_filter::$filtername #7170

Open BenMorel opened 2 years ago

BenMorel commented 2 years ago

Consider the following sample filter:

class SkipFilter extends \php_user_filter
{
    public string $filtername;

    public function filter($in, $out, &$consumed, bool $closing): int
    {
        return PSFS_PASS_ON;
    }

    public function onCreate(): bool
    {
        return true;
    }
}

Psalm complains:

ERROR: NonInvariantPropertyType - 8:19 - Property SkipFilter::$filtername has type string, not invariant with php_user_filter::$filtername of type

However, the property type string for $filtername is actually required since PHP 8.1:

So, it looks like Psalm's behaviour is currently aligned with PHP 8.0. Would it be possible to use different signatures for php_user_filter depending on the current PHP version?

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/eb30a3ef39 ```php ```
AndrolGenhald commented 2 years ago

Indeed, if you wish to do this yourself you can stub the old class somewhere (not sure which stub file would be correct), and add the 8.1 stub to stubs/Php81.phpstub.

Edit: Or it might actually make more sense to just do one stub and use the string type, if it's a non-string it'll probably cause errors anyway, so even in PHP < 8.1 it should have the docblock type string correct?

orklah commented 2 years ago

Seems like we have no particular source of types for php_user_filter. it must comes from reflection, so Psalm behaviour will depend on the php version used to run it.

This is not a good thing and as @AndrolGenhald said we need a stub for it. I think we should start with a string docblock on the property and see how it goes with CI. I'm afraid adding the type in signature will make Psalm enforce this on early versions

weirdan commented 2 years ago

Actually Psalm knows nothing about php_user_filter properties:

$ rg -i 'php_user_filter'

dictionaries/CallMap.php
10157:'php_user_filter::filter' => ['int', 'in'=>'resource', 'out'=>'resource', '&rw_consumed'=>'int', 'closing'=>'bool'],
10158:'php_user_filter::onClose' => ['void'],
10159:'php_user_filter::onCreate' => ['bool'],

dictionaries/CallMap_historical.php
14350:    'php_user_filter::filter' => ['int', 'in'=>'resource', 'out'=>'resource', '&rw_consumed'=>'int', 'closing'=>'bool'],
14351:    'php_user_filter::onClose' => ['void'],
14352:    'php_user_filter::onCreate' => ['bool'],
weirdan commented 8 months ago

We now have it in our property map, but it doesn't seem to fix the issue.

https://github.com/vimeo/psalm/blob/ceaea625f30289850c8a9274b9c79e21fa848dea/dictionaries/PropertyMap.php#L370-L374