Lines with just spaces are being prepended to the next line

pento commented 6 years ago

Given the following test case (note the \t on the blank line between $foo and $bar):

<?php

$foo = 1;

$bar = 2;

The \t is being prepended to the $bar line.

This isn't a core bug, just something I noticed while testing. 🙂

Tested with PHPCS master, WPCS develop.

jrfnl commented 6 years ago

Hi @pento I've tried to reproduce the effect, but no luck. When I run the fixer against this snippet, I see the tab (correctly) being trimmed by the Generic.WhiteSpace.SuperfluousWhiteSpace sniff.

Got any more info I can use to try and reproduce this ?

pento commented 6 years ago

Not sure what I'm doing wrong, my testing setup is a copy of https://develop.svn.wordpress.org/trunk, this phpcs.xml.dist, and a composer.json like so:

{
    "require-dev": {
        "dealerdirect/phpcodesniffer-composer-installer": "*",
        "wp-coding-standards/wpcs": "dev-develop",
        "squizlabs/php_codesniffer": "dev-master"
    }
}

Running vendor/bin/phpcbf foo.php with foo.php containing the test case, I get the incorrect result.

jrfnl commented 6 years ago

My local dev setup is of course slightly different. I've got a phpcs.xml.dist which is a little ahead of what you're using, but nothing significant for this issue. I've just uploaded it to the ticket.

Other than that I have a branch in both PHPCS as well as WPCS which I keep up to date with the latest patches submitted related to the Core project.

These are the current versions (remind me to push/update them if they seem out of date):

For WPCS, this is currently the same as the #1193 branch. https://github.com/jrfnl/PHP_CodeSniffer/tree/WP-Core/test-run
For PHPCS, there are two open PRs merged into that branch - https://github.com/squizlabs/PHP_CodeSniffer/pull/1599, https://github.com/squizlabs/PHP_CodeSniffer/pull/1645. Additionally, https://github.com/squizlabs/PHP_CodeSniffer/pull/1691 would be a nice to have as well. https://github.com/jrfnl/WordPress-Coding-Standards/tree/WPCore-test-run

None of these should explain the difference though....

jrfnl commented 6 years ago

@pento Just thinking - could you run phpcbf over the test file with -vv and post the bottom bit of the output you receive ? Or even the complete output ? That should help me figure out what is going on.

pento commented 6 years ago

Soooo..... doing that made me realise it wasn't really a \t, it was actually four spaces. 🙂

Here's the `phpcbf` output

``` Processing foo.php *** START PHP TOKENIZING *** Process token [0]: T_OPEN_TAG => \n Process token [2]: T_VARIABLE => $foo Process token [3]: T_WHITESPACE => · Process token 4 : T_EQUAL => = Process token [5]: T_WHITESPACE => · Process token [6]: T_LNUMBER => 1 Process token 7 : T_SEMICOLON => ; Process token [8]: T_WHITESPACE => \n\t\n Process token [9]: T_VARIABLE => $bar Process token [10]: T_WHITESPACE => · Process token 11 : T_EQUAL => = Process token [12]: T_WHITESPACE => · Process token [13]: T_LNUMBER => 2 Process token 14 : T_SEMICOLON => ; Process token [15]: T_WHITESPACE => \n *** END PHP TOKENIZING *** *** START TOKEN MAP *** *** END TOKEN MAP *** *** START SCOPE MAP *** *** END SCOPE MAP *** *** START LEVEL MAP *** Process token 0 on line 1 [col:1;len:5;lvl:0;]: T_OPEN_TAG => \n Process token 2 on line 3 [col:1;len:4;lvl:0;]: T_VARIABLE => $foo Process token 3 on line 3 [col:5;len:1;lvl:0;]: T_WHITESPACE => · Process token 4 on line 3 [col:6;len:1;lvl:0;]: T_EQUAL => = Process token 5 on line 3 [col:7;len:1;lvl:0;]: T_WHITESPACE => · Process token 6 on line 3 [col:8;len:1;lvl:0;]: T_LNUMBER => 1 Process token 7 on line 3 [col:9;len:1;lvl:0;]: T_SEMICOLON => ; Process token 8 on line 3 [col:10;len:0;lvl:0;]: T_WHITESPACE => \n Process token 9 on line 4 [col:1;len:4;lvl:0;]: T_WHITESPACE => ····\n Process token 10 on line 5 [col:1;len:4;lvl:0;]: T_VARIABLE => $bar Process token 11 on line 5 [col:5;len:1;lvl:0;]: T_WHITESPACE => · Process token 12 on line 5 [col:6;len:1;lvl:0;]: T_EQUAL => = Process token 13 on line 5 [col:7;len:1;lvl:0;]: T_WHITESPACE => · Process token 14 on line 5 [col:8;len:1;lvl:0;]: T_LNUMBER => 2 Process token 15 on line 5 [col:9;len:1;lvl:0;]: T_SEMICOLON => ; Process token 16 on line 5 [col:10;len:0;lvl:0;]: T_WHITESPACE => \n *** END LEVEL MAP *** *** START ADDITIONAL PHP PROCESSING *** *** END ADDITIONAL PHP PROCESSING *** [PHP => 17 tokens in 5 lines]... DONE in 24ms (1 fixable violations) => Fixing file: 1/1 violations remaining PHP_CodeSniffer\Standards\Squiz\Sniffs\WhiteSpace\SuperfluousWhitespaceSniff (line 205) replaced token 9 (T_WHITESPACE) "\t\n$bar" => "\n$bar" => Fixing file: 1/1 violations remaining [made 1 pass]... * fixed 1 violations, starting loop 2 * => Fixing file: 0/1 violations remaining [made 2 passes]... DONE in 5ms => File was overwritten ```

jrfnl commented 6 years ago

Ah, now we're getting somewhere. I can reproduce the effect when the whitespace is spaces instead of a tab.

Looks like this is a bug in the upstream Generic.WhiteSpace.DisallowSpaceIndent sniff. I already have a PR open for that sniff upstream. I'll have a look to see if this can be fixed in the same PR or warrants a second one.

jrfnl commented 6 years ago

I've opened a separate PR upstream specifically to address this issue: https://github.com/squizlabs/PHP_CodeSniffer/pull/1702

jrfnl commented 6 years ago

A fix for this has been merged upstream and was included in the PHPCS 3.1.1 release.

WordPress / WordPress-Coding-Standards

Lines with just spaces are being prepended to the next line #1192