From 35c45ad4c0bfeb33ff6d6e19bed956002c005eab Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Mon, 26 Feb 2024 14:18:10 -0300 Subject: [PATCH 1/5] Generic/UselessOverridingMethod: consider case-insensitivity of method names Method names in PHP are case insensitive for ASCII characters. This commit changes the sniff to compare the name of the declared method with the name of the called method in a case-insensitive manner for the ASCII characters. The sniff will now flag methods as useless when PHP would consider the called method the same as the declared method, even when the called method name is in a different case as the name used in the function declaration. Includes unit tests. --- .../CodeAnalysis/UselessOverridingMethodSniff.php | 4 ++-- .../UselessOverridingMethodUnitTest.1.inc | 14 ++++++++++++++ .../UselessOverridingMethodUnitTest.php | 2 ++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php b/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php index 1b79fd3393..4a129d7d2f 100644 --- a/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php +++ b/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php @@ -97,11 +97,11 @@ public function process(File $phpcsFile, $stackPtr) return; } - // Find next non empty token index, should be the function name. + // Find next non empty token index, should be the name of the method being called. $next = $phpcsFile->findNext(Tokens::$emptyTokens, ($next + 1), null, true); // Skip for invalid code or other method. - if ($next === false || $tokens[$next]['content'] !== $methodName) { + if ($next === false || strcasecmp($tokens[$next]['content'], $methodName) !== 0) { return; } diff --git a/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.1.inc b/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.1.inc index 0d8ca6d172..b23dc7c1d0 100644 --- a/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.1.inc +++ b/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.1.inc @@ -64,6 +64,20 @@ class Bar { public function sameNumberDifferentParameters($a, $b) { return parent::sameNumberDifferentParameters($this->prop[$a], $this->{$b}); } + + public function differentCase() { + return parent::DIFFERENTcase(); + } + + public function differentCaseSameNonAnsiiCharáctêrs() { + // This should be flagged, only ASCII chars have changed case. + return parent::DIFFERENTcaseSameNonAnsiiCharáctêrs(); + } + + public function differentCaseDifferentNonAnsiiCharáctêrs() { + // This should not be flagged as non-ASCII chars have changed case, making this a different method name. + return parent::DIFFERENTcaseDifferentNonAnsiiCharÁctÊrs(); + } } abstract class AbstractFoo { diff --git a/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.php b/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.php index 7206fcc4b5..e3064d789d 100644 --- a/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.php +++ b/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.php @@ -54,6 +54,8 @@ public function getWarningList($testFile='') 16 => 1, 38 => 1, 56 => 1, + 68 => 1, + 72 => 1, ]; default: return []; From 2da8cce6093a18a1c8a4b5e5ddbc002928ef7308 Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Mon, 26 Feb 2024 16:45:41 -0300 Subject: [PATCH 2/5] Generic/UselessOverridingMethod: improve sniff handling of a parse error This commit improves how the sniff handles code that is missing the closing parenthesis in the parent method call. Now, the code will intentionally bail early in such cases. Before, the sniff would unintentionally bail when such cases were found at a later point and would incorrectly consider everything after the opening parenthesis to be the first parameter of the parent method. The commit includes a test. --- .../CodeAnalysis/UselessOverridingMethodSniff.php | 2 +- .../CodeAnalysis/UselessOverridingMethodUnitTest.6.inc | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.6.inc diff --git a/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php b/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php index 4a129d7d2f..8e15104f01 100644 --- a/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php +++ b/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php @@ -109,7 +109,7 @@ public function process(File $phpcsFile, $stackPtr) $next = $phpcsFile->findNext(Tokens::$emptyTokens, ($next + 1), null, true); // Skip for invalid code. - if ($next === false || $tokens[$next]['code'] !== T_OPEN_PARENTHESIS) { + if ($next === false || $tokens[$next]['code'] !== T_OPEN_PARENTHESIS || isset($tokens[$next]['parenthesis_closer']) === false) { return; } diff --git a/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.6.inc b/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.6.inc new file mode 100644 index 0000000000..186d4ae30a --- /dev/null +++ b/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.6.inc @@ -0,0 +1,10 @@ + Date: Tue, 27 Feb 2024 11:07:22 -0300 Subject: [PATCH 3/5] Generic/UselessOverridingMethod: remove redundant call to count() This commit simply replaces a call to `count($token)` with `$phpcsFile->numTokens`. --- .../Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php b/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php index 8e15104f01..0bc5ea83c2 100644 --- a/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php +++ b/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php @@ -115,8 +115,7 @@ public function process(File $phpcsFile, $stackPtr) $parameters = ['']; $parenthesisCount = 1; - $count = count($tokens); - for (++$next; $next < $count; ++$next) { + for (++$next; $next < $phpcsFile->numTokens; ++$next) { $code = $tokens[$next]['code']; if ($code === T_OPEN_PARENTHESIS) { From f38ea418401dcbc4b66e1616e7c142ef5def05c8 Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Tue, 27 Feb 2024 11:32:44 -0300 Subject: [PATCH 4/5] Generic/UselessOverridingMethod: remove `$next === false` checks Early on in the sniff code, it is checked if the method has a scope opener (which is never true if there is no scope closer). So there always will be at least one token after the scope opener, the scope closer. This means that `$next === false` checks are not necessary. `$next` will never be false. To be extra careful, a check to confirm that the scope closer is present was added to the beginning of the sniff. --- .../CodeAnalysis/UselessOverridingMethodSniff.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php b/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php index 0bc5ea83c2..1d55c59c7a 100644 --- a/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php +++ b/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php @@ -56,7 +56,7 @@ public function process(File $phpcsFile, $stackPtr) $token = $tokens[$stackPtr]; // Skip function without body. - if (isset($token['scope_opener']) === false) { + if (isset($token['scope_opener'], $token['scope_closer']) === false) { return; } @@ -93,7 +93,7 @@ public function process(File $phpcsFile, $stackPtr) $next = $phpcsFile->findNext(Tokens::$emptyTokens, ($next + 1), null, true); // Skip for invalid code. - if ($next === false || $tokens[$next]['code'] !== T_DOUBLE_COLON) { + if ($tokens[$next]['code'] !== T_DOUBLE_COLON) { return; } @@ -101,7 +101,7 @@ public function process(File $phpcsFile, $stackPtr) $next = $phpcsFile->findNext(Tokens::$emptyTokens, ($next + 1), null, true); // Skip for invalid code or other method. - if ($next === false || strcasecmp($tokens[$next]['content'], $methodName) !== 0) { + if (strcasecmp($tokens[$next]['content'], $methodName) !== 0) { return; } @@ -109,7 +109,7 @@ public function process(File $phpcsFile, $stackPtr) $next = $phpcsFile->findNext(Tokens::$emptyTokens, ($next + 1), null, true); // Skip for invalid code. - if ($next === false || $tokens[$next]['code'] !== T_OPEN_PARENTHESIS || isset($tokens[$next]['parenthesis_closer']) === false) { + if ($tokens[$next]['code'] !== T_OPEN_PARENTHESIS || isset($tokens[$next]['parenthesis_closer']) === false) { return; } @@ -134,7 +134,7 @@ public function process(File $phpcsFile, $stackPtr) }//end for $next = $phpcsFile->findNext(Tokens::$emptyTokens, ($next + 1), null, true); - if ($next === false || $tokens[$next]['code'] !== T_SEMICOLON) { + if ($tokens[$next]['code'] !== T_SEMICOLON) { return; } From 70710042f9b0096766d67063d49580ab86c91910 Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Fri, 1 Mar 2024 15:01:16 -0300 Subject: [PATCH 5/5] Generic/UselessOverridingMethod: bail if not checking a class method The UselessOverridingMethod is triggered when the T_FUNCTION token is found. This token is used for regular functions and also methods. The sniff should run only for class and trait methods (including abstract and anonymous classes). Those are the only methods that can have a parent. So this commit changes the sniff to bail early when dealing with a regular functions, nested functions, interfaces methods and enum methods. --- .../UselessOverridingMethodSniff.php | 19 +++++++ .../UselessOverridingMethodUnitTest.1.inc | 57 +++++++++++++++++++ .../UselessOverridingMethodUnitTest.php | 16 ++++-- 3 files changed, 86 insertions(+), 6 deletions(-) diff --git a/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php b/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php index 1d55c59c7a..4c984a16b1 100644 --- a/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php +++ b/src/Standards/Generic/Sniffs/CodeAnalysis/UselessOverridingMethodSniff.php @@ -28,6 +28,17 @@ class UselessOverridingMethodSniff implements Sniff { + /** + * Object-Oriented scopes in which a call to parent::method() can exist. + * + * @var array Keys are the token constants, value is irrelevant. + */ + private $validOOScopes = [ + T_CLASS => true, + T_ANON_CLASS => true, + T_TRAIT => true, + ]; + /** * Registers the tokens that this sniff wants to listen for. @@ -60,6 +71,14 @@ public function process(File $phpcsFile, $stackPtr) return; } + $conditions = $token['conditions']; + $lastCondition = end($conditions); + + // Skip functions that are not a method part of a class, anon class or trait. + if (isset($this->validOOScopes[$lastCondition]) === false) { + return; + } + // Get function name. $methodName = $phpcsFile->getDeclarationName($stackPtr); diff --git a/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.1.inc b/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.1.inc index b23dc7c1d0..8cdd04d326 100644 --- a/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.1.inc +++ b/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.1.inc @@ -78,10 +78,26 @@ class Bar { // This should not be flagged as non-ASCII chars have changed case, making this a different method name. return parent::DIFFERENTcaseDifferentNonAnsiiCharÁctÊrs(); } + + public function nestedFunctionShouldBailEarly() { + function nestedFunctionShouldBailEarly() { + // Invalid code needed to ensure an error is NOT triggered and the sniff bails early when handling nested function. + parent::nestedFunctionShouldBailEarly(); + } + } } abstract class AbstractFoo { abstract public function sniffShouldBailEarly(); + + public function uselessMethodInAbstractClass() { + parent::uselessMethodInAbstractClass(); + } + + public function usefulMethodInAbstractClass() { + $a = 1; + parent::usefulMethodInAbstractClass($a); + } } interface InterfaceFoo { @@ -90,4 +106,45 @@ interface InterfaceFoo { trait TraitFoo { abstract public function sniffShouldBailEarly(); + + public function usefulMethodInTrait() { + parent::usefulMethodInTrait(); + + return 1; + } + + public function uselessMethodInTrait() { + return parent::uselessMethodInTrait(); + } +} + +enum EnumFoo { + public function sniffShouldBailEarly() { + // Invalid code needed to ensure an error is NOT triggered and the sniff bails early when handling an enum method. + parent::sniffShouldBailEarly(); + } +} + +function shouldBailEarly() { + // Invalid code needed to ensure an error is NOT triggered and the sniff bails early when handling a regular function. + parent::shouldBailEarly(); +} + +$anon = new class extends ParentClass { + public function uselessOverridingMethod() { + parent::uselessOverridingMethod(); + } + + public function usefulOverridingMethod() { + $a = 10; + parent::usefulOverridingMethod($a); + } +}; + +function foo() { + $anon = new class extends ParentClass { + public function uselessOverridingMethod() { + parent::uselessOverridingMethod(); + } + }; } diff --git a/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.php b/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.php index e3064d789d..2740a40188 100644 --- a/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.php +++ b/src/Standards/Generic/Tests/CodeAnalysis/UselessOverridingMethodUnitTest.php @@ -50,12 +50,16 @@ public function getWarningList($testFile='') switch ($testFile) { case 'UselessOverridingMethodUnitTest.1.inc': return [ - 4 => 1, - 16 => 1, - 38 => 1, - 56 => 1, - 68 => 1, - 72 => 1, + 4 => 1, + 16 => 1, + 38 => 1, + 56 => 1, + 68 => 1, + 72 => 1, + 93 => 1, + 116 => 1, + 134 => 1, + 146 => 1, ]; default: return [];