✨ Extended string normalization

Correctly normalize mixed string and interpolation. Use `{$c}` over `${c}` after PHP 8.2 deprecation.
exercism · Dec 5, 2024 · 3a3656e · 3a3656e
1 parent 8bc567f
commit 3a3656e
Show file tree

Hide file tree

Showing 2 changed files with 92 additions and 23 deletions.
diff --git a/phpunit-tests/StringTest.php b/phpunit-tests/StringTest.php
@@ -4,6 +4,9 @@
 
 namespace App\Tests;
 
+use Generator;
+use PHPUnit\Framework\Attributes\DataProvider;
+
 use function var_export;
 
 /**
@@ -46,15 +49,16 @@ public function testDoubleQuotesWithEscapes(): void
 
     public function testDoubleQuotesEncapsulation(): void
     {
+        // `${a}` is deprecated since PHP8.2
         $code = <<<'CODE'
             <?php
-            "encapsed $a or ${a}.";
+            "encapsed $a or {$a} or ${a}.";
             CODE;
 
         $this->assertRepresentation(
             $code,
             <<<'EOF'
-            'encapsed ' . $v0 . ' or ' . $v0 . '.';
+            'encapsed ' . $v0 . ' or ' . $v0 . ' or ' . $v0 . '.';
             EOF,
             '{"v0":"a"}',
         );
@@ -130,35 +134,56 @@ public function testNowdoc(): void
         );
     }
 
-    public function testUselessConcatenation(): void
+    /** @return Generator<string, array{string, string}, void, void> */
+    public static function uselessConcatenationProvider(): iterable
     {
-        $code = <<<'CODE'
+        yield 'basic' => ["'testA' . 'testB' . 'testC';", "'testAtestBtestC';"];
+        yield 'right' => ["'testA' . ('testB' . 'testC');", "'testAtestBtestC';"];
+        yield 'left' => ["('testA' . 'testB') . 'testC';", "'testAtestBtestC';"];
+        yield 'both' => ["('testA' . 'testB') . ('testC' . 'testD');", "'testAtestBtestCtestD';"];
+    }
+
+    #[DataProvider('uselessConcatenationProvider')]
+    public function testUselessConcatenation(string $input, string $output): void
+    {
+        $code = <<<CODE
             <?php
-            'testA' . 'testB' . 'testC';
+            $input
             CODE;
 
         $this->assertRepresentation(
             $code,
-            <<<'EOF'
-            'testAtestBtestC';
+            <<<EOF
+            $output
             EOF,
             '{}',
         );
     }
 
-    public function testUselessRightConcatenation(): void
+    /** @return Generator<string, array{string, string}, void, void> */
+    public static function uselessInterpolatedConcatenationProvider(): iterable
     {
-        $code = <<<'CODE'
+        yield 'left left' => ['"{$c}testA" . \'testB\';', '$v0 . \'testAtestB\';'];
+        yield 'left right' => ['"testA{$c}" . \'testB\';', '\'testA\' . $v0 . \'testB\';'];
+        yield 'right left' => ['\'testA\' . "{$c}testB";', '\'testA\' . $v0 . \'testB\';'];
+        yield 'right right' => ['\'testA\' . "testB{$c}";', '\'testAtestB\' . $v0;'];
+        yield 'left left and right right' => ['"{$c}testA" . "testB{$c}";', '$v0 . \'testAtestB\' . $v0;'];
+    }
+
+    #[DataProvider('uselessInterpolatedConcatenationProvider')]
+    public function testUselessInterpolatedConcatenation(string $input, string $output): void
+    {
+        $code = <<<CODE
             <?php
-            'testA' . ('testB' . 'testC');
+            $input
             CODE;
 
         $this->assertRepresentation(
             $code,
-            <<<'EOF'
-            'testAtestBtestC';
+            <<<EOF
+            $output
             EOF,
-            '{}',
+            '{"v0":"c"}',
         );
     }
 }
diff --git a/src/NormalizeNodeVisitor.php b/src/NormalizeNodeVisitor.php
@@ -28,8 +28,11 @@
 use PhpParser\NodeVisitorAbstract;
 
 use function array_map;
+use function array_push;
 use function array_shift;
+use function array_splice;
 use function assert;
+use function count;
 use function is_string;
 
 /**
@@ -151,9 +154,7 @@ private function normalizeString(String_ $string): void
     private function normalizeInterpolatedString(InterpolatedString $string): Node
     {
         $parts = array_map(
-            static fn (Node $part) => $part instanceof InterpolatedStringPart
-                ? new String_($part->value, ['kind' => String_::KIND_SINGLE_QUOTED])
-                : $part,
+            static fn (Node $part) => $part instanceof InterpolatedStringPart ? new String_($part->value) : $part,
             $string->parts,
         );
 
@@ -169,16 +170,59 @@ private function normalizeInterpolatedString(InterpolatedString $string): Node
     /**
      * TRANSFORM: Simplify useless concat such as `'a' . 'b'` => `'ab'`
      */
-    private function simplifyUselessConcat(Concat $concat): String_|null
+    private function simplifyUselessConcat(Concat $concat): String_|Concat
     {
-        if ($concat->left instanceof String_ && $concat->right instanceof String_) {
-            return new String_(
-                $concat->left->value . $concat->right->value,
-                ['kind' => String_::KIND_SINGLE_QUOTED],
-            );
+        // 1. Flatten Concat-tree to an array of nodes: Concat('0', Concat('1', $a)) => ['0','1',$a]
+        $nodes = $this->unwrapConcat($concat);
+        // 2. Merge consecutive String_: ['0','1',$a,'2','3',$b,'4','5'] => ['01',$a,'23',$b,'45']
+        $index = count($nodes) - 1;
+        while ($index > 0) {
+            $left = $nodes[$index - 1];
+            $right = $nodes[$index];
+            if ($left instanceof String_ && $right instanceof String_) {
+                array_splice($nodes, $index - 1, 2, [new String_($left->value . $right->value)]);
+            }
+
+            $index--;
         }
 
-        return null;
+        // 3. Re-build a Concat-tree, left-based associativity to avoid extra-parentheses:
+        // ['01',$a,'23'] => Concat(Concat('01', $a), '23')
+        $node = array_shift($nodes);
+        assert($node !== null, 'Concat has at least 1 node');
+        while ($right = array_shift($nodes)) {
+            $node = new Concat($node, $right);
+        }
+
+        assert(
+            $node instanceof String_ || $node instanceof Concat,
+            'Either everything was collapsed to a singled String_ or we have a top-level Concat remaining',
+        );
+
+        return $node;
+    }
+
+    /**
+     * Unwrap a tree of Concat to a flat array of nodes
+     *
+     * @return list<Node\Expr>
+     */
+    private function unwrapConcat(Concat $concat): array
+    {
+        $nodes = [];
+        if ($concat->left instanceof Concat) {
+            array_push($nodes, ...$this->unwrapConcat($concat->left));
+        } else {
+            $nodes[] = $concat->left;
+        }
+
+        if ($concat->right instanceof Concat) {
+            array_push($nodes, ...$this->unwrapConcat($concat->right));
+        } else {
+            $nodes[] = $concat->right;
+        }
+
+        return $nodes;
     }
 
     /**