Skip to content
New issue

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

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

Already on GitHub? # to your account

Generic/RequireExplicitBooleanOperatorPrecedence: remove unreachable condition #635

Conversation

rodrigoprimo
Copy link
Contributor

@rodrigoprimo rodrigoprimo commented Oct 18, 2024

Description

This PR reverts the changes to the Generic.CodeAnalysis.RequireExplicitBooleanOperatorPrecedence sniff introduced in PHPCSStandards/PHPCSExtra@7b38efb. The sniff's tests remain relevant, so they were preserved.

The original commit was added to fix false positives that the sniff was triggering when handling boolean operators inside a match (see PHPCSStandards/PHPCSExtra#271 (review) -1634348864 and PHPCSStandards/PHPCSExtra#271 (comment) ). Example:

match (true) {
    $a || ($b && $c) => true,
};

I believe the false positive was actually caused by a bug in File::findStartOfStatement(). This bug was then fixed in b82438f which rendered the changes to the sniff itself unnecessary and the removed condition unreachable.

Before this fix, when processing the code example above, File::findStartOfStatement() returned the variable $a as the start of the statement for the && boolean operator. This meant that $previous would be set to ||, and the removed condition would be needed to ensure the sniff would bail instead of triggering an error.

After this fix, File::findStartOfStatement() returns $b as the start of the statement, and then $previous is set to false. Thus, the sniff bails before reaching the removed condition.

Including Tokens::$blockOpeners in RequireExplicitBooleanOperatorPrecedenceSniff::$searchTargets was necessary only for the removed condition, so it was removed as well.

cc @TimWolla in case you are available to check this change you are the author of the sniff.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

unreachable condition

This commit reverts the changes to the
`Generic.CodeAnalysis.RequireExplicitBooleanOperatorPrecedence` sniff
introduced in
PHPCSStandards/PHPCSExtra@7b38efb.
The sniff's tests remain relevant, so they were preserved.

The original commit was added to fix false positives that the sniff was
triggering when handling boolean operators inside a match (see
PHPCSStandards/PHPCSExtra#271 (review)
-1634348864 and
PHPCSStandards/PHPCSExtra#271 (comment)
). Example:

```php
match (true) {
    $a || ($b && $c) => true,
};
```

I believe the false positive was actually caused by a bug in
`File::findStartOfStatement()`. This bug was then fixed in
PHPCSStandards@b82438f
which rendered the changes to the sniff itself unnecessary and the
removed condition unreachable.

Before this fix, when processing the code example above,
`File::findStartOfStatement()` returned the variable `$a` as the
start of the statement for the `&&` boolean operator. This meant that
`$previous` would be set to `||` and the removed condition would be
needed to ensure the sniff would bail instead of triggering an error.

After this fix, `File::findStartOfStatement()` returns `$b` as the start
of the statement and then `$previous` is set to `false` and the sniff
bails before reaching the removed condition.

Including `Tokens::$blockOpeners` in
`RequireExplicitBooleanOperatorPrecedenceSniff::$searchTargets` was
necessary only for the removed condition, so it was removed as well.
@TimWolla
Copy link
Contributor

This bug was then fixed in 82438f2 which rendered the changes to the sniff itself unnecessary and the removed condition unreachable.

Unfortunately that's a dead link.

in case you are available to check this change you are the author of the sniff

I don't really remember any more than what you found out yourself. If tests are still green, then this should be good. Thanks 😄

@rodrigoprimo
Copy link
Contributor Author

Thanks for the quick reply, @TimWolla.

Unfortunately that's a dead link.

Ops, my bad. I just fixed the link.

@TimWolla
Copy link
Contributor

Ops, my bad. I just fixed the link.

Thank you. Based on the description of the commit message, I agree that this PR makes sense.

@jrfnl jrfnl added this to the 3.11.0 milestone Oct 28, 2024
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. Thanks @rodrigoprimo!

@jrfnl jrfnl merged commit 0f5680a into PHPCSStandards:master Oct 28, 2024
43 checks passed
@jrfnl jrfnl deleted the require-explicit-boolean-operator-precedence-remove-unreachable-condition branch October 28, 2024 00:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants