Add reassigned variable warning

[kkmuffme]

If there is code like:

$hello = 'world';
$hello = 'abc';
echo $hello;

the first $hello declaration is not reported as "unused" even though it should, since it isn't used & this line is useless.

This case often happens when refactoring (of course in a more complicated way, with more code between the lines)

[sirbrillig]

Hm... this is an interesting issue. I'd love to be able to catch things like this, but it's tricky.

I guess this would be: if a variable is assigned, then assigned again before being used, show a warning. I wonder if there are legitimate cases where that would be a problem, though?

What about this example?

$name = 'unknown';
foreach ($people as $person) {
  if ($person->id === $data->id) {
    $name = $person->name;
  }
}
echo "The name is {$name}";

That would cause a warning for $name being assigned twice but it's not a useful one.

While that example could be refactored to a single array_reduce() statement, using foreach in this way seems very common to me. Can you think of a way to identify the case you're describing so that it would not cause false positives?

[kkmuffme]

I actually just caught this since we moved to PHPStorm which also highlights these cases. Most common use case (where I actually found this) is that code was refactored, and some earlier assignments were kept accidentally, thus leading to this unused variable assignment.

Your foreach example is totally highlighting the issue - thus I would suggest to limit the scope in foreach/while/for to the loop itself, to avoid false positives.

[sirbrillig]

I would suggest to limit the scope in foreach/while/for to the loop itself, to avoid false positives.

I thought of another one:

$name = 'unknown';
if ($user === 'admin') {
  $name = 'administrator';
}
echo $name;

So: within the current scope or within the current foreach/while/for/do-while/if/else/else-if/switch block, if a variable is assigned, then assigned again (not entering into sub foreach/while/for/do-while/if/else/else-if/switch blocks), report a warning.

Pretty complex logic. I'll see if I can write up some test cases.

[sirbrillig]

Oh, also, I suppose we do want to enter the statements themselves, just not their blocks:

$name = 'unknown';
for ($name = 1; $name++; $name < 5) { // Should report a warning
  echo 'hello';
} 

[sirbrillig]

I added a draft with tests in #102 but it may take a while to work out the logic necessary for those tests to pass. Help is welcome!

[kkmuffme]

Just checked & it looks good.

If you could merge it, I would run it through our whole code base to check for any false positives, but afaik there isn't anything that you haven't catered to in the tests I can think of either.

[kkmuffme]

hey, any update on this? would be great if it could be released

[sirbrillig]

Thank you for checking in. As I mentioned above, all I've added are (failing) tests. Actually writing the code to do this would be pretty complex and I haven't had time to work on it lately. If you'd like to give it a try, you'd be more than welcome. The tests in that PR can be used as a way to track your progress.