From 81a5fad2d5ccbb430e697f04e255424ad9f0d37f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 25 Oct 2023 06:11:13 +0200 Subject: [PATCH] Squiz/FunctionSpacing: bug fix - prevent fixer conflict with itself The `Squiz.WhiteSpace.FunctionSpacing` sniff demands # lines above and below a function declaration, but when determining the number of lines above or below, it does not care for additional code on the same line as the function declaration. I.e.: it does not demand that a function declaration starts on its own line or that the close brace is on its own line (there are other sniffs for that). The sniff also tries to prevent "double" errors for the same issue, i.e. when two functions are each on their own line without blank lines between them, the sniff will only throw one error for "spacing after" the first function and will not throw an error for "spacing before" for the second function. To determine whether the "spacing before" error needs to be hidden, the sniff tries to check whether the tokens on the previous line indicate the line contains a function declaration. As things were, however, this check could _bow out_ too early as it stopped at a scope opener for a wrapping construct (class), however, that wrapping construct _could_ be declared on the same line as the function, which means that in that case, the sniff would not determine the `$foundLines` correctly, as it stops on the current line at the scope opener instead of on a non-blank line above the function line. This commit fixes this bug by changing the `$stopAt` value to contain the wrapping scope opener applicable to the code _before_ the function line. This allows the `$foundLines` to be determined correctly and prevents the fixer conflict. Includes additional tests. Fixes 3904 --- .../Sniffs/WhiteSpace/FunctionSpacingSniff.php | 15 +++++++-------- .../WhiteSpace/FunctionSpacingUnitTest.1.inc | 8 ++++++++ .../FunctionSpacingUnitTest.1.inc.fixed | 15 +++++++++++++++ .../Tests/WhiteSpace/FunctionSpacingUnitTest.php | 2 ++ 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/WhiteSpace/FunctionSpacingSniff.php b/src/Standards/Squiz/Sniffs/WhiteSpace/FunctionSpacingSniff.php index 1f4704b91f..e0626d6a4b 100644 --- a/src/Standards/Squiz/Sniffs/WhiteSpace/FunctionSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/WhiteSpace/FunctionSpacingSniff.php @@ -262,22 +262,21 @@ public function process(File $phpcsFile, $stackPtr) $prevContent = $phpcsFile->findPrevious(T_WHITESPACE, ($tokens[$prevContent]['comment_opener'] - 1), null, true); } - $prevLineToken = $prevContent; - // Before we throw an error, check that we are not throwing an error // for another function. We don't want to error for no blank lines after // the previous function and no blank lines before this one as well. - $prevLine = ($tokens[$prevContent]['line'] - 1); - $i = ($stackPtr - 1); - $foundLines = 0; - $stopAt = 0; - if (isset($tokens[$stackPtr]['conditions']) === true) { - $conditions = $tokens[$stackPtr]['conditions']; + if (isset($tokens[$prevLineToken]['conditions']) === true) { + $conditions = $tokens[$prevLineToken]['conditions']; $conditions = array_keys($conditions); $stopAt = array_pop($conditions); } + $prevLineToken = $prevContent; + $prevLine = ($tokens[$prevContent]['line'] - 1); + $i = ($stackPtr - 1); + $foundLines = 0; + while ($currentLine !== $prevLine && $currentLine > 1 && $i > $stopAt) { if ($tokens[$i]['code'] === T_FUNCTION) { // Found another interface or abstract function. diff --git a/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.1.inc b/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.1.inc index 36a287f077..b03a0ed82c 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.1.inc @@ -574,3 +574,11 @@ class ClassWithAttributes { // phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 2 // phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 2 // phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 2 + +// Issue #3904. +echo 'this line belongs with the #3904 test'; +class Person {public function __construct($name){}} +echo 'this line belongs with the #3904 test'; + +function Foo() {} function bar($name){} +echo 'this line belongs with the #3904 test'; diff --git a/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.1.inc.fixed index ac2df1cb7b..443824ca14 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.1.inc.fixed +++ b/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.1.inc.fixed @@ -656,3 +656,18 @@ class ClassWithAttributes { // phpcs:set Squiz.WhiteSpace.FunctionSpacing spacing 2 // phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingBeforeFirst 2 // phpcs:set Squiz.WhiteSpace.FunctionSpacing spacingAfterLast 2 + +// Issue #3904. +echo 'this line belongs with the #3904 test'; + + +class Person {public function __construct($name){}} + + +echo 'this line belongs with the #3904 test'; + + +function Foo() {} function bar($name){} + + +echo 'this line belongs with the #3904 test'; diff --git a/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.php b/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.php index fabb6adb92..f21327640f 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/WhiteSpace/FunctionSpacingUnitTest.php @@ -95,6 +95,8 @@ public function getErrorList($testFile='') 553 => 1, 560 => 1, 566 => 1, + 580 => 2, + 583 => 3, ]; case 'FunctionSpacingUnitTest.2.inc':