Skip to content

Commit e922ddf

Browse files
jrfnljrfnl
committed
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.
1 parent 3611637 commit e922ddf

File tree

4 files changed

+86
-52
lines changed

4 files changed

+86
-52
lines changed

‎src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php

+57-52
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,9 @@ public function processMultiLineDeclaration($phpcsFile, $stackPtr, $tokens)
308308
return;
309309
}
310310

311-
// The opening brace needs to be one space away from the closing parenthesis.
311+
// The opening brace needs to be on the same line as the closing parenthesis.
312+
// There should only be one space between the closing parenthesis - or the end of the
313+
// return type - and the opening brace.
312314
$opener = $tokens[$stackPtr]['scope_opener'];
313315
if ($tokens[$opener]['line'] !== $tokens[$closeBracket]['line']) {
314316
$error = 'The closing parenthesis and the %s of a multi-line function declaration must be on the same line';
@@ -320,67 +322,70 @@ public function processMultiLineDeclaration($phpcsFile, $stackPtr, $tokens)
320322
$data = ['arrow'];
321323
}
322324

323-
$fix = $phpcsFile->addFixableError($error, $opener, $code, $data);
324-
if ($fix === true) {
325-
$prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($opener - 1), $closeBracket, true);
326-
$phpcsFile->fixer->beginChangeset();
327-
$phpcsFile->fixer->addContent($prev, ' '.$tokens[$opener]['content']);
328-
329-
// If the opener is on a line by itself, removing it will create
330-
// an empty line, so just remove the entire line instead.
331-
$prev = $phpcsFile->findPrevious(T_WHITESPACE, ($opener - 1), $closeBracket, true);
332-
$next = $phpcsFile->findNext(T_WHITESPACE, ($opener + 1), null, true);
333-
334-
if ($tokens[$prev]['line'] < $tokens[$opener]['line']
335-
&& $tokens[$next]['line'] > $tokens[$opener]['line']
336-
) {
337-
// Clear the whole line.
338-
for ($i = ($prev + 1); $i < $next; $i++) {
339-
if ($tokens[$i]['line'] === $tokens[$opener]['line']) {
340-
$phpcsFile->fixer->replaceToken($i, '');
325+
$prev = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($opener - 1), $closeBracket, true);
326+
if ($tokens[$prev]['line'] === $tokens[$opener]['line']) {
327+
// End of the return type is not on the same line as the close parenthesis.
328+
$phpcsFile->addError($error, $opener, $code, $data);
329+
} else {
330+
$fix = $phpcsFile->addFixableError($error, $opener, $code, $data);
331+
if ($fix === true) {
332+
$phpcsFile->fixer->beginChangeset();
333+
$phpcsFile->fixer->addContent($prev, ' '.$tokens[$opener]['content']);
334+
335+
// If the opener is on a line by itself, removing it will create
336+
// an empty line, so just remove the entire line instead.
337+
$prev = $phpcsFile->findPrevious(T_WHITESPACE, ($opener - 1), $closeBracket, true);
338+
$next = $phpcsFile->findNext(T_WHITESPACE, ($opener + 1), null, true);
339+
340+
if ($tokens[$prev]['line'] < $tokens[$opener]['line']
341+
&& $tokens[$next]['line'] > $tokens[$opener]['line']
342+
) {
343+
// Clear the whole line.
344+
for ($i = ($prev + 1); $i < $next; $i++) {
345+
if ($tokens[$i]['line'] === $tokens[$opener]['line']) {
346+
$phpcsFile->fixer->replaceToken($i, '');
347+
}
348+
}
349+
} else {
350+
// Just remove the opener.
351+
$phpcsFile->fixer->replaceToken($opener, '');
352+
if ($tokens[$next]['line'] === $tokens[$opener]['line']) {
353+
$phpcsFile->fixer->replaceToken(($opener + 1), '');
341354
}
342355
}
343-
} else {
344-
// Just remove the opener.
345-
$phpcsFile->fixer->replaceToken($opener, '');
346-
if ($tokens[$next]['line'] === $tokens[$opener]['line']) {
347-
$phpcsFile->fixer->replaceToken(($opener + 1), '');
348-
}
349-
}
350356

351-
$phpcsFile->fixer->endChangeset();
357+
$phpcsFile->fixer->endChangeset();
358+
}//end if
352359
}//end if
360+
}//end if
361+
362+
$prev = $tokens[($opener - 1)];
363+
if ($prev['code'] !== T_WHITESPACE) {
364+
$length = 0;
353365
} else {
354-
$prev = $tokens[($opener - 1)];
355-
if ($prev['code'] !== T_WHITESPACE) {
356-
$length = 0;
357-
} else {
358-
$length = strlen($prev['content']);
359-
}
366+
$length = strlen($prev['content']);
367+
}
360368

361-
if ($length !== 1) {
362-
$error = 'There must be a single space between the closing parenthesis and the %s of a multi-line function declaration; found %s spaces';
363-
$code = 'SpaceBeforeOpenBrace';
364-
$data = ['opening brace'];
369+
if ($length !== 1) {
370+
$error = 'There must be a single space before the %s of a multi-line function declaration; found %s spaces';
371+
$code = 'SpaceBeforeOpenBrace';
372+
$data = ['opening brace'];
365373

366-
if ($tokens[$stackPtr]['code'] === T_FN) {
367-
$code = 'SpaceBeforeArrow';
368-
$data = ['arrow'];
369-
}
374+
if ($tokens[$stackPtr]['code'] === T_FN) {
375+
$code = 'SpaceBeforeArrow';
376+
$data = ['arrow'];
377+
}
370378

371-
$data[] = $length;
379+
$data[] = $length;
372380

373-
$fix = $phpcsFile->addFixableError($error, ($opener - 1), $code, $data);
374-
if ($fix === true) {
375-
if ($length === 0) {
376-
$phpcsFile->fixer->addContentBefore($opener, ' ');
377-
} else {
378-
$phpcsFile->fixer->replaceToken(($opener - 1), ' ');
379-
}
381+
$fix = $phpcsFile->addFixableError($error, ($opener - 1), $code, $data);
382+
if ($fix === true) {
383+
if ($length === 0) {
384+
$phpcsFile->fixer->addContentBefore($opener, ' ');
385+
} else {
386+
$phpcsFile->fixer->replaceToken(($opener - 1), ' ');
380387
}
381-
382-
return;
383-
}//end if
388+
}
384389
}//end if
385390

386391
}//end processMultiLineDeclaration()

‎src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc

+14
Original file line numberDiff line numberDiff line change
@@ -439,3 +439,17 @@ $arrowMultiLineArgs = fn (
439439
$longVar1, $longerVar2,
440440

441441
& ... $muchLongerVar3) => $longVar1;
442+
443+
// Prevent fixer conflict with itself.
444+
function foo(
445+
$param1,
446+
)
447+
: \SomeClass
448+
{
449+
}
450+
451+
function foo(
452+
$param1,
453+
$param2
454+
) : // comment.
455+
\Package\Sub\SomeClass {}

‎src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed

+13
Original file line numberDiff line numberDiff line change
@@ -437,3 +437,16 @@ $arrowMultiLineArgs = fn (
437437
$longVar1, $longerVar2,
438438
& ... $muchLongerVar3
439439
) => $longVar1;
440+
441+
// Prevent fixer conflict with itself.
442+
function foo(
443+
$param1,
444+
)
445+
: \SomeClass {
446+
}
447+
448+
function foo(
449+
$param1,
450+
$param2
451+
) : // comment.
452+
\Package\Sub\SomeClass {}

‎src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php

+2
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,8 @@ public function getErrorList($testFile='FunctionDeclarationUnitTest.inc')
104104
436 => 2,
105105
440 => 1,
106106
441 => 2,
107+
448 => 1,
108+
455 => 1,
107109
];
108110
} else {
109111
$errors = [

0 commit comments

Comments
 (0)