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

✨ New Universal.CodeAnalysis.NoDoubleNegative sniff #277

Merged
merged 1 commit into from
Nov 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
Comment on lines +52 to +58
Copy link

Choose a reason for hiding this comment

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

Out of curiosity: why not use the constructor for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's rare for a sniff to have a constructor and the register() method is normally only called once anyway, so as good a place as any to do this in.


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