-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
269
Universal/Sniffs/CodeAnalysis/NoDoubleNegativeSniff.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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, ''); | ||
} | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.