PromotedConstructorPropertyFixer - do not promote with different name…

… when new one is already used (#767)
kubawerlos · Mar 27, 2022 · 69072e9 · 69072e9
1 parent 98d936a
commit 69072e9
Show file tree

Hide file tree

Showing 5 changed files with 105 additions and 3 deletions.
diff --git a/README.md b/README.md
@@ -3,7 +3,7 @@
 [![Latest stable version](https://img.shields.io/packagist/v/kubawerlos/php-cs-fixer-custom-fixers.svg?label=current%20version)](https://packagist.org/packages/kubawerlos/php-cs-fixer-custom-fixers)
 [![PHP version](https://img.shields.io/packagist/php-v/kubawerlos/php-cs-fixer-custom-fixers.svg)](https://php.net)
 [![License](https://img.shields.io/github/license/kubawerlos/php-cs-fixer-custom-fixers.svg)](LICENSE)
-![Tests](https://img.shields.io/badge/tests-3405-brightgreen.svg)
+![Tests](https://img.shields.io/badge/tests-3410-brightgreen.svg)
 [![Downloads](https://img.shields.io/packagist/dt/kubawerlos/php-cs-fixer-custom-fixers.svg)](https://packagist.org/packages/kubawerlos/php-cs-fixer-custom-fixers)
 
 [![CI Status](https://github.com/kubawerlos/php-cs-fixer-custom-fixers/workflows/CI/badge.svg?branch=main)](https://github.com/kubawerlos/php-cs-fixer-custom-fixers/actions)

diff --git a/src/Analyzer/Analysis/ConstructorAnalysis.php b/src/Analyzer/Analysis/ConstructorAnalysis.php
@@ -36,6 +36,27 @@ public function getConstructorIndex(): int
         return $this->constructorIndex;
     }
 
+    /**
+     * @return array<string>
+     */
+    public function getConstructorParameterNames(): array
+    {
+        $openParenthesis = $this->tokens->getNextTokenOfKind($this->constructorIndex, ['(']);
+        \assert(\is_int($openParenthesis));
+        $closeParenthesis = $this->tokens->findBlockEnd(Tokens::BLOCK_TYPE_PARENTHESIS_BRACE, $openParenthesis);
+
+        $constructorParameterNames = [];
+        for ($index = $openParenthesis + 1; $index < $closeParenthesis; $index++) {
+            if (!$this->tokens[$index]->isGivenKind(\T_VARIABLE)) {
+                continue;
+            }
+
+            $constructorParameterNames[] = $this->tokens[$index]->getContent();
+        }
+
+        return $constructorParameterNames;
+    }
+
     /**
      * @return array<int, string>
      */

diff --git a/src/Fixer/PromotedConstructorPropertyFixer.php b/src/Fixer/PromotedConstructorPropertyFixer.php
@@ -129,6 +129,7 @@ private function promoteProperties(Tokens $tokens, int $classIndex, ConstructorA
         $isDoctrineEntity = $this->isDoctrineEntity($tokens, $classIndex);
         $properties = $this->getClassProperties($tokens, $classIndex);
 
+        $constructorParameterNames = $constructorAnalysis->getConstructorParameterNames();
         $constructorPromotableParameters = $constructorAnalysis->getConstructorPromotableParameters();
         $constructorPromotableAssignments = $constructorAnalysis->getConstructorPromotableAssignments();
 
@@ -153,6 +154,9 @@ private function promoteProperties(Tokens $tokens, int $classIndex, ConstructorA
             $assignedPropertyIndex = $tokens->getPrevTokenOfKind($constructorPromotableAssignments[$constructorParameterName], [[\T_STRING]]);
             $oldParameterName = $tokens[$constructorParameterIndex]->getContent();
             $newParameterName = '$' . $tokens[$assignedPropertyIndex]->getContent();
+            if ($oldParameterName !== $newParameterName && \in_array($newParameterName, $constructorParameterNames, true)) {
+                continue;
+            }
 
             $tokensToInsert = $this->removePropertyAndReturnTokensToInsert($tokens, $propertyIndex);
 

diff --git a/tests/Analyzer/Analysis/ConstructorAnalysisTest.php b/tests/Analyzer/Analysis/ConstructorAnalysisTest.php
@@ -22,6 +22,56 @@
  */
 final class ConstructorAnalysisTest extends TestCase
 {
+    /**
+     * @param array<string> $expected
+     *
+     * @dataProvider provideGettingConstructorParameterNamesCases
+     */
+    public function testGettingConstructorParameterNames(array $expected, string $code): void
+    {
+        $tokens = Tokens::fromCode($code);
+        $analysis = new ConstructorAnalysis($tokens, 11);
+
+        self::assertSame(11, $analysis->getConstructorIndex());
+        self::assertSame($expected, $analysis->getConstructorParameterNames());
+    }
+
+    /**
+     * @return iterable<array{array<string>, string}>
+     */
+    public static function provideGettingConstructorParameterNamesCases(): iterable
+    {
+        yield 'parameters without types' => [
+            ['$a', '$b', '$c'],
+            '<?php class Foo {
+                public function __construct($a, $b, $c) {}
+            }',
+        ];
+
+        yield 'parameters with types' => [
+            ['$b', '$c', '$i', '$s'],
+            '<?php class Foo {
+                public function __construct(bool $b, callable $c, int $i, string $s) {}
+            }',
+        ];
+
+        yield 'parameters with defaults' => [
+            ['$x', '$y', '$z'],
+            '<?php class Foo {
+                public function __construct(Foo $x = null, ?Bar $y = null, float $z = 3.14) {}
+            }',
+        ];
+
+        if (\PHP_VERSION_ID >= 80000) {
+            yield 'some already promoted' => [
+                ['$a', '$b', '$q', '$s', '$t'],
+                '<?php class Foo {
+                    public function __construct(public array $a, bool $b, protected ?Bar\Baz\Qux $q, string $s, private OtherType $t) {}
+                }',
+            ];
+        }
+    }
+
     /**
      * @param array<int, string> $expected
      *
@@ -66,8 +116,8 @@ public function __construct(array $a, bool $b, callable $c1, CALLABLE $c1, int $
             yield 'some already promoted' => [
                 [22 => '$b', 39 => '$s'],
                 '<?php class Foo {
-                public function __construct(public array $a, bool $b, protected ?Bar\Baz\Qux $q, string $s, private OtherType $t) {}
-            }',
+                    public function __construct(public array $a, bool $b, protected ?Bar\Baz\Qux $q, string $s, private OtherType $t) {}
+                }',
             ];
         }
     }

diff --git a/tests/Fixer/PromotedConstructorPropertyFixerTest.php b/tests/Fixer/PromotedConstructorPropertyFixerTest.php
@@ -688,6 +688,33 @@ public function __construct(string $y) {
             ',
         ];
 
+        yield 'do not promote with different name when new one is already used' => [
+            '<?php class Foo {
+                private string $x;
+                public function __construct(
+                    callable $x,
+                    string $y,
+                    private string $z,
+                ) {
+                    $this->x = $y;
+                }
+            }
+            ',
+            '<?php class Foo {
+                private string $x;
+                private string $z;
+                public function __construct(
+                    callable $x,
+                    string $y,
+                    string $z,
+                ) {
+                    $this->x = $y;
+                    $this->z = $z;
+                }
+            }
+            ',
+        ];
+
         yield 'promote with different name when not defined' => [
             '<?php class Foo {
                 public function __construct(public string $x) {