search

sirbrillig / phpcs-variable-analysis

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

Start inline block scope at end of condition rather than at keyword #230

Closed sirbrillig closed 3 years ago

sirbrillig commented 3 years ago

When trying to determine if a variable used within an else block has been defined, we need to determine the scope of the associated if block. This is because if the definition was within that block, then the usage inside the else is out of scope. Here's an example:

if (something()) {
  $x = 'hello';
} else {
  echo $x; // $x is undefined here
}

When the if block is using curly braces, identifying the start and end of the block is easy. However, it's a little more difficult when the block is inline:

if (something())
  $x = 'hello';
else
  echo $x; // $x is undefined

In the inline case, we have to search for the start and end of the block. Prior to this PR, the block was considered to go from the if statement to the first semicolon. This works for the above case, but it doesn't work if the variable is being defined within the condition:

if ($x = getData())
  whatever();
else
  echo $x; // $x is defined, even though falsy

In this PR we modify that logic so that the block is instead considered to go from the end of the condition to the first semicolon.

Reported by @djibarian in https://github.com/sirbrillig/phpcs-variable-analysis/issues/228

jrfnl commented 3 years ago

Just read the above (excellent!) explanation and another test case came to mind, which I fear may not always be handled correctly, though I'd love be wrong ;-)

Mind: I fully realize that analysing this case is something for which static analysis tools like PHPCS are not really suitable.

if ($a === true && $b = callMe()) {
    // Do something
} else {
    // $b will only exist in this scope if `$a` was `true` and $b "falsey".
}
sirbrillig commented 3 years ago

The main goal of this PR was to make the behavior of inline block scope detection work the same as the behavior of curly-brace block scope detection, but you're right anyway.

I haven't come up with a way to handle it within this sniff. I think it's one of those issues where we may just have to accept a false negative (one of many reasons why assignment within a condition is probably not a great idea). Maybe I should make an issue though at least to document the flaw.