From 007374344925cdb3c30ead5355897c1589f9ff1a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 8 Sep 2017 00:07:22 +0200 Subject: [PATCH 1/3] Squiz/InlineComment: solve fixer conflict 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. --- .../Squiz/Sniffs/Commenting/InlineCommentSniff.php | 7 ++++++- .../Squiz/Tests/Commenting/InlineCommentUnitTest.inc | 9 +++++++++ .../Tests/Commenting/InlineCommentUnitTest.inc.fixed | 9 +++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/Standards/Squiz/Sniffs/Commenting/InlineCommentSniff.php b/src/Standards/Squiz/Sniffs/Commenting/InlineCommentSniff.php index 752ba69845..f8d0f4fd52 100644 --- a/src/Standards/Squiz/Sniffs/Commenting/InlineCommentSniff.php +++ b/src/Standards/Squiz/Sniffs/Commenting/InlineCommentSniff.php @@ -278,6 +278,12 @@ public function process(File $phpcsFile, $stackPtr) // Finally, the line below the last comment cannot be empty if this inline // comment is on a line by itself. if ($tokens[$previousContent]['line'] < $tokens[$stackPtr]['line']) { + $next = $phpcsFile->findNext(T_WHITESPACE, ($stackPtr + 1), null, true); + if ($next === false) { + // Ignore if the comment is the last non-whitespace token in a file. + return; + } + $start = false; for ($i = ($stackPtr + 1); $i < $phpcsFile->numTokens; $i++) { if ($tokens[$i]['line'] === ($tokens[$stackPtr]['line'] + 1)) { @@ -292,7 +298,6 @@ public function process(File $phpcsFile, $stackPtr) $error = 'There must be no blank line following an inline comment'; $fix = $phpcsFile->addFixableError($error, $stackPtr, 'SpacingAfter'); if ($fix === true) { - $next = $phpcsFile->findNext(T_WHITESPACE, ($stackPtr + 1), null, true); $phpcsFile->fixer->beginChangeset(); for ($i = ($stackPtr + 1); $i < $next; $i++) { if ($tokens[$i]['line'] === $tokens[$next]['line']) { diff --git a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc index 33316a8826..91167bea58 100644 --- a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc +++ b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc @@ -122,3 +122,12 @@ echo $foo; * Comments about the include */ include_once($blah); + +/* + * N.B.: The below test line must be the last test in the file. + * Testing that a new line after an inline comment when it's the last non-whitespace + * token in a file, does *not* throw an error as this would conflict with the common + * "new line required at end of file" rule. + */ + +// For this test line having an empty line below it, is fine. diff --git a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc.fixed index b23d3f62ed..4caffc43b8 100644 --- a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc.fixed +++ b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc.fixed @@ -115,3 +115,12 @@ echo $foo; * Comments about the include */ include_once($blah); + +/* + * N.B.: The below test line must be the last test in the file. + * Testing that a new line after an inline comment when it's the last non-whitespace + * token in a file, does *not* throw an error as this would conflict with the common + * "new line required at end of file" rule. + */ + +// For this test line having an empty line below it, is fine. From ae276991a683619da2ddd773d9f435fa8e15d218 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 25 Sep 2017 09:21:54 +0200 Subject: [PATCH 2/3] Squiz/InlineComment: Add fixed file for JS test cases. --- package.xml | 1 + .../Commenting/InlineCommentUnitTest.js.fixed | 119 ++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100644 src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js.fixed diff --git a/package.xml b/package.xml index 1dcabfb2b0..19680b8a63 100644 --- a/package.xml +++ b/package.xml @@ -1219,6 +1219,7 @@ http://pear.php.net/dtd/package-2.0.xsd"> + diff --git a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js.fixed b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js.fixed new file mode 100644 index 0000000000..aeeb2e77c4 --- /dev/null +++ b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js.fixed @@ -0,0 +1,119 @@ +// Some content here. +var code = 'hello'; + +// This comment contains # multiple +// hash signs (#). +code = 'hello'; + +/** + * This is the first line of a function comment. + * This is the second line. + */ +function testFunction() +{ + // Callback methods which are added by external objects. + this.callbacks = {}; + +}//end testFunction() + +/** + * This is the first line of a class comment. + * This is the second line. + */ +myClass.prototype = { + + /** + * This is the first line of a method comment. + * This is the second line. + */ + load: function(url, callback) + { + // Some code here. + } +}; + +// some code goes here! +/* + A longer comment goes here. + It spans multiple lines!! + Or does it? +*/ + +// 0This is a simple multi-line +// comment! +code = 'hello'; + +// This is not valid. +code = 'hello'; + +// Neither is this! +code = 'hello'; + +code = 'hello'; + +/** Neither is this! **/ +code = 'hello'; + +/** + * This is the first line of a function comment. + * This is the second line. + */ +var myFunction = function() { +} + +/** + * This is the first line of a function comment. + * This is the second line. + */ +myFunction = function() { +} + +/** + * This is the first line of a function comment. + * This is the second line. + */ +myClass.myFunction = function() { +} + +dfx.getIframeDocument = function(iframe) +{ + return doc; + +};//end dfx.getIframeDocument() + +mig.Gallery.prototype = { + + init: function(cb) + { + + },//end init() + + imageClicked: function(id) + { + + }//end imageClicked() + +}; + +// Here is some inline example code: +// -> One +// -> One.One +// -> Two +/* + Here is some inline example code: + -> One + -> One.One + -> Two +*/ + + +var foo = 'foo'; // Var set to foo. + +console.info(foo); + +// Comment here. +console.info(foo); + +// ** +* invalid comment +*/ From 97bbb197dd3bcfb30fb117dc0acf50c1a23e5bbb Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 25 Sep 2017 09:55:14 +0200 Subject: [PATCH 3/3] Squiz/InlineComment: Fix two more issues where errors are incorrectly not being thrown 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` where not always being thrown when they should be. Includes additional unit tests demonstrating the issue. --- .../Sniffs/Commenting/InlineCommentSniff.php | 16 ++++++++++++---- .../Tests/Commenting/InlineCommentUnitTest.inc | 6 ++++++ .../Commenting/InlineCommentUnitTest.inc.fixed | 6 ++++++ .../Tests/Commenting/InlineCommentUnitTest.js | 6 ++++++ .../Commenting/InlineCommentUnitTest.js.fixed | 6 ++++++ .../Tests/Commenting/InlineCommentUnitTest.php | 4 ++++ 6 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Commenting/InlineCommentSniff.php b/src/Standards/Squiz/Sniffs/Commenting/InlineCommentSniff.php index f8d0f4fd52..55f5e14584 100644 --- a/src/Standards/Squiz/Sniffs/Commenting/InlineCommentSniff.php +++ b/src/Standards/Squiz/Sniffs/Commenting/InlineCommentSniff.php @@ -211,11 +211,14 @@ public function process(File $phpcsFile, $stackPtr) // The below section determines if a comment block is correctly capitalised, // and ends in a full-stop. It will find the last comment in a block, and // work its way up. - $nextComment = $phpcsFile->findNext(array(T_COMMENT), ($stackPtr + 1), null, false); - if (($nextComment !== false) - && (($tokens[$nextComment]['line']) === ($tokens[$stackPtr]['line'] + 1)) + $nextComment = $phpcsFile->findNext(T_COMMENT, ($stackPtr + 1), null, false); + if ($nextComment !== false + && $tokens[$nextComment]['line'] === ($tokens[$stackPtr]['line'] + 1) ) { - return; + $nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($stackPtr + 1), $nextComment, true); + if ($nextNonWhitespace === false) { + return; + } } $topComment = $stackPtr; @@ -225,6 +228,11 @@ public function process(File $phpcsFile, $stackPtr) break; } + $nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($topComment + 1), $lastComment, true); + if ($nextNonWhitespace !== false) { + break; + } + $lastComment = $topComment; } diff --git a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc index 91167bea58..7942042a07 100644 --- a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc +++ b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc @@ -123,6 +123,12 @@ echo $foo; */ include_once($blah); +// some comment without capital or full stop +echo $foo; // An unrelated comment. + +// An unrelated comment. +echo $foo; // some comment without capital or full stop + /* * N.B.: The below test line must be the last test in the file. * Testing that a new line after an inline comment when it's the last non-whitespace diff --git a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc.fixed index 4caffc43b8..156d343e0f 100644 --- a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc.fixed +++ b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.inc.fixed @@ -116,6 +116,12 @@ echo $foo; */ include_once($blah); +// some comment without capital or full stop +echo $foo; // An unrelated comment. + +// An unrelated comment. +echo $foo; // some comment without capital or full stop + /* * N.B.: The below test line must be the last test in the file. * Testing that a new line after an inline comment when it's the last non-whitespace diff --git a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js index 7d8bd15d31..6b1093e6ac 100644 --- a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js +++ b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js @@ -121,3 +121,9 @@ console.info(foo); //** * invalid comment */ + +// some comment without capital or full stop +console.log(foo); // An unrelated comment. + +// An unrelated comment. +console.log(foo); // some comment without capital or full stop diff --git a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js.fixed b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js.fixed index aeeb2e77c4..20e5041e68 100644 --- a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js.fixed +++ b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.js.fixed @@ -117,3 +117,9 @@ console.info(foo); // ** * invalid comment */ + +// some comment without capital or full stop +console.log(foo); // An unrelated comment. + +// An unrelated comment. +console.log(foo); // some comment without capital or full stop diff --git a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.php b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.php index 07d6365f90..d821b2e5ef 100644 --- a/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.php +++ b/src/Standards/Squiz/Tests/Commenting/InlineCommentUnitTest.php @@ -44,6 +44,8 @@ public function getErrorList($testFile='InlineCommentUnitTest.inc') 96 => 1, 97 => 3, 118 => 1, + 126 => 2, + 130 => 2, ); return $errors; @@ -60,6 +62,8 @@ public function getErrorList($testFile='InlineCommentUnitTest.inc') 104 => 3, 118 => 1, 121 => 1, + 125 => 2, + 129 => 2, ); default: return array();