sirbrillig / phpcs-variable-analysis

Find undefined and unused variables with the PHP Codesniffer static analysis tool.
Other
135 stars 14 forks source link

Variable reported as unused on an assignment by reference #305

Closed rodrigoaguilera closed 1 year ago

rodrigoaguilera commented 1 year ago

I have the following code

$wrapper_id = $field_id . '_wrapper';
if (!isset($form[$field_id]) && isset($form[$wrapper_id])) {
  $element = &$form[$wrapper_id][$field_id];
} else {
  $element = &$form[$field_id];
}
$element['test'] = 'test';

(Last line added to illustrate that the variable is indeed "used").

It reports that $element is an unused variable in the first line it appears. If I initialize $element the phpcs warning is gone but it should be possible to declare a variable like that.

sirbrillig commented 1 year ago

Reproduced and I have a test case for this. Thanks for the report!

I think what's happening is that the scope created by the if/else blocks is not correctly being interpreted as part of the enclosing (function) scope... or somehow there's a mismatch between the code that identifies writes to reference variables as "use" of that variable.

sirbrillig commented 1 year ago

Interesting. It turns out it's the else block that is the problem. If you remove that, the issue does not occur.

This exposes two bugs.

First, the expression $element['test'] = 'test'; is not considered an assignment and instead is considered a read. That's a pretty big problem and I'm surprised that hasn't caused issues before. I created https://github.com/sirbrillig/phpcs-variable-analysis/issues/307 to look into that. Still, even if we change that to something more explicit like echo $element;, the warning you saw remains.

Second, when we process the assignment-by-reference inside the else block, it triggers an "end of scope" check because the variable has already been assigned by reference inside the if block (yes, both blocks don't run in the same invocation but the linter doesn't know that). When we reassign a reference variable, we need to know if it has been used first.

For example, this is the intended behavior:

$foo = &$bar;  // Warning that $foo is unused because once it is reassigned, it is a different variable.
$foo = &$yaz;
echo $foo;

When we reach the end of the scope, and at that point the variable $element has indeed not been used, which is what triggers the warning. Our sniff should be more aware that variables assigned in an else statement must ignore any assignment inside an attached if.

This should not trigger a warning:

if ( $x ) {
  $foo = &$bar;
} else {
  $foo = &$yaz;
}
echo $foo;