Skip to content

Commit d479811

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 db548df commit d479811

File tree

4 files changed

+88
-52
lines changed

4 files changed

+88
-52
lines changed

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

Lines changed: 59 additions & 52 deletions
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,72 @@ 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
359+
360+
return;
352361
}//end if
362+
}//end if
363+
364+
$prev = $tokens[($opener - 1)];
365+
if ($prev['code'] !== T_WHITESPACE) {
366+
$length = 0;
353367
} else {
354-
$prev = $tokens[($opener - 1)];
355-
if ($prev['code'] !== T_WHITESPACE) {
356-
$length = 0;
357-
} else {
358-
$length = strlen($prev['content']);
359-
}
368+
$length = strlen($prev['content']);
369+
}
360370

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'];
371+
if ($length !== 1) {
372+
$error = 'There must be a single space before the %s of a multi-line function declaration; found %s spaces';
373+
$code = 'SpaceBeforeOpenBrace';
374+
$data = ['opening brace'];
365375

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

371-
$data[] = $length;
381+
$data[] = $length;
372382

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-
}
383+
$fix = $phpcsFile->addFixableError($error, ($opener - 1), $code, $data);
384+
if ($fix === true) {
385+
if ($length === 0) {
386+
$phpcsFile->fixer->addContentBefore($opener, ' ');
387+
} else {
388+
$phpcsFile->fixer->replaceToken(($opener - 1), ' ');
380389
}
381-
382-
return;
383-
}//end if
390+
}
384391
}//end if
385392

386393
}//end processMultiLineDeclaration()

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -486,3 +486,17 @@ $arrowMultiLineArgs = fn (
486486
$longVar1, $longerVar2,
487487

488488
& ... $muchLongerVar3) => $longVar1;
489+
490+
// Prevent fixer conflict with itself.
491+
function foo(
492+
$param1,
493+
)
494+
: \SomeClass
495+
{
496+
}
497+
498+
function foo(
499+
$param1,
500+
$param2
501+
) : // comment.
502+
\Package\Sub\SomeClass {}

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -484,3 +484,16 @@ $arrowMultiLineArgs = fn (
484484
$longVar1, $longerVar2,
485485
& ... $muchLongerVar3
486486
) => $longVar1;
487+
488+
// Prevent fixer conflict with itself.
489+
function foo(
490+
$param1,
491+
)
492+
: \SomeClass {
493+
}
494+
495+
function foo(
496+
$param1,
497+
$param2
498+
) : // comment.
499+
\Package\Sub\SomeClass {}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,8 @@ public function getErrorList($testFile='FunctionDeclarationUnitTest.inc')
104104
483 => 2,
105105
487 => 1,
106106
488 => 2,
107+
495 => 1,
108+
502 => 2,
107109
];
108110
} else {
109111
$errors = [

0 commit comments

Comments
 (0)