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

Closed
Daimona opened this issue Apr 19, 2023 · 1 comment · Fixed by PHPCSStandards/PHP_CodeSniffer#647

Comments

@Daimona
Copy link
Contributor

Daimona commented Apr 19, 2023

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):

  • OS: Ubuntu 22.04.2
  • PHP: 8.2.4
  • PHPCS: 3.7.2
  • Standard: Custom
@DannyvdSluijs
Copy link

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 b0ecdf1)

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

Successfully merging a pull request may close this issue.

3 participants