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.WhiteSpace.ScopeIndent false positive with multiline `yield from` #3808

Open Daimona opened 1 year ago

Daimona commented 1 year ago

Describe the bug Generic.WhiteSpace.ScopeIndent reports multiline yield from statements as being incorrectly indented even if the indentation is correct. I've only tested this with tab indentation.

Code sample

<?php

function test() {
    yield
        from [ 3, 4 ];
}

Custom ruleset

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
    <arg name="tab-width" value="4" />
    <rule ref="Generic.WhiteSpace.ScopeIndent">
        <properties>
            <property name="tabIndent" value="true" />
        </properties>
    </rule>
</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 1 ERROR AFFECTING 1 LINE
    --------------------------------------------------------------------------------------------------------------------------------
    5 | ERROR | [x] Line indented incorrectly; expected at least 1 tabs, found 2 spaces
    |       |     (Generic.WhiteSpace.ScopeIndent.Incorrect)
    --------------------------------------------------------------------------------------------------------------------------------
    PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
    --------------------------------------------------------------------------------------------------------------------------------

On top of that, if you try to autofix the file it will enter an "endless" loop and not do anything:

$ vendor/bin/phpcbf -vvv test.php

...
=> Fixing file: 1/1 violations remaining
---START FILE CONTENT---
1|<?php
2|
3|function test() {
4|  yield
5|      from [ 3, 4 ];
6|}
7|
8|
--- END FILE CONTENT ---
    Generic.WhiteSpace.ScopeIndent:1537 replaced token 12 (T_YIELD_FROM on line 5) "\t\tfrom" => "\tfrom"
        => Fixing file: 1/1 violations remaining [made 1 pass]...               
    * fixed 1 violations, starting loop 2 *
---START FILE CONTENT---
1|<?php
2|
3|function test() {
4|  yield
5|  from [ 3, 4 ];
6|}
7|
8|
--- END FILE CONTENT ---
    Generic.WhiteSpace.ScopeIndent:1537 replaced token 12 (T_YIELD_FROM on line 5) "\tfrom" => "\tfrom"
        => Fixing file: 1/1 violations remaining [made 2 passes]...             
    * fixed 1 violations, starting loop 3 *
---START FILE CONTENT---
1|<?php
2|
3|function test() {
4|  yield
5|  from [ 3, 4 ];
6|}
7|
8|
--- END FILE CONTENT ---
    **** Generic.WhiteSpace.ScopeIndent:1537 has possible conflict with another sniff on loop 1; caused by the following change ****
    **** replaced token 12 (T_YIELD_FROM on line 5) "\tfrom" => "\tfrom" ****
    **** ignoring all changes until next loop ****
        => Fixing file: 0/1 violations remaining [made 3 passes]...             
    * fixed 0 violations, starting loop 4 *
---START FILE CONTENT---
1|<?php
2|
3|function test() {
4|  yield
5|  from [ 3, 4 ];
6|}
7|
8|
--- END FILE CONTENT ---
...

Expected behavior PHPCS should not report any error. Whether separating yield and from is a good idea is beyond the scope of this bug report. However, there's nothing wrong in the code above and the error message is also confusing ("found 2 spaces").

Versions (please complete the following information):

DannyvdSluijs commented 1 year ago

It seems the root cause of this problem might be in splitting the yield and from on separate lines. Checking the dump of the $tokens variable shows the following (part of it):

  [11] =>
  array(8) {
    'type' =>
    string(12) "T_YIELD_FROM"
    'code' =>
    int(282)
    'content' =>
    string(6) "yield
"
    'line' =>
    int(4)
    'column' =>
    int(5)
    'length' =>
    int(5)
    'level' =>
    int(1)
    'conditions' =>
    array(1) {
      [2] =>
      int(310)
    }
  }
  [12] =>
  array(8) {
    'type' =>
    string(12) "T_YIELD_FROM"
    'code' =>
    int(282)
    'content' =>
    string(6) "         from"
    'line' =>
    int(5)
    'column' =>
    int(1)
    'length' =>
    int(6)
    'level' =>
    int(1)
    'conditions' =>
    array(1) {
      [2] =>
      int(310)
    }
  }

Where you can see that both the yield and the from are considered to be the same type of token (T_YIELD_FROM) and that the newline and tabs leading up to it are not parse as individual tokens (T_WHITESPACE) making for the false positive. Also checking the PHP.net documentation on tokens you can see on the bottom row of the table that yield from is a single token.

These observations where done using PHP 8.2.9 and PHPCS on the master branch (commit b0ecdf10f)