Merge pull request #52 from PHPCSStandards/feature/3736-pear-function…

…declaration-fix-fixer-creating-parse-error PEAR/FunctionDeclaration: bug fix x 2 - prevent fixer creating a parse error + prevent fixer conflict
PHPCSStandards · Dec 5, 2023 · 68e7d9e · 68e7d9e
2 parents 66b4b36 + dcc257f
commit 68e7d9e
Show file tree

Hide file tree

Showing 5 changed files with 107 additions and 44 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -109,6 +109,10 @@ 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 #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
@@ -297,63 +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 just 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']) {
-                        $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
@@ -465,3 +465,26 @@ 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
+    )
+    {}
+}
+
+// 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
@@ -463,3 +463,25 @@ 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
+    ) {
+    }
+}
+
+// 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
@@ -99,6 +99,9 @@ public function getErrorList($testFile='FunctionDeclarationUnitTest.inc')
                 371 => 1,
                 402 => 1,
                 406 => 1,
+                475 => 1,
+                483 => 1,
+                490 => 2,
             ];
         } else {
             $errors = [
-Original file line number
+Diff line change
@@ Expand Up @@
 => 1,
 => 1,
 => 1,
+=> 1,
+=> 1,
+=> 2,
                 ];
             } else {
                 $errors = [
@@ Expand Down @@