Skip to content

Commit

Permalink
✨ New Universal.CodeAnalysis.NoDoubleNegative sniff
Browse files Browse the repository at this point in the history
... to detect double negatives (`!!`) and advise to use a boolean cast instead.

The sniff will correctly handle a situation where even more consecutive not operators are found.

In the case, the number of operators is uneven, it will auto-fix to a single not operator.

And when a double not operator is found before an expression involving `instanceof`, the error will still be thrown, but not auto-fix as the `instanceof` operator is nested right between the `!` operator and a `(bool)` cast operator precedence wise, so auto-fixing without also adding parentheses would change the behaviour of the code.

Includes fixer.
Includes unit tests.
Includes documentation.

Ref: https://www.php.net/manual/en/language.operators.precedence.php
  • Loading branch information
jrfnl committed Sep 21, 2023
1 parent 746c319 commit 8ec8a17
Show file tree
Hide file tree
Showing 5 changed files with 587 additions and 0 deletions.
27 changes: 27 additions & 0 deletions Universal/Docs/CodeAnalysis/NoDoubleNegativeStandard.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?xml version="1.0"?>
<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd"
title="No Double Negative"
>
<standard>
<![CDATA[
Detects double negation in code, which is effectively the same as a boolean cast, but with a much higher cognitive load.
]]>
</standard>
<code_comparison>
<code title="Valid: using singular negation or a boolean cast.">
<![CDATA[
$var = $a && <em>!</em> $b;
if(<em>(bool)</em> callMe($a)) {}
]]>
</code>
<code title="Invalid: using double negation (or more).">
<![CDATA[
$var = $a && <em>! !</em> $b;
if(<em>! ! !</em> callMe($a)) {}
]]>
</code>
</code_comparison>
</documentation>
269 changes: 269 additions & 0 deletions Universal/Sniffs/CodeAnalysis/NoDoubleNegativeSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,269 @@
<?php
/**
* PHPCSExtra, a collection of sniffs and standards for use with PHP_CodeSniffer.
*
* @package PHPCSExtra
* @copyright 2023 PHPCSExtra Contributors
* @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 PHP_CodeSniffer\Util\Tokens;
use PHPCSUtils\BackCompat\BCFile;
use PHPCSUtils\Utils\GetTokensAsString;
use PHPCSUtils\Utils\Parentheses;

/**
* Detects double negation in code, which is effectively the same as a boolean cast,
* but with a much higher cognitive load.
*
* The sniff will only autofix if the precedence change from boolean not to boolean cast
* will not cause a behavioural change (as it would with instanceof).
*
* @since 1.2.0
*/
final class NoDoubleNegativeSniff implements Sniff
{

/**
* Operators with lower precedence than the not-operator.
*
* Used to determine when to stop searching for `instanceof`.
*
* @since 1.2.0
*
* @var array<int|string, int|string>
*/
private $operatorsWithLowerPrecedence;

/**
* Returns an array of tokens this test wants to listen for.
*
* @since 1.2.0
*
* @return array<int|string>
*/
public function register()
{
// Collect all the operators only once.
$this->operatorsWithLowerPrecedence = Tokens::$assignmentTokens;
$this->operatorsWithLowerPrecedence += Tokens::$booleanOperators;
$this->operatorsWithLowerPrecedence += Tokens::$comparisonTokens;
$this->operatorsWithLowerPrecedence += Tokens::$operators;
$this->operatorsWithLowerPrecedence[\T_INLINE_THEN] = \T_INLINE_THEN;
$this->operatorsWithLowerPrecedence[\T_INLINE_ELSE] = \T_INLINE_ELSE;

return [\T_BOOLEAN_NOT];
}

/**
* 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 int|void Integer stack pointer to skip forward or void to continue
* normal file processing.
*/
public function process(File $phpcsFile, $stackPtr)
{
$tokens = $phpcsFile->getTokens();

$notCount = 1;
$lastNot = $stackPtr;
for ($afterNot = ($stackPtr + 1); $afterNot < $phpcsFile->numTokens; $afterNot++) {
if (isset(Tokens::$emptyTokens[$tokens[$afterNot]['code']])) {
continue;
}

if ($tokens[$afterNot]['code'] === \T_BOOLEAN_NOT) {
$lastNot = $afterNot;
++$notCount;
continue;
}

break;
}

if ($notCount === 1) {
// Singular unary not-operator. Nothing to do.
return;
}

$found = \trim(GetTokensAsString::compact($phpcsFile, $stackPtr, $lastNot));
$data = [$found];

if (($notCount % 2) === 1) {
/*
* Oh dear... silly code time, found a triple negative (or other uneven number),
* this should just be a singular not-operator.
*/
$fix = $phpcsFile->addFixableError(
'Triple negative (or more) detected. Use a singular not (!) operator instead. Found: %s',
$stackPtr,
'FoundTriple',
$data
);

if ($fix === true) {
$phpcsFile->fixer->beginChangeset();

$this->removeNotAndTrailingSpaces($phpcsFile, $stackPtr, $lastNot);

$phpcsFile->fixer->endChangeset();
}

// Only throw one error, even if there are more than two not-operators.
return $lastNot;
}

/*
* Found a double negative, which should be a boolean cast.
*/

$fixable = true;

/*
* If whatever is being "cast" is within parentheses, we're good.
* If not, we need to prevent creating a change in behaviour
* when what follows is an `$x instanceof ...` expression, as
* the "instanceof" operator is right between a boolean cast
* and the ! operator precedence-wise.
*
* Note: this only applies to double negative, not triple negative.
*
* @link https://www.php.net/language.operators.precedence
*/
if ($tokens[$afterNot]['code'] !== \T_OPEN_PARENTHESIS) {
$end = Parentheses::getLastCloser($phpcsFile, $stackPtr);
if ($end === false) {
$end = BCFile::findEndOfStatement($phpcsFile, $stackPtr);
}

for ($nextRelevant = $afterNot; $nextRelevant < $end; $nextRelevant++) {
if (isset(Tokens::$emptyTokens[$tokens[$nextRelevant]['code']])) {
continue;
}

if ($tokens[$nextRelevant]['code'] === \T_INSTANCEOF) {
$fixable = false;
break;
}

if (isset($this->operatorsWithLowerPrecedence[$tokens[$nextRelevant]['code']])) {
// The expression the `!` belongs to has ended.
break;
}

// Skip over anything within some form of brackets.
if (isset($tokens[$nextRelevant]['scope_closer'])
&& ($nextRelevant === $tokens[$nextRelevant]['scope_opener']
|| $nextRelevant === $tokens[$nextRelevant]['scope_condition'])
) {
$nextRelevant = $tokens[$nextRelevant]['scope_closer'];
continue;
}

if (isset($tokens[$nextRelevant]['bracket_opener'], $tokens[$nextRelevant]['bracket_closer'])
&& $nextRelevant === $tokens[$nextRelevant]['bracket_opener']
) {
$nextRelevant = $tokens[$nextRelevant]['bracket_closer'];
continue;
}

if ($tokens[$nextRelevant]['code'] === \T_OPEN_PARENTHESIS
&& isset($tokens[$nextRelevant]['parenthesis_closer'])
) {
$nextRelevant = $tokens[$nextRelevant]['parenthesis_closer'];
continue;
}

// Skip over attributes (just in case).
if ($tokens[$nextRelevant]['code'] === \T_ATTRIBUTE
&& isset($tokens[$nextRelevant]['attribute_closer'])
) {
$nextRelevant = $tokens[$nextRelevant]['attribute_closer'];
continue;
}
}
}

$error = 'Double negative detected. Use a (bool) cast %s instead. Found: %s';
$code = 'FoundDouble';
$data = [
'',
$found,
];

if ($fixable === false) {
$code = 'FoundDoubleWithInstanceof';
$data[0] = 'and parentheses around the instanceof expression';

// Don't auto-fix in combination with instanceof.
$phpcsFile->addError($error, $stackPtr, $code, $data);

// Only throw one error, even if there are more than two not-operators.
return $lastNot;
}

$fix = $phpcsFile->addFixableError($error, $stackPtr, $code, $data);

if ($fix === true) {
$phpcsFile->fixer->beginChangeset();

$this->removeNotAndTrailingSpaces($phpcsFile, $stackPtr, $lastNot);

$phpcsFile->fixer->replaceToken($lastNot, '(bool)');

$phpcsFile->fixer->endChangeset();
}

// Only throw one error, even if there are more than two not-operators.
return $lastNot;
}

/**
* Remove boolean not-operators and trailing whitespace after those,
* but don't remove comments or trailing whitespace after comments.
*
* @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.
* @param int $lastNot The position of the last boolean not token
* in the chain.
*
* @return void
*/
private function removeNotAndTrailingSpaces(File $phpcsFile, $stackPtr, $lastNot)
{
$tokens = $phpcsFile->getTokens();
$ignore = false;

for ($i = $stackPtr; $i < $lastNot; $i++) {
if (isset(Tokens::$commentTokens[$tokens[$i]['code']])) {
// Ignore comments and whitespace after comments.
$ignore = true;
continue;
}

if ($tokens[$i]['code'] === \T_WHITESPACE && $ignore === false) {
$phpcsFile->fixer->replaceToken($i, '');
continue;
}

if ($tokens[$i]['code'] === \T_BOOLEAN_NOT) {
$ignore = false;
$phpcsFile->fixer->replaceToken($i, '');
}
}
}
}
Loading

0 comments on commit 8ec8a17

Please sign in to comment.