Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic.Formatting.MultipleStatementAlignment false positive for match() #3862

Open
3 tasks done
LastDragon-ru opened this issue Jul 31, 2023 · 2 comments
Open
3 tasks done

Comments

@LastDragon-ru
Copy link

LastDragon-ru commented Jul 31, 2023

Describe the bug

Generic.Formatting.MultipleStatementAlignment doesn't work correctly with match().

Code sample

<?php declare(strict_types = 1);

namespace Bug;

class A {
    public function bug(bool $b): void {
        $a   = match ($b) {
            true    => 1,
            default => false,
        };
        $var = 123;

        echo $a.$var;
    }
}

Custom ruleset

<?xml version="1.0"?>
<ruleset name="Bug"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:noNamespaceSchemaLocation="./vendor/squizlabs/php_codesniffer/phpcs.xsd">
    <file>./packages</file>
    <rule ref="Generic.Formatting.MultipleStatementAlignment"/>
</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 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------
 7 | WARNING | [x] Equals sign not aligned correctly; expected 1 space but found 3 spaces
------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------

Expected behavior

No error.

Versions (please complete the following information)

Operating System Win 10
PHP version 8.2
PHP_CodeSniffer version 3.7.2
Standard custom
Install type Composer (global/local)

Please confirm:

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

DannyvdSluijs commented Sep 4, 2023

Seeing the error and the code example I think PHPCS is making the correct suggestion (I get exactly the same error). From the docs (./bin/phpcs --standard=Generic --generator=Text) you can read:

-----------------------------------------------------------
| GENERIC CODING STANDARD: ALIGNING BLOCKS OF ASSIGNMENTS |
-----------------------------------------------------------

There should be one space on either side of an equals sign used to assign a value to a variable. In
the case of a block of related assignments, more space may be inserted to promote readability.

----------------------------------------- CODE COMPARISON ------------------------------------------
| Equals signs aligned                           | Not aligned; harder to read                     |
----------------------------------------------------------------------------------------------------
| $shortVar        = (1 + 2);                    | $shortVar = (1 + 2);                            |
| $veryLongVarName = 'string';                   | $veryLongVarName = 'string';                    |
| $var             = foo($bar, $baz);            | $var = foo($bar, $baz);                         |
----------------------------------------------------------------------------------------------------

In the example $a and $var don't belong to the same "block of related assignments", or at least is my assumption (and my assumption is wrong as pointed out below by jrfnl)

This was checked just now using the main branch (on commit b6264f9).

@jrfnl
Copy link
Contributor

jrfnl commented Sep 4, 2023

In the example $a and $var don't belong to the same "block of related assignments", or at least is my assumption

That's actually not true. They do belong to the same block of assignments, multi-line argument or not.
A block of assignments is only broken by a blank line a comment or another type of non-assignment statement.

While opinions may vary whether that is correct, the sniff should at least be consistent and for consistency, the above should be seen as a bug.


Expanded code sample showing other multiline assignments
<?php declare(strict_types = 1);

namespace Bug;

class A {
    public function bug(bool $b): void {
        $a   = match ($b) {
            true    => 1,
            default => false,
        };
        $var = 123;
        $a   = match ($b) {
            true    => 1,
            default => false,
        };

        $a   = function ($b) {
            return $b;
        };
        $var = 123;
        $a   = function ($b) {
            return $b;
        };

        $a   = 'multi-'
            . 'line';
        $var = 123;
        $a   = 'multi-'
            . 'line';

        echo $a.$var;
    }
}

Result:

----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------
 7 | WARNING | [x] Equals sign not aligned correctly; expected 1 space but found 3 spaces
   |         |     (Generic.Formatting.MultipleStatementAlignment.IncorrectWarning)
----------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------

In other words: the first match on line 7 is treated inconsistently compared to other multi-line assignments, which all expect alignment.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants