Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Variable reported as unused on an assignment by reference #305

Closed
rodrigoaguilera opened this issue Aug 1, 2023 · 2 comments · Fixed by #306
Closed

Variable reported as unused on an assignment by reference #305

rodrigoaguilera opened this issue Aug 1, 2023 · 2 comments · Fixed by #306
Labels

Comments

@rodrigoaguilera
Copy link

rodrigoaguilera commented Aug 1, 2023

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 sirbrillig added the bug label Aug 1, 2023
@sirbrillig
Copy link
Owner

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
Copy link
Owner

sirbrillig commented Aug 5, 2023

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 #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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants