From ed6c437526c972e08f222165b14368028ead2321 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 5 Jan 2023 03:40:05 +0100 Subject: [PATCH 1/2] PEAR/FunctionDeclaration: bug fix - prevent fixer from creating a parse error Issue as described in 3736. The fixer would try to remove superfluous whitespace remaining after the move of the opening brace to the previous line, but did not take into account that there may not be any whitespace to removed, i.e. that the `$opener +1` token could be the same as the `$next` token. Fixed now. Includes unit test. Fixes 3736 --- CHANGELOG.md | 2 ++ .../PEAR/Sniffs/Functions/FunctionDeclarationSniff.php | 6 ++++-- .../PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc | 9 +++++++++ .../Functions/FunctionDeclarationUnitTest.inc.fixed | 9 +++++++++ .../PEAR/Tests/Functions/FunctionDeclarationUnitTest.php | 1 + 5 files changed, 25 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e14018ba8e..802d42c905 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -109,6 +109,8 @@ The file documents changes to the PHP_CodeSniffer project. - Thanks to Juliette Reinders Folmer (@jrfnl) for the patch - Fixed bug #3728 : PHP 8.2 | PSR1/SideEffects: allow for readonly classes - Thanks to Juliette Reinders Folmer (@jrfnl) for the patch +- Fixed bug #3736 : PEAR/FunctionDeclaration: prevent fixer removing the close brace (and creating a parse error) when there is no space between the open brace and close brace of a function + - Thanks to Juliette Reinders Folmer (@jrfnl) for the patch - Fixed bug #3770 : Squiz/NonExecutableCode: prevent false positives for switching between PHP and HTML - Thanks to Dan Wallis (@fredden) for the patch - Fixed bug #3773 : Tokenizer/PHP: tokenization of the readonly keyword when used in combination with PHP 8.2 disjunctive normal types diff --git a/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php b/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php index af18a2b82d..58a232107f 100644 --- a/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php +++ b/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php @@ -308,7 +308,7 @@ public function processMultiLineDeclaration($phpcsFile, $stackPtr, $tokens) $phpcsFile->fixer->addContent($prev, ' {'); // If the opener is on a line by itself, removing it will create - // an empty line, so just remove the entire line instead. + // an empty line, so remove the entire line instead. $prev = $phpcsFile->findPrevious(T_WHITESPACE, ($opener - 1), $closeBracket, true); $next = $phpcsFile->findNext(T_WHITESPACE, ($opener + 1), null, true); @@ -324,7 +324,9 @@ public function processMultiLineDeclaration($phpcsFile, $stackPtr, $tokens) } else { // Just remove the opener. $phpcsFile->fixer->replaceToken($opener, ''); - if ($tokens[$next]['line'] === $tokens[$opener]['line']) { + if ($tokens[$next]['line'] === $tokens[$opener]['line'] + && ($opener + 1) !== $next + ) { $phpcsFile->fixer->replaceToken(($opener + 1), ''); } } diff --git a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc index fb7cb52f4d..52beaff772 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc @@ -465,3 +465,12 @@ new ExceptionMessage(), ) { } } + +// Issue #3736 - prevent the fixer creating a parse error by removing the function close brace. +class Test +{ + public function __construct( + protected int $id + ) + {} +} diff --git a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed index f82b5665a5..7d90f9a412 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed @@ -463,3 +463,12 @@ new ExceptionMessage(), ) { } } + +// Issue #3736 - prevent the fixer creating a parse error by removing the function close brace. +class Test +{ + public function __construct( + protected int $id + ) { + } +} diff --git a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php index 93a19d1b0d..871374959c 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php @@ -99,6 +99,7 @@ public function getErrorList($testFile='FunctionDeclarationUnitTest.inc') 371 => 1, 402 => 1, 406 => 1, + 475 => 1, ]; } else { $errors = [ From dcc257f5f547c3dc7012b011da0b6ff893e0eb90 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 5 Jan 2023 03:54:22 +0100 Subject: [PATCH 2/2] 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. --- CHANGELOG.md | 2 + .../Functions/FunctionDeclarationSniff.php | 101 ++++++++++-------- .../Functions/FunctionDeclarationUnitTest.inc | 14 +++ .../FunctionDeclarationUnitTest.inc.fixed | 13 +++ .../Functions/FunctionDeclarationUnitTest.php | 2 + 5 files changed, 86 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 802d42c905..c01e8dd7ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -111,6 +111,8 @@ The file documents changes to the PHP_CodeSniffer project. - Thanks to Juliette Reinders Folmer (@jrfnl) for the patch - Fixed bug #3736 : PEAR/FunctionDeclaration: prevent fixer removing the close brace (and creating a parse error) when there is no space between the open brace and close brace of a function - Thanks to Juliette Reinders Folmer (@jrfnl) for the patch +- Fixed bug #3739 : PEAR/FunctionDeclaration: prevent fixer conflict (and potentially creating a parse error) for unconventionally formatted return types + - Thanks to Juliette Reinders Folmer (@jrfnl) for the patch - Fixed bug #3770 : Squiz/NonExecutableCode: prevent false positives for switching between PHP and HTML - Thanks to Dan Wallis (@fredden) for the patch - Fixed bug #3773 : Tokenizer/PHP: tokenization of the readonly keyword when used in combination with PHP 8.2 disjunctive normal types diff --git a/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php b/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php index 58a232107f..4cc24a2d19 100644 --- a/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php +++ b/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php @@ -297,65 +297,74 @@ 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 opening brace of a multi-line function declaration must be on the same line'; - $fix = $phpcsFile->addFixableError($error, $opener, 'NewlineBeforeOpenBrace'); - if ($fix === true) { - $prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($opener - 1), $closeBracket, true); - $phpcsFile->fixer->beginChangeset(); - $phpcsFile->fixer->addContent($prev, ' {'); - - // If the opener is on a line by itself, removing it will create - // an empty line, so remove the entire line instead. - $prev = $phpcsFile->findPrevious(T_WHITESPACE, ($opener - 1), $closeBracket, true); - $next = $phpcsFile->findNext(T_WHITESPACE, ($opener + 1), null, true); + $code = 'NewlineBeforeOpenBrace'; - 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'] - && ($opener + 1) !== $next - ) { - $phpcsFile->fixer->replaceToken(($opener + 1), ''); - } - } - - $phpcsFile->fixer->endChangeset(); - }//end if - } else { - $prev = $tokens[($opener - 1)]; - if ($prev['code'] !== T_WHITESPACE) { - $length = 0; + $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); } else { - $length = strlen($prev['content']); - } - - 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]); + $fix = $phpcsFile->addFixableError($error, $opener, $code); if ($fix === true) { - if ($length === 0) { - $phpcsFile->fixer->addContentBefore($opener, ' '); + $phpcsFile->fixer->beginChangeset(); + $phpcsFile->fixer->addContent($prev, ' {'); + + // If the opener is on a line by itself, removing it will create + // an empty line, so 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 { - $phpcsFile->fixer->replaceToken(($opener - 1), ' '); + // Just remove the opener. + $phpcsFile->fixer->replaceToken($opener, ''); + if ($tokens[$next]['line'] === $tokens[$opener]['line'] + && ($opener + 1) !== $next + ) { + $phpcsFile->fixer->replaceToken(($opener + 1), ''); + } } - } + + $phpcsFile->fixer->endChangeset(); + }//end if return; }//end if }//end if + $prev = $tokens[($opener - 1)]; + if ($prev['code'] !== T_WHITESPACE) { + $length = 0; + } else { + $length = strlen($prev['content']); + } + + if ($length !== 1) { + $error = 'There must be a single space between the closing parenthesis/return type and the opening brace of a multi-line function declaration; found %s spaces'; + $fix = $phpcsFile->addFixableError($error, ($opener - 1), 'SpaceBeforeOpenBrace', [$length]); + if ($fix === true) { + if ($length === 0) { + $phpcsFile->fixer->addContentBefore($opener, ' '); + } else { + $phpcsFile->fixer->replaceToken(($opener - 1), ' '); + } + } + } + }//end processMultiLineDeclaration() diff --git a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc index 52beaff772..6ba3bd9f6a 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc @@ -474,3 +474,17 @@ class Test ) {} } + +// 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 7d90f9a412..8248ee1dc8 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed @@ -472,3 +472,16 @@ class Test ) { } } + +// 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 871374959c..d12c8c08bc 100644 --- a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php +++ b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php @@ -100,6 +100,8 @@ public function getErrorList($testFile='FunctionDeclarationUnitTest.inc') 402 => 1, 406 => 1, 475 => 1, + 483 => 1, + 490 => 2, ]; } else { $errors = [