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.ControlStructures.InlineControlStructure fixer moves new PHPCS annotations #1932

Closed
jrfnl opened this issue Mar 12, 2018 · 4 comments
Milestone

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Mar 12, 2018

This is one of the last few sniffs in which I know there are still issues with the PHPCS annotations.

In this case, the fixer moves trailing PHPCS annotations down to subsequent lines which changes their meaning completely.

Example code for the unit tests:

while (!$this->readLine($tokens, $tag)); //phpcs:ignore Standard.Category.Sniff

foreach ($stringParade as $hit)
    $hitParade[] = $hit + 0; // phpcs:disable Standard.Category.Sniff

if ($bar)
    if ($foo) echo 'hi'; /* @phpcs:ignore Standard.Category.Sniff */

Expected fix:

while (!$this->readLine($tokens, $tag)) {} //phpcs:ignore Standard.Category.Sniff
}
foreach ($stringParade as $hit) {
    $hitParade[] = $hit + 0; // phpcs:disable Standard.Category.Sniff
}

if ($bar) {
    if ($foo) { echo 'hi'; /* @phpcs:ignore Standard.Category.Sniff */
    }
}

Fix currently being made by the sniff as it is now:

while (!$this->readLine($tokens, $tag)) {} //phpcs:ignore Standard.Category.Sniff

foreach ($stringParade as $hit) {
    $hitParade[] = $hit + 0;
} // phpcs:disable Standard.Category.Sniff

if ($bar) {
    if ($foo) { echo 'hi';
    }
} /* @phpcs:ignore Standard.Category.Sniff */

Changing conditions which look for T_COMMENT, to ones which also take Tokens::$phpcsCommentTokens into account, does not really help all that much (the first one is worse, the second better, the third still just as bad):

while (!$this->readLine($tokens, $tag)) { 
//phpcs:ignore Standard.Category.Sniff
}
foreach ($stringParade as $hit) {
    $hitParade[] = $hit + 0; // phpcs:disable Standard.Category.Sniff
}

if ($bar) {
    if ($foo) { echo 'hi';
    }
} /* @phpcs:ignore Standard.Category.Sniff */

I've stared at this one way too long by now and haven't figured out a solution yet.

I'm pretty sure the issue is somewhere in these lines:

if ($next === false || $tokens[$next]['line'] !== $tokens[$end]['line']) {
// Looks for completely empty statements.
$next = $phpcsFile->findNext(T_WHITESPACE, ($closer + 1), ($end + 1), true);
// Account for a comment on the end of the line.
for ($endLine = $end; $endLine < $phpcsFile->numTokens; $endLine++) {
if (isset($tokens[($endLine + 1)]) === false
|| $tokens[$endLine]['line'] !== $tokens[($endLine + 1)]['line']
) {
break;
}
}
if ($tokens[$endLine]['code'] !== T_COMMENT) {
$endLine = $end;
}
} else {
$next = ($end + 1);
$endLine = $end;
}

@gsherwood
Copy link
Member

gsherwood commented Mar 23, 2018

I have some hacked up code that may produce better results. But I'm about to rush out, so I
I've thrown the contents of the sniff here in case my machine blows up, or you want to take a look: https://gist.github.com/gsherwood/191464ec17838d3a53bb13bdd3f93dfa

Note: this is a mess, but it took a while to figure out how the fixer was even working here.

@gsherwood gsherwood changed the title Generic/InlineControlStructure: fixer moves new PHPCS annotations Generic.ControlStructures.InlineControlStructure fixer moves new PHPCS annotations Mar 29, 2018
@gsherwood gsherwood added this to the 3.3.0 milestone Mar 29, 2018
gsherwood added a commit that referenced this issue Mar 29, 2018
@gsherwood
Copy link
Member

I've had a shot at a complete fix. It's not 100% consistent but I don't really want to work on this sniff any more - it's taken too many hours already. It is better though, but could use more testing if you are able to help.

Your sample code block is now fixed like this:

while (!$this->readLine($tokens, $tag)) {} //phpcs:ignore Standard.Category.Sniff

foreach ($stringParade as $hit) {
    $hitParade[] = $hit + 0; // phpcs:disable Standard.Category.Sniff
}
if ($bar) {
    if ($foo) { echo 'hi'; /* @phpcs:ignore Standard.Category.Sniff */
    }
}

@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 29, 2018

@gsherwood You're a star! I'll try and have a look at this over the weekend, but have total confidence in your fix ;-)

@gsherwood
Copy link
Member

Closing this as I haven't had any additional problems reported.

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

No branches or pull requests

2 participants