From 779bbe1a2fb3c4799f6b017535ae7345a244a5cb Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 31 Mar 2024 16:00:02 +0200 Subject: [PATCH 1/2] Squiz/OperatorSpacing: move parse error test to own test case file --- .../Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc | 3 --- .../Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc.fixed | 3 --- .../Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.3.inc | 6 ++++++ 3 files changed, 6 insertions(+), 6 deletions(-) create mode 100644 src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.3.inc diff --git a/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc index 765e7ab717..157260b995 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc @@ -482,6 +482,3 @@ match ($a) { 'b', 'c', 'd' => -2, default => -3, }; - -/* Intentional parse error. This has to be the last test in the file. */ -$a = 10 + diff --git a/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc.fixed index ada77fa859..6f8ef985b2 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc.fixed +++ b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc.fixed @@ -476,6 +476,3 @@ match ($a) { 'b', 'c', 'd' => -2, default => -3, }; - -/* Intentional parse error. This has to be the last test in the file. */ -$a = 10 + diff --git a/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.3.inc b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.3.inc new file mode 100644 index 0000000000..8d6cf19e4c --- /dev/null +++ b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.3.inc @@ -0,0 +1,6 @@ + Date: Sun, 31 Mar 2024 16:32:24 +0200 Subject: [PATCH 2/2] Squiz/OperatorSpacing: bug fix - prevent fixer conflict Another one in the fixer conflict series. When running the `Squiz` standard over all test case files, a fixer conflict in the `Squiz.WhiteSpace.OperatorSpacing` sniff was discovered via the tests in the `./src/Standards/Generic/Tests/CodeAnalysis/RequireExplicitBooleanOperatorPrecedenceUnitTest.inc` file for a code sample like this: ```php $foo = $var // Comment. ? false // Comment. : true; ``` The conflict essentially comes down to the `Squiz.WhiteSpace.OperatorSpacing` trying to remove the new line before the `?` and the `:` operator, but failing to do so as the new line is included in the comment token on the previous line and the sniff only adjusts/removes whitespace tokens. Original errors for the code sample added in the test case file: ``` 487 | ERROR | [x] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) 488 | ERROR | [x] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) 493 | ERROR | [x] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) 494 | ERROR | [x] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) 499 | ERROR | [x] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) 504 | ERROR | [x] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) ``` The fix proposed in this PR changes the sniff to make the `SpacingBefore` error code non-fixable if the previous non-whitespace token is a comment token - even when the comment token doesn't contain a new line. While the sniff _could_ auto-fix when the comment token doesn't contain a new line, I have chosen to disable the auto-fixer for those cases anyway as it is not for the sniff to determine whether the comment should be moved, removed or should stay where it is. With these changes, the code sample in the test case file now yields the following errors: ``` 487 | ERROR | [x] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) 488 | ERROR | [x] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) 493 | ERROR | [ ] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) 494 | ERROR | [ ] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) 499 | ERROR | [ ] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) 504 | ERROR | [ ] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore) ``` This effectively fixes the fixer conflict. :point_right: The diff will be easier to review while ignoring whitespace changes. ---
Fixer Conflict details ``` * fixed 0 violations, starting loop 48 * => Changeset started by Squiz.WhiteSpace.OperatorSpacing:258 Q: Squiz.WhiteSpace.OperatorSpacing:267 replaced token 911 (T_WHITESPACE on line 72) " ?" => " ?" A: Squiz.WhiteSpace.OperatorSpacing:268 replaced token 911 (T_WHITESPACE on line 72) " ?" => " ?" => Changeset ended: 1 changes applied => Changeset started by Squiz.WhiteSpace.OperatorSpacing:258 Q: Squiz.WhiteSpace.OperatorSpacing:267 replaced token 925 (T_WHITESPACE on line 73) " :" => " :" A: Squiz.WhiteSpace.OperatorSpacing:268 replaced token 925 (T_WHITESPACE on line 73) " :" => " :" => Changeset ended: 1 changes applied => Fixing file: 2/2 violations remaining [made 48 passes]... * fixed 2 violations, starting loop 49 * => Changeset started by Squiz.WhiteSpace.OperatorSpacing:258 Q: Squiz.WhiteSpace.OperatorSpacing:267 replaced token 911 (T_WHITESPACE on line 72) " ?" => " ?" **** Squiz.WhiteSpace.OperatorSpacing:268 has possible conflict with another sniff on loop 47; caused by the following change **** **** replaced token 911 (T_WHITESPACE on line 72) " ?" => " ?" **** **** ignoring all changes until next loop **** => Changeset failed to apply => Fixing file: 0/2 violations remaining [made 49 passes]... ```
--- Related to 152 --- .../WhiteSpace/OperatorSpacingSniff.php | 36 ++++++++++++------- .../WhiteSpace/OperatorSpacingUnitTest.1.inc | 26 ++++++++++++++ .../OperatorSpacingUnitTest.1.inc.fixed | 24 +++++++++++++ .../WhiteSpace/OperatorSpacingUnitTest.php | 6 ++++ 4 files changed, 79 insertions(+), 13 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/WhiteSpace/OperatorSpacingSniff.php b/src/Standards/Squiz/Sniffs/WhiteSpace/OperatorSpacingSniff.php index 97007fc458..2a87978a0c 100644 --- a/src/Standards/Squiz/Sniffs/WhiteSpace/OperatorSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/WhiteSpace/OperatorSpacingSniff.php @@ -238,7 +238,8 @@ public function process(File $phpcsFile, $stackPtr) ) { // Throw an error for assignments only if enabled using the sniff property // because other standards allow multiple spaces to align assignments. - if ($tokens[($stackPtr - 2)]['line'] !== $tokens[$stackPtr]['line']) { + $prevNonWhitespace = $phpcsFile->findPrevious(T_WHITESPACE, ($stackPtr - 1), null, true); + if ($tokens[$prevNonWhitespace]['line'] !== $tokens[$stackPtr]['line']) { $found = 'newline'; } else { $found = $tokens[($stackPtr - 1)]['length']; @@ -253,20 +254,29 @@ public function process(File $phpcsFile, $stackPtr) $operator, $found, ]; - $fix = $phpcsFile->addFixableError($error, $stackPtr, 'SpacingBefore', $data); - if ($fix === true) { - $phpcsFile->fixer->beginChangeset(); - if ($found === 'newline') { - $i = ($stackPtr - 2); - while ($tokens[$i]['code'] === T_WHITESPACE) { - $phpcsFile->fixer->replaceToken($i, ''); - $i--; + + if (isset(Tokens::$commentTokens[$tokens[$prevNonWhitespace]['code']]) === true) { + // Throw a non-fixable error if the token on the previous line is a comment token, + // as in that case it's not for the sniff to decide where the comment should be moved to + // and it would get us into unfixable situations as the new line char is included + // in the contents of the comment token. + $phpcsFile->addError($error, $stackPtr, 'SpacingBefore', $data); + } else { + $fix = $phpcsFile->addFixableError($error, $stackPtr, 'SpacingBefore', $data); + if ($fix === true) { + $phpcsFile->fixer->beginChangeset(); + if ($found === 'newline') { + $i = ($stackPtr - 2); + while ($tokens[$i]['code'] === T_WHITESPACE) { + $phpcsFile->fixer->replaceToken($i, ''); + $i--; + } } - } - $phpcsFile->fixer->replaceToken(($stackPtr - 1), ' '); - $phpcsFile->fixer->endChangeset(); - } + $phpcsFile->fixer->replaceToken(($stackPtr - 1), ' '); + $phpcsFile->fixer->endChangeset(); + } + }//end if }//end if }//end if diff --git a/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc index 157260b995..29acf308a6 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc @@ -482,3 +482,29 @@ match ($a) { 'b', 'c', 'd' => -2, default => -3, }; + +$foo = $var + ? 10 + : true; + +// Safeguard that a non-fixable error is thrown when there is a new line before the operator, +// but the last non-whitespace token before the operator is a comment token. +$foo = $var // Comment + ? false /* Comment */ + : true; + +$foo = $var // phpcs: ignore Stnd.Cat.Sniff -- for reasons. + + + ? $something /** + * Don't ask, but someone might have a docblock between the lines. It's valid PHP after all. + */ + + + : true; + +// phpcs:set Squiz.WhiteSpace.OperatorSpacing ignoreNewlines true +$foo = $var // Comment + ? false // Comment + : true; +// phpcs:set Squiz.WhiteSpace.OperatorSpacing ignoreNewlines false diff --git a/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc.fixed index 6f8ef985b2..5c94e3658b 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc.fixed +++ b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc.fixed @@ -476,3 +476,27 @@ match ($a) { 'b', 'c', 'd' => -2, default => -3, }; + +$foo = $var ? 10 : true; + +// Safeguard that a non-fixable error is thrown when there is a new line before the operator, +// but the last non-whitespace token before the operator is a comment token. +$foo = $var // Comment + ? false /* Comment */ + : true; + +$foo = $var // phpcs: ignore Stnd.Cat.Sniff -- for reasons. + + + ? $something /** + * Don't ask, but someone might have a docblock between the lines. It's valid PHP after all. + */ + + + : true; + +// phpcs:set Squiz.WhiteSpace.OperatorSpacing ignoreNewlines true +$foo = $var // Comment + ? false // Comment + : true; +// phpcs:set Squiz.WhiteSpace.OperatorSpacing ignoreNewlines false diff --git a/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.php b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.php index 6718664058..e34a2ec4a8 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.php @@ -104,6 +104,12 @@ public function getErrorList($testFile='') 265 => 2, 266 => 2, 271 => 2, + 487 => 1, + 488 => 1, + 493 => 1, + 494 => 1, + 499 => 1, + 504 => 1, ]; case 'OperatorSpacingUnitTest.js':