Add Universal.CodeAnalysis.MixedBooleanOperator #271
Conversation
This is a clone of the proposed sniff in squizlabs/PHP_CodeSniffer#3205. Adding it to PHPCSExtra to make it generally available quicker than by including it in core CodeSniffer.
There was a problem hiding this comment.
Hi @TimWolla, oh well, figured I may as well review it straight away. Thanks for getting this PR transferred over!
Naming of the sniff
Question: should the sniff name be MixedBooleanOperator or MixedBooleanOperators (note the plural) ?
Personally, I'm leaning towards the plural form as if there is only one operator, the issue doesn't exist and there will never be a mix.
Note: we need to settle on a good name now as renaming the sniff later would be a breaking change.
Premise of the sniff
As things are, the sniff only concerns itself with the boolean and/or operators.
Questions:
- Should it be documented (in the class docblock) why the boolean not
!operator is not included in this sniff ? - Should the sniff also apply to the logical
and/or/xoroperators ?
If not, maybe we should open an issue to create a sister-sniff for the logical operators ?
Review notes:
Docs: checked against the typical issues (line length too long, non-space indentation, missing <em> tags etc) and everything is 💯 okay.
Sniff & tests: see my inline notes.
Metrics are not relevant for this sniff. ✔️
Auto-fixing could be possible for this sniff, but would involve an assumption about the correctness of the code as written, so I'm fine with this sniff not having a fixer and letting a human decide where the parentheses should be. ✔️
I'm also trying to think if there is a situation in which the sniff should stop scanning or behave differently when an operator with a higher precedence is encountered ?
Note: the $phpcsFile->findStartOfStatement($stackPtr) call does not take this into account, so the sniff would have to.
Other
The build failures related to the code coverage checks are a known issue for PRs from forks and I'm working with the Coveralls team on a solution. You don't need to worry about this (though checking code coverage locally for the sniff would be good and will show that the sniff as-is is not completely covered by tests).
Hope this review helps. Please let me know if you have any questions or would like me to clarify any of my remarks.
|
Handled the obvious stuff already. Will look into the stuff that needs some additional thinking later. Thank you for the review so far! |
|
|
||
| if ( | ||
| $previous === false | ||
| || \in_array($tokens[$previous]['code'], [\T_INLINE_THEN, \T_INLINE_ELSE], true) |
There was a problem hiding this comment.
Should these be implicitly handled when setting local to true for findPrevious?
There was a problem hiding this comment.
PHPCS does not take these into account. Whether it should or not, is a different debate and I'd rather not pollute this PR with that discussion as we can't solve it in the PHPCSExtra repo anyway.
This makes sense. Should this also include the “Binary” part within the name? i.e. MixedBinaryBooleanOperators?
With including the Binary in the name this should be clear. But I can add a short blurb (or you add some suggestion using GitHub's feature.
Included now.
Yes, this Sniff is intentionally written for detecting possibly incorrect code and we found several instances in our code base with the initial version I proposed. That's why autofixing must not happen.
The Sniff tripping over such a situation would likely indicate that the human would also be confused. I am fine with some false-positives should there be such a situation, you can't have enough parentheses to make precedence clear. And if you mix
I wasn't able to get coverage running locally, due to the ancient PHPUnit requirement that is not trivially compatible with PHP 8.2. But with the updated code there isn't really much logic left in the sniff itself. If you would check that it's up to 100% coverage now, I'd appreciate it. |
|
The commit history is a little messy due to the number of changes required due to my lack of knowledge of CodeSniffer. I can squash it for you once you are happy with the diff. |
Yes please. Would be great if you could do that once the PR is ready for merge. |
There was a problem hiding this comment.
I've given the sniff another look over and left some comments, but it looks like a number of my remarks from my previous comment haven't been addressed yet:
- Deciding on a final name for the sniff.
- Deciding whether to include the "binary" terminology in the messaging. I'm unsure whether that would clarify things or cause more confusion.
- "Boolean operators" vs "logical operators" terminology.
- Adding some tests which involve arrow functions.
While I don't want to blame the upstream library, I must say that this is very surprising behavior for constructs that are so similar. Is this a bug in CodeSniffer itself or is this intentional? If this is intentional, do you have some specific advice / code snippet on how to fix this in this Sniff, ideally without replicating the entire logic?
Looks like this was done by design in PHPCS upstream, though I'm not sure that was the correct choice. Having said that, I don't think comparing match conditions with array keys is an even remotely fair comparion. Array keys are always singular (one key per item), while match branches can have multiple comma-separated conditions, which, what with the comma also being the end of the "return" statement makes parsing match expressions based on the tokens a lot harder.
If needs be, I'd be okay with adding the simplified match code to the tests and marking this as a known false positive + opening an issue to follow up on this later (and adding a link to the issue as a comment to the test).
Yes, I plan to do the renaming and related changes once the functionality is clear and correct to avoid unnecessary churn. I was waiting for advice regarding the match statement, before proceeding. All the stuff that is not yet collapsed still is on my list. |
|
Okay, I managed to fix the handling for the match case and added arrow function tests. I believe this should now be everything with regard to the implementation. I'll think about the naming adjustments once you confirm the implementation is correct and complete. Do I still need “syntax error” tests or did that become irrelevant with the simplified implementation? |
@TimWolla Thank you for making those changes and updates. I've tried to come up with cases which could break the current implementation, but my imagination struck out, so I think we're good. Well done! 🎉 Might be worth adding the following extra tests still to safeguard things further, but these are already handled correctly: // Not ok.
match (true) {
$a || ($b && $c) && $d => true,
$b && $c['a'] || $d => true,
$b && ${$var} || $d => true,
};
I'm happy with the implementation as it is now. Let's get the name sorted & get this merged!
With the current implementation, I don't see a case where a parse error could cause the sniff to throw a PHP notice, so as far as I'm concerned it's not needed for the sniff as-it-is now. |
|
Okay, I:
|
|
Will squash the entire commit history once you approve that everything is good. |
|
FYI: I've moved this PR out of the 1.2.0 milestone as, what with the PHPCS repo move, the sniff can go into the main PHPCS repo as soon as its been pulled there (as it's been extensively reviewed here already), which is what the Op indicated originally as the preferred option. I've discussed this with @TimWolla off-GH. |
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 https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/502/commits /b82438f2e1199fb29f4825782dad686168f70352 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.
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.
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 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.
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 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.
As previously discussed on Fediverse. I've copied over the logic and tests as-is, just making the necessary adjustments to fit the code style of PHPCSExtra, to make it easy to include the Sniff in CodeSniffer itself in the future.
This is a clone of the proposed sniff in squizlabs/PHP_CodeSniffer#3205. Adding it to PHPCSExtra to make it generally available quicker than by including it in core CodeSniffer.