Prove regression from #1893 #1899

dhensby · 2018-02-15T12:44:38Z

The test that was merged with #1893 didn't actually test that lines after a user statement weren't blank as PHPCS would determine this to be the end of the file due to lack of class declaration or other valid code lines.

…ent" This reverts commit f147caf.

dhensby · 2018-02-15T14:23:48Z

Right, I've pushed a fix that allows a trailing comment on the same line as a use statement.

I'd have thought this should be a config option, but as #1893 was merged without a new config I've assumed it's intended that trailing comments are allowed.

iammattcoleman · 2018-02-15T15:34:58Z

src/Standards/PSR2/Sniffs/Namespaces/UseDeclarationSniff.php

@@ -137,12 +137,19 @@ public function process(File $phpcsFile, $stackPtr)
            return;
        }

-        $end = $phpcsFile->findNext(T_SEMICOLON, ($stackPtr + 1));
+        $end         = $phpcsFile->findNext(T_SEMICOLON, ($stackPtr + 1));
+        $nextComment = $phpcsFile->findNext(T_COMMENT, ($end + 1));


Should this be Tokens::$emptyTokens to allow for phpcs:ignore, etc.?

Side note: What we are checking for here is the next inline comment, if that's on the same line as the use declaration then that can be classed as the "end" of the use declaration instead of the semicolon.

As I see it: Allowing for any "empty token" would mean a /* could be allowed (which it shouldn't be IMO) and other such tokens that shouldn't or couldn't be there.

I'm happy to be told that it should use Tokens::$emptyTokens I'm just of the opinion that the only thing that can come next should be an inline comment.

3.2.0 changed how overrides are annotated, which added several tokens for comments containing phpcs: overrides: https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.2.0

My PR changed T_COMMENT to Tokens::$emptyTokens in order to allow the T_PHPCS_* tokens. That also allows T_DOC_COMMENT_*, which seemed safe but isn't 100% correct in hindsight. I think this should be an array with T_COMMENT and T_PHPCS_*.

@jrfnl Does that sound right to you?

I'll have a look at this PR later today.

dhensby · 2018-02-15T18:08:35Z

@iammattcoleman i've quickly tried to add Tokens::$emptyTokens and a while loop to get this working but I'm now getting other failures:

PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

...............................................................  63 / 294 ( 21%)
............................................................... 126 / 294 ( 42%)
............................................................... 189 / 294 ( 64%)
.F.............................................SS.............. 252 / 294 ( 85%)
.......................................S..                      294 / 294 (100%)

Tests generated 618 unique error codes; 350 were fixable (56.63%)

Time: 27.52 seconds, Memory: 38.00MB

There was 1 failure:

1) PHP_CodeSniffer\Standards\PSR2\Tests\Namespaces\UseDeclarationUnitTest::testSniff
[LINE 12] Expected 0 error(s) in UseDeclarationUnitTest.1.inc but found 1 error(s). The error(s) found were:
 -> There must be one blank line after the last USE statement; 0 found; (PSR2.Namespaces.UseDeclaration.SpaceAfterLastUse)
[LINE 2] Expected 1 error(s) in UseDeclarationUnitTest.10.inc but found 0 error(s).
[LINE 10] Expected 2 error(s) in UseDeclarationUnitTest.2.inc but found 1 error(s). The error(s) found were:
 -> USE declarations must go after the first namespace declaration (PSR2.Namespaces.UseDeclaration.UseAfterNamespace)
Fixed version of UseDeclarationUnitTest.2.inc does not match expected version in UseDeclarationUnitTest.2.inc.fixed; the diff is
--- src/Standards/PSR2/Tests/Namespaces/UseDeclarationUnitTest.2.inc.fixed
+++ PHP_CodeSniffer
@@ -10,6 +10,7 @@

 use ArrayObject;

+
 $foo = 'bar';

 ?>
Fixed version of UseDeclarationUnitTest.3.inc does not match expected version in UseDeclarationUnitTest.3.inc.fixed; the diff is
--- src/Standards/PSR2/Tests/Namespaces/UseDeclarationUnitTest.3.inc.fixed
+++ PHP_CodeSniffer
@@ -3,8 +3,9 @@

 use someNS\A;
 use someNS\B;
+class

-class Bug
+Bug
 {
     public function __construct()
     {
[LINE 2] Expected 0 error(s) in UseDeclarationUnitTest.4.inc but found 1 error(s). The error(s) found were:
 -> There must be one blank line after the last USE statement; 0 found; (PSR2.Namespaces.UseDeclaration.SpaceAfterLastUse)
[LINE 19] Expected 1 error(s) in UseDeclarationUnitTest.5.inc but found 2 error(s). The error(s) found were:
 -> There must be one USE keyword per declaration (PSR2.Namespaces.UseDeclaration.MultipleDeclarations)
 -> There must be one blank line after the last USE statement; 0 found; (PSR2.Namespaces.UseDeclaration.SpaceAfterLastUse)
Fixed version of UseDeclarationUnitTest.5.inc does not match expected version in UseDeclarationUnitTest.5.inc.fixed; the diff is
--- src/Standards/PSR2/Tests/Namespaces/UseDeclarationUnitTest.5.inc.fixed
+++ PHP_CodeSniffer
@@ -27,6 +27,7 @@
 use function foo\math\cos;
 use function foo\math\cosh;

+
 class PHP7 {

 }
[LINE 2] Expected 0 error(s) in UseDeclarationUnitTest.8.inc but found 1 error(s). The error(s) found were:
 -> There must be one blank line after the last USE statement; 0 found; (PSR2.Namespaces.UseDeclaration.SpaceAfterLastUse)
[LINE 2] Expected 0 error(s) in UseDeclarationUnitTest.9.inc but found 1 error(s). The error(s) found were:
 -> There must be one blank line after the last USE statement; 0 found; (PSR2.Namespaces.UseDeclaration.SpaceAfterLastUse)

/vagrant/www/vendor/squizlabs/php_codesniffer/tests/Standards/AbstractSniffUnitTest.php:205
/vagrant/www/vendor/squizlabs/php_codesniffer/tests/TestSuite.php:28

FAILURES!
Tests: 294, Assertions: 302, Failures: 1, Skipped: 3.

This is the diff I tried:

diff --git a/src/Standards/PSR2/Sniffs/Namespaces/UseDeclarationSniff.php b/src/Standards/PSR2/Sniffs/Namespaces/UseDecindex 4448a08..390629b 100644
--- a/src/Standards/PSR2/Sniffs/Namespaces/UseDeclarationSniff.php
+++ b/src/Standards/PSR2/Sniffs/Namespaces/UseDeclarationSniff.php
@@ -138,13 +138,16 @@ class UseDeclarationSniff implements Sniff
         }

         $end         = $phpcsFile->findNext(T_SEMICOLON, ($stackPtr + 1));
-        $nextComment = $phpcsFile->findNext(T_COMMENT, ($end + 1));
+        $nextComment = $end;

-        // The end of the line is allowed to be a comment as well as a semi colon.
-        if ($nextComment !== false && $tokens[$nextComment]['line'] === $tokens[$end]['line']) {
+        do {
+            $nextComment = $phpcsFile->findNext(Tokens::$emptyTokens, ($nextComment + 1));
+        } while ($nextComment !== false && $tokens[$nextComment]['line'] === $tokens[$end]['line']);
+
+        if ($nextComment !== false) {
             $end = $nextComment;
         }
-
+
         if ($end === false) {
             return;
         }

dhensby · 2018-02-15T18:10:42Z

Gah, I see my problem but don't have time to fix right now. We need the $nextComment from the previous iteration

dhensby · 2018-02-15T20:02:35Z

OK - I've pushed in another commit that uses Tokens::$emptyTokens and passes the tests. Maintainers have push rights to the branch so can reset that commit out if it's decided it's not needed.

iammattcoleman · 2018-02-15T20:15:32Z

@gsherwood @jrfnl This should probably be included in 3.2.3.

dhensby · 2018-02-15T20:17:29Z

@gsherwood @jrfnl This should probably be included in 3.2.3.

Yep, unless you want 3.2.3 to be broken :)

iammattcoleman · 2018-02-15T23:44:29Z

@gsherwood I'm helping with the update to PHP 7.2 for Ubuntu 18.04. As of now, PHP_CodeSniffer 3.2.2 is slated to be included. I've let them know that there are some important fixes coming soon, but I felt you should know that they're currently packaging 3.2.2 along with its known issues.

jrfnl · 2018-02-16T06:09:10Z

As per @iammattcoleman @dhensby request, I've had a look at this PR. I believe the logic can be simplified slightly by just checking for either next statement or the start of the next line, whichever comes first and not focusing completely on "next comment".

I've just send in a PR to @dhensby's repo with an optional suggestion for amending this PR.
See dhensby#1

jrfnl · 2018-02-17T21:35:13Z

Since the original downstream PR to amend this PR, @iammattcoleman and me have identified some additional issues and the downstream PR has been updated to allow for problems in the fixer when encountering various combinations of trailing comments after use statements.
So my downstream PR is no longer optional.

... covering different variations of comments after the last use statement.

…us combinations of comments

PSR2/UseDeclaration: simplify the fix

dhensby · 2018-02-18T00:29:17Z

I've merged the downstream PR - all looks good - thanks for the thorough investigation

jrfnl · 2018-02-18T01:47:00Z

@dhensby Thanks for working with me to get this sorted.

dhensby · 2018-02-18T17:00:47Z

@jrfnl no problem! Thanks for the help :)

gsherwood · 2018-02-18T23:38:52Z

@dhensby Thanks for finding and fixing this before release. Very much appreciated. Thanks to those who reviewed as well.

dhensby added 2 commits February 15, 2018 12:42

Prove regression from squizlabs#1893

b2ce88b

Revert "PSR2/UseDeclaration: allow comments after the last USE statem…

9d81b3c

…ent" This reverts commit f147caf.

dhensby force-pushed the pulls/test-is-bs branch 2 times, most recently from 8723383 to 9577ba8 Compare February 15, 2018 14:32

FIX squizlabs#1891 Where inline comment fails UseDeclaration sniff

0690ce0

dhensby force-pushed the pulls/test-is-bs branch from 9577ba8 to 0690ce0 Compare February 15, 2018 14:39

iammattcoleman reviewed Feb 15, 2018

View reviewed changes

dhensby force-pushed the pulls/test-is-bs branch from 0690ce0 to 50dacfd Compare February 15, 2018 20:00

Use Tokens::$emptyTokens to determine end of use statement

1f1e919

dhensby force-pushed the pulls/test-is-bs branch from 50dacfd to 1f1e919 Compare February 15, 2018 20:01

andrewandante mentioned this pull request Feb 16, 2018

SS4 Compatability silverstripe/silverstripe-staticpublishqueue#70

Merged

15 tasks

jrfnl mentioned this pull request Feb 16, 2018

Sniff/isClassProperty: add unit tests covering multi-property definitions PHPCompatibility/PHPCompatibility#583

Merged

jrfnl and others added 5 commits February 18, 2018 00:33

PSR2/UseDeclaration: simplify the fix

f3843e5

PSR2/UseDeclaration: add one more additional unit test

0142d75

PSR2/UseDeclaration: add yet more additional unit tests

bd96102

... covering different variations of comments after the last use statement.

PSR2/UseDeclaration: allow the fixer to function correctly with vario…

1530543

…us combinations of comments

Merge pull request #1 from jrfnl/feature/test-is-bs-simplification

5d58565

PSR2/UseDeclaration: simplify the fix

jrfnl approved these changes Feb 18, 2018

View reviewed changes

iammattcoleman approved these changes Feb 18, 2018

View reviewed changes

gsherwood added this to the 3.2.3 milestone Feb 18, 2018

gsherwood merged commit 5d58565 into squizlabs:master Feb 18, 2018

gsherwood added a commit that referenced this pull request Feb 18, 2018

Changelog for #1898 + #1899 (ref #1893 + #1891)

47cdda1

gsherwood mentioned this pull request Feb 18, 2018

PHPCS erroneously identifying docblock as blank lines #1898

Closed

dhensby deleted the pulls/test-is-bs branch February 19, 2018 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prove regression from #1893 #1899

Prove regression from #1893 #1899

dhensby commented Feb 15, 2018

dhensby commented Feb 15, 2018 •

edited

Loading

iammattcoleman Feb 15, 2018

dhensby Feb 15, 2018

iammattcoleman Feb 15, 2018

jrfnl Feb 15, 2018

dhensby commented Feb 15, 2018 •

edited

Loading

dhensby commented Feb 15, 2018 •

edited

Loading

dhensby commented Feb 15, 2018

iammattcoleman commented Feb 15, 2018

dhensby commented Feb 15, 2018

iammattcoleman commented Feb 15, 2018 •

edited

Loading

jrfnl commented Feb 16, 2018

jrfnl commented Feb 17, 2018

dhensby commented Feb 18, 2018

jrfnl commented Feb 18, 2018

dhensby commented Feb 18, 2018

gsherwood commented Feb 18, 2018

Prove regression from #1893 #1899

Prove regression from #1893 #1899

Conversation

dhensby commented Feb 15, 2018

dhensby commented Feb 15, 2018 • edited Loading

iammattcoleman Feb 15, 2018

Choose a reason for hiding this comment

dhensby Feb 15, 2018

Choose a reason for hiding this comment

iammattcoleman Feb 15, 2018

Choose a reason for hiding this comment

jrfnl Feb 15, 2018

Choose a reason for hiding this comment

dhensby commented Feb 15, 2018 • edited Loading

dhensby commented Feb 15, 2018 • edited Loading

dhensby commented Feb 15, 2018

iammattcoleman commented Feb 15, 2018

dhensby commented Feb 15, 2018

iammattcoleman commented Feb 15, 2018 • edited Loading

jrfnl commented Feb 16, 2018

jrfnl commented Feb 17, 2018

dhensby commented Feb 18, 2018

jrfnl commented Feb 18, 2018

dhensby commented Feb 18, 2018

gsherwood commented Feb 18, 2018

dhensby commented Feb 15, 2018 •

edited

Loading

dhensby commented Feb 15, 2018 •

edited

Loading

dhensby commented Feb 15, 2018 •

edited

Loading

iammattcoleman commented Feb 15, 2018 •

edited

Loading