squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.66k stars 1.48k forks source link

Presence of a goto statement or brace blocks mess the indentation #3571

Open Geolim4 opened 2 years ago

Geolim4 commented 2 years ago

Describe the bug PHPCS keeps wrongly indent my code when a goto or an additional set of brackets is added.

https://github.com/PHPSocialNetwork/phpfastcache/commit/37f3ca4ed592858648517371994875adb9eae33c

Code sample

<?php
{
    {
        if(true)
        {
            {
                $someCode = null;
            }
        }
    }
}

Reformatted code

<?php
{
    {
if (true) {
    {
        $someCode = null;
    }
}
    }
}

Custom ruleset

<?xml version="1.0"?>
<ruleset name="Phpfastcache Standards">
    <description>The Phpfastcache Coding Standards</description>

    <arg name="colors"/>
    <arg value="p"/>
    <ini name="memory_limit" value="256M"/>
    <rule ref="PSR2"/>

    <exclude-pattern>tests/*</exclude-pattern>
    <exclude-pattern>bin/*</exclude-pattern>
    <exclude-pattern>docs/*</exclude-pattern>
    <exclude-pattern>vendor/*</exclude-pattern>

    <rule ref="Generic.Files.LineLength">
        <properties>
            <property name="lineLimit" value="170"/>
            <property name="absoluteLineLimit" value="170"/>
        </properties>
    </rule>
</ruleset>

To reproduce Check out the CI build: https://app.travis-ci.com/github/PHPSocialNetwork/phpfastcache/jobs/565171795#L1053

Expected behavior No error and no wrong reindentation

Versions (please complete the following information):

Additional context image image

sybrew commented 2 years ago

Another example, where a message is displayed at // Generic.WhiteSpace.ScopeIndent.IncorrectExact

<?php

gotolabel: {
    this_is_fine();
    if ( false_positive() ) { // Generic.WhiteSpace.ScopeIndent.IncorrectExact
        this_is_fine();
} else {
    false_negative();
}

    $b = this_is_fine();
}

Desired code (with no warning of this sort):

<?php

gotolabel: {
    this_is_fine();
    if ( this_is_fine() ) { // Generic.WhiteSpace.ScopeIndent.IncorrectExact
        this_is_fine();
    } else { // Generic.WhiteSpace.ScopeIndent.IncorrectExact
        this_is_fine();
    } // Generic.WhiteSpace.ScopeIndent.IncorrectExact

    $b = this_is_fine();
}
Geolim4 commented 2 years ago

Hello @gsherwood can this lexer bug be fixed please ? 😢

It's an major flaw blocking builds :(

gsherwood commented 2 years ago

The ScopeIndent sniff doesn't see unnecessary braces as scopes for indenting. Just adding braces around your code when they aren't owned by any control structure is not supported by this sniff.

Changing that is likely going to be pretty difficult to get right due to the amount of new test cases to try and come up with and the fact that it would need to be optional. Unless it's something impacting a lot of developers, it's not something I can prioritise.

Geolim4 commented 2 years ago

Hello,

Thank you for your reply. It's also impacting go-to statements as well as namespace braces, not only non-statement braces 😅

On Tue, 28 Jun 2022, 12:36 am Greg Sherwood, @.***> wrote:

The ScopeIndent sniff doesn't see unnecessary braces as scopes for indenting. Just adding braces around your code when they aren't owned by any control structure is not supported by this sniff.

Changing that is likely going to be pretty difficult to get right due to the amount of new test cases to try and come up with and the fact that it would need to be optional. Unless it's something impacting a lot of developers, it's not something I can prioritise.

— Reply to this email directly, view it on GitHub https://github.com/squizlabs/PHP_CodeSniffer/issues/3571#issuecomment-1167995172, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKFGZ73EXGSG45LCLBIXALVRIUGPANCNFSM5SACJBKQ . You are receiving this because you authored the thread.Message ID: @.***>

gsherwood commented 2 years ago

Thank you for your reply. It's also impacting go-to statements as well as namespace braces, not only non-statement braces

Goto statements are not block structures and do not use braces. The braces in your example are purely decorative.

I do not believe there is any issue with indentation of namespace blocks. Namespace blocks can have braces in PHP and PHPCS assigns them correctly. The ScopeIndent sniff has tests for namespace blocks with braces, so any issues specifically with these is something I'd look at.

sybrew commented 2 years ago

Whoop, my mistake on the terminology. I use scope-indents in combination with goto handles so I can see what's where once collapsed without adding comment lines.

In procedural programming, rather than conventional objective or functional programming, these scope-indents are lifesavers. Procedural programming is becoming more of a thing because modern IDEs/code-editors allow collapsing scopes. I don't believe SOLID has a long future once more people catch on.

An example is visible on line 101 below (skips to 113). Line 115 won't allow this because there are no braces.

image

Detecting scope-indents, whether preceded with a goto handle or not, would be tremendously helpful in these scenarios. If you welcome PRs, I'd love to help get the ball rolling.

Geolim4 commented 2 years ago

Detecting scope-indents, whether preceded with a goto handle or not, would be tremendously helpful in these scenarios. If you welcome PRs, I'd love to help get the ball rolling.

I agree, a scope-indent sometimes allows (aside goto statements) for better code clarity in complex scenarios. This change would be welcomed by lot of developers including me obviously.