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.
PHPCSStandards · Nov 11, 2023 · 661224e · 661224e
1 parent fc17f86
commit 661224e
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 46 deletions.
diff --git 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
@@ -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
@@ -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
@@ -100,6 +100,8 @@ public function getErrorList($testFile='FunctionDeclarationUnitTest.inc')
                 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,
+=> 2,
                 ];
             } else {
                 $errors = [
@@ Expand Down @@