A combination of sniffs and their fixers produces syntactically invalid code

[morozov]

Below are the relevant dependency versions:

$ composer show # truncated

doctrine/coding-standard       7.0.2   The Doctrine Coding Standard is a ...
slevomat/coding-standard       6.1.4   Slevomat Coding Standard for PHP_C...
squizlabs/php_codesniffer      3.5.4   PHP_CodeSniffer tokenizes PHP, Jav...

Consider the following code example:

<?php

declare(strict_types=1);

class Problem
{
    public function test() : int
    {
        if (true) { // Comment
            return 1;
        } else if (true) { // Comment
            return 2;
        } else { // Comment
            return 3;
        }
    }
}

All coding standard violations are reported as fixable and seem to be non-conflicting:

------------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
------------------------------------------------------------------------------------------------
 10 | ERROR | [x] Expected 1 lines before "return", found 0.
    |       |     (SlevomatCodingStandard.ControlStructures.JumpStatementsSpacing.IncorrectLinesCountBeforeControlStructure)
 11 | ERROR | [x] Usage of ELSE IF is discouraged; use ELSEIF instead
    |       |     (PSR2.ControlStructures.ElseIfDeclaration.NotAllowed)
 12 | ERROR | [x] Expected 1 lines before "return", found 0.
    |       |     (SlevomatCodingStandard.ControlStructures.JumpStatementsSpacing.IncorrectLinesCountBeforeControlStructure)
 13 | ERROR | [x] Remove useless else to reduce code nesting.
    |       |     (SlevomatCodingStandard.ControlStructures.EarlyExit.UselessElse)
 14 | ERROR | [x] Expected 1 lines before "return", found 0.
    |       |     (SlevomatCodingStandard.ControlStructures.JumpStatementsSpacing.IncorrectLinesCountBeforeControlStructure)
------------------------------------------------------------------------------------------------

An attempt to fix this file using phpcs fails:

----------------------------------------------------------------------
FILE                                                  FIXED  REMAINING
----------------------------------------------------------------------
Problem.php                                           FAILED TO FIX
----------------------------------------------------------------------
A TOTAL OF 1 ERROR WERE FIXED IN 1 FILE
----------------------------------------------------------------------
PHPCBF FAILED TO FIX 1 FILE
----------------------------------------------------------------------

and produces the following code:

<?php

declare(strict_types=1);

class Problem
{
    public function test() : int
    {
        if (true) { // Comment
            return 1;
        }

        if (true) { // Comment
            return 2;
        }
    } else { // Comment


        return 3;
    }
}
}

$ php -l Problem.php

Parse error: syntax error, unexpected 'else' (T_ELSE), expecting function (T_FUNCTION) or const (T_CONST) in Problem.php on line 16
Errors parsing Problem.php

Replacing else if with elseif solves the syntax error issue but still doesn’t help the auto-fixing the violations.

[kukulich]

It should be fixed in bc3d492

But it only works with elseif.

[morozov]

@kukulich although the fixed sniff no longer corrupts the file, it doesn't fix it either. Even with else if replaced with elseif:

cat > composer.json << 'EOF'
{
    "require-dev": {
        "doctrine/coding-standard": "^7.0.2",
        "slevomat/coding-standard": "dev-master as 6.1.5"
    }
}
EOF

cat > phpcs.xml.dist << 'EOF'
<?xml version="1.0"?>
<ruleset>
    <rule ref="Doctrine"/>
</ruleset>
EOF

cat > Problem.php << 'EOF'
<?php

declare(strict_types=1);

class Problem
{
    public function test() : int
    {
        if (true) { // Comment
            return 1;
        } elseif (true) { // Comment
            return 2;
        } else { // Comment
            return 3;
        }
    }
}
EOF

composer install

vendor/bin/phpcbf --version
# PHP_CodeSniffer version 3.5.4 (stable) by Squiz (http://www.squiz.net)

composer show | grep slevomat
# slevomat/coding-standard dev-master a741c09 Slevomat Coding Standar...

vendor/bin/phpcbf Problem.php

# ----------------------------------------------------------------------
# FILE                                                  FIXED  REMAINING
# ----------------------------------------------------------------------
# /tmp/phpcs/Problem.php                                FAILED TO FIX
# ----------------------------------------------------------------------
# A TOTAL OF 3 ERRORS WERE FIXED IN 1 FILE
# ----------------------------------------------------------------------
# PHPCBF FAILED TO FIX 1 FILE
# ----------------------------------------------------------------------

cat Problem.php
# <?php
# 
# declare(strict_types=1);
# 
# class Problem
# {
#     public function test() : int
#     {
#         if (true) { // Comment
#             return 1;
#         } elseif (true) { // Comment
#             return 2;
#         } else { // Comment
#             return 3;
#         }
#     }
# }

[kukulich]

Strange, I used the code in my tests: bc3d492#diff-d6aa3d124fe7d65433e3980b6b6fb0bfR456

bc3d492#diff-b68d5ff564633e1b09b18146ab6377b8R522

But I use tabs. Maybe I should try spaces in the test.

[morozov]

Maybe I should try spaces in the test.

It looks like that. In #881 (comment), the inline comment extracted from the else block is improperly aligned using spaces but is properly aligned in the test that uses tabs.

[kukulich]

Improved in a05d520

The bug was in something else :)

[morozov]

Thanks for the update. I just tried fixing the code example from #890 (comment) using dev-master a72bd4b and the Doctrine Coding Standard, and PHPCBF still fails to fix the file.

I see that there’s a similar file fixed in the test added by a05d520. Is there potentially a conflict with other sniffs?

[kukulich]

It can be. Can you try to enable only EarlyExitSniff?

[morozov]

phpcbf --sniffs=SlevomatCodingStandard.ControlStructures.EarlyExit Problem.php

Yes, this way it works.

[kukulich]

JumpStatementsSpacing can conflict with it I think if you use it too.

[morozov]

phpcbf --exclude=SlevomatCodingStandard.ControlStructures.JumpStatementsSpacing Problem.php

You’re right. The above command was able to fix the file. Also applying the JumpStatementsSpacing sniff individually afterward worked too.

Is it something that could be fixed in a separate issue? Or it’s by design?

In any event, this issue seems to be solved. Thanks a lot!

[kukulich]

I've just tried one small change in master. If it does not work I will give up. PHPCS fixer works in 50 loops. I think it should remove else in first loop and modify elseif and add new line in second loop so 50 loops should be enough.

[morozov]

No, still cannot fix it (I tried d767b5e). I’ve experienced some more other conflicts when fixing the codebase, so I think I'll do some broader analysis of the failures first and then document them. Then we could decide if those conflicts are fixable or it’s by design and what the recommended approach to resolve them is.

Thanks again!

[kukulich]

Ok, closing for now :)

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.