Autofixer for Squiz.WhiteSpace.FunctionSpacing goes into "infinite" loop for method defined on the same line as a class

[Daimona]

Describe the bug

In the (intentionally bad) snippet of code below, I have a method defined on the same line as its class. When running PHPCS on it with the Squiz.WhiteSpace.FunctionSpacing sniff enabled (ruleset below), it reports that 1 blank line is expected before the method. However, if I then try to autofix that with phpcbf, it tries to add the line in the wrong place, thus going into an infinite loop because it detects that the issue still hasn't been fixed; it only aborts upon reaching the limit of 50 passes.

Code sample

<?php

class Person { public function __construct($name){} }

Custom ruleset

<?xml version="1.0"?>
<ruleset>
    <arg name="tab-width" value="4" />

    <rule ref="Squiz.WhiteSpace.FunctionSpacing">
        <properties>
            <property name="spacing" value="1" />
        </properties>
    </rule>
</ruleset>

To reproduce

Steps to reproduce the behavior:

1. Create a file called test.php with the code sample above
2. Run phpcs test.php
3. See error message displayed

   ----------------------------------------------------------------------
   FOUND 1 ERROR AFFECTING 1 LINE
   ----------------------------------------------------------------------
   3 | ERROR | [x] Expected 1 blank line before function; 0 found
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------

4. Run phpcbf -vvv test.php
5. See that it fails to fix the file. The full output shows that it's adding the line right after the PHP open tag:

(... all the previous passes...)
---START FILE CONTENT---
 1|<?php
 2|
 3|
 4|
 5|
 6|
 7|
 8|
 9|
10|
11|
12|
13|
14|
15|
16|
17|
18|
19|
20|
21|
22|
23|
24|
25|
26|
27|
28|class Person { public function __construct($name){} }
29|
30|
--- END FILE CONTENT ---
    Squiz.WhiteSpace.FunctionSpacing:342 replaced token 0 (T_OPEN_TAG on line 1) "<?php\n" => "<?php\n\n"
        => Fixing file: 1/1 violations remaining [made 50 passes]...            
    * fixed 1 violations, starting loop 51 *
    *** Reached maximum number of loops with 1 violations left unfixed ***
ERROR in 33ms

PHPCBF RESULT SUMMARY
----------------------------------------------------------------------
FILE                                                  FIXED  REMAINING
----------------------------------------------------------------------
(...)/test.php            FAILED TO FIX
----------------------------------------------------------------------
A TOTAL OF 0 ERRORS WERE FIXED IN 1 FILE
----------------------------------------------------------------------
PHPCBF FAILED TO FIX 1 FILE
----------------------------------------------------------------------

Expected behavior

Ideally, the issue should be fixed. Or if that's hard to do, at least PHPCS should not go into an endless loop, and instead report the issue as unfixable.

Versions (please complete the following information)

Operating System	Ubuntu 22.04
PHP version	8.2.11
PHP_CodeSniffer version	master
Standard	See above
Install type	Composer local

Please confirm:

• [x] I have searched the issue list and am not opening a duplicate issue.
• [x] I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
• [x] I have verified the issue still exists in the master branch of PHP_CodeSniffer.

[jrfnl]

@Daimona Thanks for reporting this issue. I have been able to reproduce this with the provided information and can confirm the bug.

Ideally, the issue should be fixed.

I'm hoping the issue can be fixed in the sniff.

Or if that's hard to do, at least PHPCS should not go into an endless loop, and instead report the issue as unfixable.

But I don't understand this remark, I mean: this is exactly what PHPCS is doing: reporting the issue as unfixable (by checking 50 loops and bowing out if there is still a fixer conflict).

[jrfnl]

Oh and I'm also wondering why the sniff does not report the AfterLast error code for this code snippet as with the closer being on the same line as the class close brace, I would also expect that error to be reported.

[jrfnl]

Oh and I'm also wondering why the sniff does not report the AfterLast error code for this code snippet as with the closer being on the same line as the class close brace, I would also expect that error to be reported.

Actually, looks like the sniff doesn't care about code before or after the function. Just about the blank lines above/below the code, which explains why the AfterLast error doesn't show as lines at the end of the file are ignored as they generally have to comply with their own rules.

[jrfnl]

@Daimona PR #3905 should fix this issue. Testing appreciated.

[Daimona]

But I don't understand this remark, I mean: this is exactly what PHPCS is doing: reporting the issue as unfixable (by checking 50 loops and bowing out if there is still a fixer conflict).

Ah, sorry, I should have stated this more clearly. What I meant is: if this is a hard-to-fix edge case, then an autofix should not be offered at all, instead of offering it but then having it fail. The main reason why I mentioned this is that I observed a severe performance degradation, and while I haven't fully investigated it, I suspect it might have been caused by this autofixer.

For the small code snippet I used as an example here it doesn't really make any difference, but when testing a whole standard on this file, phpcbf takes ~200 ms without the Squiz.WhiteSpace.FunctionSpacing enabled, and ~1.5 s with that sniff enabled. As I said, I haven't looked into why the failed autofix has such a severe impact, but it does seem to be the cause.

@Daimona PR #3905 should fix this issue. Testing appreciated.

I can confirm that the PR fixes this bug (as the violation is no longer reported), thank you!

[jrfnl]

But I don't understand this remark, I mean: this is exactly what PHPCS is doing: reporting the issue as unfixable (by checking 50 loops and bowing out if there is still a fixer conflict).

Ah, sorry, I should have stated this more clearly. What I meant is: if this is a hard-to-fix edge case, then an autofix should not be offered at all, instead of offering it but then having it fail.

I see what you mean and yes, I agree. The problem is finding those edge-cases. Code can be written in so many different ways.

Since I started contributing to this project, I've learned to write incredible convoluted code for test cases, but even though my fantasy goes far, I often still miss some edge cases I just didn't think off. Writing good sniffs is hard.

Please do keep reporting these kind of things as it really helps to make sniffs better.

The main reason why I mentioned this is that I observed a severe performance degradation, and while I haven't fully investigated it, I suspect it might have been caused by this autofixer.

For the small code snippet I used as an example here it doesn't really make any difference, but when testing a whole standard on this file, phpcbf takes ~200 ms without the Squiz.WhiteSpace.FunctionSpacing enabled, and ~1.5 s with that sniff enabled. As I said, I haven't looked into why the failed autofix has such a severe impact, but it does seem to be the cause.

You might be interested in doing some testing with the Performance report (open PR #3810), that should help in identifying which sniffs are particularly slow.

I already identified a couple of bottlenecks, in part via that report. There is now also a "Performance" tag with which those type of issues are labeled.

[Daimona]

Since I started contributing to this project, I've learned to write incredible convoluted code for test cases, but even though my fantasy goes far, I often still miss some edge cases I just didn't think off. Writing good sniffs is hard.

As someone who wrote a few sniffs and has worked on static analyzers, I wholeheartedly agree! In fact, I discovered this bug as part of an exploration I did to try and come up with an absolutely horrendous piece of code :-)

You might be interested in doing some testing with the Performance report (open PR #3810), that should help in identifying which sniffs are particularly slow.

Thanks, I'll definitely look into that!

[jrfnl]

FYI: the fix for this issue is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).