From 8ec8a17bc504b707a1579499cbbae89618785259 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 21 Sep 2023 03:05:18 +0200 Subject: [PATCH] :sparkles: New `Universal.CodeAnalysis.NoDoubleNegative` sniff ... 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 --- .../CodeAnalysis/NoDoubleNegativeStandard.xml | 27 ++ .../CodeAnalysis/NoDoubleNegativeSniff.php | 269 ++++++++++++++++++ .../CodeAnalysis/NoDoubleNegativeUnitTest.inc | 104 +++++++ .../NoDoubleNegativeUnitTest.inc.fixed | 101 +++++++ .../CodeAnalysis/NoDoubleNegativeUnitTest.php | 86 ++++++ 5 files changed, 587 insertions(+) create mode 100644 Universal/Docs/CodeAnalysis/NoDoubleNegativeStandard.xml create mode 100644 Universal/Sniffs/CodeAnalysis/NoDoubleNegativeSniff.php create mode 100644 Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.inc create mode 100644 Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.inc.fixed create mode 100644 Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.php diff --git a/Universal/Docs/CodeAnalysis/NoDoubleNegativeStandard.xml b/Universal/Docs/CodeAnalysis/NoDoubleNegativeStandard.xml new file mode 100644 index 0000000..953149b --- /dev/null +++ b/Universal/Docs/CodeAnalysis/NoDoubleNegativeStandard.xml @@ -0,0 +1,27 @@ + + + + + + + + ! $b; + +if((bool) callMe($a)) {} + ]]> + + + ! ! $b; + +if(! ! ! callMe($a)) {} + ]]> + + + diff --git a/Universal/Sniffs/CodeAnalysis/NoDoubleNegativeSniff.php b/Universal/Sniffs/CodeAnalysis/NoDoubleNegativeSniff.php new file mode 100644 index 0000000..549c8cd --- /dev/null +++ b/Universal/Sniffs/CodeAnalysis/NoDoubleNegativeSniff.php @@ -0,0 +1,269 @@ + + */ + private $operatorsWithLowerPrecedence; + + /** + * Returns an array of tokens this test wants to listen for. + * + * @since 1.2.0 + * + * @return array + */ + 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, ''); + } + } + } +} diff --git a/Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.inc b/Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.inc new file mode 100644 index 0000000..dd8d871 --- /dev/null +++ b/Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.inc @@ -0,0 +1,104 @@ +prop instanceof Something); +$var = !!!($something->method() instanceof Something); +$var = !!!!($something['key']->my['key']->chained instanceof Something); + +$var = !!(self::ENUM_SET_AS_CONSTANT instanceof \Something); +$var = !!(parent::ENUM_SET_AS_CONSTANT instanceof Something); +$var = !! (static::ENUM_SET_AS_CONSTANT instanceof \Something); + +if (!! + (function ($p) { + return new $p; +})('dummy') instanceof Foo) {} + +// If an instanceof is after another operator with lower precedence, we're good. +if (!!$something && $somethingElse instanceof \Something) {} +if (!!$something() || $somethingElse instanceof \Something) {} +if (!! $boolean !== $somethingElse instanceof Something) {} +while (!! $boolean = $somethingElse instanceof \Something) {} +if ( ! ! $boolean and $somethingElse instanceof \Something) {} +$var = !!$boolean ? $somethingElse instanceof Something : true; +$var = !!$boolean ?? $somethingElse instanceof \Something; + +// Instanceof where the not does not apply to. +if (!!$something($somethingElse instanceof \Something)) {} +if (!!['key' => $somethingElse instanceof \Something]['key']) {} +if (!!$array[$somethingElse instanceof \Something ? 'keyA' : 'keyB']) {} +if (!!match($a) { 'foo' => $a instanceof Foo, 'bar' => $a instanceof Bar } === true) {} + +// Note: this contains an intentional parse error, nothing to worry about. +if (!! + #[\Attr(self::ENUM_SET_AS_CONSTANT instanceof Foo)] + (function ($p) { + return $p instanceof Foo; +})('dummy')) {} + +// If it's an instanceof with triple not, we're good. +$var = !!!$something->method() instanceof \Something; + +// Make the error non-autofixable when used in combination with instanceof without parentheses to avoid issues with precedence. +if(!!$something instanceof \Something) {} + +$var = !!$something->prop instanceof \Something; + +$var = !!self::ENUM_SET_AS_CONSTANT instanceof \Something; +$var = (!!parent::ENUM_SET_AS_CONSTANT instanceof \Something); +$var = !! static::ENUM_SET_AS_CONSTANT instanceof \Something; + +$var = (!!!!$something['key']->my['key']->chained instanceof \Something); + +$var = !!$something[$a + 1] instanceof \Something; +$var = !!$something[++$a] instanceof \Something; + +$var = !!callMe($something[$a + 1]) instanceof \Something; +$var = !!new ClassA instanceof ('std' . 'Class'); + +if (!!match($a) { 'foo' => new Foo, 'bar' => new Bar } instanceof Bar) {} + +// Intentional parse error/live coding. +// This needs to be the last test in the file. +if(! diff --git a/Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.inc.fixed b/Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.inc.fixed new file mode 100644 index 0000000..185f919 --- /dev/null +++ b/Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.inc.fixed @@ -0,0 +1,101 @@ +prop instanceof Something); +$var = !($something->method() instanceof Something); +$var = (bool)($something['key']->my['key']->chained instanceof Something); + +$var = (bool)(self::ENUM_SET_AS_CONSTANT instanceof \Something); +$var = (bool)(parent::ENUM_SET_AS_CONSTANT instanceof Something); +$var = (bool) (static::ENUM_SET_AS_CONSTANT instanceof \Something); + +if ((bool) + (function ($p) { + return new $p; +})('dummy') instanceof Foo) {} + +// If an instanceof is after another operator with lower precedence, we're good. +if ((bool)$something && $somethingElse instanceof \Something) {} +if ((bool)$something() || $somethingElse instanceof \Something) {} +if ((bool) $boolean !== $somethingElse instanceof Something) {} +while ((bool) $boolean = $somethingElse instanceof \Something) {} +if ( (bool) $boolean and $somethingElse instanceof \Something) {} +$var = (bool)$boolean ? $somethingElse instanceof Something : true; +$var = (bool)$boolean ?? $somethingElse instanceof \Something; + +// Instanceof where the not does not apply to. +if ((bool)$something($somethingElse instanceof \Something)) {} +if ((bool)['key' => $somethingElse instanceof \Something]['key']) {} +if ((bool)$array[$somethingElse instanceof \Something ? 'keyA' : 'keyB']) {} +if ((bool)match($a) { 'foo' => $a instanceof Foo, 'bar' => $a instanceof Bar } === true) {} + +// Note: this contains an intentional parse error, nothing to worry about. +if ((bool) + #[\Attr(self::ENUM_SET_AS_CONSTANT instanceof Foo)] + (function ($p) { + return $p instanceof Foo; +})('dummy')) {} + +// If it's an instanceof with triple not, we're good. +$var = !$something->method() instanceof \Something; + +// Make the error non-autofixable when used in combination with instanceof without parentheses to avoid issues with precedence. +if(!!$something instanceof \Something) {} + +$var = !!$something->prop instanceof \Something; + +$var = !!self::ENUM_SET_AS_CONSTANT instanceof \Something; +$var = (!!parent::ENUM_SET_AS_CONSTANT instanceof \Something); +$var = !! static::ENUM_SET_AS_CONSTANT instanceof \Something; + +$var = (!!!!$something['key']->my['key']->chained instanceof \Something); + +$var = !!$something[$a + 1] instanceof \Something; +$var = !!$something[++$a] instanceof \Something; + +$var = !!callMe($something[$a + 1]) instanceof \Something; +$var = !!new ClassA instanceof ('std' . 'Class'); + +if (!!match($a) { 'foo' => new Foo, 'bar' => new Bar } instanceof Bar) {} + +// Intentional parse error/live coding. +// This needs to be the last test in the file. +if(! diff --git a/Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.php b/Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.php new file mode 100644 index 0000000..ec377b1 --- /dev/null +++ b/Universal/Tests/CodeAnalysis/NoDoubleNegativeUnitTest.php @@ -0,0 +1,86 @@ + Key is the line number, value is the number of expected errors. + */ + public function getErrorList() + { + return [ + 16 => 1, + 18 => 1, + 19 => 1, + 20 => 1, + 21 => 1, + 22 => 1, + 24 => 1, + 30 => 1, + 34 => 1, + 43 => 1, + 45 => 1, + 46 => 1, + 47 => 1, + 49 => 1, + 50 => 1, + 51 => 1, + 53 => 1, + 59 => 1, + 60 => 1, + 61 => 1, + 62 => 1, + 63 => 1, + 64 => 1, + 65 => 1, + 68 => 1, + 69 => 1, + 70 => 1, + 71 => 1, + 74 => 1, + 81 => 1, + 84 => 1, + 86 => 1, + 88 => 1, + 89 => 1, + 90 => 1, + 92 => 1, + 94 => 1, + 95 => 1, + 97 => 1, + 98 => 1, + 100 => 1, + ]; + } + + /** + * Returns the lines where warnings should occur. + * + * @return array Key is the line number, value is the number of expected warnings. + */ + public function getWarningList() + { + return []; + } +}