Skip to content

Commit

Permalink
PEAR/FunctionDeclaration: prevent fixer conflict
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jrfnl
jrfnl committed Sep 11, 2022
1 parent 3611637 commit fa2f2df
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 52 deletions.
111 changes: 59 additions & 52 deletions src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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()
Expand Down
14 changes: 14 additions & 0 deletions src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -439,3 +439,17 @@ $arrowMultiLineArgs = fn (
$longVar1, $longerVar2,

& ... $muchLongerVar3) => $longVar1;

// Prevent fixer conflict with itself.
function foo(
$param1,
)
: \SomeClass
{
}

function foo(
$param1,
$param2
) : // comment.
\Package\Sub\SomeClass {}
13 changes: 13 additions & 0 deletions src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.inc.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -437,3 +437,16 @@ $arrowMultiLineArgs = fn (
$longVar1, $longerVar2,
& ... $muchLongerVar3
) => $longVar1;

// Prevent fixer conflict with itself.
function foo(
$param1,
)
: \SomeClass {
}

function foo(
$param1,
$param2
) : // comment.
\Package\Sub\SomeClass {}
2 changes: 2 additions & 0 deletions src/Standards/PEAR/Tests/Functions/FunctionDeclarationUnitTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ public function getErrorList($testFile='FunctionDeclarationUnitTest.inc')
436 => 2,
440 => 1,
441 => 2,
448 => 1,
455 => 2,
];
} else {
$errors = [
Expand Down

0 comments on commit fa2f2df

Please sign in to comment.