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.
squizlabs · May 4, 2023 · d479811 · d479811
1 parent db548df
commit d479811
Show file tree

Hide file tree

Showing 4 changed files with 88 additions and 52 deletions.
diff --git a/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php b/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php
@@ -308,7 +308,9 @@ 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 %s of a multi-line function declaration must be on the same line';
@@ -320,67 +322,72 @@ public function processMultiLineDeclaration($phpcsFile, $stackPtr, $tokens)
                 $data = ['arrow'];
             }
 
-            $fix = $phpcsFile->addFixableError($error, $opener, $code, $data);
-            if ($fix === true) {
-                $prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($opener - 1), $closeBracket, true);
-                $phpcsFile->fixer->beginChangeset();
-                $phpcsFile->fixer->addContent($prev, ' '.$tokens[$opener]['content']);
-
-                // 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);
-
-                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, '');
+            $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, $data);
+            } else {
+                $fix = $phpcsFile->addFixableError($error, $opener, $code, $data);
+                if ($fix === true) {
+                    $phpcsFile->fixer->beginChangeset();
+                    $phpcsFile->fixer->addContent($prev, ' '.$tokens[$opener]['content']);
+
+                    // 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);
+
+                    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), '');
                         }
                     }
-                } else {
-                    // Just remove the opener.
-                    $phpcsFile->fixer->replaceToken($opener, '');
-                    if ($tokens[$next]['line'] === $tokens[$opener]['line']) {
-                        $phpcsFile->fixer->replaceToken(($opener + 1), '');
-                    }
-                }
 
-                $phpcsFile->fixer->endChangeset();
+                    $phpcsFile->fixer->endChangeset();
+                }//end if
+
+                return;
             }//end if
+        }//end if
+
+        $prev = $tokens[($opener - 1)];
+        if ($prev['code'] !== T_WHITESPACE) {
+            $length = 0;
         } else {
-            $prev = $tokens[($opener - 1)];
-            if ($prev['code'] !== T_WHITESPACE) {
-                $length = 0;
-            } else {
-                $length = strlen($prev['content']);
-            }
+            $length = strlen($prev['content']);
+        }
 
-            if ($length !== 1) {
-                $error = 'There must be a single space between the closing parenthesis and the %s of a multi-line function declaration; found %s spaces';
-                $code  = 'SpaceBeforeOpenBrace';
-                $data  = ['opening brace'];
+        if ($length !== 1) {
+            $error = 'There must be a single space before the %s of a multi-line function declaration; found %s spaces';
+            $code  = 'SpaceBeforeOpenBrace';
+            $data  = ['opening brace'];
 
-                if ($tokens[$stackPtr]['code'] === T_FN) {
-                    $code = 'SpaceBeforeArrow';
-                    $data = ['arrow'];
-                }
+            if ($tokens[$stackPtr]['code'] === T_FN) {
+                $code = 'SpaceBeforeArrow';
+                $data = ['arrow'];
+            }
 
-                $data[] = $length;
+            $data[] = $length;
 
-                $fix = $phpcsFile->addFixableError($error, ($opener - 1), $code, $data);
-                if ($fix === true) {
-                    if ($length === 0) {
-                        $phpcsFile->fixer->addContentBefore($opener, ' ');
-                    } else {
-                        $phpcsFile->fixer->replaceToken(($opener - 1), ' ');
-                    }
+            $fix = $phpcsFile->addFixableError($error, ($opener - 1), $code, $data);
+            if ($fix === true) {
+                if ($length === 0) {
+                    $phpcsFile->fixer->addContentBefore($opener, ' ');
+                } else {
+                    $phpcsFile->fixer->replaceToken(($opener - 1), ' ');
                 }
-
-                return;
-            }//end if
+            }
         }//end if
 
     }//end processMultiLineDeclaration()

diff --git a/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc b/src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc
@@ -486,3 +486,17 @@ $arrowMultiLineArgs = fn (
     $longVar1, $longerVar2,
 
   & ...  $muchLongerVar3) => $longVar1;
+
+// 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
@@ -484,3 +484,16 @@ $arrowMultiLineArgs = fn (
     $longVar1, $longerVar2,
     & ...  $muchLongerVar3
 ) => $longVar1;
+
+// 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
@@ -104,6 +104,8 @@ public function getErrorList($testFile='FunctionDeclarationUnitTest.inc')
                 483 => 2,
                 487 => 1,
                 488 => 2,
+                495 => 1,
+                502 => 2,
             ];
         } else {
             $errors = [