From 03428fd74fca506131470344df55c140f7860d97 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 17 May 2024 13:07:31 +0200 Subject: [PATCH 1/2] Generic/FunctionCallArgumentSpacing: efficiency fix - skip over arrow functions PHP 7.4 introduced arrow functions. While arrow functions being passed as an argument in a function call will not lead to false positives for this sniff - at least, I haven't been able to come up with a code sample in which it would [^1] -, skipping over them is still beneficial as it prevents unnecessary token walking. Mind: the `scope_closer` of the arrow function _may_ be the comma separating the arrow function argument from the next function call argument, so we need to step one token back to prevent false negatives on those comma's. Either way, this is now handled. Includes unit tests. [^1]: comma's in arrow functions will always be nested within parentheses, within a short array or within a nested closure or anonymous class, all of which the sniff already ignores. --- .../Functions/FunctionCallArgumentSpacingSniff.php | 6 ++++++ .../FunctionCallArgumentSpacingUnitTest.1.inc | 12 ++++++++++++ .../FunctionCallArgumentSpacingUnitTest.1.inc.fixed | 12 ++++++++++++ .../FunctionCallArgumentSpacingUnitTest.php | 2 ++ 4 files changed, 32 insertions(+) diff --git a/src/Standards/Generic/Sniffs/Functions/FunctionCallArgumentSpacingSniff.php b/src/Standards/Generic/Sniffs/Functions/FunctionCallArgumentSpacingSniff.php index 1ae4822569..4653fa5f1e 100644 --- a/src/Standards/Generic/Sniffs/Functions/FunctionCallArgumentSpacingSniff.php +++ b/src/Standards/Generic/Sniffs/Functions/FunctionCallArgumentSpacingSniff.php @@ -109,6 +109,7 @@ public function checkSpacing(File $phpcsFile, $stackPtr, $openBracket) $find = [ T_COMMA, T_CLOSURE, + T_FN, T_ANON_CLASS, T_OPEN_SHORT_ARRAY, ]; @@ -120,6 +121,11 @@ public function checkSpacing(File $phpcsFile, $stackPtr, $openBracket) // Skip closures. $nextSeparator = $tokens[$nextSeparator]['scope_closer']; continue; + } else if ($tokens[$nextSeparator]['code'] === T_FN) { + // Skip arrow functions, but don't skip the arrow function closer as it is likely to + // be the comma separating it from the next function call argument (or the parenthesis closer). + $nextSeparator = ($tokens[$nextSeparator]['scope_closer'] - 1); + continue; } else if ($tokens[$nextSeparator]['code'] === T_OPEN_SHORT_ARRAY) { // Skips arrays using short notation. $nextSeparator = $tokens[$nextSeparator]['bracket_closer']; diff --git a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc index 29ef802d55..c893b98c21 100644 --- a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc +++ b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc @@ -177,3 +177,15 @@ $foo = new MyClass( #[AttributeName(1,2)] $callable = myCallable(...); + +// Skip over PHP 7.4 arrow functions. +// While any commas belonging to the code within the arrow function would always need to be within parentheses +// or within a short array, so there aren't any false positives, the sniff also does not need to examine these, +// so will be more efficient skipping over arrow functions. +$foobar = functionCallFnParamA( + fn ($foo,$bar) => [1,2,3], + $args, +); + +$foobar = functionCallFnParamB(fn ($foo,$bar) => [1,2,3] ,$args); +$foobar = functionCallFnParamC($args, fn ($foo,$bar) => [1,2,3] , ); diff --git a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc.fixed b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc.fixed index 3f0cb0fca6..c54ac190fe 100644 --- a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc.fixed +++ b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc.fixed @@ -177,3 +177,15 @@ $foo = new MyClass( #[AttributeName(1, 2)] $callable = myCallable(...); + +// Skip over PHP 7.4 arrow functions. +// While any commas belonging to the code within the arrow function would always need to be within parentheses +// or within a short array, so there aren't any false positives, the sniff also does not need to examine these, +// so will be more efficient skipping over arrow functions. +$foobar = functionCallFnParamA( + fn ($foo,$bar) => [1,2,3], + $args, +); + +$foobar = functionCallFnParamB(fn ($foo,$bar) => [1,2,3], $args); +$foobar = functionCallFnParamC($args, fn ($foo,$bar) => [1,2,3], ); diff --git a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.php b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.php index 7088428772..6706b3a90c 100644 --- a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.php +++ b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.php @@ -66,6 +66,8 @@ public function getErrorList($testFile='') 162 => 2, 170 => 1, 177 => 1, + 190 => 2, + 191 => 2, ]; default: From 57a9ed734e2ab06c062fe0e463745eab483cf722 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 17 May 2024 13:28:12 +0200 Subject: [PATCH 2/2] Generic/FunctionCallArgumentSpacing: bug fix - ignore commas in nested match structures PHP 8.0 introduced match control structures, which can be passed in a function call (though probably/hopefully this is not very common as it makes for hard to comprehend code). The comma's within match control structures should be checked by a sniff which handled that control structure and should not be treated as comma's belonging to the function call. As things are, this is currently not the case, which leads to false positives. Fixed now. Includes test. --- .../Sniffs/Functions/FunctionCallArgumentSpacingSniff.php | 4 +++- .../Functions/FunctionCallArgumentSpacingUnitTest.1.inc | 8 ++++++++ .../FunctionCallArgumentSpacingUnitTest.1.inc.fixed | 8 ++++++++ .../Functions/FunctionCallArgumentSpacingUnitTest.php | 1 + 4 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/Standards/Generic/Sniffs/Functions/FunctionCallArgumentSpacingSniff.php b/src/Standards/Generic/Sniffs/Functions/FunctionCallArgumentSpacingSniff.php index 4653fa5f1e..bf93c55338 100644 --- a/src/Standards/Generic/Sniffs/Functions/FunctionCallArgumentSpacingSniff.php +++ b/src/Standards/Generic/Sniffs/Functions/FunctionCallArgumentSpacingSniff.php @@ -112,13 +112,15 @@ public function checkSpacing(File $phpcsFile, $stackPtr, $openBracket) T_FN, T_ANON_CLASS, T_OPEN_SHORT_ARRAY, + T_MATCH, ]; while (($nextSeparator = $phpcsFile->findNext($find, ($nextSeparator + 1), $closeBracket)) !== false) { if ($tokens[$nextSeparator]['code'] === T_CLOSURE || $tokens[$nextSeparator]['code'] === T_ANON_CLASS + || $tokens[$nextSeparator]['code'] === T_MATCH ) { - // Skip closures. + // Skip closures, anon class declarations and match control structures. $nextSeparator = $tokens[$nextSeparator]['scope_closer']; continue; } else if ($tokens[$nextSeparator]['code'] === T_FN) { diff --git a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc index c893b98c21..393ee10b66 100644 --- a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc +++ b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc @@ -189,3 +189,11 @@ $foobar = functionCallFnParamA( $foobar = functionCallFnParamB(fn ($foo,$bar) => [1,2,3] ,$args); $foobar = functionCallFnParamC($args, fn ($foo,$bar) => [1,2,3] , ); + +// Ignore spacing within PHP 8.0 match control structures, which may have their own rules. +$foobar = functionCallMatchParam( + match($foo) { + 1,2,3 => 'something',4,5,6 => 'else',default => 'works' + } , // But check the spacing again once the match expression has finished. + $args +); diff --git a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc.fixed b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc.fixed index c54ac190fe..a50f695e45 100644 --- a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc.fixed +++ b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.1.inc.fixed @@ -189,3 +189,11 @@ $foobar = functionCallFnParamA( $foobar = functionCallFnParamB(fn ($foo,$bar) => [1,2,3], $args); $foobar = functionCallFnParamC($args, fn ($foo,$bar) => [1,2,3], ); + +// Ignore spacing within PHP 8.0 match control structures, which may have their own rules. +$foobar = functionCallMatchParam( + match($foo) { + 1,2,3 => 'something',4,5,6 => 'else',default => 'works' + }, // But check the spacing again once the match expression has finished. + $args +); diff --git a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.php b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.php index 6706b3a90c..35bf11cb72 100644 --- a/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.php +++ b/src/Standards/Generic/Tests/Functions/FunctionCallArgumentSpacingUnitTest.php @@ -68,6 +68,7 @@ public function getErrorList($testFile='') 177 => 1, 190 => 2, 191 => 2, + 197 => 1, ]; default: