From 926a7e02bc1d856a65b3f4b232e6305ef0f00d3b Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Wed, 2 Mar 2022 18:37:04 +0700 Subject: [PATCH] [Privatization] Add constant support on ChangeReadOnlyVariableWithDefaultValueToConstantRector (#1890) * [Privatization] Add constant support on ChangeReadOnlyVariableWithDefaultValueToConstantRector * implemented * [ci-review] Rector Rectify * add missing Use ConstFetch * [ci-review] Rector Rectify * rename method * phpstan * [ci-review] Rector Rectify * flip check * dependency circular between ExprAnalyzer and ArrayManipulator for check dynamic value * [ci-review] Rector Rectify * [ci-review] Rector Rectify * clean up * phpstan * [ci-review] Rector Rectify * final touch: clean up * [ci-review] Rector Rectify Co-authored-by: GitHub Action --- packages/NodeNestingScope/ContextAnalyzer.php | 4 +- .../Fixture/some_constant.php.inc | 35 +++++++++++ .../Php81/NodeAnalyzer/ComplexNewAnalyzer.php | 29 ++-------- src/Console/ConsoleApplication.php | 8 ++- src/NodeAnalyzer/ExprAnalyzer.php | 58 ++++++------------- src/NodeManipulator/ArrayManipulator.php | 54 ++++++++++++----- src/NodeManipulator/VariableManipulator.php | 13 ++--- 7 files changed, 110 insertions(+), 91 deletions(-) create mode 100644 rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/some_constant.php.inc diff --git a/packages/NodeNestingScope/ContextAnalyzer.php b/packages/NodeNestingScope/ContextAnalyzer.php index 8b4ce344119..d19e58d9c44 100644 --- a/packages/NodeNestingScope/ContextAnalyzer.php +++ b/packages/NodeNestingScope/ContextAnalyzer.php @@ -77,9 +77,7 @@ public function isInIf(Node $node): bool public function isHasAssignWithIndirectReturn(Node $node, If_ $if): bool { - $loopNodes = self::LOOP_NODES; - - foreach ($loopNodes as $loopNode) { + foreach (self::LOOP_NODES as $loopNode) { $loopObjectType = new ObjectType($loopNode); $parentType = $this->nodeTypeResolver->getType($node); $superType = $parentType->isSuperTypeOf($loopObjectType); diff --git a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/some_constant.php.inc b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/some_constant.php.inc new file mode 100644 index 00000000000..6391a72a9b4 --- /dev/null +++ b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/some_constant.php.inc @@ -0,0 +1,35 @@ + +----- + diff --git a/rules/Php81/NodeAnalyzer/ComplexNewAnalyzer.php b/rules/Php81/NodeAnalyzer/ComplexNewAnalyzer.php index 17b78f8e787..e54310f0b41 100644 --- a/rules/Php81/NodeAnalyzer/ComplexNewAnalyzer.php +++ b/rules/Php81/NodeAnalyzer/ComplexNewAnalyzer.php @@ -7,18 +7,15 @@ use PhpParser\Node\Expr; use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\ArrayItem; -use PhpParser\Node\Expr\ClassConstFetch; -use PhpParser\Node\Expr\ConstFetch; use PhpParser\Node\Expr\New_; -use PhpParser\Node\Identifier; -use PhpParser\Node\Name; use PhpParser\Node\Name\FullyQualified; -use PhpParser\Node\Scalar; use Rector\Core\NodeAnalyzer\ExprAnalyzer; +use Rector\Core\NodeManipulator\ArrayManipulator; final class ComplexNewAnalyzer { public function __construct( + private readonly ArrayManipulator $arrayManipulator, private readonly ExprAnalyzer $exprAnalyzer ) { } @@ -38,15 +35,12 @@ public function isDynamic(New_ $new): bool continue; } + // new inside array is allowed for New in initializer if ($value instanceof Array_ && $this->isAllowedArray($value)) { continue; } - if ($value instanceof Scalar) { - continue; - } - - if ($this->isAllowedConstFetchOrClassConstFeth($value)) { + if (! $this->exprAnalyzer->isDynamicValue($value)) { continue; } @@ -56,19 +50,6 @@ public function isDynamic(New_ $new): bool return false; } - private function isAllowedConstFetchOrClassConstFeth(Expr $expr): bool - { - if (! in_array($expr::class, [ConstFetch::class, ClassConstFetch::class], true)) { - return false; - } - - if ($expr instanceof ClassConstFetch) { - return $expr->class instanceof Name && $expr->name instanceof Identifier; - } - - return true; - } - private function isAllowedNew(Expr $expr): bool { if ($expr instanceof New_) { @@ -80,7 +61,7 @@ private function isAllowedNew(Expr $expr): bool private function isAllowedArray(Array_ $array): bool { - if (! $this->exprAnalyzer->isDynamicArray($array)) { + if (! $this->arrayManipulator->isDynamicArray($array)) { return true; } diff --git a/src/Console/ConsoleApplication.php b/src/Console/ConsoleApplication.php index 74fe7cdb804..1b727f7fff4 100644 --- a/src/Console/ConsoleApplication.php +++ b/src/Console/ConsoleApplication.php @@ -25,13 +25,17 @@ final class ConsoleApplication extends Application */ private const NAME = 'Rector'; + /** + * @var string + */ + private const VERSION = VersionResolver::PACKAGE_VERSION; + /** * @param Command[] $commands */ public function __construct(CommandNaming $commandNaming, array $commands = []) { - $version = VersionResolver::PACKAGE_VERSION; - parent::__construct(self::NAME, $version); + parent::__construct(self::NAME, self::VERSION); foreach ($commands as $command) { $commandName = $commandNaming->resolveFromCommand($command); diff --git a/src/NodeAnalyzer/ExprAnalyzer.php b/src/NodeAnalyzer/ExprAnalyzer.php index e36db2e1289..9baa639673f 100644 --- a/src/NodeAnalyzer/ExprAnalyzer.php +++ b/src/NodeAnalyzer/ExprAnalyzer.php @@ -6,14 +6,16 @@ use PhpParser\Node\Expr; use PhpParser\Node\Expr\Array_; -use PhpParser\Node\Expr\ArrayItem; +use PhpParser\Node\Expr\ClassConstFetch; +use PhpParser\Node\Expr\ConstFetch; use PhpParser\Node\Expr\Variable; use PhpParser\Node\FunctionLike; +use PhpParser\Node\Identifier; +use PhpParser\Node\Name; use PhpParser\Node\Scalar; -use PhpParser\Node\Scalar\LNumber; -use PhpParser\Node\Scalar\String_; use PHPStan\PhpDocParser\Ast\PhpDoc\ParamTagValueNode; use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfoFactory; +use Rector\Core\NodeManipulator\ArrayManipulator; use Rector\Core\PhpParser\Comparing\NodeComparator; use Rector\Core\PhpParser\Node\BetterNodeFinder; use Rector\NodeNameResolver\NodeNameResolver; @@ -24,7 +26,8 @@ public function __construct( private readonly NodeComparator $nodeComparator, private readonly BetterNodeFinder $betterNodeFinder, private readonly PhpDocInfoFactory $phpDocInfoFactory, - private readonly NodeNameResolver $nodeNameResolver + private readonly NodeNameResolver $nodeNameResolver, + private readonly ArrayManipulator $arrayManipulator ) { } @@ -61,52 +64,29 @@ public function isNonTypedFromParam(Expr $expr): bool return false; } - public function isDynamicArray(Array_ $array): bool + public function isDynamicValue(Expr $expr): bool { - foreach ($array->items as $item) { - if (! $item instanceof ArrayItem) { - continue; - } - - $key = $item->key; - - if (! $this->isAllowedArrayKey($key)) { - return true; + if (! $expr instanceof Array_) { + if ($expr instanceof Scalar) { + return false; } - $value = $item->value; - if (! $this->isAllowedArrayValue($value)) { - return true; - } + return ! $this->isAllowedConstFetchOrClassConstFeth($expr); } - return false; + return $this->arrayManipulator->isDynamicArray($expr); } - private function isAllowedArrayKey(?Expr $expr): bool + private function isAllowedConstFetchOrClassConstFeth(Expr $expr): bool { - if (! $expr instanceof Expr) { - return true; - } - - return in_array($expr::class, [String_::class, LNumber::class], true); - } - - private function isAllowedArrayValue(Expr $expr): bool - { - if ($expr instanceof Array_) { - return true; + if (! in_array($expr::class, [ConstFetch::class, ClassConstFetch::class], true)) { + return false; } - return $this->isAllowedArrayOrScalar($expr); - } - - private function isAllowedArrayOrScalar(Expr $expr): bool - { - if (! $expr instanceof Array_) { - return $expr instanceof Scalar; + if ($expr instanceof ClassConstFetch) { + return $expr->class instanceof Name && $expr->name instanceof Identifier; } - return ! $this->isDynamicArray($expr); + return true; } } diff --git a/src/NodeManipulator/ArrayManipulator.php b/src/NodeManipulator/ArrayManipulator.php index 9f14392ad55..6ab18c82ee0 100644 --- a/src/NodeManipulator/ArrayManipulator.php +++ b/src/NodeManipulator/ArrayManipulator.php @@ -4,42 +4,50 @@ namespace Rector\Core\NodeManipulator; +use PhpParser\Node\Expr; use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\ArrayItem; -use PhpParser\Node\Scalar; +use PhpParser\Node\Scalar\LNumber; use PhpParser\Node\Scalar\String_; use Rector\ChangesReporting\Collector\RectorChangeCollector; +use Rector\Core\NodeAnalyzer\ExprAnalyzer; +use Symfony\Contracts\Service\Attribute\Required; final class ArrayManipulator { + private ExprAnalyzer $exprAnalyzer; + public function __construct( private readonly RectorChangeCollector $rectorChangeCollector ) { } - public function isArrayOnlyScalarValues(Array_ $array): bool + #[Required] + public function autowire(ExprAnalyzer $exprAnalyzer): void { - foreach ($array->items as $arrayItem) { - if (! $arrayItem instanceof ArrayItem) { + $this->exprAnalyzer = $exprAnalyzer; + } + + public function isDynamicArray(Array_ $array): bool + { + foreach ($array->items as $item) { + if (! $item instanceof ArrayItem) { continue; } - if ($arrayItem->value instanceof Array_) { - if (! $this->isArrayOnlyScalarValues($arrayItem->value)) { - return false; - } + $key = $item->key; - continue; + if (! $this->isAllowedArrayKey($key)) { + return true; } - if ($arrayItem->value instanceof Scalar) { - continue; + $value = $item->value; + if (! $this->isAllowedArrayValue($value)) { + return true; } - - return false; } - return true; + return false; } public function addItemToArrayUnderKey(Array_ $array, ArrayItem $newArrayItem, string $key): void @@ -97,4 +105,22 @@ public function hasKeyName(ArrayItem $arrayItem, string $name): bool return $arrayItem->key->value === $name; } + + private function isAllowedArrayKey(?Expr $expr): bool + { + if (! $expr instanceof Expr) { + return true; + } + + return in_array($expr::class, [String_::class, LNumber::class], true); + } + + private function isAllowedArrayValue(Expr $expr): bool + { + if ($expr instanceof Array_) { + return ! $this->isDynamicArray($expr); + } + + return ! $this->exprAnalyzer->isDynamicValue($expr); + } } diff --git a/src/NodeManipulator/VariableManipulator.php b/src/NodeManipulator/VariableManipulator.php index fb284e2d570..89929d45d2e 100644 --- a/src/NodeManipulator/VariableManipulator.php +++ b/src/NodeManipulator/VariableManipulator.php @@ -7,14 +7,13 @@ use PhpParser\Node; use PhpParser\Node\Arg; use PhpParser\Node\Expr; -use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\Variable; -use PhpParser\Node\Scalar; use PhpParser\Node\Scalar\Encapsed; use PhpParser\Node\Scalar\EncapsedStringPart; use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; +use Rector\Core\NodeAnalyzer\ExprAnalyzer; use Rector\Core\PhpParser\Comparing\NodeComparator; use Rector\Core\PhpParser\Node\BetterNodeFinder; use Rector\NodeNameResolver\NodeNameResolver; @@ -25,13 +24,13 @@ final class VariableManipulator { public function __construct( - private readonly ArrayManipulator $arrayManipulator, private readonly AssignManipulator $assignManipulator, private readonly BetterNodeFinder $betterNodeFinder, private readonly SimpleCallableNodeTraverser $simpleCallableNodeTraverser, private readonly NodeNameResolver $nodeNameResolver, private readonly VariableToConstantGuard $variableToConstantGuard, - private readonly NodeComparator $nodeComparator + private readonly NodeComparator $nodeComparator, + private readonly ExprAnalyzer $exprAnalyzer ) { } @@ -53,7 +52,7 @@ function (Node $node) use (&$assignsOfArrayToVariable) { return null; } - if (! $node->expr instanceof Array_ && ! $node->expr instanceof Scalar) { + if ($this->exprAnalyzer->isDynamicValue($node->expr)) { return null; } @@ -61,10 +60,6 @@ function (Node $node) use (&$assignsOfArrayToVariable) { return null; } - if ($node->expr instanceof Array_ && ! $this->arrayManipulator->isArrayOnlyScalarValues($node->expr)) { - return null; - } - if ($this->isTestCaseExpectedVariable($node->var)) { return null; }