Squiz.Commenting.InlineComment will fail to fix comments at the end of the file

[jrfnl]

I came across this issue when running phpcbf with the complete WordPress standard against the WPCS unit test files in an attempt to find fixer conflicts.

When an inline comment is the last non-whitespace content in a file, the sniff will report an error, but will always fail to fix it as the value of $next (formely on line 295) would be false.
The file would get the "FAILED TO FIX" status as there is still one "fixable" error remaining, even though the file is correctly overwritten by the fixed file.

As whether or not there should be a blank line at the end of a file is another matter and is covered by the Generic.Files.EndFileNewline / Generic.Files.EndFileNoNewLine sniffs, this specific case should IMHO not be handled by this sniff anyway.

The fix I'm proposing is to exit out of the sniff if the inline comment being examined is the last non-whitespace token in the file.

Includes unit tests demonstrating the issue.

[jrfnl]

@gsherwood Could this small fix please be tagged for the 3.1.1 release ? 🙏

[gsherwood]

I don't get a conflict between the InlineComment sniff and the EndFileNewline sniffs. I do get two errors that can't both be fixed, but the fact that $next returns false during fixing stops any fixes from being applied. The result should actually be properly checked so the error is not marked as fixable, but the file still gets fixed correctly.

Here is a sample file:

<?php
// For this test line having an empty line below it, is fine.

And a test command:

$ phpcs temp.php --standard=Squiz,Generic --sniffs=Squiz.Commenting.InlineComment,Generic.Files.EndFileNewline --report=diff
--- temp.php
+++ PHP_CodeSniffer
@@ -1,2 +1,2 @@
 <?php
-// For this test line having an empty line below it, is fine.
\ No newline at end of file
+// For this test line having an empty line below it, is fine.

The fixer for InlineComment does stuff like this:

E: [Line 2] There must be no blank line following an inline comment (Squiz.Commenting.InlineComment.SpacingAfter)
	=> Changeset started by PHP_CodeSniffer\Standards\Squiz\Sniffs\Commenting\InlineCommentSniff (line 296)
	=> Changeset ended: 0 changes applied

Are you sure it is this specific combination of sniffs causing your problem?

[jrfnl]

The result should actually be properly checked so the error is not marked as fixable, but the file still gets fixed correctly.

True, the file gets fixed correctly, but the status it gets is: "Failed to fix" as after fixing there's one (un-)fixable error left.

The fix in the branch makes sure that the error is ignored completely if it's the last non-whitespace token in the file.

Are you sure it is this specific combination of sniffs causing your problem?

Yes, I'm sure. It's not even a combination of sniffs, it's just this single sniff causing the issue.

To reproduce:
Take just the adjusted unit test case file from this branch and run:

phpcbf -p -s ./path/to/InlineCommentUnitTest.inc --standard=Squiz --sniffs=Squiz.Commenting.InlineComment

The output will be:

PHPCBF RESULT SUMMARY
------------------------------------------------------------------------------------------
FILE                                                                      FIXED  REMAINING
------------------------------------------------------------------------------------------
...niffer\src\Standards\Squiz\Tests\Commenting\InlineCommentUnitTest.inc  FAILED TO FIX
------------------------------------------------------------------------------------------
A TOTAL OF 13 ERRORS WERE FIXED IN 1 FILE
------------------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 1 FILE
------------------------------------------------------------------------------------------

With the fix in this branch applied, the output will be:

PHPCBF RESULT SUMMARY
------------------------------------------------------------------------------------------
FILE                                                                      FIXED  REMAINING
------------------------------------------------------------------------------------------
...niffer\src\Standards\Squiz\Tests\Commenting\InlineCommentUnitTest.inc  13     2
------------------------------------------------------------------------------------------
A TOTAL OF 13 ERRORS WERE FIXED IN 1 FILE
------------------------------------------------------------------------------------------

[jrfnl]

Just had another look at the fixed file and I'm surprised that line 44 (original file) / line 40 (fixed file) does not give an error anymore, so will have a look at that as well.

Same goes for line 97 / 91 and line 103 / 96.

[jrfnl]

Ok, I've added two more commits:

1. Adding a fixed file for the js test cases.
2. Fixing the issue I mention in my previous comment where errors about capitalization and invalid comment end characters where not being thrown because the start and end of a comment block was being identified incorrectly. Includes unit tests demonstrating the issue.

And yes, those two should probably have their own PRs, but as they would conflict with this PR it made sense to add them here. Let me know if you want me to split the PR anyhow.

[gsherwood]

Sorry, I misunderstood the original issue, but was able to replicate it. Thanks a lot for the fix.

[jrfnl]

Thanks @gsherwood !

You may want to also add a changelog entry for the secondary fix I made to the sniff (third commit)

If two subsequent lines contained comments with code between them, they would be seen as part of the same comment block, while in reality there are not.

This meant that errors for NotCapital and InvalidEndChar were not always being thrown when they should be.

[gsherwood]

You may want to also add a changelog entry for the secondary fix

Good idea :) Thanks.

[jrfnl]

@gsherwood Now that this fix is in, the road is clear to add checking for fixer conflicts to the Travis builds.
I'm preparing a PR for this for the WordPress Coding Standards.

Would you be interested in me pulling a similar PR here ?

It would do a basic test for each standard (excluding Generic) to find fixer conflicts by running the fixer over the test files of the standards twice. The second time the exit code should be 0, allowing the build to pass. If it's not, that indicates a fixer conflict.

For efficiency, I'd suggest only running this check once per build using an environment variable.
To be fair, the same could be done for the code style check as the unit tests already check that the sniffs function correctly across versions, so the code style check for PHPCS itself should not need to be run in each individual build.

What do you think ?

[gsherwood]

Would you be interested in me pulling a similar PR here ?

I think that would be really interesting and would love to see it. So... yes please :)

For efficiency, I'd suggest only running this check once per build using an environment variable.
To be fair, the same could be done for the code style check as the unit tests already check that the sniffs function correctly across versions, so the code style check for PHPCS itself should not need to be run in each individual build.

The reason I run PHPCS over itself on all environments is basically as an integration test for each PHP version - not just to confirm I am meeting the coding standards. I'd probably do the same thing for these new integration tests for checking the standards themselves unless they are really slow.

I should probably be doing a PEAR install and checking that as well...

[jrfnl]

I think that would be really interesting and would love to see it. So... yes please :)

I'll prepare a PR for you to pursue.

The reason I run PHPCS over itself on all environments is basically as an integration test for each PHP version - not just to confirm I am meeting the coding standards. I'd probably do the same thing for these new integration tests for checking the standards themselves unless they are really slow.

Fair enough. As these checks will have to run for each standard separately, it will add significantly to the build time if these are run on every build.

I should probably be doing a PEAR install and checking that as well...

For that matter, I can also include a (one build only as that really is sufficient) couple of commands which validate the XML rulesets and check the code style of them.

[jrfnl]

Hmm... an initial run is already throwing up quite some issues... both fixer conflicts as well as hard bugs in sniffs (undefined index errors and more).

Those will all need to be fixed before I can pull this PR, so that will take a little while before we are through that.

Other findings:

• The MySource standard can not be tested this way as it includes an exclude-pattern which explicitely excludes tests directories.
• PSR1 can be tested, but doesn't contain any fixable sniffs.

I'm doing a run now with --ignore-annotations to see if the results get any better.

You can see the results of the various builds here: https://travis-ci.org/jrfnl/PHP_CodeSniffer/branches - branch: feature/build-check-for-fixer-conflicts