Psalm: Unused array when only write access in loop

Created on 20 Sep 2020  Â·  10Comments  Â·  Source: vimeo/psalm

As detected in https://github.com/vimeo/psalm/pull/4222

Here is a reproducible case:
https://psalm.dev/r/0faf41dfd1

This should have been caught as unused.

This may be only applicable to scalar/null/resource values. In case of an object, it could have a destructor which call will be delayed if the object is in an array.

bug hard problems

All 10 comments

I found these snippets:


https://psalm.dev/r/0faf41dfd1

<?php

$a = [];

while((bool)rand(0,1)){
    $a[] = 1;   
}
Psalm output (using commit 7093253):

No issues!

@orklah does PHPStorm consider $a to be unused here?

<?php
$a = 0;
$b = 0;

while (rand(0,1)){
    $b = $a + 1;
    $a = $b;
}

echo $b;

@muglug No, it's not flagged as unused

But it _is_ flagged for this?

<?php
$a = 0;

while (rand(0,1)){
    $b = $a + 1;
    $a = $b;
}

Well, neither, but:

  • $b will be flagged as redundant and proposed fix is to transform the inside of the loop to $a = $a + 1
  • At that point, this statement will be flagged and this suggestion appears $a++
  • And at last, $a++ will be flagged as unused because there is no more read access with $a

(All the above is the PHP Inspection EA plugin, not native PHPStorm)

PHPStorm inference engine is not at all at Psalm's level. This is just a succession of well recognised patterns that will be individually flagged. That's why it was able to flag an array with only write access but at the same time, it will fail to detect that the only usage of a variable is to write in itself.

Thanks.

I don't actually think I'll be able to fix the general case, then – at least, it's beyond the data structures Psalm currently uses to track usage.

Basically there's no way to see a difference between

<?php
$a = 0;

while (rand(0,1)){
    $b = $a + 1;
    $a = $b;
}

and

<?php
$a = 0;

while (rand(0,1)){
    $b = $a + 1;
    echo $b;
    $a = $b;
}

without using flow-based analysis (e.g. used in Psalm's taint analysis) which I think is overkill.

What about the example I posted in the initial post? Your example seem way more convoluted. Any variable without read access should be flagged as unused, no?

I normally like to solve for the general case, and I think it might actually be possible to piggy-back on the existing taint detection logic here.

This is now fixed!

Wah! This was a massive amount of work!

Thanks!

Was this page helpful?
0 / 5 - 0 ratings