Merge pull request #427 from PHPCSStandards/feature/squiz-operatorspacing-prevent-fixer-conflict

Squiz/OperatorSpacing: bug fix - prevent fixer conflict

Author: jrfnl

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
 

src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc

@@ -483,5 +483,28 @@ match ($a) {
     default => -3,
 };
 
-/* Intentional parse error. This has to be the last test in the file. */
-$a = 10 +
+$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

src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc.fixed

@@ -477,5 +477,26 @@ match ($a) {
     default => -3,
 };
 
-/* Intentional parse error. This has to be the last test in the file. */
-$a = 10 +
+$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

src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.3.inc

@@ -0,0 +1,6 @@
+<?php
+
+// Intentional parse error (unfinished statement).
+// This has to be the only test in the file.
+
+$a = 10 +

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':