From 23e551b0e2348b84a54dcd9ced92ee28731bfbc5 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 10 Jan 2025 09:54:03 +0100 Subject: [PATCH 1/3] Squiz/FunctionDeclarationArgumentSpacing: refactor logic for for "spacing after comma" check This refactor has two purposes: * Reduce code duplication. * Prevent introducing even more code duplication when a new error code will be introduced in the next commit. --- ...unctionDeclarationArgumentSpacingSniff.php | 87 +++++++++---------- 1 file changed, 39 insertions(+), 48 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index 2621a43615..0a71060a21 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -370,55 +370,46 @@ public function processBracket($phpcsFile, $openBracket) } if ($checkComma === true) { - if ($param['type_hint_token'] === false) { - $spacesAfter = 0; - if ($tokens[($commaToken + 1)]['code'] === T_WHITESPACE) { - $spacesAfter = $tokens[($commaToken + 1)]['length']; - } + $typeOfNext = 'argument'; + $typeOfNextShort = 'Arg'; + $contentOfNext = $param['name']; + + if ($param['type_hint_token'] !== false) { + $typeOfNext = 'type hint'; + $typeOfNextShort = 'Hint'; + $contentOfNext = $param['type_hint']; + } - if ($spacesAfter === 0) { - $error = 'Expected 1 space between comma and argument "%s"; 0 found'; - $data = [$param['name']]; - $fix = $phpcsFile->addFixableError($error, $commaToken, 'NoSpaceBeforeArg', $data); - if ($fix === true) { - $phpcsFile->fixer->addContent($commaToken, ' '); - } - } else if ($spacesAfter !== 1) { - $error = 'Expected 1 space between comma and argument "%s"; %s found'; - $data = [ - $param['name'], - $spacesAfter, - ]; - - $fix = $phpcsFile->addFixableError($error, $commaToken, 'SpacingBeforeArg', $data); - if ($fix === true) { - $phpcsFile->fixer->replaceToken(($commaToken + 1), ' '); - } - }//end if - } else { - $hint = $param['type_hint']; - - if ($tokens[($commaToken + 1)]['code'] !== T_WHITESPACE) { - $error = 'Expected 1 space between comma and type hint "%s"; 0 found'; - $data = [$hint]; - $fix = $phpcsFile->addFixableError($error, $commaToken, 'NoSpaceBeforeHint', $data); - if ($fix === true) { - $phpcsFile->fixer->addContent($commaToken, ' '); - } - } else { - $gap = $tokens[($commaToken + 1)]['length']; - if ($gap !== 1) { - $error = 'Expected 1 space between comma and type hint "%s"; %s found'; - $data = [ - $hint, - $gap, - ]; - $fix = $phpcsFile->addFixableError($error, $commaToken, 'SpacingBeforeHint', $data); - if ($fix === true) { - $phpcsFile->fixer->replaceToken(($commaToken + 1), ' '); - } - } - }//end if + $spacesAfter = 0; + if ($tokens[($commaToken + 1)]['code'] === T_WHITESPACE) { + $spacesAfter = $tokens[($commaToken + 1)]['length']; + } + + if ($spacesAfter === 0) { + $error = 'Expected 1 space between comma and %s "%s"; 0 found'; + $errorCode = 'NoSpaceBefore'.$typeOfNextShort; + $data = [ + $typeOfNext, + $contentOfNext, + ]; + + $fix = $phpcsFile->addFixableError($error, $commaToken, $errorCode, $data); + if ($fix === true) { + $phpcsFile->fixer->addContent($commaToken, ' '); + } + } else if ($spacesAfter !== 1) { + $error = 'Expected 1 space between comma and %s "%s"; %s found'; + $errorCode = 'SpacingBefore'.$typeOfNextShort; + $data = [ + $typeOfNext, + $contentOfNext, + $spacesAfter, + ]; + + $fix = $phpcsFile->addFixableError($error, $commaToken, $errorCode, $data); + if ($fix === true) { + $phpcsFile->fixer->replaceToken(($commaToken + 1), ' '); + } }//end if }//end if }//end if From 68c96b5e07de808820cd48dcdb9839fa5d99b6e4 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 10 Jan 2025 10:29:31 +0100 Subject: [PATCH 2/3] Squiz/FunctionDeclarationArgumentSpacing: special case "spacing after comma" vs constructor property promotion While incorrect spacing after a comma for constructor property promotion parameters would already be flagged and fixed by the sniff, the error message and code were incorrect/unclear. Given the following test code: ```php class PropertyPromotionSpacingAfterComma { public function __construct(private string|int $propA, protected bool $correctSpace, public MyClass $tooMuchSpace, readonly string $noSpace) {} } ``` Previously the following would be reported: ``` 198 | ERROR | [x] Expected 1 space between comma and type hint "MyClass"; 2 found | | (Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforeHint) 198 | ERROR | [x] Expected 1 space between comma and type hint "string"; 0 found | | (Squiz.Functions.FunctionDeclarationArgumentSpacing.NoSpaceBeforeHint) ``` Take note of the "type hint" in the message and the "Hint" suffix for the error codes. With the fix from this commit in place, this will now be reported as follows: ``` 198 | ERROR | [x] Expected 1 space between comma and property modifier "public"; 2 found | | (Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforePropertyModifier) 198 | ERROR | [x] Expected 1 space between comma and property modifier "readonly"; 0 found | | (Squiz.Functions.FunctionDeclarationArgumentSpacing.NoSpaceBeforePropertyModifier) ``` Includes tests. --- .../Functions/FunctionDeclarationArgumentSpacingSniff.php | 7 ++++++- .../FunctionDeclarationArgumentSpacingUnitTest.1.inc | 4 ++++ .../FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed | 4 ++++ .../FunctionDeclarationArgumentSpacingUnitTest.php | 1 + 4 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index 0a71060a21..19e76fa4f0 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -374,7 +374,12 @@ public function processBracket($phpcsFile, $openBracket) $typeOfNextShort = 'Arg'; $contentOfNext = $param['name']; - if ($param['type_hint_token'] !== false) { + if (isset($param['property_visibility']) === true) { + $typeOfNext = 'property modifier'; + $typeOfNextShort = 'PropertyModifier'; + $modifier = $phpcsFile->findNext(Tokens::$emptyTokens, ($commaToken + 1), $param['token'], true); + $contentOfNext = $tokens[$modifier]['content']; + } else if ($param['type_hint_token'] !== false) { $typeOfNext = 'type hint'; $typeOfNextShort = 'Hint'; $contentOfNext = $param['type_hint']; diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc index 764e47331d..c7f6c27c6b 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc @@ -195,3 +195,7 @@ function newlineBeforeCommaFixerRespectsComments( , $paramC=30 , string $paramC='foo' ) {} + +class PropertyPromotionSpacingAfterComma { + public function __construct(private string|int $propA, protected bool $correctSpace, public MyClass $tooMuchSpace,readonly string $noSpace) {} +} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed index 1de259a75d..57d25a4783 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed @@ -171,3 +171,7 @@ function newlineBeforeCommaFixerRespectsComments( $paramC=30, string $paramC='foo' ) {} + +class PropertyPromotionSpacingAfterComma { + public function __construct(private string|int $propA, protected bool $correctSpace, public MyClass $tooMuchSpace, readonly string $noSpace) {} +} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index 79854ca9b1..205639c3da 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -89,6 +89,7 @@ public function getErrorList($testFile='') 193 => 1, 195 => 1, 196 => 1, + 200 => 2, ]; default: From 62c11006882436634c868f21e29c4e922f7c2d33 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 10 Jan 2025 10:58:28 +0100 Subject: [PATCH 3/3] Squiz/FunctionDeclarationArgumentSpacing: handle modifiers for constructor property promotion The spacing after visibility/`readonly` modifiers for constructor property promotion were so far not checked by this sniff. While the `Squiz.WhiteSpace.ScopeKeywordSpacing` sniff will already handle this, that sniff may not be in use in all standards using this sniff. As things were, this sniff was just no longer feature complete for the task this sniff is supposed to handle: spacing of function declaration arguments. This commit adds handling the spacing after modifiers used for constructor property promotion to this sniff. The spacing requirements are aligned with the spacing expectations of the `Squiz.WhiteSpace.ScopeKeywordSpacing` sniff, so the sniffs should not conflict with each other. Additionally, the new checks in this sniff have dedicated error codes, which means that - if there would be a conflict anywhere - the modifier spacing checks within this sniff can easily be turned off. Includes tests. --- ...unctionDeclarationArgumentSpacingSniff.php | 65 +++++++++++++++++++ ...onDeclarationArgumentSpacingUnitTest.1.inc | 11 ++++ ...arationArgumentSpacingUnitTest.1.inc.fixed | 9 +++ ...tionDeclarationArgumentSpacingUnitTest.php | 5 ++ 4 files changed, 90 insertions(+) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index 19e76fa4f0..26fd541f06 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -306,6 +306,71 @@ public function processBracket($phpcsFile, $openBracket) }//end if }//end if + if (isset($param['visibility_token']) === true && $param['visibility_token'] !== false) { + $visibilityToken = $param['visibility_token']; + $afterVisibilityToken = $phpcsFile->findNext(T_WHITESPACE, ($visibilityToken + 1), $param['token'], true); + + $spacesAfter = 0; + if ($afterVisibilityToken !== false + && $tokens[$visibilityToken]['line'] !== $tokens[$afterVisibilityToken]['line'] + ) { + $spacesAfter = 'newline'; + } else if ($tokens[($visibilityToken + 1)]['code'] === T_WHITESPACE) { + $spacesAfter = $tokens[($visibilityToken + 1)]['length']; + } + + if ($spacesAfter !== 1) { + $error = 'Expected 1 space after visibility modifier "%s"; %s found'; + $data = [ + $tokens[$visibilityToken]['content'], + $spacesAfter, + ]; + + $fix = $phpcsFile->addFixableError($error, $visibilityToken, 'SpacingAfterVisbility', $data); + if ($fix === true) { + $phpcsFile->fixer->beginChangeset(); + $phpcsFile->fixer->addContent($visibilityToken, ' '); + + for ($i = ($visibilityToken + 1); $tokens[$i]['code'] === T_WHITESPACE; $i++) { + $phpcsFile->fixer->replaceToken($i, ''); + } + + $phpcsFile->fixer->endChangeset(); + } + }//end if + }//end if + + if (isset($param['readonly_token']) === true) { + $readonlyToken = $param['readonly_token']; + $afterReadonlyToken = $phpcsFile->findNext(T_WHITESPACE, ($readonlyToken + 1), $param['token'], true); + + $spacesAfter = 0; + if ($afterReadonlyToken !== false + && $tokens[$readonlyToken]['line'] !== $tokens[$afterReadonlyToken]['line'] + ) { + $spacesAfter = 'newline'; + } else if ($tokens[($readonlyToken + 1)]['code'] === T_WHITESPACE) { + $spacesAfter = $tokens[($readonlyToken + 1)]['length']; + } + + if ($spacesAfter !== 1) { + $error = 'Expected 1 space after readonly modifier; %s found'; + $data = [$spacesAfter]; + + $fix = $phpcsFile->addFixableError($error, $readonlyToken, 'SpacingAfterReadonly', $data); + if ($fix === true) { + $phpcsFile->fixer->beginChangeset(); + $phpcsFile->fixer->addContent($readonlyToken, ' '); + + for ($i = ($readonlyToken + 1); $tokens[$i]['code'] === T_WHITESPACE; $i++) { + $phpcsFile->fixer->replaceToken($i, ''); + } + + $phpcsFile->fixer->endChangeset(); + } + }//end if + }//end if + $commaToken = false; if ($paramNumber > 0 && $params[($paramNumber - 1)]['comma_token'] !== false) { $commaToken = $params[($paramNumber - 1)]['comma_token']; diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc index c7f6c27c6b..f80a567557 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc @@ -199,3 +199,14 @@ function newlineBeforeCommaFixerRespectsComments( class PropertyPromotionSpacingAfterComma { public function __construct(private string|int $propA, protected bool $correctSpace, public MyClass $tooMuchSpace,readonly string $noSpace) {} } + +class PropertyPromotionSpacingAfterModifier { + public function __construct( + private$noSpace, + public MyClass $tooMuchSpace, + protected readonly string $tooMuchSpaceX2, + readonly + public + string $tooMuchSpaceNewLines, + ) {} +} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed index 57d25a4783..c82e3fb016 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed @@ -175,3 +175,12 @@ function newlineBeforeCommaFixerRespectsComments( class PropertyPromotionSpacingAfterComma { public function __construct(private string|int $propA, protected bool $correctSpace, public MyClass $tooMuchSpace, readonly string $noSpace) {} } + +class PropertyPromotionSpacingAfterModifier { + public function __construct( + private $noSpace, + public MyClass $tooMuchSpace, + protected readonly string $tooMuchSpaceX2, + readonly public string $tooMuchSpaceNewLines, + ) {} +} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index 205639c3da..9dcade35a9 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -90,6 +90,11 @@ public function getErrorList($testFile='') 195 => 1, 196 => 1, 200 => 2, + 205 => 1, + 206 => 1, + 207 => 2, + 208 => 1, + 209 => 1, ]; default: