diff --git a/phpstan.neon b/phpstan.neon index 877ea93439a..32f65367d17 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -336,7 +336,6 @@ parameters: message: '#Class cognitive complexity is \d+, keep it under 50#' paths: - src/PhpParser/Node/BetterNodeFinder.php - - rules/Removing/NodeManipulator/ComplexNodeRemover.php - message: '#Parameter \#2 \$length of function str_split expects int<1, max\>, int given#' diff --git a/rules-tests/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/constructor_with_middle_params.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/constructor_with_middle_params.php.inc index b16183a5c9f..f3be144c55e 100644 --- a/rules-tests/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/constructor_with_middle_params.php.inc +++ b/rules-tests/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/constructor_with_middle_params.php.inc @@ -29,7 +29,7 @@ namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRecto final class ConstructorWithMiddleParams { - public function __construct(\stdClass $stdClass) + public function __construct(int $contentFinder, \stdClass $stdClass) { $this->init($stdClass); } diff --git a/rules/Removing/NodeManipulator/ComplexNodeRemover.php b/rules/Removing/NodeManipulator/ComplexNodeRemover.php index feb8566a10a..5941966981f 100644 --- a/rules/Removing/NodeManipulator/ComplexNodeRemover.php +++ b/rules/Removing/NodeManipulator/ComplexNodeRemover.php @@ -50,38 +50,42 @@ final class ComplexNodeRemover &$isPartoFAnotherAssign ) { // here should be checked all expr like stmts that can hold assign, e.f. if, foreach etc. etc. - if ($node instanceof Expression) { - $nodeExpr = $node->expr; + if (! $node instanceof Expression) { + return null; + } - // remove direct assigns - if ($nodeExpr instanceof Assign) { - $assign = $nodeExpr; + $nodeExpr = $node->expr; - // skip double assigns - if ($assign->expr instanceof Assign) { - $isPartoFAnotherAssign = true; + // remove direct assigns + if (! $nodeExpr instanceof Assign) { + return null; + } + + $assign = $nodeExpr; + + // skip double assigns + if ($assign->expr instanceof Assign) { + $isPartoFAnotherAssign = true; + return null; + } + + $propertyFetches = $this->resolvePropertyFetchFromDimFetch($assign->var); + if ($propertyFetches === []) { + return null; + } + + foreach ($propertyFetches as $propertyFetch) { + if (! $this->nodeNameResolver->isName($propertyFetch->var, 'this')) { + continue; + } + + if ($this->nodeNameResolver->isName($propertyFetch->name, $propertyName)) { + if (! $removeAssignSideEffect && $this->sideEffectNodeDetector->detect($assign->expr)) { + $hasSideEffect = true; return null; } - $propertyFetches = $this->resolvePropertyFetchFromDimFetch($assign->var); - if ($propertyFetches === []) { - return null; - } - - foreach ($propertyFetches as $propertyFetch) { - if (! $this->nodeNameResolver->isName($propertyFetch->var, 'this')) { - continue; - } - - if ($this->nodeNameResolver->isName($propertyFetch->name, $propertyName)) { - if (! $removeAssignSideEffect && $this->sideEffectNodeDetector->detect($assign->expr)) { - $hasSideEffect = true; - return null; - } - - $this->nodeRemover->removeNode($node); - } - } + $this->nodeRemover->removeNode($node); } } @@ -143,6 +147,7 @@ final class ComplexNodeRemover } $stmts = (array) $classMethod->stmts; + $paramKeysToBeRemoved = []; foreach ($stmts as $key => $stmt) { if (! $stmt instanceof Expression) { @@ -150,17 +155,37 @@ final class ComplexNodeRemover } $stmtExpr = $stmt->expr; - if ($stmtExpr instanceof Assign && $stmtExpr->var instanceof PropertyFetch) { - $propertyFetch = $stmtExpr->var; - if ($this->nodeNameResolver->isName($propertyFetch, $propertyName)) { - unset($classMethod->stmts[$key]); - if ($stmtExpr->expr instanceof Variable) { - $this->clearParamFromConstructor($classMethod, $stmtExpr->expr); - } - } + if (! $stmtExpr instanceof Assign) { + continue; + } + + if (! $stmtExpr->var instanceof PropertyFetch) { + continue; + } + + $propertyFetch = $stmtExpr->var; + if (! $this->nodeNameResolver->isName($propertyFetch, $propertyName)) { + continue; + } + + unset($classMethod->stmts[$key]); + + if (! $stmtExpr->expr instanceof Variable) { + continue; + } + + $key = $this->resolveToBeClearedParamFromConstructor($classMethod, $stmtExpr->expr); + if (is_int($key)) { + $paramKeysToBeRemoved[] = $key; } } + + if ($paramKeysToBeRemoved === []) { + return; + } + + $this->processRemoveParamWithKeys($classMethod->getParams(), $paramKeysToBeRemoved); } /** @@ -186,7 +211,7 @@ final class ComplexNodeRemover return $propertyFetches; } - private function clearParamFromConstructor(ClassMethod $classMethod, Variable $assignedVariable): void + private function resolveToBeClearedParamFromConstructor(ClassMethod $classMethod, Variable $assignedVariable): ?int { // is variable used somewhere else? skip it $variables = $this->betterNodeFinder->findInstanceOf($classMethod, Variable::class); @@ -198,18 +223,20 @@ final class ComplexNodeRemover // there is more than 1 use, keep it in the constructor if (count($paramNamedVariables) > 1) { - return; + return null; } $paramName = $this->nodeNameResolver->getName($assignedVariable); if (! is_string($paramName)) { - return; + return null; } foreach ($classMethod->params as $paramKey => $param) { if ($this->nodeNameResolver->isName($param->var, $paramName)) { - unset($classMethod->params[$paramKey]); + return $paramKey; } } + + return null; } } diff --git a/tests/Issues/RemoveUnusedParamInMiddle/Fixture/do_not_remove_parameter_in_middle.php.inc b/tests/Issues/RemoveUnusedParamInMiddle/Fixture/do_not_remove_parameter_in_middle.php.inc new file mode 100644 index 00000000000..7b805b044f7 --- /dev/null +++ b/tests/Issues/RemoveUnusedParamInMiddle/Fixture/do_not_remove_parameter_in_middle.php.inc @@ -0,0 +1,53 @@ +propertyA = $propertyA; + $this->propertyB = $propertyB; + $this->propertyC = $propertyC; + } + + public function run() + { + echo $this->propertyA; + echo $this->propertyC; + } +} + +?> +----- +propertyA = $propertyA; + $this->propertyC = $propertyC; + } + + public function run() + { + echo $this->propertyA; + echo $this->propertyC; + } +} + +?> diff --git a/tests/Issues/RemoveUnusedParamInMiddle/RemoveUnusedParamInMiddleTest.php b/tests/Issues/RemoveUnusedParamInMiddle/RemoveUnusedParamInMiddleTest.php new file mode 100644 index 00000000000..dbc30b7a7d1 --- /dev/null +++ b/tests/Issues/RemoveUnusedParamInMiddle/RemoveUnusedParamInMiddleTest.php @@ -0,0 +1,33 @@ +doTestFileInfo($fileInfo); + } + + /** + * @return Iterator + */ + public function provideData(): Iterator + { + return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + public function provideConfigFilePath(): string + { + return __DIR__ . '/config/configured_rule.php'; + } +} diff --git a/tests/Issues/RemoveUnusedParamInMiddle/config/configured_rule.php b/tests/Issues/RemoveUnusedParamInMiddle/config/configured_rule.php new file mode 100644 index 00000000000..f8a81d0bee5 --- /dev/null +++ b/tests/Issues/RemoveUnusedParamInMiddle/config/configured_rule.php @@ -0,0 +1,12 @@ +rule(RemoveUnusedConstructorParamRector::class); + $rectorConfig->rule(RemoveUnusedPrivatePropertyRector::class); +};