From eced4411654852ff1a04edda72a0ecc653cfe237 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 9 Sep 2022 16:57:42 +0200 Subject: [PATCH 1/3] PEAR/FunctionDeclaration: examine arrow function declarations PHP 7.4 arrow functions were not yet being taken into account for this sniff. Fixed now. Includes unit tests. --- .../Functions/FunctionDeclarationSniff.php | 53 ++++++++++++++----- .../Functions/FunctionDeclarationUnitTest.inc | 21 ++++++++ .../FunctionDeclarationUnitTest.inc.fixed | 21 ++++++++ .../Functions/FunctionDeclarationUnitTest.php | 5 ++ 4 files changed, 88 insertions(+), 12 deletions(-) diff --git a/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php b/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php index 14479e2c0a..89f1938c3f 100644 --- a/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php +++ b/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php @@ -46,6 +46,7 @@ public function register() return [ T_FUNCTION, T_CLOSURE, + T_FN, ]; }//end register() @@ -75,7 +76,9 @@ public function process(File $phpcsFile, $stackPtr) $openBracket = $tokens[$stackPtr]['parenthesis_opener']; $closeBracket = $tokens[$stackPtr]['parenthesis_closer']; - if (strtolower($tokens[$stackPtr]['content']) === 'function') { + if (strtolower($tokens[$stackPtr]['content']) === 'function' + || strtolower($tokens[$stackPtr]['content']) === 'fn' + ) { // Must be one space after the FUNCTION keyword. if ($tokens[($stackPtr + 1)]['content'] === $phpcsFile->eolChar) { $spaces = 'newline'; @@ -86,9 +89,13 @@ public function process(File $phpcsFile, $stackPtr) } if ($spaces !== 1) { - $error = 'Expected 1 space after FUNCTION keyword; %s found'; - $data = [$spaces]; - $fix = $phpcsFile->addFixableError($error, $stackPtr, 'SpaceAfterFunction', $data); + $error = 'Expected 1 space after %s keyword; %s found'; + $data = [ + strtoupper($tokens[$stackPtr]['content']), + $spaces, + ]; + + $fix = $phpcsFile->addFixableError($error, $stackPtr, 'SpaceAfterFunction', $data); if ($fix === true) { if ($spaces === 0) { $phpcsFile->fixer->addContent($stackPtr, ' '); @@ -99,9 +106,9 @@ public function process(File $phpcsFile, $stackPtr) } }//end if - // Must be no space before the opening parenthesis. For closures, this is - // enforced by the previous check because there is no content between the keywords - // and the opening parenthesis. + // Must be no space before the opening parenthesis. For closures and arrow functions, + // this is enforced by the previous check because there is no content between + // the keywords and the opening parenthesis. // Unfinished closures are tokenized as T_FUNCTION however, and can be excluded // by checking for the scope_opener. if ($tokens[$stackPtr]['code'] === T_FUNCTION @@ -257,6 +264,10 @@ public function isMultiLineDeclaration($phpcsFile, $stackPtr, $openBracket, $tok */ public function processSingleLineDeclaration($phpcsFile, $stackPtr, $tokens) { + if ($tokens[$stackPtr]['code'] === T_FN) { + return; + } + if ($tokens[$stackPtr]['code'] === T_CLOSURE) { $sniff = new OpeningFunctionBraceKernighanRitchieSniff(); } else { @@ -300,12 +311,20 @@ public function processMultiLineDeclaration($phpcsFile, $stackPtr, $tokens) // The opening brace needs to be one space away from the closing parenthesis. $opener = $tokens[$stackPtr]['scope_opener']; if ($tokens[$opener]['line'] !== $tokens[$closeBracket]['line']) { - $error = 'The closing parenthesis and the opening brace of a multi-line function declaration must be on the same line'; - $fix = $phpcsFile->addFixableError($error, $opener, 'NewlineBeforeOpenBrace'); + $error = 'The closing parenthesis and the %s of a multi-line function declaration must be on the same line'; + $code = 'NewlineBeforeOpenBrace'; + $data = ['opening brace']; + + if ($tokens[$stackPtr]['code'] === T_FN) { + $code = 'NewlineBeforeArrow'; + $data = ['arrow']; + } + + $fix = $phpcsFile->addFixableError($error, $opener, $code, $data); if ($fix === true) { $prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($opener - 1), $closeBracket, true); $phpcsFile->fixer->beginChangeset(); - $phpcsFile->fixer->addContent($prev, ' {'); + $phpcsFile->fixer->addContent($prev, ' '.$tokens[$opener]['content']); // If the opener is on a line by itself, removing it will create // an empty line, so just remove the entire line instead. @@ -340,8 +359,18 @@ public function processMultiLineDeclaration($phpcsFile, $stackPtr, $tokens) } if ($length !== 1) { - $error = 'There must be a single space between the closing parenthesis and the opening brace of a multi-line function declaration; found %s spaces'; - $fix = $phpcsFile->addFixableError($error, ($opener - 1), 'SpaceBeforeOpenBrace', [$length]); + $error = 'There must be a single space between the closing parenthesis and the %s of a multi-line function declaration; found %s spaces'; + $code = 'SpaceBeforeOpenBrace'; + $data = ['opening brace']; + + if ($tokens[$stackPtr]['code'] === T_FN) { + $code = 'SpaceBeforeArrow'; + $data = ['arrow']; + } + + $data[] = $length; + + $fix = $phpcsFile->addFixableError($error, ($opener - 1), $code, $data); if ($fix === true) { if ($length === 0) { $phpcsFile->fixer->addContentBefore($opener, ' '); diff --git a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc index fb7cb52f4d..7b18bd81c5 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc @@ -465,3 +465,24 @@ new ExceptionMessage(), ) { } } + +$arrowNoArgs = fn () => $retrievedfromscope; + +$arrowSingleLineArgs = fn (Type $param1, int $param2, string $param3): \ReturnType => $retrievedfromscope; + +$arrowMultiLineArgs = fn ( + $longVar1, + $longerVar2, + &...$muchLongerVar3 +) => $longVar1; + +$arrowNoArgs = fn( ) + => $retrievedfromscope; + +$arrowSingleLineArgs = fn( Type $param1 , int $param2, string $param3 + ) : \ReturnType => $retrievedfromscope; + +$arrowMultiLineArgs = fn ( + $longVar1, $longerVar2, + + & ... $muchLongerVar3) => $longVar1; diff --git a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed index f82b5665a5..f00173db5c 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed @@ -463,3 +463,24 @@ new ExceptionMessage(), ) { } } + +$arrowNoArgs = fn () => $retrievedfromscope; + +$arrowSingleLineArgs = fn (Type $param1, int $param2, string $param3): \ReturnType => $retrievedfromscope; + +$arrowMultiLineArgs = fn ( + $longVar1, + $longerVar2, + &...$muchLongerVar3 +) => $longVar1; + +$arrowNoArgs = fn ( ) + => $retrievedfromscope; + +$arrowSingleLineArgs = fn ( Type $param1 , int $param2, string $param3 +) : \ReturnType => $retrievedfromscope; + +$arrowMultiLineArgs = fn ( + $longVar1, $longerVar2, + & ... $muchLongerVar3 +) => $longVar1; diff --git a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php index 01ab3e8482..8bbfa9365a 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php @@ -99,6 +99,11 @@ public function getErrorList($testFile='FunctionDeclarationUnitTest.inc') 371 => 1, 402 => 1, 406 => 1, + 479 => 1, + 482 => 1, + 483 => 2, + 487 => 1, + 488 => 2, ]; } else { $errors = [ From db548df885102b303ce841d9aae1d05da3bba162 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 11 Sep 2022 04:21:29 +0200 Subject: [PATCH 2/3] Squiz/MultiLineFunctionDeclaration: add tests for arrow function declarations The Squiz sniff inherits the change from the `PEAR.Functions.FunctionDeclaration` sniff. This commit adds tests to document the behaviour and safeguard the inherited change. --- .../MultiLineFunctionDeclarationUnitTest.inc | 21 +++++++++++++++++++ ...iLineFunctionDeclarationUnitTest.inc.fixed | 21 +++++++++++++++++++ .../MultiLineFunctionDeclarationUnitTest.php | 6 ++++++ 3 files changed, 48 insertions(+) diff --git a/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.inc b/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.inc index 811c56ec14..c3af6ce55f 100644 --- a/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.inc +++ b/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.inc @@ -302,3 +302,24 @@ new ExceptionMessage(), ) { } } + +$arrowNoArgs = fn () => $retrievedfromscope; + +$arrowSingleLineArgs = fn (Type $param1, int $param2, string $param3): \ReturnType => $retrievedfromscope; + +$arrowMultiLineArgs = fn ( + $longVar1, + $longerVar2, + &...$muchLongerVar3 +) => $longVar1; + +$arrowNoArgs = fn( ) + => $retrievedfromscope; + +$arrowSingleLineArgs = fn( Type $param1 , int $param2, string $param3 + ) : \ReturnType => $retrievedfromscope; + +$arrowMultiLineArgs = fn ( + $longVar1, $longerVar2, + + & ... $muchLongerVar3) => $longVar1; diff --git a/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.inc.fixed index c38e3ecc0a..ce0421644c 100644 --- a/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.inc.fixed @@ -314,3 +314,24 @@ new ExceptionMessage(), ) { } } + +$arrowNoArgs = fn () => $retrievedfromscope; + +$arrowSingleLineArgs = fn (Type $param1, int $param2, string $param3): \ReturnType => $retrievedfromscope; + +$arrowMultiLineArgs = fn ( + $longVar1, + $longerVar2, + &...$muchLongerVar3 +) => $longVar1; + +$arrowNoArgs = fn ( ) + => $retrievedfromscope; + +$arrowSingleLineArgs = fn ( Type $param1 , int $param2, string $param3) : \ReturnType => $retrievedfromscope; + +$arrowMultiLineArgs = fn ( + $longVar1, + $longerVar2, + & ... $muchLongerVar3 +) => $longVar1; diff --git a/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.php b/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.php index 5208ad0cdb..4fb74ea194 100644 --- a/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/MultiLineFunctionDeclarationUnitTest.php @@ -70,6 +70,12 @@ public function getErrorList($testFile='MultiLineFunctionDeclarationUnitTest.inc 252 => 1, 253 => 1, 254 => 1, + 316 => 1, + 319 => 1, + 320 => 1, + 323 => 1, + 324 => 1, + 325 => 2, ]; } else { $errors = [ From d479811c77196a0862d01ae9b285895094cc2bcb Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 11 Sep 2022 03:41:46 +0200 Subject: [PATCH 3/3] PEAR/FunctionDeclaration: prevent fixer conflict If a return type declaration was not confined to one line, the sniff could have a fixer conflict with itself. The fixer would also potentially remove a close curly on the same line, causing parse errors. Fixed now. The diff will be most straight forward to review while ignoring whitespace changes. Includes unit tests. --- .../Functions/FunctionDeclarationSniff.php | 111 ++++++++++-------- .../Functions/FunctionDeclarationUnitTest.inc | 14 +++ .../FunctionDeclarationUnitTest.inc.fixed | 13 ++ .../Functions/FunctionDeclarationUnitTest.php | 2 + 4 files changed, 88 insertions(+), 52 deletions(-) diff --git a/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php b/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php index 89f1938c3f..6f0380dd96 100644 --- a/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php +++ b/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php @@ -308,7 +308,9 @@ public function processMultiLineDeclaration($phpcsFile, $stackPtr, $tokens) return; } - // The opening brace needs to be one space away from the closing parenthesis. + // The opening brace needs to be on the same line as the closing parenthesis. + // There should only be one space between the closing parenthesis - or the end of the + // return type - and the opening brace. $opener = $tokens[$stackPtr]['scope_opener']; if ($tokens[$opener]['line'] !== $tokens[$closeBracket]['line']) { $error = 'The closing parenthesis and the %s of a multi-line function declaration must be on the same line'; @@ -320,67 +322,72 @@ public function processMultiLineDeclaration($phpcsFile, $stackPtr, $tokens) $data = ['arrow']; } - $fix = $phpcsFile->addFixableError($error, $opener, $code, $data); - if ($fix === true) { - $prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($opener - 1), $closeBracket, true); - $phpcsFile->fixer->beginChangeset(); - $phpcsFile->fixer->addContent($prev, ' '.$tokens[$opener]['content']); - - // If the opener is on a line by itself, removing it will create - // an empty line, so just remove the entire line instead. - $prev = $phpcsFile->findPrevious(T_WHITESPACE, ($opener - 1), $closeBracket, true); - $next = $phpcsFile->findNext(T_WHITESPACE, ($opener + 1), null, true); - - if ($tokens[$prev]['line'] < $tokens[$opener]['line'] - && $tokens[$next]['line'] > $tokens[$opener]['line'] - ) { - // Clear the whole line. - for ($i = ($prev + 1); $i < $next; $i++) { - if ($tokens[$i]['line'] === $tokens[$opener]['line']) { - $phpcsFile->fixer->replaceToken($i, ''); + $prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($opener - 1), $closeBracket, true); + if ($tokens[$prev]['line'] === $tokens[$opener]['line']) { + // End of the return type is not on the same line as the close parenthesis. + $phpcsFile->addError($error, $opener, $code, $data); + } else { + $fix = $phpcsFile->addFixableError($error, $opener, $code, $data); + if ($fix === true) { + $phpcsFile->fixer->beginChangeset(); + $phpcsFile->fixer->addContent($prev, ' '.$tokens[$opener]['content']); + + // If the opener is on a line by itself, removing it will create + // an empty line, so just remove the entire line instead. + $prev = $phpcsFile->findPrevious(T_WHITESPACE, ($opener - 1), $closeBracket, true); + $next = $phpcsFile->findNext(T_WHITESPACE, ($opener + 1), null, true); + + if ($tokens[$prev]['line'] < $tokens[$opener]['line'] + && $tokens[$next]['line'] > $tokens[$opener]['line'] + ) { + // Clear the whole line. + for ($i = ($prev + 1); $i < $next; $i++) { + if ($tokens[$i]['line'] === $tokens[$opener]['line']) { + $phpcsFile->fixer->replaceToken($i, ''); + } + } + } else { + // Just remove the opener. + $phpcsFile->fixer->replaceToken($opener, ''); + if ($tokens[$next]['line'] === $tokens[$opener]['line']) { + $phpcsFile->fixer->replaceToken(($opener + 1), ''); } } - } else { - // Just remove the opener. - $phpcsFile->fixer->replaceToken($opener, ''); - if ($tokens[$next]['line'] === $tokens[$opener]['line']) { - $phpcsFile->fixer->replaceToken(($opener + 1), ''); - } - } - $phpcsFile->fixer->endChangeset(); + $phpcsFile->fixer->endChangeset(); + }//end if + + return; }//end if + }//end if + + $prev = $tokens[($opener - 1)]; + if ($prev['code'] !== T_WHITESPACE) { + $length = 0; } else { - $prev = $tokens[($opener - 1)]; - if ($prev['code'] !== T_WHITESPACE) { - $length = 0; - } else { - $length = strlen($prev['content']); - } + $length = strlen($prev['content']); + } - if ($length !== 1) { - $error = 'There must be a single space between the closing parenthesis and the %s of a multi-line function declaration; found %s spaces'; - $code = 'SpaceBeforeOpenBrace'; - $data = ['opening brace']; + if ($length !== 1) { + $error = 'There must be a single space before the %s of a multi-line function declaration; found %s spaces'; + $code = 'SpaceBeforeOpenBrace'; + $data = ['opening brace']; - if ($tokens[$stackPtr]['code'] === T_FN) { - $code = 'SpaceBeforeArrow'; - $data = ['arrow']; - } + if ($tokens[$stackPtr]['code'] === T_FN) { + $code = 'SpaceBeforeArrow'; + $data = ['arrow']; + } - $data[] = $length; + $data[] = $length; - $fix = $phpcsFile->addFixableError($error, ($opener - 1), $code, $data); - if ($fix === true) { - if ($length === 0) { - $phpcsFile->fixer->addContentBefore($opener, ' '); - } else { - $phpcsFile->fixer->replaceToken(($opener - 1), ' '); - } + $fix = $phpcsFile->addFixableError($error, ($opener - 1), $code, $data); + if ($fix === true) { + if ($length === 0) { + $phpcsFile->fixer->addContentBefore($opener, ' '); + } else { + $phpcsFile->fixer->replaceToken(($opener - 1), ' '); } - - return; - }//end if + } }//end if }//end processMultiLineDeclaration() diff --git a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc index 7b18bd81c5..c726e9acf6 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc @@ -486,3 +486,17 @@ $arrowMultiLineArgs = fn ( $longVar1, $longerVar2, & ... $muchLongerVar3) => $longVar1; + +// Prevent fixer conflict with itself. +function foo( + $param1, +) +: \SomeClass + { + } + +function foo( + $param1, + $param2 +) : // comment. + \Package\Sub\SomeClass {} diff --git a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed index f00173db5c..21e4b5f83e 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed @@ -484,3 +484,16 @@ $arrowMultiLineArgs = fn ( $longVar1, $longerVar2, & ... $muchLongerVar3 ) => $longVar1; + +// Prevent fixer conflict with itself. +function foo( + $param1, +) +: \SomeClass { + } + +function foo( + $param1, + $param2 +) : // comment. + \Package\Sub\SomeClass {} diff --git a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php index 8bbfa9365a..c42a59a3db 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php @@ -104,6 +104,8 @@ public function getErrorList($testFile='FunctionDeclarationUnitTest.inc') 483 => 2, 487 => 1, 488 => 2, + 495 => 1, + 502 => 2, ]; } else { $errors = [