Skip to content

Commit

Permalink
Squiz/MultiLineFunctionDeclaration: bug fix - skip over attributes
Browse files Browse the repository at this point in the history
The sniff looks for `T_COMMA` tokens to find the start of the next parameter and skips over parenthesis sets and square brackets sets (like short arrays) to prevent mismatching on a `T_COMMA` which is not a parameter separator.

This logic did not take parameter attributes into account, which can contain multiple comma-separated attributes, so should also be skipped over.

Fixed now. Includes plenty of tests.

Also includes minor stability fix for the parentheses/square brackets skipping.

Notes:
* It could be argued that the sniff should use the `File::getMethodParameters()` method to do the parameter parsing instead. This could be done in a future iteration, but will need to be evaluated carefully for side-effects.
* This sniff extends the PEAR `FunctionDeclaration` sniff. It has been verified that that sniff is not affected by this bug.

Fixes 608
  • Loading branch information
jrfnl committed Sep 14, 2024
1 parent c0e8f4d commit f39847a
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -227,16 +227,21 @@ public function processBracket($phpcsFile, $openBracket, $tokens, $type='functio
// Each line between the brackets should contain a single parameter.
for ($i = ($openBracket + 1); $i < $closeBracket; $i++) {
// Skip brackets, like arrays, as they can contain commas.
if (isset($tokens[$i]['bracket_opener']) === true) {
if (isset($tokens[$i]['bracket_closer']) === true) {
$i = $tokens[$i]['bracket_closer'];
continue;
}

if (isset($tokens[$i]['parenthesis_opener']) === true) {
if (isset($tokens[$i]['parenthesis_closer']) === true) {
$i = $tokens[$i]['parenthesis_closer'];
continue;
}

if (isset($tokens[$i]['attribute_closer']) === true) {
$i = $tokens[$i]['attribute_closer'];
continue;
}

if ($tokens[$i]['code'] !== T_COMMA) {
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,3 +302,55 @@ new ExceptionMessage(),
) {
}
}

// Issue #608 - multi-attributes are not handled correctly.
function ParamWithMultiAttributeOnSameLine(
#[AttributeA, AttributeB] string $param,
) {
}

function ParamWithMultiAttributeOnSameLineWithParamsShouldNotBeSeenAsMultipleFnParams(
#[AttributeA(10, 'test'), AttributeB([1, 2, 3,])] string $param,
) {
}

function ParamWithMultiAttributeOnSameLine(
#[AttributeA, AttributeB] string $paramA, int $paramB
) {
}

function ParamWithMultiAttributeOnSameLineWithParamsShouldNotBeSeenAsMultipleFnParams(
#[AttributeA(10, 'test'), AttributeB([1, 2, 3,])] string $param, int $paramB
) {
}

function ParamWithAttributeOnOwnLineShouldNotBeSeenAsMultipleFnParams(
#[Attribute]
string $param,
) {
}

function ParamWithMultipleAttributesOnOwnLineShouldNotBeSeenAsMultipleFnParams(
#[AttributeA]
#[AttributeB]
string $param,
) {
}

function ParamWithAttributeOnOwnLineWithParamsShouldNotBeSeenAsMultipleFnParamse(
#[Attribute(10, 20)]
string $param,
) {
}

function ParamWithMultiAttributeOnOwnLineShouldNotBeSeenAsMultipleFnParams(
#[AttributeA, AttributeB]
string $param,
) {
}

function ParamWithMultiAttributeOnOwnLineWithParamsShouldNotBeSeenAsMultipleFnParams(
#[AttributeA(10, 'test'), AttributeB([1, 2, 3,])]
string $param,
) {
}
Original file line number Diff line number Diff line change
Expand Up @@ -314,3 +314,57 @@ new ExceptionMessage(),
) {
}
}

// Issue #608 - multi-attributes are not handled correctly.
function ParamWithMultiAttributeOnSameLine(
#[AttributeA, AttributeB] string $param,
) {
}

function ParamWithMultiAttributeOnSameLineWithParamsShouldNotBeSeenAsMultipleFnParams(
#[AttributeA(10, 'test'), AttributeB([1, 2, 3,])] string $param,
) {
}

function ParamWithMultiAttributeOnSameLine(
#[AttributeA, AttributeB] string $paramA,
int $paramB
) {
}

function ParamWithMultiAttributeOnSameLineWithParamsShouldNotBeSeenAsMultipleFnParams(
#[AttributeA(10, 'test'), AttributeB([1, 2, 3,])] string $param,
int $paramB
) {
}

function ParamWithAttributeOnOwnLineShouldNotBeSeenAsMultipleFnParams(
#[Attribute]
string $param,
) {
}

function ParamWithMultipleAttributesOnOwnLineShouldNotBeSeenAsMultipleFnParams(
#[AttributeA]
#[AttributeB]
string $param,
) {
}

function ParamWithAttributeOnOwnLineWithParamsShouldNotBeSeenAsMultipleFnParamse(
#[Attribute(10, 20)]
string $param,
) {
}

function ParamWithMultiAttributeOnOwnLineShouldNotBeSeenAsMultipleFnParams(
#[AttributeA, AttributeB]
string $param,
) {
}

function ParamWithMultiAttributeOnOwnLineWithParamsShouldNotBeSeenAsMultipleFnParams(
#[AttributeA(10, 'test'), AttributeB([1, 2, 3,])]
string $param,
) {
}
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ public function getErrorList($testFile='')
252 => 1,
253 => 1,
254 => 1,
318 => 1,
323 => 1,
];
} else {
$errors = [
Expand Down

0 comments on commit f39847a

Please sign in to comment.