From 7ae6441eb9baac30e96cd9a5e6188ca0eb8d0bd0 Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Thu, 11 Jan 2024 11:03:25 -0300 Subject: [PATCH 1/5] Generic/UnusedFunctionParameter: rename test case file To add more tests with syntax errors in separate files in subsequent commits. --- ... => UnusedFunctionParameterUnitTest.1.inc} | 0 .../UnusedFunctionParameterUnitTest.php | 44 +++++++++++-------- 2 files changed, 26 insertions(+), 18 deletions(-) rename src/Standards/Generic/Tests/CodeAnalysis/{UnusedFunctionParameterUnitTest.inc => UnusedFunctionParameterUnitTest.1.inc} (100%) diff --git a/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.inc b/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.1.inc similarity index 100% rename from src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.inc rename to src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.1.inc diff --git a/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.php b/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.php index 980dad8dac..a5c1a8e1c0 100644 --- a/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.php +++ b/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.php @@ -41,27 +41,35 @@ public function getErrorList() * The key of the array should represent the line number and the value * should represent the number of warnings that should occur on that line. * + * @param string $testFile The name of the file being tested. + * * @return array */ - public function getWarningList() + public function getWarningList($testFile='') { - return [ - 3 => 1, - 7 => 1, - 78 => 1, - 94 => 1, - 100 => 1, - 106 => 1, - 117 => 1, - 121 => 2, - 125 => 2, - 163 => 1, - 172 => 1, - 228 => 2, - 232 => 2, - 244 => 2, - 248 => 2, - ]; + switch ($testFile) { + case 'UnusedFunctionParameterUnitTest.1.inc': + return [ + 3 => 1, + 7 => 1, + 78 => 1, + 94 => 1, + 100 => 1, + 106 => 1, + 117 => 1, + 121 => 2, + 125 => 2, + 163 => 1, + 172 => 1, + 228 => 2, + 232 => 2, + 244 => 2, + 248 => 2, + ]; + + default: + return []; + }//end switch }//end getWarningList() From 2573b67959f54819d5551397d2589ead40111c72 Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Thu, 11 Jan 2024 11:13:12 -0300 Subject: [PATCH 2/5] Generic/UnusedFunctionParameter: remove unused variable I found this while working on improving code coverage for this sniff. The value of this variable is defined again on line 112 and then only used right below it on line 113 (https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/84acf4e56f110db8e75cb9a575c5727df637643c/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php#L112-L113) so it should be safe to remove it. --- .../Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php b/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php index 66fe781ac7..698ea3723a 100644 --- a/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php +++ b/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php @@ -94,7 +94,6 @@ public function process(File $phpcsFile, $stackPtr) $errorCode = 'Found'; $implements = false; - $extends = false; if ($token['code'] === T_FUNCTION) { $classPtr = $phpcsFile->getCondition($stackPtr, T_CLASS); From 0219a955373fc1d8d5eba2ae016f87c05ffdb1fc Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Thu, 11 Jan 2024 12:39:09 -0300 Subject: [PATCH 3/5] Generic/UnusedFunctionParameter: rename two variables This commit renames `$tmp` variables using more descriptive names to make it easier to understand the code. --- .../UnusedFunctionParameterSniff.php | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php b/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php index 698ea3723a..6b6e2c0a70 100644 --- a/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php +++ b/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php @@ -173,18 +173,27 @@ public function process(File $phpcsFile, $stackPtr) // A return statement as the first content indicates an interface method. if ($code === T_RETURN) { - $tmp = $phpcsFile->findNext(Tokens::$emptyTokens, ($next + 1), null, true); - if ($tmp === false && $implements !== false) { + $firstNonEmptyTokenAfterReturn = $phpcsFile->findNext(Tokens::$emptyTokens, ($next + 1), null, true); + if ($firstNonEmptyTokenAfterReturn === false && $implements !== false) { return; } // There is a return. - if ($tokens[$tmp]['code'] === T_SEMICOLON && $implements !== false) { + if ($tokens[$firstNonEmptyTokenAfterReturn]['code'] === T_SEMICOLON && $implements !== false) { return; } - $tmp = $phpcsFile->findNext(Tokens::$emptyTokens, ($tmp + 1), null, true); - if ($tmp !== false && $tokens[$tmp]['code'] === T_SEMICOLON && $implements !== false) { + $secondNonEmptyTokenAfterReturn = $phpcsFile->findNext( + Tokens::$emptyTokens, + ($firstNonEmptyTokenAfterReturn + 1), + null, + true + ); + + if ($secondNonEmptyTokenAfterReturn !== false + && $tokens[$secondNonEmptyTokenAfterReturn]['code'] === T_SEMICOLON + && $implements !== false + ) { // There is a return . return; } From f8dd4aa6b5946085a5c30a1aecd486bc77b5f389 Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Thu, 11 Jan 2024 12:45:16 -0300 Subject: [PATCH 4/5] Generic/UnusedFunctionParameter: improve code coverage --- .../UnusedFunctionParameterUnitTest.1.inc | 23 +++++++++++++++++++ .../UnusedFunctionParameterUnitTest.2.inc | 5 ++++ .../UnusedFunctionParameterUnitTest.3.inc | 5 ++++ .../UnusedFunctionParameterUnitTest.php | 1 + 4 files changed, 34 insertions(+) create mode 100644 src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.2.inc create mode 100644 src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.3.inc diff --git a/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.1.inc b/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.1.inc index 154f03157b..f13253d301 100644 --- a/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.1.inc +++ b/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.1.inc @@ -249,3 +249,26 @@ class MagicMethodsWithParamsNotDictatedByPHPInChildClass extends SomeParent{ $this->foo = $foo; } } + +/** + * Methods that throw an exception or return on the first line and are part + * of a class that implements an interface should not trigger the sniff. + */ +class InterfaceMethodNotImplement implements SomeInterface { + public function notImplemented($param) { + throw new Exception('Not implemented.'); + } + + public function notImplemented2($param) { + return 'Not implemented.'; + } +} + +/** + * Should trigger the sniff as this method is not part of an interface. + */ +class MethodThrowsException { + public function throwsException($param) { + throw new Exception(); + } +} diff --git a/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.2.inc b/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.2.inc new file mode 100644 index 0000000000..d39eefe305 --- /dev/null +++ b/src/Standards/Generic/Tests/CodeAnalysis/UnusedFunctionParameterUnitTest.2.inc @@ -0,0 +1,5 @@ + 2, 244 => 2, 248 => 2, + 271 => 1, ]; default: From e8811932409837f9013744403495dd40f3a8e70f Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Thu, 11 Jan 2024 12:51:20 -0300 Subject: [PATCH 5/5] Generic/UnusedFunctionParameter: remove unreacheable if condition At this point in the code, there should always be a non-empty token after the return statement, even if only the scope closer, and thus the if condition will never be true. If there is a syntax error and there is nothing after the return, the code will never reach this point as in the beginning of the method the code checks if `$token['scope_opener']` is set and this only happens when there is also a scope_closer. --- .../Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php b/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php index 6b6e2c0a70..e076b83dce 100644 --- a/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php +++ b/src/Standards/Generic/Sniffs/CodeAnalysis/UnusedFunctionParameterSniff.php @@ -174,11 +174,6 @@ public function process(File $phpcsFile, $stackPtr) // A return statement as the first content indicates an interface method. if ($code === T_RETURN) { $firstNonEmptyTokenAfterReturn = $phpcsFile->findNext(Tokens::$emptyTokens, ($next + 1), null, true); - if ($firstNonEmptyTokenAfterReturn === false && $implements !== false) { - return; - } - - // There is a return. if ($tokens[$firstNonEmptyTokenAfterReturn]['code'] === T_SEMICOLON && $implements !== false) { return; }