From 37b06023b666cac8ec2997e8e1605f93c25d3b6f Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Wed, 4 Aug 2021 15:37:55 +0200 Subject: [PATCH] [DeadCode] Fix removal of variable-used promoted property (#593) --- .github/workflows/code_analysis.yaml | 5 + .../Fixture/user_variable.php.inc | 29 ++++ .../RemoveUnusedPromotedPropertyRector.php | 16 +- rules/Naming/Naming/VariableNaming.php | 145 ------------------ rules/Php72/Rector/Assign/ListEachRector.php | 4 - src/PhpParser/Node/BetterNodeFinder.php | 22 --- 6 files changed, 44 insertions(+), 177 deletions(-) create mode 100644 rules-tests/DeadCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector/Fixture/user_variable.php.inc diff --git a/.github/workflows/code_analysis.yaml b/.github/workflows/code_analysis.yaml index ad12a709f36..82a5520882f 100644 --- a/.github/workflows/code_analysis.yaml +++ b/.github/workflows/code_analysis.yaml @@ -29,6 +29,11 @@ jobs: name: 'PHPStan for config' run: composer phpstan-config + - + name: Commented Code + run: vendor/bin/easy-ci check-commented-code src packages rules tests packages-tests rules-tests --line-limit 5 --ansi + + # see https://github.com/rectorphp/rector-generator - name: 'Rector Generate From Recipe' diff --git a/rules-tests/DeadCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector/Fixture/user_variable.php.inc b/rules-tests/DeadCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector/Fixture/user_variable.php.inc new file mode 100644 index 00000000000..2ee0e3ef935 --- /dev/null +++ b/rules-tests/DeadCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector/Fixture/user_variable.php.inc @@ -0,0 +1,29 @@ + +----- + diff --git a/rules/DeadCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector.php b/rules/DeadCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector.php index d6f99e8ade5..75c154db4da 100644 --- a/rules/DeadCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector.php +++ b/rules/DeadCode/Rector/ClassMethod/RemoveUnusedPromotedPropertyRector.php @@ -5,6 +5,7 @@ namespace Rector\DeadCode\Rector\ClassMethod; use PhpParser\Node; +use PhpParser\Node\Expr\Variable; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassMethod; use Rector\Core\NodeManipulator\PropertyManipulator; @@ -90,7 +91,8 @@ public function refactor(Node $node): ?Node } foreach ($node->getParams() as $param) { - if ($param->flags === 0) { + // only private local scope; removing public property might be dangerous + if ($param->flags !== Class_::MODIFIER_PRIVATE) { continue; } @@ -98,11 +100,6 @@ public function refactor(Node $node): ?Node continue; } - // only private local scope; removing public property might be dangerous - if ($param->flags !== Class_::MODIFIER_PRIVATE) { - continue; - } - $paramName = $this->getName($param); $propertyFetches = $this->propertyFetchFinder->findLocalPropertyFetchesByName($class, $paramName); @@ -110,6 +107,13 @@ public function refactor(Node $node): ?Node continue; } + // is variable used? only remove property, keep param + $variable = $this->betterNodeFinder->findVariableOfName((array) $node->stmts, $paramName); + if ($variable instanceof Variable) { + $param->flags = 0; + continue; + } + // remove param $this->removeNode($param); } diff --git a/rules/Naming/Naming/VariableNaming.php b/rules/Naming/Naming/VariableNaming.php index b4f9c17c08b..4ef1a904368 100644 --- a/rules/Naming/Naming/VariableNaming.php +++ b/rules/Naming/Naming/VariableNaming.php @@ -38,12 +38,6 @@ public function __construct( ) { } -// public function resolveFromNode(Node $node): ?string -// { -// $nodeType = $this->nodeTypeResolver->getStaticType($node); -// return $this->resolveFromNodeAndType($node, $nodeType); -// } - public function resolveFromNodeAndType(Node $node, Type $type): ?string { $variableName = $this->resolveBareFromNode($node); @@ -111,23 +105,6 @@ public function resolveFromFuncCallFirstArgumentWithSuffix( return $this->createCountedValueName($bareName, $scope); } -// private function resolveFromNodeAndType(Node $node, Type $type): ?string -// { -// $variableName = $this->resolveBareFromNode($node); -// if ($variableName === null) { -// return null; -// } -// -// // adjust static to specific class -// if ($variableName === 'this' && $type instanceof ThisType) { -// $shortClassName = $this->nodeNameResolver->getShortName($type->getClassName()); -// $variableName = lcfirst($shortClassName); -// } -// -// $stringy = new Stringy($variableName); -// return (string) $stringy->camelize(); -// } - public function resolveFromNode(Node $node): ?string { $nodeType = $this->nodeTypeResolver->getStaticType($node); @@ -174,23 +151,6 @@ private function resolveBareFromNode(Node $node): ?string return null; } -// private function unwrapNode(Node $node): ?Node -// { -// if ($node instanceof Arg) { -// return $node->value; -// } -// -// if ($node instanceof Cast) { -// return $node->expr; -// } -// -// if ($node instanceof Ternary) { -// return $node->if; -// } -// -// return $node; -// } - private function resolveParamNameFromArrayDimFetch(ArrayDimFetch $arrayDimFetch): ?string { while ($arrayDimFetch instanceof ArrayDimFetch) { @@ -270,111 +230,6 @@ private function unwrapNode(Node $node): ?Node return $node; } -// private function resolveParamNameFromArrayDimFetch(ArrayDimFetch $arrayDimFetch): ?string -// { -// while ($arrayDimFetch instanceof ArrayDimFetch) { -// if ($arrayDimFetch->dim instanceof Scalar) { -// $valueName = $this->nodeNameResolver->getName($arrayDimFetch->var); -// $dimName = $this->valueResolver->getValue($arrayDimFetch->dim); -// -// $stringy = new Stringy($dimName); -// $dimName = (string) $stringy->upperCamelize(); -// -// return $valueName . $dimName; -// } -// -// $arrayDimFetch = $arrayDimFetch->var; -// } -// -// return $this->resolveBareFromNode($arrayDimFetch); -// } - -// private function resolveBareFromNode(Node $node): ?string -// { -// $node = $this->unwrapNode($node); -// -// if ($node instanceof ArrayDimFetch) { -// return $this->resolveParamNameFromArrayDimFetch($node); -// } -// -// if ($node instanceof PropertyFetch) { -// return $this->resolveFromPropertyFetch($node); -// } -// -// if ($node instanceof MethodCall || $node instanceof NullsafeMethodCall || $node instanceof StaticCall) { -// return $this->resolveFromMethodCall($node); -// } -// -// if ($node instanceof New_) { -// return $this->resolveFromNew($node); -// } -// -// if ($node instanceof FuncCall) { -// return $this->resolveFromNode($node->name); -// } -// -// if (! $node instanceof Node) { -// throw new NotImplementedYetException(); -// } -// -// $paramName = $this->nodeNameResolver->getName($node); -// if ($paramName !== null) { -// return $paramName; -// } -// -// if ($node instanceof String_) { -// return $node->value; -// } -// -// return null; -// } - -// private function resolveFromNew(New_ $new): string -// { -// if ($new->class instanceof Name) { -// $className = $this->nodeNameResolver->getName($new->class); -// return $this->nodeNameResolver->getShortName($className); -// } -// -// throw new NotImplementedYetException(); -// } -// -// /** -// * @param MethodCall|NullsafeMethodCall|StaticCall $node -// */ -// private function resolveFromMethodCall(Node $node): ?string -// { -// if ($node->name instanceof MethodCall) { -// return $this->resolveFromMethodCall($node->name); -// } -// -// $methodName = $this->nodeNameResolver->getName($node->name); -// if (! is_string($methodName)) { -// return null; -// } -// -// return $methodName; -// } -// -// private function resolveFromPropertyFetch(PropertyFetch $propertyFetch): string -// { -// $varName = $this->nodeNameResolver->getName($propertyFetch->var); -// if (! is_string($varName)) { -// throw new NotImplementedYetException(); -// } -// -// $propertyName = $this->nodeNameResolver->getName($propertyFetch->name); -// if (! is_string($propertyName)) { -// throw new NotImplementedYetException(); -// } -// -// if ($varName === 'this') { -// return $propertyName; -// } -// -// return $varName . ucfirst($propertyName); -// } - private function resolveBareFuncCallArgumentName( FuncCall $funcCall, string $fallbackName, diff --git a/rules/Php72/Rector/Assign/ListEachRector.php b/rules/Php72/Rector/Assign/ListEachRector.php index 78815c6d9d0..b6bfe11ab4a 100644 --- a/rules/Php72/Rector/Assign/ListEachRector.php +++ b/rules/Php72/Rector/Assign/ListEachRector.php @@ -90,10 +90,6 @@ public function refactor(Node $node): ?Node } // both: list($key, $value) = each($values); - // ↓ - // $key = key($values); - // $value = current($values); - // next($values); $currentFuncCall = $this->nodeFactory->createFuncCall('current', $eachFuncCall->args); $secondArrayItem = $listNode->items[1]; diff --git a/src/PhpParser/Node/BetterNodeFinder.php b/src/PhpParser/Node/BetterNodeFinder.php index 8d7ce128324..fad765049b6 100644 --- a/src/PhpParser/Node/BetterNodeFinder.php +++ b/src/PhpParser/Node/BetterNodeFinder.php @@ -125,28 +125,6 @@ public function hasVariableOfName(Node | array $nodes, string $name): bool return (bool) $this->findVariableOfName($nodes, $name); } -// /** -// * @param Node|Stmt[] $nodes -// * @return Variable[] -// */ -// public function findVariablesOfName(Node | array $nodes, string $variableName): array -// { -// /** @var Variable[] $variables */ -// $variables = $this->findInstanceOf($nodes, Variable::class); -// -// $variablesOfName = []; -// -// foreach ($variables as $variable) { -// if (! $this->nodeNameResolver->isName($variable, $variableName)) { -// continue; -// } -// -// $variablesOfName[] = $variable; -// } -// -// return $variablesOfName; -// } - /** * @param Node|Node[] $nodes * @return Variable|null