vimeo / psalm

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

PropertyTypeCoercion for @psalm-var intersect type #7879

Open michnovka opened 2 years ago

michnovka commented 2 years ago

In my code I have:

$view->vars['attr']['class'] = 'form-control';

And it results in this error:

ERROR: PropertyTypeCoercion - src/Form/Type/AdminEntityType.php:29:13 - $view->vars expects 'array{attr: array<array-key, mixed>, value: mixed}<string, mixed>',  parent type 'array{attr: mixed, value: mixed}<string, mixed>' provided (see https://psalm.dev/198)

The variable is defined like this in parent class:

   /**
     * The variables assigned to this view.
     */
    public $vars = [
        'value' => null,
        'attr' => [],
    ];
psalm-github-bot[bot] commented 2 years ago

Hey @michnovka, can you reproduce the issue on https://psalm.dev ?

michnovka commented 2 years ago

No I cannot, but changing to

$view->vars['attr'] = ['class' => 'form-control'];

Fixes the issue, which makes me believe this has to be a bug in psalm.

The FormView is a Symfony class, I cannot edit its PHPDoc

AndrolGenhald commented 2 years ago

I've noticed this before too, but I've never had the time to go debug it. Can you try adding a /** @psalm-trace $view */; and perhaps /** @psalm-trace $tmp */ $tmp = $view->vars;?

No I cannot, but changing to ... Fixes the issue, which makes me believe this has to be a bug in psalm.

Not necessarily. I don't remember how far I got last time I looked into this, but if Psalm thinks $view->vars['attr'] might be an ArrayObject (ie it's already incorrect) then $view->vars['attr']['class'] = 'form-control'; will result in the wrong type for $view->vars['attr'] while $view->vars['attr'] = ['class' => 'form-control']; will correct the type.

michnovka commented 2 years ago
INFO: Trace - src/Form/Type/AdminEntityType.php:27:9 - $tmp: array{attr: array<array-key, mixed>, value: mixed}<string, mixed> (see https://psalm.dev/224)
        /** @psalm-trace $tmp */
        $tmp = $view->vars;
michnovka commented 2 years ago

Any idea what we can do to address this? Tons of errors whenever I access $view->vars :

ERROR: PropertyTypeCoercion - src/Form/Type/DateRangeType.php:28:13 - $view->vars expects 'array{attr: array<array-key, mixed>, value: mixed}<string, mixed>',  parent type 'array{attr: mixed, show_clear_button: true, value: mixed}<string, mixed>' provided (see https://psalm.dev/198)
            $view->vars['show_clear_button'] = true;
michnovka commented 2 years ago

I reproduced it @AndrolGenhald

https://psalm.dev/r/93d9653415

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

I found these snippets:

https://psalm.dev/r/93d9653415 ```php }&array */ public array $vars = [ 'value' => null, 'attr' => [], ]; /** * @psalm-var ?self */ public $parent; /** * @psalm-var array */ public $children = []; } $view = new FormView(); $view->vars['attr']['a'] = true; ``` ``` Psalm output (using commit b5b5c20): INFO: MixedArrayAssignment - 32:1 - Cannot access array value on mixed variable $view->vars['attr']['a'] ERROR: PropertyTypeCoercion - 32:1 - $view->vars expects 'array{attr: array, value: mixed}', parent type 'array{attr: mixed, value: mixed}' provided ```
michnovka commented 2 years ago

The issue seems to be the intersection type as changing

     * @psalm-var array{value: ?T, attr: array<array-key, mixed>}&array<string, mixed>

for

     * @psalm-var array{value: ?T, attr: array<array-key, mixed>}

resolves the issue

AndrolGenhald commented 2 years ago

That's definitely a Psalm bug then, Psalm's always behaved a bit weird when intersecting or unioning certain array types.

michnovka commented 2 years ago

If interested, psalm uses https://github.com/psalm/psalm-plugin-symfony/blob/master/src/Stubs/common/Component/Form/FormView.stubphp as the default Symfony FormView class has no psalm PHPDoc

michnovka commented 2 years ago

Is this even a valid definition?

     * @psalm-var array{value: ?T, attr: array<array-key, mixed>}&array<string, mixed>

Is this how you specify an array type with multiple keys, but define just some explicitly in {}?

AndrolGenhald commented 2 years ago

Is this even a valid definition?

Yes, it's valid because array shapes default to being unsealed, meaning an array with extra keys set still satisfies the type (ie ["foo" => 1, "bar" => 2] satisfies array{foo: int} despite the extra property).

Unfortunately the way arrays and array-shapes are intersected right now has some edge cases, and it's not really possible to correctly express an array with extra keys constrained to a type that doesn't match the defined keys, but Psalm currently just ignores this inconsistency. I would like to eventually do something like TypeScript's index signatures like I mentioned here, but I haven't had much time to work on Psalm lately.

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

I found these snippets:

https://psalm.dev/r/e0971d3777 ```php */ $arr = []; /** @psalm-trace $arr */; ``` ``` Psalm output (using commit f960d71): INFO: Trace - 5:25 - $arr: array{foo: 1} INFO: UnusedVariable - 4:1 - $arr is never referenced or the value is not used ```
michnovka commented 2 years ago

Ok, there seems to be some "type flattening" going on: https://psalm.dev/r/2489730359 when working on multi-dimensional arrays

See the MixedArrayAssignment error which in my opinion is a bug

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

I found these snippets:

https://psalm.dev/r/2489730359 ```php }&array */ $x = [ 'attr' => [], ]; /** @psalm-trace $x */ $x['attr']['foo'] = 'bar'; ``` ``` Psalm output (using commit f960d71): INFO: Trace - 7:1 - $x: array{attr: array} INFO: MixedArrayAssignment - 12:1 - Cannot access array value on mixed variable $x['attr']['foo'] INFO: Trace - 12:1 - $x: array{attr: mixed} INFO: UnusedVariable - 7:1 - $x is never referenced or the value is not used ```