From 4c5c195ed606cc67d88ae1a334c1349a8a0195dc Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 30 Oct 2024 09:53:59 +0100 Subject: [PATCH 1/5] Generic/DuplicateClassName: bug fix - ignore namespace operator The `namespace` keyword can also be used as an operator, in which case, the sniff as it were would throw false positives. The fix now applies for this incidentally also fixes a nasty bug which could throw the sniff into an eternal loop when an unfinished namespace statement was encountered during live coding. Includes unit tests. **Note**: the unit test for the live coding situation has to be the last unit test file for this sniff so as to not influence the other tests. To that end, it has been numbered `99`. --- .../Classes/DuplicateClassNameSniff.php | 33 +++++++++++-------- .../Classes/DuplicateClassNameUnitTest.7.inc | 9 +++++ .../Classes/DuplicateClassNameUnitTest.99.inc | 7 ++++ 3 files changed, 36 insertions(+), 13 deletions(-) create mode 100644 src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.7.inc create mode 100644 src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.99.inc diff --git a/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php b/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php index 19ff1edf64..7b407480b7 100644 --- a/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php +++ b/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php @@ -11,6 +11,7 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; +use PHP_CodeSniffer\Util\Tokens; class DuplicateClassNameSniff implements Sniff { @@ -68,19 +69,25 @@ public function process(File $phpcsFile, $stackPtr) // Keep track of what namespace we are in. if ($tokens[$stackPtr]['code'] === T_NAMESPACE) { - $nsEnd = $phpcsFile->findNext( - [ - T_NS_SEPARATOR, - T_STRING, - T_WHITESPACE, - ], - ($stackPtr + 1), - null, - true - ); - - $namespace = trim($phpcsFile->getTokensAsString(($stackPtr + 1), ($nsEnd - $stackPtr - 1))); - $stackPtr = $nsEnd; + $nextNonEmpty = $phpcsFile->findNext(Tokens::$emptyTokens, ($stackPtr + 1), null, true); + if ($nextNonEmpty !== false + // Ignore namespace keyword used as operator. + && $tokens[$nextNonEmpty]['code'] !== T_NS_SEPARATOR + ) { + $nsEnd = $phpcsFile->findNext( + [ + T_NS_SEPARATOR, + T_STRING, + T_WHITESPACE, + ], + ($stackPtr + 1), + null, + true + ); + + $namespace = trim($phpcsFile->getTokensAsString(($stackPtr + 1), ($nsEnd - $stackPtr - 1))); + $stackPtr = $nsEnd; + } } else { $nameToken = $phpcsFile->findNext(T_STRING, $stackPtr); $name = $tokens[$nameToken]['content']; diff --git a/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.7.inc b/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.7.inc new file mode 100644 index 0000000000..34d170a5d1 --- /dev/null +++ b/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.7.inc @@ -0,0 +1,9 @@ + Date: Wed, 30 Oct 2024 09:57:39 +0100 Subject: [PATCH 2/5] Generic/DuplicateClassName: bug fix - ignore comments in namespace declarations A comment in a namespace declaration statement would throw the sniff off, leading to false positives and false negatives. Fixed now. Includes unit tests. Additionally, a unit test for classes declared within a scoped global namespace has also been added. --- .../Classes/DuplicateClassNameSniff.php | 27 ++++++++++--------- .../Classes/DuplicateClassNameUnitTest.7.inc | 4 +++ .../Classes/DuplicateClassNameUnitTest.8.inc | 8 ++++++ .../Classes/DuplicateClassNameUnitTest.9.inc | 5 ++++ .../Classes/DuplicateClassNameUnitTest.php | 12 +++++++++ 5 files changed, 43 insertions(+), 13 deletions(-) create mode 100644 src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.8.inc create mode 100644 src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.9.inc diff --git a/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php b/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php index 7b407480b7..a2c287cf3a 100644 --- a/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php +++ b/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php @@ -74,19 +74,20 @@ public function process(File $phpcsFile, $stackPtr) // Ignore namespace keyword used as operator. && $tokens[$nextNonEmpty]['code'] !== T_NS_SEPARATOR ) { - $nsEnd = $phpcsFile->findNext( - [ - T_NS_SEPARATOR, - T_STRING, - T_WHITESPACE, - ], - ($stackPtr + 1), - null, - true - ); - - $namespace = trim($phpcsFile->getTokensAsString(($stackPtr + 1), ($nsEnd - $stackPtr - 1))); - $stackPtr = $nsEnd; + $namespace = ''; + for ($i = $nextNonEmpty; $i < $phpcsFile->numTokens; $i++) { + if (isset(Tokens::$emptyTokens[$tokens[$i]['code']]) === true) { + continue; + } + + if ($tokens[$i]['code'] !== T_STRING && $tokens[$i]['code'] !== T_NS_SEPARATOR) { + break; + } + + $namespace .= $tokens[$i]['content']; + } + + $stackPtr = $i; } } else { $nameToken = $phpcsFile->findNext(T_STRING, $stackPtr); diff --git a/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.7.inc b/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.7.inc index 34d170a5d1..95688e9679 100644 --- a/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.7.inc +++ b/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.7.inc @@ -7,3 +7,7 @@ interface YourInterface {} namespace F; namespace\functionCall(); class MyClass {} + +namespace /*comment*/ G; +namespace\functionCall(); +class MyClass {} diff --git a/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.8.inc b/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.8.inc new file mode 100644 index 0000000000..4ae5578269 --- /dev/null +++ b/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.8.inc @@ -0,0 +1,8 @@ + 1]; + case 'DuplicateClassNameUnitTest.8.inc': + return [ + 7 => 1, + 8 => 1, + ]; + + case 'DuplicateClassNameUnitTest.9.inc': + return [ + 3 => 1, + 4 => 1, + ]; + default: return []; }//end switch From 3dd91e2f9ffffae9002558610b9023e6e0a45359 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 30 Oct 2024 09:33:54 +0100 Subject: [PATCH 3/5] Generic/DuplicateClassName: bug fix - namespace is reset on PHP open tag As things were, the sniff listens to the `T_OPEN_TAG` token, processes the file until the first `T_CLOSE_TAG` and then waits again for a new `T_OPEN_TAG`. However, every time the sniff is called, the namespace is reset, even when still processing the same file, i.e. when the namespace is still in effect. This led to both false positives as well as false negatives. Fixed now, by always processing the complete file in one go and not returning on a `T_CLOSE_TAG`. Includes unit tests. --- .../Sniffs/Classes/DuplicateClassNameSniff.php | 9 ++------- .../Tests/Classes/DuplicateClassNameUnitTest.10.inc | 6 ++++++ .../Tests/Classes/DuplicateClassNameUnitTest.11.inc | 13 +++++++++++++ .../Tests/Classes/DuplicateClassNameUnitTest.php | 3 +++ 4 files changed, 24 insertions(+), 7 deletions(-) create mode 100644 src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.10.inc create mode 100644 src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.11.inc diff --git a/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php b/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php index a2c287cf3a..8c02b9b3c5 100644 --- a/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php +++ b/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php @@ -56,17 +56,10 @@ public function process(File $phpcsFile, $stackPtr) T_TRAIT, T_ENUM, T_NAMESPACE, - T_CLOSE_TAG, ]; $stackPtr = $phpcsFile->findNext($findTokens, ($stackPtr + 1)); while ($stackPtr !== false) { - if ($tokens[$stackPtr]['code'] === T_CLOSE_TAG) { - // We can stop here. The sniff will continue from the next open - // tag when PHPCS reaches that token, if there is one. - return; - } - // Keep track of what namespace we are in. if ($tokens[$stackPtr]['code'] === T_NAMESPACE) { $nextNonEmpty = $phpcsFile->findNext(Tokens::$emptyTokens, ($stackPtr + 1), null, true); @@ -120,6 +113,8 @@ public function process(File $phpcsFile, $stackPtr) $stackPtr = $phpcsFile->findNext($findTokens, ($stackPtr + 1)); }//end while + return $phpcsFile->numTokens; + }//end process() diff --git a/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.10.inc b/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.10.inc new file mode 100644 index 0000000000..d5ab20e3ed --- /dev/null +++ b/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.10.inc @@ -0,0 +1,6 @@ + + + + + + 1, ]; + case 'DuplicateClassNameUnitTest.11.inc': + return [13 => 1]; + default: return []; }//end switch From a7d86930e601b970c812ef3fedb02b7cc5501f66 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 30 Oct 2024 10:17:57 +0100 Subject: [PATCH 4/5] Generic/DuplicateClassName: use helper method Use the `File::getDeclarationName()` method instead of finding the name of an OO structure in the sniff itself. This also prevents a potential situation where if a class name could not be found, the `findNext()` method would return `false`, which would then be used in the `$tokens[$nameToken]['content']` leading to `false` being juggled to `0` and a class name of `findNext(T_STRING, $stackPtr); - $name = $tokens[$nameToken]['content']; - if ($namespace !== '') { - $name = $namespace.'\\'.$name; - } + $name = $phpcsFile->getDeclarationName($stackPtr); + if (empty($name) === false) { + if ($namespace !== '') { + $name = $namespace.'\\'.$name; + } - $compareName = strtolower($name); - if (isset($this->foundClasses[$compareName]) === true) { - $type = strtolower($tokens[$stackPtr]['content']); - $file = $this->foundClasses[$compareName]['file']; - $line = $this->foundClasses[$compareName]['line']; - $error = 'Duplicate %s name "%s" found; first defined in %s on line %s'; - $data = [ - $type, - $name, - $file, - $line, - ]; - $phpcsFile->addWarning($error, $stackPtr, 'Found', $data); - } else { - $this->foundClasses[$compareName] = [ - 'file' => $phpcsFile->getFilename(), - 'line' => $tokens[$stackPtr]['line'], - ]; - } + $compareName = strtolower($name); + if (isset($this->foundClasses[$compareName]) === true) { + $type = strtolower($tokens[$stackPtr]['content']); + $file = $this->foundClasses[$compareName]['file']; + $line = $this->foundClasses[$compareName]['line']; + $error = 'Duplicate %s name "%s" found; first defined in %s on line %s'; + $data = [ + $type, + $name, + $file, + $line, + ]; + $phpcsFile->addWarning($error, $stackPtr, 'Found', $data); + } else { + $this->foundClasses[$compareName] = [ + 'file' => $phpcsFile->getFilename(), + 'line' => $tokens[$stackPtr]['line'], + ]; + } + }//end if }//end if $stackPtr = $phpcsFile->findNext($findTokens, ($stackPtr + 1)); diff --git a/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.97.inc b/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.97.inc new file mode 100644 index 0000000000..d1bcdd3fd2 --- /dev/null +++ b/src/Standards/Generic/Tests/Classes/DuplicateClassNameUnitTest.97.inc @@ -0,0 +1,8 @@ + Date: Wed, 30 Oct 2024 11:00:22 +0100 Subject: [PATCH 5/5] Generic/DuplicateClassName: performance fix As things were, the sniff would unconditionally walk a complete file, making it one of the top 15 slowest sniffs. However, OO structures in PHP cannot be nested, so once we've found an OO declaration, we can skip to the end of it before continuing the token walking. See: https://3v4l.org/pbSTG This small tweak makes a significant difference in the sniff performance without any impact on the sniff results. --- .../Generic/Sniffs/Classes/DuplicateClassNameSniff.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php b/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php index 71a65a0a62..ca1ed09991 100644 --- a/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php +++ b/src/Standards/Generic/Sniffs/Classes/DuplicateClassNameSniff.php @@ -109,6 +109,10 @@ public function process(File $phpcsFile, $stackPtr) ]; } }//end if + + if (isset($tokens[$stackPtr]['scope_closer']) === true) { + $stackPtr = $tokens[$stackPtr]['scope_closer']; + } }//end if $stackPtr = $phpcsFile->findNext($findTokens, ($stackPtr + 1));