diff --git a/composer.json b/composer.json index e9e9c2a44598..da0f058b6bf8 100644 --- a/composer.json +++ b/composer.json @@ -49,7 +49,6 @@ "Rector\\Symfony\\": "packages/Symfony/src", "Rector\\CakePHP\\": "packages/CakePHP/src", "Rector\\Php\\": "packages/Php/src", - "Rector\\Jms\\": "packages/Jms/src", "Rector\\RemovingStatic\\": "packages/RemovingStatic/src", "Rector\\Silverstripe\\": "packages/Silverstripe/src", "Rector\\Sensio\\": "packages/Sensio/src", @@ -85,7 +84,6 @@ "Rector\\DomainDrivenDesign\\Tests\\": "packages/DomainDrivenDesign/tests", "Rector\\Guzzle\\Tests\\": "packages/Guzzle/tests", "Rector\\Php\\Tests\\": "packages/Php/tests", - "Rector\\Jms\\Tests\\": "packages/Jms/tests", "Rector\\RemovingStatic\\Tests\\": "packages/RemovingStatic/tests", "Rector\\Symfony\\Tests\\": "packages/Symfony/tests", "Rector\\Silverstripe\\Tests\\": "packages/Silverstripe/tests", diff --git a/packages/Php/src/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector.php b/packages/Php/src/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector.php index 3ab09145098b..36fb09d3b8af 100644 --- a/packages/Php/src/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector.php +++ b/packages/Php/src/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector.php @@ -9,20 +9,25 @@ use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\Ternary; use PhpParser\Node\Expr\Variable; +use PHPStan\Type\Constant\ConstantArrayType; +use PHPStan\Type\Constant\ConstantStringType; use Rector\Rector\AbstractRector; use Rector\RectorDefinition\CodeSample; use Rector\RectorDefinition\RectorDefinition; /** * @see https://wiki.php.net/rfc/spread_operator_for_array + * @see https://twitter.com/nikita_ppv/status/1126470222838366209 */ final class ArraySpreadInsteadOfArrayMergeRector extends AbstractRector { public function getDefinition(): RectorDefinition { - return new RectorDefinition('Change array_merge() to spread operator', [ - new CodeSample( - <<<'CODE_SAMPLE' + return new RectorDefinition( + 'Change array_merge() to spread operator, except values with possible string key values', + [ + new CodeSample( + <<<'CODE_SAMPLE' class SomeClass { public function run($iter1, $iter2) @@ -37,21 +42,22 @@ public function run($iter1, $iter2) } } CODE_SAMPLE - , - <<<'CODE_SAMPLE' + , + <<<'CODE_SAMPLE' class SomeClass { public function run($iter1, $iter2) { - $values = [ ...$iter1, ...$iter2 ]; + $values = [...$iter1, ...$iter2]; // Or to generalize to all iterables - $anotherValues = [ ...$iter1, ...$iter2 ]; + $anotherValues = [...$iter1, ...$iter2]; } } CODE_SAMPLE - ), - ]); + ), + ] + ); } /** @@ -111,12 +117,17 @@ private function resolveValue(Expr $expr): Expr return $expr; } - private function refactorArray(Node $node): Array_ + private function refactorArray(FuncCall $funcCall): ?Array_ { $array = new Array_(); - foreach ($node->args as $arg) { + foreach ($funcCall->args as $arg) { $value = $arg->value; + + if ($this->shouldSkipArrayForInvalidTypeOrKeys($value)) { + return null; + } + $value = $this->resolveValue($value); $array->items[] = $this->createUnpackedArrayItem($value); @@ -137,4 +148,24 @@ private function createUnpackedArrayItem(Expr $expr): ArrayItem { return new ArrayItem($expr, null, false, [], true); } + + private function shouldSkipArrayForInvalidTypeOrKeys(Expr $expr): bool + { + // we have no idea what it is → cannot change it + if (! $this->isArrayType($expr)) { + return true; + } + + $arrayStaticType = $this->getStaticType($expr); + if ($arrayStaticType instanceof ConstantArrayType) { + foreach ($arrayStaticType->getKeyTypes() as $keyType) { + // key cannot be string + if ($keyType instanceof ConstantStringType) { + return true; + } + } + } + + return false; + } } diff --git a/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/ArraySpreadInsteadOfArrayMergeRectorTest.php b/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/ArraySpreadInsteadOfArrayMergeRectorTest.php index 9e6deac30312..8a97ccfac875 100644 --- a/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/ArraySpreadInsteadOfArrayMergeRectorTest.php +++ b/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/ArraySpreadInsteadOfArrayMergeRectorTest.php @@ -16,7 +16,10 @@ public function test(): void $this->doTestFiles([ __DIR__ . '/Fixture/fixture.php.inc', __DIR__ . '/Fixture/iterator_to_array.php.inc', - __DIR__ . '/Fixture/simple_array_merge.php.inc', + __DIR__ . '/Fixture/integer_keys.php.inc', + // see caveat: https://twitter.com/nikita_ppv/status/1126470222838366209 + __DIR__ . '/Fixture/skip_simple_array_merge.php.inc', + __DIR__ . '/Fixture/skip_string_keys.php.inc', ]); } diff --git a/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/Fixture/integer_keys.php.inc b/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/Fixture/integer_keys.php.inc new file mode 100644 index 000000000000..c3a94abed743 --- /dev/null +++ b/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/Fixture/integer_keys.php.inc @@ -0,0 +1,33 @@ + 'two']; + $iter2 = [1 => 'four']; + + return array_merge($iter1, $iter2); + } +} + +?> +----- + 'two']; + $iter2 = [1 => 'four']; + + return [...$iter1, ...$iter2]; + } +} + +?> diff --git a/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/Fixture/simple_array_merge.php.inc b/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/Fixture/simple_array_merge.php.inc deleted file mode 100644 index 0cedbf028093..000000000000 --- a/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/Fixture/simple_array_merge.php.inc +++ /dev/null @@ -1,27 +0,0 @@ - ------ - diff --git a/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/Fixture/skip_simple_array_merge.php.inc b/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/Fixture/skip_simple_array_merge.php.inc new file mode 100644 index 000000000000..db959a4975a2 --- /dev/null +++ b/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/Fixture/skip_simple_array_merge.php.inc @@ -0,0 +1,11 @@ + 'two']; + $iter2 = ['three' => 'four']; + + return array_merge($iter1, $iter2); + } + + public function go() + { + $iter1 = [1 => 'two']; + $iter2 = ['three' => 'four']; + + return array_merge($iter1, $iter2); + } +} diff --git a/phpstan.neon b/phpstan.neon index 63f5b78f0a73..b5ba43e6a63f 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -77,7 +77,6 @@ parameters: # false positive, resolved in previous method - '#Parameter (.*?) of method Rector\\PhpParser\\Node\\Manipulator\\IdentifierManipulator\:\:(.*?)\(\) expects PhpParser\\Node\\Expr\\ClassConstFetch\|PhpParser\\Node\\Expr\\MethodCall\|PhpParser\\Node\\Expr\\PropertyFetch\|PhpParser\\Node\\Expr\\StaticCall\|PhpParser\\Node\\Stmt\\ClassMethod, PhpParser\\Node given#' - - '#Parameter \#1 \$callables of method Rector\\Collector\\CallableCollectorPopulator::populate\(\) expects (.*?) given#' # intentionally incorrect - part of the test - '#Parameter \#2 \$codeSamples of class Rector\\RectorDefinition\\RectorDefinition constructor expects array, array given#' @@ -150,12 +149,10 @@ parameters: - '#Method Rector\\NodeContainer\\ParsedNodesByType\:\:(.*?)\(\) should return PhpParser\\Node\\Stmt\\(.*?)\|null but returns PhpParser\\Node\|null#' - '#Method Rector\\NodeContainer\\ParsedNodesByType\:\:findImplementersOfInterface\(\) should return array but returns array#' - '#PHPDoc tag @param for parameter \$classLike with type PhpParser\\Builder\\Trait_\|PhpParser\\Node\\Stmt\\Interface_ is not subtype of native type PhpParser\\Node\\Stmt\\ClassLike#' - - '#Method Rector\\CodingStyle\\Rector\\Namespace_\\ImportFullyQualifiedNamesRector\:\:getShortName\(\) should return string but returns string\|false#' - '#Access to an undefined property PhpParser\\Node\\Expr\\Error\|PhpParser\\Node\\Expr\\Variable\:\:\$name#' - '#Empty array passed to foreach#' - '#Method Rector\\RemovingStatic\\UniqueObjectFactoryFactory\:\:resolveClassShortName\(\) should return string but returns string\|false#' - '#Strict comparison using \=\=\= between PhpParser\\Node\\Expr\\ArrayItem and null will always evaluate to false#' - - '#Anonymous function should have native typehint "string"#' - '#Parameter \#2 \.\.\.\$args of function array_merge expects array, array\|false given#' - '#Method Rector\\Collector\\CallableCollectorPopulator\:\:populate\(\) should return array but returns array#' - '#Access to an undefined property PhpParser\\Node\\Expr\:\:\$args#'