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

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

Open LastDragon-ru opened 1 year ago

LastDragon-ru commented 1 year ago

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:

DannyvdSluijs commented 11 months ago

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 b6264f9f9).

jrfnl commented 11 months ago

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 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.