From 4845ef085fac4353e93286f82915b3defabff065 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 --- .../PEAR/Sniffs/Functions/FunctionDeclarationSniff.php | 6 ++++-- .../PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc | 9 +++++++++ .../Functions/FunctionDeclarationUnitTest.inc.fixed | 9 +++++++++ .../PEAR/Tests/Functions/FunctionDeclarationUnitTest.php | 1 + 4 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php b/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php index 14479e2c0a..d907fca2c6 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 01ab3e8482..519b38e4e6 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 a498afbd19f058dd57fa5d2a699ec455c4f638df 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. --- .../Functions/FunctionDeclarationSniff.php | 101 ++++++++++-------- .../Functions/FunctionDeclarationUnitTest.inc | 14 +++ .../FunctionDeclarationUnitTest.inc.fixed | 13 +++ .../Functions/FunctionDeclarationUnitTest.php | 2 + 4 files changed, 84 insertions(+), 46 deletions(-) diff --git a/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php b/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php index d907fca2c6..380b6ca248 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 519b38e4e6..b29a279c9d 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 = [