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

Prove regression from #1893 #1899

Merged
merged 9 commits into from
Feb 18, 2018
11 changes: 9 additions & 2 deletions src/Standards/PSR2/Sniffs/Namespaces/UseDeclarationSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// 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']) {
$end = $nextComment;
}

if ($end === false) {
return;
}

$next = $phpcsFile->findNext(Tokens::$emptyTokens, ($end + 1), null, true);
$next = $phpcsFile->findNext(T_WHITESPACE, ($end + 1), null, true);

if ($next === false || $tokens[$next]['code'] === T_CLOSE_TAG) {
return;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php
use Foo\Bar; // trailing comment
// This should fail.

class Baz
{

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php
use Foo\Bar; // trailing comment

// This should fail.

class Baz
{

}
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
<?php
use Foo\Bar; //comment
use Foo\Bar;

// This should pass.
class Baz
{

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php
use Foo\Bar; // trailing comment

// This should pass.
class Baz
{

}
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ public function getErrorList($testFile='')
18 => 1,
19 => 1,
];
case 'UseDeclarationUnitTest.10.inc':
return [2 => 1];
default:
return [];
}//end switch
Expand Down