PSR2/ClassDeclaration: bug fix - prevent fixer conflict

[jrfnl]

Description

Yet another one in the fixer conflict series.

When running the Squiz standard over all test case files, a fixer conflict between the Squiz.Classes.ClassDeclaration, which extends the PSR2.Classes.ClassDeclaration sniff, and the Squiz.Commenting.InlineComment sniff was discovered via the tests in the tests\Core\Tokenizer\UndoNamespacedNameSingleTokenTest.inc file for a code sample like this:

class MyClass
    /* testExtendedFQN */
    extends \Vendor\Level\FQN
    /* testImplementsRelative */
    implements namespace\Name,
        /* testImplementsFQN */
        \Fully\Qualified,
        /* testImplementsUnqualified */
        Unqualified,
        /* testImplementsPartiallyQualified */
        Sub\Level\Name
{}

The conflict essentially comes down to the PSR2.Classes.ClassDeclaration sniff adding a new line between an inline comment (which is already on its own line) and the name of an interface being implemented, which the Squiz.Commenting.InlineComment sniff would then remove again.

Isolating the code sample even further, the conflict can be reproduced even without the Squiz.Commenting.InlineComment sniff.

The short of it was, that the PSR2.Classes.ClassDeclaration sniff did not take into account that there could be comments between the interface names in a multi-line implements and would then get its knickers in a twist.

Original errors for the code sample added in the test case file:

 302 | ERROR | [x] Only one interface may be specified per line in a multi-line implements declaration
     |       |     (PSR2.Classes.ClassDeclaration.InterfaceSameLine)
 310 | ERROR | [x] Only one interface may be specified per line in a multi-line implements declaration
     |       |     (PSR2.Classes.ClassDeclaration.InterfaceSameLine)
 316 | ERROR | [x] Only one interface may be specified per line in a multi-line implements declaration
     |       |     (PSR2.Classes.ClassDeclaration.InterfaceSameLine)

The fix proposed in this PR changes the logic of the sniff to take comments between interface names into account in the following manner:

• If there is a comment before the interface name and the comment is on the same line, move the interface name to a new line via the InterfaceSameLine error.
• However, if there is a comment before the interface name and the comment is on not on the same line, we know that the interface name is already on its own line, so don't trigger the InterfaceSameLine error. Other potential errors (like indentation) will continue to be checked for.

With these changes, the code sample in the test case file now yields the following errors:

 310 | ERROR | [x] Expected 4 spaces before interface name; 0 found (PSR2.Classes.ClassDeclaration.InterfaceWrongIndent)
 316 | ERROR | [x] Only one interface may be specified per line in a multi-line implements declaration
     |       |     (PSR2.Classes.ClassDeclaration.InterfaceSameLine)

This effectively fixes the fixer conflict.

---

Fixer Conflict details

        * fixed 0 violations, starting loop 48 *
        => Changeset started by PSR2.Classes.ClassDeclaration:362
                Q: PSR2.Classes.ClassDeclaration:368 replaced token 1078 (T_WHITESPACE on line 289) "\n\" => "\"
                Q: PSR2.Classes.ClassDeclaration:371 replaced token 1077 (T_PHPCS_DISABLE on line 288) "// phpcs:disable Stnd.Cat.Sniff -- For reasons.\n" => "// phpcs:disable Stnd.Cat.Sniff -- For reasons.\n\n"
                A: PSR2.Classes.ClassDeclaration:372 replaced token 1078 (T_WHITESPACE on line 289) "\n\" => "\"
                A: PSR2.Classes.ClassDeclaration:372 replaced token 1077 (T_PHPCS_DISABLE on line 288) "// phpcs:disable Stnd.Cat.Sniff -- For reasons.\n" => "// phpcs:disable Stnd.Cat.Sniff -- For reasons.\n\n"
        => Changeset ended: 2 changes applied
        => Changeset started by PSR2.Classes.ClassDeclaration:362
                Q: PSR2.Classes.ClassDeclaration:368 replaced token 1099 (T_WHITESPACE on line 297) "\nAnotherInterface" => "AnotherInterface"
                Q: PSR2.Classes.ClassDeclaration:371 replaced token 1098 (T_COMMENT on line 296) "// Comment.\n" => "// Comment.\n\n"
                A: PSR2.Classes.ClassDeclaration:372 replaced token 1099 (T_WHITESPACE on line 297) "\nAnotherInterface" => "AnotherInterface"
                A: PSR2.Classes.ClassDeclaration:372 replaced token 1098 (T_COMMENT on line 296) "// Comment.\n" => "// Comment.\n\n"
        => Changeset ended: 2 changes applied
        => Fixing file: 4/53 violations remaining [made 48 passes]...
        * fixed 4 violations, starting loop 49 *
        => Changeset started by PSR2.Classes.ClassDeclaration:362
                Q: PSR2.Classes.ClassDeclaration:368 replaced token 1078 (T_WHITESPACE on line 289) "\n\" => "\"
                Q: PSR2.Classes.ClassDeclaration:371 replaced token 1077 (T_PHPCS_DISABLE on line 288) "// phpcs:disable Stnd.Cat.Sniff -- For reasons.\n" => "// phpcs:disable Stnd.Cat.Sniff -- For reasons.\n\n"
                **** PSR2.Classes.ClassDeclaration:372 has possible conflict with another sniff on loop 47; caused by the following change ****
                **** replaced token 1078 (T_WHITESPACE on line 289) "\n\" => "\" ****
                **** ignoring all changes until next loop ****
        => Changeset failed to apply
        => Fixing file: 0/53 violations remaining [made 49 passes]...

---

Suggested changelog entry

PSR2.Classes.ClassDeclaration will no longer try to auto-fix multi-line interface implements statements if these are interlaced with comments on their own line. This prevents a potential fixer conflict.

Related issues/external references

Related to #152
Related to #425

Types of changes

• Bug fix (non-breaking change which fixes an issue)

[fredden]

I've spent quite some time understanding the changes here and the associated tests. The changes in this pull request make sense.

Given we have #425, I'll hold my thoughts on some of the related-but-not-directly-in-scope-here points. For example: having 10 empty lines after a line-comment (// like this) in a multi-line implements list is an acceptable thing according to this sniff.

[jrfnl]

I've spent quite some time understanding the changes here and the associated tests.

@fredden Thank you for doing that!

For example: having 10 empty lines after a line-comment (// like this) in a multi-line implements list is an acceptable thing according to this sniff.

Well, there are two considerations to take into account regarding this:

1. Whether or not empty lines are allowed after an inline comment (and how many), is something which IMO is outside the scope of this sniff and should be determined by an inline comment sniff, like the Squiz.Commenting.InlineComment sniff, as comments have their own rules.
   When the PSR2.Classes.ClassDeclaration sniff + the Squiz.Commenting.InlineComment sniff are combined in one (custom) ruleset, with the code as per this PR, the empty lines will be removed (while previously this would have led to a fixer conflict which would have prevented the file from being fixed at all).
2. The second thing is that it should be verified that the sniff is actually in line with PSR-2 (including the errata).
   What I mean by that is as follows:
   Given the following code sample (multiple blank lines between interface names, no comments):

   class ClassName extends ParentClass implements
       \Foo\Bar\Countable,
   
   
   
       \Serializable
   {}

   The sniff would currently throw a Only one interface may be specified per line in a multi-line implements declaration (PSR2.Classes.ClassDeclaration.InterfaceSameLine) error.
   And the fixer would remove the blank lines.
   However, PSR-2 states "Lists of implements MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one interface per line.".
   Given the above, there are two things to question about the sniff:
   1. If each interface name is already on its own line, should the sniff enforce "no blank lines between interface name lines" ? I don't see anything in PSR-2 which says it should, so this looks like something where the sniffs goes beyond its remit.
   2. If it would be decided that it should (possibly because this has been clarified on the PSR mailinglist or in the PSR-2 errata, or even because not doing so would change the expected behaviour of the sniff), is it correct that this is handled by the InterfaceSameLine error code ? Or should this be a separate error code, something like NoBlankLinesBetweenInterfaceNames ?
      If I would have ever come across a multi-line implements like the above and would then have seen the Only one interface may be specified per line in a multi-line implements declaration error, I would have reported this as a bug in the sniff as the sniff is reporting something which is just not true (as each interface is already on its own line).

Was that what you were thinking off too ? (And yes, we should probably move this to #425 😉 )

Either way, this PR fixes a specific bug and only that. Fixing other things is outside the scope of this PR, but is definitely something which should be looked into via #425 and I would not be surprised if this will lead to another half dozen PRs with additional fixes.

[jrfnl]

FYI: for reference I have reposted my above comment in #425 (comment) to make sure it is not lost once this PR has been closed.

[jrfnl]

Rebased without changes. Merging once the build passes.