fix: `no_useless_concat_operator` - do not break variable (#7827)

src/Fixer/Operator/NoUselessConcatOperatorFixer.php

@@ -26,6 +26,13 @@ use PhpCsFixer\Preg;
 use PhpCsFixer\Tokenizer\Token;
 use PhpCsFixer\Tokenizer\Tokens;
 
+/**
+ * @phpstan-type _ConcatOperandType array{
+ *     start: int,
+ *     end: int,
+ *     type: self::STR_*,
+ * }
+ */
 final class NoUselessConcatOperatorFixer extends AbstractFixer implements ConfigurableFixerInterface
 {
     private const STR_DOUBLE_QUOTE = 0;
@@ -47,7 +54,7 @@ final class NoUselessConcatOperatorFixer extends AbstractFixer implements Config
      * {@inheritdoc}
      *
      * Must run before DateTimeCreateFromFormatCallFixer, EregToPregFixer, PhpUnitDedicateAssertInternalTypeFixer, RegularCallableCallFixer, SetTypeToCastFixer.
-     * Must run after NoBinaryStringFixer, SingleQuoteFixer.
+     * Must run after ExplicitStringVariableFixer, NoBinaryStringFixer, SingleQuoteFixer.
      */
     public function getPriority(): int
     {
@@ -105,16 +112,8 @@ final class NoUselessConcatOperatorFixer extends AbstractFixer implements Config
     }
 
     /**
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $firstOperand
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $secondOperand
+     * @param _ConcatOperandType $firstOperand
+     * @param _ConcatOperandType $secondOperand
      */
     private function fixConcatOperation(Tokens $tokens, array $firstOperand, int $concatIndex, array $secondOperand): void
     {
@@ -130,6 +129,10 @@ final class NoUselessConcatOperatorFixer extends AbstractFixer implements Config
         }
 
         if (self::STR_DOUBLE_QUOTE_VAR === $firstOperand['type'] && self::STR_DOUBLE_QUOTE_VAR === $secondOperand['type']) {
+            if ($this->operandsCanNotBeMerged($tokens, $firstOperand, $secondOperand)) {
+                return;
+            }
+
             $this->mergeConstantEscapedStringVarOperands($tokens, $firstOperand, $concatIndex, $secondOperand);
 
             return;
@@ -146,6 +149,10 @@ final class NoUselessConcatOperatorFixer extends AbstractFixer implements Config
             [$operand1, $operand2] = $operandPair;
 
             if (self::STR_DOUBLE_QUOTE_VAR === $operand1['type'] && self::STR_DOUBLE_QUOTE === $operand2['type']) {
+                if ($this->operandsCanNotBeMerged($tokens, $operand1, $operand2)) {
+                    return;
+                }
+
                 $this->mergeConstantEscapedStringVarOperands($tokens, $firstOperand, $concatIndex, $secondOperand);
 
                 return;
@@ -169,6 +176,10 @@ final class NoUselessConcatOperatorFixer extends AbstractFixer implements Config
                 $operantContent = $tokens[$operand2['start']]->getContent();
 
                 if ($this->isSimpleQuotedStringContent($operantContent)) {
+                    if ($this->operandsCanNotBeMerged($tokens, $operand1, $operand2)) {
+                        return;
+                    }
+
                     $this->mergeConstantEscapedStringVarOperands($tokens, $firstOperand, $concatIndex, $secondOperand);
                 }
 
@@ -180,11 +191,7 @@ final class NoUselessConcatOperatorFixer extends AbstractFixer implements Config
     /**
      * @param -1|1 $direction
      *
-     * @return null|array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * }
+     * @return null|_ConcatOperandType
      */
     private function getConcatOperandType(Tokens $tokens, int $index, int $direction): ?array
     {
@@ -216,16 +223,8 @@ final class NoUselessConcatOperatorFixer extends AbstractFixer implements Config
     }
 
     /**
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $firstOperand
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $secondOperand
+     * @param _ConcatOperandType $firstOperand
+     * @param _ConcatOperandType $secondOperand
      */
     private function mergeConstantEscapedStringOperands(
         Tokens $tokens,
@@ -249,16 +248,8 @@ final class NoUselessConcatOperatorFixer extends AbstractFixer implements Config
     }
 
     /**
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $firstOperand
-     * @param array{
-     *     start: int,
-     *     end: int,
-     *     type: self::STR_*,
-     * } $secondOperand
+     * @param _ConcatOperandType $firstOperand
+     * @param _ConcatOperandType $secondOperand
      */
     private function mergeConstantEscapedStringVarOperands(
         Tokens $tokens,
@@ -266,7 +257,7 @@ final class NoUselessConcatOperatorFixer extends AbstractFixer implements Config
         int $concatOperatorIndex,
         array $secondOperand
     ): void {
-        // build uo the new content
+        // build up the new content
         $newContent = '';
 
         foreach ([$firstOperand, $secondOperand] as $operant) {
@@ -336,4 +327,36 @@ final class NoUselessConcatOperatorFixer extends AbstractFixer implements Config
 
         return false;
     }
+
+    /**
+     * @param _ConcatOperandType $firstOperand
+     * @param _ConcatOperandType $secondOperand
+     */
+    private function operandsCanNotBeMerged(Tokens $tokens, array $firstOperand, array $secondOperand): bool
+    {
+        // If the first operand does not end with a variable, no variables would be broken by concatenation.
+        if (self::STR_DOUBLE_QUOTE_VAR !== $firstOperand['type']) {
+            return false;
+        }
+        if (!$tokens[$firstOperand['end'] - 1]->isGivenKind(T_VARIABLE)) {
+            return false;
+        }
+
+        $allowedPatternsForSecondOperand = [
+            '/^\s.*/', // e.g. " foo", ' bar', " $baz"
+            '/^-(?!\>)/', // e.g. "-foo", '-bar', "-$baz"
+        ];
+
+        // If the first operand ends with a variable, the second operand should match one of the allowed patterns.
+        // Otherwise, the concatenation can break a variable in the first operand.
+        foreach ($allowedPatternsForSecondOperand as $allowedPattern) {
+            $secondOperandInnerContent = substr($tokens->generatePartialCode($secondOperand['start'], $secondOperand['end']), 1, -1);
+
+            if (Preg::match($allowedPattern, $secondOperandInnerContent)) {
+                return false;
+            }
+        }
+
+        return true;
+    }
 }

src/Fixer/StringNotation/ExplicitStringVariableFixer.php

@@ -52,11 +52,12 @@ final class ExplicitStringVariableFixer extends AbstractFixer
     /**
      * {@inheritdoc}
      *
+     * Must run before NoUselessConcatOperatorFixer.
      * Must run after BacktickToShellExecFixer.
      */
     public function getPriority(): int
     {
-        return 0;
+        return 6;
     }
 
     public function isCandidate(Tokens $tokens): bool

tests/AutoReview/FixerFactoryTest.php

@@ -442,6 +442,9 @@ final class FixerFactoryTest extends TestCase
                 'heredoc_to_nowdoc',
                 'single_quote',
             ],
+            'explicit_string_variable' => [
+                'no_useless_concat_operator',
+            ],
             'final_class' => [
                 'protected_to_private',
                 'self_static_accessor',

tests/Fixer/Operator/NoUselessConcatOperatorFixerTest.php

@@ -136,6 +136,53 @@ $text2 = "intro:   "."   "." #a
             ',
         ];
 
+        yield 'do not fix if the execution result would be different' => [
+            '<?php
+                echo "abc $d" . "[$e]";
+                echo "abc $d" . "[3]";
+                echo "abc $d" . \'[3]\';
+                echo "abc $d" . "->e";
+                echo "abc $d" . \'->e\';
+                echo "abc $d" . "->$e";
+                echo "abc $d" . "?->e";
+                echo "abc $d" . "?->$e";
+                echo "abc $d" . \'?->e\';
+            ',
+        ];
+
+        yield 'do not fix if variables would be broken' => [
+            '<?php
+                echo "abc $d" . "e $f";
+                echo "abc $d" . "e";
+                echo "abc $d" . \'e\';
+                echo "abc $d" . "😃"; // with emoji
+                echo "私の名前は$name" . "です"; // multibyte characters (Japanese)
+            ',
+        ];
+
+        yield 'fix if variables would not be broken' => [
+            '<?php
+                echo "$a b";
+                echo "$a bcde";
+                echo "abc ${d}e";
+                echo "abc $d-efg";
+                echo "$a bcdef";
+                echo "abc ${d}ef";
+                echo "abc $d-efgh";
+                echo "abc $d-$e";
+            ',
+            '<?php
+                echo "$a" . " b";
+                echo "$a bc" . "de";
+                echo "abc ${d}" . "e";
+                echo "abc $d" . "-efg";
+                echo "$a bc" . \'def\';
+                echo "abc ${d}" . \'ef\';
+                echo "abc $d" . \'-efgh\';
+                echo "abc $d" . "-$e";
+            ',
+        ];
+
         yield 'single quote concat single quote but with line break after' => [
             "<?php \$fh = 'x'. // some comment
 'y';",

tests/Fixtures/Integration/priority/explicit_string_variable,no_useless_concat_operator.test

@@ -0,0 +1,13 @@
+--TEST--
+Integration of fixers: explicit_string_variable,no_useless_concat_operator.
+--RULESET--
+{"no_useless_concat_operator": true, "explicit_string_variable": true}
+--EXPECT--
+<?php
+
+$foo = "bar {$baz}qux";
+
+--INPUT--
+<?php
+
+$foo = "bar $baz" . "qux";