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

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

Merged
merged 2 commits into from
Mar 9, 2021

Conversation

sirbrillig
Copy link
Owner

@sirbrillig sirbrillig commented Mar 9, 2021

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

@sirbrillig sirbrillig merged commit e76e816 into 2.10 Mar 9, 2021
@sirbrillig sirbrillig deleted the fix-inline-if-else-scope-definition branch March 9, 2021 22:32
@jrfnl
Copy link
Collaborator

jrfnl commented Mar 9, 2021

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

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.

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

Successfully merging this pull request may close these issues.

2 participants