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

Add Universal.CodeAnalysis.MixedBooleanOperator #271

Closed
wants to merge 36 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
6b72184
Add Universal.CodeAnalysis.MixedBooleanOperator
TimWolla Sep 15, 2023
9414411
Fully qualify constants in MixedBooleanOperatorSniff
TimWolla Sep 15, 2023
cca88c1
Improve error message in MixedBooleanOperatorSniff
TimWolla Sep 15, 2023
b4a8ed0
Use `BCFile::findStartOfStatement` in MixedBooleanOperatorSniff
TimWolla Sep 15, 2023
0cc2f34
Add test with an anonymous function having attributes in MixedBoolean…
TimWolla Sep 15, 2023
a051300
Extend attribute example for MixedBooleanOperatorUnitTest
TimWolla Sep 15, 2023
b8c6d30
Handle curly braces in MixedBooleanOperatorSniff
TimWolla Sep 15, 2023
ce00faf
Remove unreachability assertions in MixedBooleanOperatorSniff
TimWolla Sep 15, 2023
4a34225
Satisfy PHPStan
TimWolla Sep 18, 2023
1e87cae
Add “Not OK” comments to MixedBooleanOperatorUnitTest.inc
TimWolla Sep 18, 2023
8f83191
Make use of a local search in `findPrevious()`
TimWolla Sep 18, 2023
b713551
Extend test
TimWolla Sep 18, 2023
54fa62c
Fix handling of ternaries
TimWolla Sep 18, 2023
5c82b55
Handle all binary boolean operators
TimWolla Sep 18, 2023
06b47f1
Leverage Tokens::$booleanOperators
TimWolla Sep 19, 2023
ba07bcf
Report only the first operator if a type within a single expression
TimWolla Sep 19, 2023
13e9660
Update README
TimWolla Sep 19, 2023
7a71b8a
Clean up implementation
TimWolla Sep 19, 2023
6f8e5ba
Improve naming of the class property
TimWolla Sep 19, 2023
2985f82
Drop unused empty error data
TimWolla Sep 20, 2023
20541c0
Rename MixedBooleanOperatorSniff::$previousTokens and add docblock
TimWolla Sep 20, 2023
9df7cd2
Add `if()` example to MixedBooleanOperatorStandard.xml
TimWolla Sep 21, 2023
855e932
Do not use `array_merge()`
TimWolla Sep 21, 2023
9df244b
Fix codestyle
TimWolla Sep 21, 2023
27f6552
Consistentify the error message
TimWolla Sep 21, 2023
c07507c
Hard wrap the error message
TimWolla Oct 6, 2023
ecc5bd4
Fix formatting in MixedBooleanOperatorStandard.xml
TimWolla Oct 6, 2023
7b38efb
Fix match arms
TimWolla Oct 6, 2023
6d84a98
Add additional match tests
TimWolla Oct 6, 2023
14f2d22
Add arrow function test
TimWolla Oct 6, 2023
9f78b15
Run composer fixcs
TimWolla Oct 8, 2023
12ce668
Add additional test
TimWolla Oct 16, 2023
312beb6
Rename Sniff
TimWolla Oct 16, 2023
d152ea7
Reference related sniffs
TimWolla Oct 16, 2023
3126cd6
Expand documentation
TimWolla Oct 16, 2023
100115b
Unify `@copyright` line
TimWolla Oct 16, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ Detects `foreach` control structures which use the same variable for both the ke

Note: The fixer will maintain the existing behaviour of the code. This may not be the _intended_ behaviour.

#### `Universal.CodeAnalysis.MixedBooleanOperator` :books:

Forbids mixing `&&` and `||` within a single expression without making precedence clear using parentheses.

#### `Universal.CodeAnalysis.NoEchoSprintf` :wrench: :books:

Detects use of the inefficient `echo [v]sprintf(...);` combi. Use `[v]printf()` instead.
Expand Down
35 changes: 35 additions & 0 deletions Universal/Docs/CodeAnalysis/MixedBooleanOperatorStandard.xml
TimWolla marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?xml version="1.0"?>
<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd"
title="Mixed Boolean Operator"
>
<standard>
<![CDATA[
Forbid mixing different binary boolean operators within a single expression without making precedence clear using parentheses.
]]>
</standard>
<code_comparison>
<code title="Valid: Making precedence clear with parentheses.">
<![CDATA[
$one = false;
$two = false;
$three = true;

$result = <em>($one && $two) || $three</em>;
$result2 = <em>$one && ($two || $three)</em>;
$result3 = <em>($one && !$two) xor $three;</em>;
$result4 = <em>$one && (!$two xor $three);</em>;
]]>
</code>
<code title="Invalid: Not using parentheses.">
<![CDATA[
$one = false;
$two = false;
$three = true;

$result = <em>$one && $two || $three</em>;
$result3 = <em>$one && !$two xor $three;</em>;
]]>
</code>
</code_comparison>
</documentation>
95 changes: 95 additions & 0 deletions Universal/Sniffs/CodeAnalysis/MixedBooleanOperatorSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
<?php
/**
* PHPCSExtra, a collection of sniffs and standards for use with PHP_CodeSniffer.
*
* @package PHPCSExtra
* @copyright 2021 WoltLab GmbH, 2023 PHPCSExtra Contributors
TimWolla marked this conversation as resolved.
Show resolved Hide resolved
* @license https://opensource.org/licenses/LGPL-3.0 LGPL3
* @link https://github.com/PHPCSStandards/PHPCSExtra
*/

namespace PHPCSExtra\Universal\Sniffs\CodeAnalysis;

use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Sniffs\Sniff;
use PHPCSUtils\BackCompat\BCFile;

/**
* Forbid mixing different binary boolean operators within a single expression without making precedence
* clear using parentheses.
*
TimWolla marked this conversation as resolved.
Show resolved Hide resolved
* @link https://github.com/squizlabs/PHP_CodeSniffer/pull/3205
* @link https://github.com/php-fig/per-coding-style/issues/22
*
* @since 1.2.0
*/
final class MixedBooleanOperatorSniff implements Sniff
{

/**
* Returns an array of tokens this test wants to listen for.
*
* @since 1.2.0
*
* @return array<int|string>
*/
public function register()
{
return [
\T_BOOLEAN_OR,
\T_BOOLEAN_AND,
\T_LOGICAL_AND,
\T_LOGICAL_OR,
\T_LOGICAL_XOR,
];
TimWolla marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Processes this test, when one of its tokens is encountered.
*
* @since 1.2.0
*
* @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned.
* @param int $stackPtr The position of the current token
* in the stack passed in $tokens.
*
* @return void
*/
public function process(File $phpcsFile, $stackPtr)
{
$tokens = $phpcsFile->getTokens();
$token = $tokens[$stackPtr];

$start = BCFile::findStartOfStatement($phpcsFile, $stackPtr);

$valid = $token['code'];

$previous = $phpcsFile->findPrevious(
\array_filter(
\array_merge(
$this->register(),
[\T_INLINE_THEN, \T_INLINE_ELSE]
),
function ($token) use ($valid) {
return $token !== $valid;
}
),
TimWolla marked this conversation as resolved.
Show resolved Hide resolved
$stackPtr,
$start,
false,
null,
true
);

if (
$previous === false
|| \in_array($tokens[$previous]['code'], [\T_INLINE_THEN, \T_INLINE_ELSE], true)
Copy link
Author

Choose a reason for hiding this comment

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

Should these be implicitly handled when setting local to true for findPrevious?

Copy link
Member

Choose a reason for hiding this comment

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

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.

jrfnl marked this conversation as resolved.
Show resolved Hide resolved
) {
return;
}

// We found a mismatching operator, thus we must report the error.
$error = "Mixing different binary boolean operators within an expression without using parentheses to clarify precedence.";
$phpcsFile->addError($error, $stackPtr, 'MissingParentheses', []);
TimWolla marked this conversation as resolved.
Show resolved Hide resolved
}
}
81 changes: 81 additions & 0 deletions Universal/Tests/CodeAnalysis/MixedBooleanOperatorUnitTest.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php
TimWolla marked this conversation as resolved.
Show resolved Hide resolved

if (true && true || true); // Not OK.
if ((true && true) || true);
if (true && (true || true));

$var = true && true || true; // Not OK.
$var = (true && true) || true;
$var = true && (true || true);

$complex = true && (true || true) && true;
$complex = true && (true || true) || true; // Not OK.

if (
true
&& true
|| true // Not OK.
);

if (
true
&& (
true
|| true
)
);

if (true && foo(true || true));
if (true && foo(true && true || true)); // Not OK.
if (true && $foo[true || true]);
if (true && $foo[true && true || true]); // Not OK.

if (true && foo(true) || true); // Not OK.
if (true && $foo[true] || true); // Not OK.
if (true && foo($foo[true]) || true); // Not OK.

$foo[] = true && true || false; // Not OK.

foo([true && true || false]); // Not OK.

if (true && true || true && true); // Not OK.

$foo = false || true && (#[\Attr(true && true || true)] function (#[\SensitiveParameter] $p) { // Not OK.
echo true || true && true; // Not OK.

return true;
})('dummy') || false; // Not OK.

$foo = false || (true && (#[\Attr((true && true) || true)] function (#[\SensitiveParameter] $p) {
echo (true || true) && true;

return true;
})('dummy')) || false;

$foo = true || true || (#[\Attr(true && true && true)] function (#[\SensitiveParameter] $p) {
echo true && true && true;

return true;
})('dummy') || false;

if (true && [true, callMe(), ${true || true}] || true); // Not OK.
if (true && [true, callMe(), ${true || true}] && true);

for (true || true || true; true && true && true; true || true || true);
for (true || true && true; true && true || true; true || true && true); // Not OK.

for ($a = true || true || true, $b = true && true && true; $a; $b);
for ($a = true || true && true, $b = true || true && true; $a; $b); // Not OK.

$foo = true || true || true ? true && true && true : true || true || true;
$foo = true && true || true // Not OK.
? true || true && true // Not OK.
: true || true && true; // Not OK.

for(true || true || true, true && true && true);
for(true && true || true, true && true || true); // Not OK.

(true && true and true); // Not OK.
(true && true or true); // Not OK.
(true and true or true); // Not OK.
(true and true xor true and true); // Not OK.
71 changes: 71 additions & 0 deletions Universal/Tests/CodeAnalysis/MixedBooleanOperatorUnitTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?php
/**
* PHPCSExtra, a collection of sniffs and standards for use with PHP_CodeSniffer.
*
* @package PHPCSExtra
* @copyright 2021 WoltLab GmbH, 2023 PHPCSExtra Contributors
* @license https://opensource.org/licenses/LGPL-3.0 LGPL3
* @link https://github.com/PHPCSStandards/PHPCSExtra
*/

namespace PHPCSExtra\Universal\Tests\CodeAnalysis;

use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;

/**
* Unit test class for the MixedBooleanOperator sniff.
*
* @covers PHPCSExtra\Universal\Sniffs\CodeAnalysis\MixedBooleanOperatorSniff
*
* @since 1.2.0
*/
final class MixedBooleanOperatorUnitTest extends AbstractSniffUnitTest
{

/**
* Returns the lines where errors should occur.
*
* @return array<int, int> Key is the line number, value is the number of expected errors.
*/
public function getErrorList()
{
return [
3 => 1,
7 => 1,
12 => 1,
17 => 1,
29 => 1,
31 => 1,
33 => 1,
34 => 1,
35 => 1,
37 => 1,
39 => 1,
41 => 2,
43 => 2,
44 => 1,
47 => 1,
61 => 1,
65 => 3,
68 => 2,
71 => 1,
72 => 1,
73 => 1,
76 => 2,
78 => 1,
79 => 1,
80 => 1,
81 => 2,
];
}

/**
* Returns the lines where warnings should occur.
*
* @return array<int, int> Key is the line number, value is the number of expected warnings.
*/
public function getWarningList()
{
return [];
}
}