From 6000b303699b3fa9cc4ec16898f38645248c0afe Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Tue, 6 Aug 2019 11:14:50 +0200 Subject: [PATCH 1/4] [DeadCode] Add RemoveSetterOnlyPropertyAndMethodCallRector --- config/set/dead-code/dead-code.yaml | 1 + .../src/Cleaner/ClassMethodCleaner.php | 7 - .../src/Attributes/Attribute/Attribute.php | 7 - .../Foreach_/ForeachToInArrayRector.php | 2 +- .../src/Node/ConcatManipulator.php | 2 +- ...anualJsonStringToJsonEncodeArrayRector.php | 2 +- .../Rector/Use_/RemoveUnusedAliasRector.php | 2 +- ...eSetterOnlyPropertyAndMethodCallRector.php | 206 ++++++++++++++++++ .../Fixture/fixture.php.inc | 42 ++++ .../Fixture/keep_static_property.php.inc | 17 ++ ...terOnlyPropertyAndMethodCallRectorTest.php | 22 ++ .../ParseStrWithResultArgumentRector.php | 4 +- .../FuncCall/PregReplaceEModifierRector.php | 2 +- .../src/PhpSpecMockCollector.php | 2 +- .../Rector/SimpleFunctionAndFilterRector.php | 2 +- phpstan.neon | 3 + src/Configuration/Configuration.php | 26 --- src/Console/Command/AbstractCommand.php | 2 +- src/PhpParser/Node/BetterNodeFinder.php | 32 +-- .../Node/Commander/NodeRemovingCommander.php | 16 +- .../Node/Manipulator/ClassManipulator.php | 115 ++++++++-- .../NodeTraverser/CallableNodeTraverser.php | 8 +- .../CallableNodeTraverserTrait.php | 4 +- .../AbstractRector/NodeCommandersTrait.php | 22 +- 24 files changed, 426 insertions(+), 122 deletions(-) delete mode 100644 packages/Architecture/src/Cleaner/ClassMethodCleaner.php create mode 100644 packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php create mode 100644 packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/fixture.php.inc create mode 100644 packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_static_property.php.inc create mode 100644 packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/RemoveSetterOnlyPropertyAndMethodCallRectorTest.php diff --git a/config/set/dead-code/dead-code.yaml b/config/set/dead-code/dead-code.yaml index fd286e754bc4..0775d03300c5 100644 --- a/config/set/dead-code/dead-code.yaml +++ b/config/set/dead-code/dead-code.yaml @@ -25,3 +25,4 @@ services: Rector\DeadCode\Rector\Instanceof_\RemoveDuplicatedInstanceOfRector: ~ Rector\DeadCode\Rector\Switch_\RemoveDuplicatedCaseInSwitchRector: ~ Rector\DeadCode\Rector\Class_\RemoveUnusedDoctrineEntityMethodAndPropertyRector: ~ + Rector\DeadCode\Rector\Class_\RemoveSetterOnlyPropertyAndMethodCallRector: ~ diff --git a/packages/Architecture/src/Cleaner/ClassMethodCleaner.php b/packages/Architecture/src/Cleaner/ClassMethodCleaner.php deleted file mode 100644 index 3f72a01d0c4b..000000000000 --- a/packages/Architecture/src/Cleaner/ClassMethodCleaner.php +++ /dev/null @@ -1,7 +0,0 @@ -traverseNodesWithCallable([$originalNode], function (Node $node): void { + $this->traverseNodesWithCallable($originalNode, function (Node $node): void { if ($node->hasAttribute('comments')) { $this->comments = array_merge($this->comments, $node->getComments()); } diff --git a/packages/CodingStyle/src/Node/ConcatManipulator.php b/packages/CodingStyle/src/Node/ConcatManipulator.php index bb461d4dc3b3..f9e9aeca5c9f 100644 --- a/packages/CodingStyle/src/Node/ConcatManipulator.php +++ b/packages/CodingStyle/src/Node/ConcatManipulator.php @@ -48,7 +48,7 @@ public function removeFirstItemFromConcat(Concat $concat): Node $newConcat = clone $concat; $firstConcatItem = $this->getFirstConcatItem($concat); - $this->callableNodeTraverser->traverseNodesWithCallable([$newConcat], function (Node $node) use ( + $this->callableNodeTraverser->traverseNodesWithCallable($newConcat, function (Node $node) use ( $firstConcatItem ): ?Expr { if (! $node instanceof Concat) { diff --git a/packages/CodingStyle/src/Rector/String_/ManualJsonStringToJsonEncodeArrayRector.php b/packages/CodingStyle/src/Rector/String_/ManualJsonStringToJsonEncodeArrayRector.php index a9e1366a250a..bd3e53b9f787 100644 --- a/packages/CodingStyle/src/Rector/String_/ManualJsonStringToJsonEncodeArrayRector.php +++ b/packages/CodingStyle/src/Rector/String_/ManualJsonStringToJsonEncodeArrayRector.php @@ -190,7 +190,7 @@ private function createAndReturnJsonEncodeFromArray(Assign $assign, Array_ $json private function replaceNodeObjectHashPlaceholdersWithNodes(Array_ $array, array $placeholderNodes): void { // traverse and replace placeholdes by original nodes - $this->traverseNodesWithCallable([$array], function (Node $node) use ($placeholderNodes): ?Expr { + $this->traverseNodesWithCallable($array, function (Node $node) use ($placeholderNodes): ?Expr { if (! $node instanceof String_) { return null; } diff --git a/packages/CodingStyle/src/Rector/Use_/RemoveUnusedAliasRector.php b/packages/CodingStyle/src/Rector/Use_/RemoveUnusedAliasRector.php index 04cb58404b64..72e1563ea11f 100644 --- a/packages/CodingStyle/src/Rector/Use_/RemoveUnusedAliasRector.php +++ b/packages/CodingStyle/src/Rector/Use_/RemoveUnusedAliasRector.php @@ -298,7 +298,7 @@ private function resolveTraitUseNames(Node $searchNode): void private function resolveDocPossibleAliases(Node $searchNode): void { - $this->traverseNodesWithCallable([$searchNode], function (Node $node): void { + $this->traverseNodesWithCallable($searchNode, function (Node $node): void { if ($node->getDocComment() === null) { return; } diff --git a/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php b/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php new file mode 100644 index 000000000000..e6ae84890fe8 --- /dev/null +++ b/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php @@ -0,0 +1,206 @@ +classManipulator = $classManipulator; + $this->parsedNodesByType = $parsedNodesByType; + } + + public function getDefinition(): RectorDefinition + { + return new RectorDefinition('Removes method that set values that are never used', [ + new CodeSample( + <<<'CODE_SAMPLE' +class SomeClass +{ + private $name; + + public function setName($name) + { + $this->name = $name; + } +} + +class ActiveOnlySetter +{ + public function run() + { + $someClass = new SomeClass(); + $someClass->setName('Tom'); + } +} +CODE_SAMPLE + , + <<<'CODE_SAMPLE' +class SomeClass +{ +} + +class ActiveOnlySetter +{ + public function run() + { + $someClass = new SomeClass(); + } +} +CODE_SAMPLE + ), + ]); + } + + /** + * @return string[] + */ + public function getNodeTypes(): array + { + return [Class_::class]; + } + + /** + * @param Class_ $node + */ + public function refactor(Node $node): ?Node + { + $this->methodCallNamesToBeRemoved = []; + + // 1. get assign only private properties + $assignOnlyPrivatePropertyNames = $this->classManipulator->getAssignOnlyPrivatePropertyNames($node); + $this->classManipulator->removeProperties($node, $assignOnlyPrivatePropertyNames); + + // 2. remove assigns + class methods with only setter assign + $this->removePropertyAssigns($node, $assignOnlyPrivatePropertyNames); + + // 3. remove setter method calls + $this->removeSetterMethodCalls($node); + + return $node; + } + + /** + * @param string[] $assignOnlyPrivatePropertyNames + */ + private function removePropertyAssigns(Class_ $class, array $assignOnlyPrivatePropertyNames): void + { + $this->traverseNodesWithCallable($class, function (Node $node) use ($assignOnlyPrivatePropertyNames): void { + if ($this->isClassMethodWithSinglePropertyAssignOfNames($node, $assignOnlyPrivatePropertyNames)) { + /** @var string $classMethodName */ + $classMethodName = $this->getName($node); + $this->methodCallNamesToBeRemoved[] = $classMethodName; + + $this->removeNode($node); + return; + } + + if ($this->isPropertyAssignWithPropertyNames($node, $assignOnlyPrivatePropertyNames)) { + $this->removeNode($node); + } + }); + } + + private function removeSetterMethodCalls(Node $node): void + { + /** @var string $className */ + $className = $this->getName($node); + $methodCallsByMethodName = $this->parsedNodesByType->findMethodCallsOnClass($className); + + /** @var string $methodName */ + foreach ($methodCallsByMethodName as $methodName => $classMethodCalls) { + if (! in_array($methodName, $this->methodCallNamesToBeRemoved, true)) { + continue; + } + + foreach ($classMethodCalls as $classMethodCall) { + $this->removeNode($classMethodCall); + } + } + } + + /** + * Looks for: + * + * public function ($value) + * { + * $this->value = $value + * } + * + * @param string[] $propertyNames + */ + private function isClassMethodWithSinglePropertyAssignOfNames(Node $node, array $propertyNames): bool + { + if (! $node instanceof ClassMethod) { + return false; + } + + if (count((array) $node->stmts) !== 1) { + return false; + } + + if (! $node->stmts[0] instanceof Expression) { + return false; + } + + /** @var Expression $onlyExpression */ + $onlyExpression = $node->stmts[0]; + + $onlyStmt = $onlyExpression->expr; + + return $this->isPropertyAssignWithPropertyNames($onlyStmt, $propertyNames); + } + + /** + * Is: "$this->value = <$value>" + * + * @param string[] $propertyNames + */ + private function isPropertyAssignWithPropertyNames(Node $node, array $propertyNames): bool + { + if (! $node instanceof Assign) { + return false; + } + + if (! $node->var instanceof PropertyFetch) { + return false; + } + + $propertyFetch = $node->var; + if (! $this->isName($propertyFetch->var, 'this')) { + return false; + } + + return $this->isNames($propertyFetch->name, $propertyNames); + } +} diff --git a/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/fixture.php.inc b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/fixture.php.inc new file mode 100644 index 000000000000..fd65f6a1520e --- /dev/null +++ b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/fixture.php.inc @@ -0,0 +1,42 @@ +name = $name; + } +} + +class ActiveOnlySetter +{ + public function run() + { + $someClass = new SomeClass(); + $someClass->setName('Tom'); + } +} + +?> +----- + diff --git a/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_static_property.php.inc b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_static_property.php.inc new file mode 100644 index 000000000000..4e7dad4ede29 --- /dev/null +++ b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_static_property.php.inc @@ -0,0 +1,17 @@ +doTestFiles([ + __DIR__ . '/Fixture/fixture.php.inc', + __DIR__ . '/Fixture/keep_static_property.php.inc', + ]); + } + + protected function getRectorClass(): string + { + return RemoveSetterOnlyPropertyAndMethodCallRector::class; + } +} diff --git a/packages/Php/src/Rector/FuncCall/ParseStrWithResultArgumentRector.php b/packages/Php/src/Rector/FuncCall/ParseStrWithResultArgumentRector.php index 2dae73d82ea3..949502e9ff07 100644 --- a/packages/Php/src/Rector/FuncCall/ParseStrWithResultArgumentRector.php +++ b/packages/Php/src/Rector/FuncCall/ParseStrWithResultArgumentRector.php @@ -70,9 +70,7 @@ public function refactor(Node $node): ?Node return null; } - $this->traverseNodesWithCallable([$nextExpression], function (Node $node) use ( - $resultVariable - ): ?Variable { + $this->traverseNodesWithCallable($nextExpression, function (Node $node) use ($resultVariable): ?Variable { if ($node instanceof FuncCall) { if ($this->isName($node, 'get_defined_vars')) { return $resultVariable; diff --git a/packages/Php/src/Rector/FuncCall/PregReplaceEModifierRector.php b/packages/Php/src/Rector/FuncCall/PregReplaceEModifierRector.php index 1e98158bd668..86d0ca13e3f7 100644 --- a/packages/Php/src/Rector/FuncCall/PregReplaceEModifierRector.php +++ b/packages/Php/src/Rector/FuncCall/PregReplaceEModifierRector.php @@ -123,7 +123,7 @@ private function createAnonymousFunctionFromString(Expr $expr): ?Closure $stmt = $contentNodes[0]->expr; - $this->traverseNodesWithCallable([$stmt], function (Node $node): Node { + $this->traverseNodesWithCallable($stmt, function (Node $node): Node { if (! $node instanceof String_) { return $node; } diff --git a/packages/PhpSpecToPHPUnit/src/PhpSpecMockCollector.php b/packages/PhpSpecToPHPUnit/src/PhpSpecMockCollector.php index f344f6726c80..a6b39bdffc41 100644 --- a/packages/PhpSpecToPHPUnit/src/PhpSpecMockCollector.php +++ b/packages/PhpSpecToPHPUnit/src/PhpSpecMockCollector.php @@ -56,7 +56,7 @@ public function resolveClassMocksFromParam(Class_ $class): array return $this->mocks[$className]; } - $this->callableNodeTraverser->traverseNodesWithCallable((array) $class->stmts, function (Node $node): void { + $this->callableNodeTraverser->traverseNodesWithCallable($class, function (Node $node): void { if (! $node instanceof ClassMethod) { return; } diff --git a/packages/Twig/src/Rector/SimpleFunctionAndFilterRector.php b/packages/Twig/src/Rector/SimpleFunctionAndFilterRector.php index 9c6d0c355521..6c09bd9d7214 100644 --- a/packages/Twig/src/Rector/SimpleFunctionAndFilterRector.php +++ b/packages/Twig/src/Rector/SimpleFunctionAndFilterRector.php @@ -124,7 +124,7 @@ public function refactor(Node $node): ?Node return null; } - $this->traverseNodesWithCallable([$node->expr], function (Node $node): ?Node { + $this->traverseNodesWithCallable($node->expr, function (Node $node): ?Node { if (! $node instanceof ArrayItem) { return null; } diff --git a/phpstan.neon b/phpstan.neon index f184fed4e428..067b9f00e260 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -186,3 +186,6 @@ parameters: # symfony future compatibility - '#Call to an undefined static method Symfony\\Component\\EventDispatcher\\EventDispatcher\:\:__construct\(\)#' - '#Rector\\EventDispatcher\\AutowiredEventDispatcher\:\:__construct\(\) calls parent constructor but parent does not have one#' + - '#In method "Rector\\Rector\\AbstractRector\:\:traverseNodesWithCallable", parameter \$nodes has no type\-hint and no @param annotation\. More info\: http\://bit\.ly/usetypehint#' + - '#In method "Rector\\FileSystemRector\\Rector\\AbstractFileSystemRector\:\:traverseNodesWithCallable", parameter \$nodes has no type\-hint and no @param annotation\. More info\: http\://bit\.ly/usetypehint#' + - '#Ternary operator condition is always true#' diff --git a/src/Configuration/Configuration.php b/src/Configuration/Configuration.php index b9d7e1e28d66..8cb0571e160d 100644 --- a/src/Configuration/Configuration.php +++ b/src/Configuration/Configuration.php @@ -22,17 +22,6 @@ final class Configuration */ private $hideAutoloadErrors = false; - /** - * Files and directories to by analysed - * @var string[] - */ - private $source = []; - - /** - * @var string - */ - private $outputFormat; - /** * @var string|null */ @@ -54,9 +43,7 @@ final class Configuration public function resolveFromInput(InputInterface $input): void { $this->isDryRun = (bool) $input->getOption(Option::OPTION_DRY_RUN); - $this->source = (array) $input->getArgument(Option::SOURCE); $this->hideAutoloadErrors = (bool) $input->getOption(Option::HIDE_AUTOLOAD_ERRORS); - $this->outputFormat = (string) $input->getOption(Option::OPTION_OUTPUT_FORMAT); $this->showProgressBar = $this->canShowProgressBar($input); $this->setRule($input->getOption(Option::OPTION_RULE)); @@ -82,11 +69,6 @@ public function getConfigFilePath(): ?string return $this->configFilePath; } - public function getOutputFormat(): string - { - return $this->outputFormat; - } - public function getPrettyVersion(): string { $version = PrettyVersions::getVersion('rector/rector'); @@ -107,14 +89,6 @@ public function isDryRun(): bool return $this->isDryRun; } - /** - * @return string[] - */ - public function getSource(): array - { - return $this->source; - } - public function shouldHideAutoloadErrors(): bool { return $this->hideAutoloadErrors; diff --git a/src/Console/Command/AbstractCommand.php b/src/Console/Command/AbstractCommand.php index d785a53a3d16..185d80a499f0 100644 --- a/src/Console/Command/AbstractCommand.php +++ b/src/Console/Command/AbstractCommand.php @@ -20,7 +20,7 @@ abstract class AbstractCommand extends Command /** * @required */ - public function setDescriptor(TextDescriptor $textDescriptor): void + public function autowireDescriptor(TextDescriptor $textDescriptor): void { $this->textDescriptor = $textDescriptor; } diff --git a/src/PhpParser/Node/BetterNodeFinder.php b/src/PhpParser/Node/BetterNodeFinder.php index 7d3ec1637480..806da82064d1 100644 --- a/src/PhpParser/Node/BetterNodeFinder.php +++ b/src/PhpParser/Node/BetterNodeFinder.php @@ -3,16 +3,12 @@ namespace Rector\PhpParser\Node; use PhpParser\Node; -use PhpParser\Node\Expr\ArrayDimFetch; -use PhpParser\Node\Expr\Assign; -use PhpParser\Node\Expr\Variable; use PhpParser\Node\Stmt; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\Expression; use PhpParser\NodeFinder; use Rector\NodeTypeResolver\Node\AttributeKey; -use Rector\PhpParser\Printer\BetterStandardPrinter; final class BetterNodeFinder { @@ -21,15 +17,9 @@ final class BetterNodeFinder */ private $nodeFinder; - /** - * @var BetterStandardPrinter - */ - private $betterStandardPrinter; - - public function __construct(NodeFinder $nodeFinder, BetterStandardPrinter $betterStandardPrinter) + public function __construct(NodeFinder $nodeFinder) { $this->nodeFinder = $nodeFinder; - $this->betterStandardPrinter = $betterStandardPrinter; } /** @@ -185,26 +175,6 @@ public function findFirstPrevious(Node $node, callable $filter): ?Node return $this->findFirstPrevious($previousExpression, $filter); } - /** - * @return Assign[] - */ - public function findAssignsOfVariable(Node $node, Variable $variable): array - { - $assignNodes = $this->findInstanceOf($node, Assign::class); - - return array_filter($assignNodes, function (Assign $assign) use ($variable): bool { - if ($this->betterStandardPrinter->areNodesEqual($assign->var, $variable)) { - return true; - } - - if ($assign->var instanceof ArrayDimFetch) { - return $this->betterStandardPrinter->areNodesEqual($assign->var->var, $variable); - } - - return false; - }); - } - /** * @param string[] $types */ diff --git a/src/PhpParser/Node/Commander/NodeRemovingCommander.php b/src/PhpParser/Node/Commander/NodeRemovingCommander.php index a5ea02c69ce8..ccf722cdb33d 100644 --- a/src/PhpParser/Node/Commander/NodeRemovingCommander.php +++ b/src/PhpParser/Node/Commander/NodeRemovingCommander.php @@ -15,6 +15,7 @@ use Rector\NodeTypeResolver\Node\AttributeKey; use Rector\PhpParser\Node\NodeFactory; use Rector\PhpParser\Node\Resolver\NameResolver; +use Rector\Reporting\RemovedNodesCollector; final class NodeRemovingCommander implements CommanderInterface { @@ -33,14 +34,25 @@ final class NodeRemovingCommander implements CommanderInterface */ private $nameResolver; - public function __construct(NodeFactory $nodeFactory, NameResolver $nameResolver) - { + /** + * @var RemovedNodesCollector + */ + private $removedNodesCollector; + + public function __construct( + NodeFactory $nodeFactory, + NameResolver $nameResolver, + RemovedNodesCollector $removedNodesCollector + ) { $this->nodeFactory = $nodeFactory; $this->nameResolver = $nameResolver; + $this->removedNodesCollector = $removedNodesCollector; } public function addNode(Node $node): void { + $this->removedNodesCollector->collect($node); + // chain call: "->method()->another()" if ($node instanceof MethodCall && $node->var instanceof MethodCall) { throw new ShouldNotHappenException( diff --git a/src/PhpParser/Node/Manipulator/ClassManipulator.php b/src/PhpParser/Node/Manipulator/ClassManipulator.php index ef9587d56d7c..49f8ae4d26bb 100644 --- a/src/PhpParser/Node/Manipulator/ClassManipulator.php +++ b/src/PhpParser/Node/Manipulator/ClassManipulator.php @@ -5,6 +5,7 @@ use PhpParser\Node; use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\PropertyFetch; +use PhpParser\Node\Expr\StaticPropertyFetch; use PhpParser\Node\Name; use PhpParser\Node\Param; use PhpParser\Node\Stmt; @@ -17,10 +18,13 @@ use PhpParser\Node\Stmt\PropertyProperty; use PhpParser\Node\Stmt\Trait_; use PhpParser\Node\Stmt\TraitUse; +use Rector\NodeTypeResolver\Node\AttributeKey; use Rector\PhpParser\Node\BetterNodeFinder; +use Rector\PhpParser\Node\Commander\NodeRemovingCommander; use Rector\PhpParser\Node\NodeFactory; use Rector\PhpParser\Node\Resolver\NameResolver; use Rector\PhpParser\Node\VariableInfo; +use Rector\PhpParser\NodeTraverser\CallableNodeTraverser; final class ClassManipulator { @@ -44,16 +48,30 @@ final class ClassManipulator */ private $betterNodeFinder; + /** + * @var CallableNodeTraverser + */ + private $callableNodeTraverser; + + /** + * @var NodeRemovingCommander + */ + private $nodeRemovingCommander; + public function __construct( NameResolver $nameResolver, NodeFactory $nodeFactory, ChildAndParentClassManipulator $childAndParentClassManipulator, - BetterNodeFinder $betterNodeFinder + BetterNodeFinder $betterNodeFinder, + CallableNodeTraverser $callableNodeTraverser, + NodeRemovingCommander $nodeRemovingCommander ) { $this->nodeFactory = $nodeFactory; $this->nameResolver = $nameResolver; $this->childAndParentClassManipulator = $childAndParentClassManipulator; $this->betterNodeFinder = $betterNodeFinder; + $this->callableNodeTraverser = $callableNodeTraverser; + $this->nodeRemovingCommander = $nodeRemovingCommander; } public function addConstructorDependency(Class_ $classNode, VariableInfo $variableInfo): void @@ -263,18 +281,7 @@ public function hasPropertyFetchAsProperty(Class_ $class, PropertyFetch $propert public function removeProperty(Class_ $class, string $propertyName): void { - foreach ($class->stmts as $key => $classStmt) { - if (! $classStmt instanceof Property) { - continue; - } - - if (! $this->nameResolver->isName($classStmt, $propertyName)) { - continue; - } - - unset($class->stmts[$key]); - break; - } + $this->removeProperties($class, [$propertyName]); } public function findMethodParamByName(ClassMethod $classMethod, string $name): ?Param @@ -337,6 +344,50 @@ public function getPublicMethodNames(Class_ $class): array return $publicMethodNames; } + /** + * @return string[] + */ + public function getAssignOnlyPrivatePropertyNames(Class_ $node): array + { + $privatePropertyNames = $this->getPrivatePropertyNames($node); + + $propertyNonAssignNames = []; + + $this->callableNodeTraverser->traverseNodesWithCallable([$node], function (Node $node) use ( + &$propertyNonAssignNames + ): void { + if (! $node instanceof PropertyFetch && ! $node instanceof StaticPropertyFetch) { + return; + } + + if (! $this->isNonAssignPropertyFetch($node)) { + return; + } + + $propertyNonAssignNames[] = $this->nameResolver->getName($node); + }); + + return array_diff($privatePropertyNames, $propertyNonAssignNames); + } + + /** + * @param string[] $propertyNames + */ + public function removeProperties(Class_ $class, array $propertyNames): void + { + $this->callableNodeTraverser->traverseNodesWithCallable($class, function (Node $node) use ($propertyNames) { + if (! $node instanceof Property) { + return null; + } + + if (! $this->nameResolver->isNames($node, $propertyNames)) { + return null; + } + + $this->removeNode($node); + }); + } + private function tryInsertBeforeFirstMethod(Class_ $classNode, Stmt $stmt): bool { foreach ($classNode->stmts as $key => $classElementNode) { @@ -437,4 +488,42 @@ private function hasMethodParameter(ClassMethod $classMethod, VariableInfo $vari return false; } + + private function removeNode(Node $node): void + { + $this->nodeRemovingCommander->addNode($node); + } + + /** + * @param PropertyFetch|StaticPropertyFetch $node + */ + private function isNonAssignPropertyFetch(Node $node): bool + { + if ($node instanceof PropertyFetch) { + if (! $this->nameResolver->isName($node->var, 'this')) { + return false; + } + + // is "$this->property = x;" assign + return ! $this->isNodeLeftPartOfAssign($node); + } + + if ($node instanceof StaticPropertyFetch) { + if (! $this->nameResolver->isName($node->class, 'self')) { + return false; + } + + // is "self::$property = x;" assign + return ! $this->isNodeLeftPartOfAssign($node); + } + + return false; + } + + private function isNodeLeftPartOfAssign(Node $node): bool + { + $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE); + + return $parentNode instanceof Assign && $parentNode->var === $node; + } } diff --git a/src/PhpParser/NodeTraverser/CallableNodeTraverser.php b/src/PhpParser/NodeTraverser/CallableNodeTraverser.php index 34717cb5a25e..15c1accfe113 100644 --- a/src/PhpParser/NodeTraverser/CallableNodeTraverser.php +++ b/src/PhpParser/NodeTraverser/CallableNodeTraverser.php @@ -10,10 +10,14 @@ final class CallableNodeTraverser { /** - * @param Node[] $nodes + * @param Node|Node[] $nodes */ - public function traverseNodesWithCallable(array $nodes, callable $callable): void + public function traverseNodesWithCallable($nodes, callable $callable): void { + if (! is_array($nodes)) { + $nodes = $nodes ? [$nodes] : []; + } + $nodeTraverser = new NodeTraverser(); $nodeTraverser->addVisitor($this->createNodeVisitor($callable)); $nodeTraverser->traverse($nodes); diff --git a/src/Rector/AbstractRector/CallableNodeTraverserTrait.php b/src/Rector/AbstractRector/CallableNodeTraverserTrait.php index bed113918f62..48e9aa7af8ea 100644 --- a/src/Rector/AbstractRector/CallableNodeTraverserTrait.php +++ b/src/Rector/AbstractRector/CallableNodeTraverserTrait.php @@ -25,9 +25,9 @@ public function setCallableNodeTraverser(CallableNodeTraverser $callableNodeTrav } /** - * @param Node[] $nodes + * @param Node|Node[] $nodes */ - public function traverseNodesWithCallable(array $nodes, callable $callable): void + public function traverseNodesWithCallable($nodes, callable $callable): void { $this->callableNodeTraverser->traverseNodesWithCallable($nodes, $callable); } diff --git a/src/Rector/AbstractRector/NodeCommandersTrait.php b/src/Rector/AbstractRector/NodeCommandersTrait.php index d31e49f722f3..62c30b75f176 100644 --- a/src/Rector/AbstractRector/NodeCommandersTrait.php +++ b/src/Rector/AbstractRector/NodeCommandersTrait.php @@ -10,7 +10,6 @@ use Rector\PhpParser\Node\Commander\NodeRemovingCommander; use Rector\PhpParser\Node\Commander\PropertyAddingCommander; use Rector\PhpParser\Node\VariableInfo; -use Rector\Reporting\RemovedNodesCollector; /** * This could be part of @see AbstractRector, but decopuling to trait @@ -40,11 +39,6 @@ trait NodeCommandersTrait */ private $useAddingCommander; - /** - * @var RemovedNodesCollector - */ - private $removedNodesCollector; - /** * @required */ @@ -52,14 +46,12 @@ public function autowireNodeCommandersTrait( NodeRemovingCommander $nodeRemovingCommander, NodeAddingCommander $nodeAddingCommander, PropertyAddingCommander $propertyAddingCommander, - UseAddingCommander $useAddingCommander, - RemovedNodesCollector $removedNodesCollector + UseAddingCommander $useAddingCommander ): void { $this->nodeRemovingCommander = $nodeRemovingCommander; $this->nodeAddingCommander = $nodeAddingCommander; $this->propertyAddingCommander = $propertyAddingCommander; $this->useAddingCommander = $useAddingCommander; - $this->removedNodesCollector = $removedNodesCollector; } protected function addNodeAfterNode(Node $newNode, Node $positionNode): void @@ -89,8 +81,6 @@ protected function removeNode(Node $node): void $this->nodeRemovingCommander->addNode($node); $this->notifyNodeChangeFileInfo($node); - - $this->removedNodesCollector->collect($node); } protected function isNodeRemoved(Node $node): bool @@ -98,16 +88,6 @@ protected function isNodeRemoved(Node $node): bool return $this->nodeRemovingCommander->isNodeRemoved($node); } - protected function addUseImport(Node $node, string $useImport): void - { - $this->useAddingCommander->addUseImport($node, $useImport); - } - - protected function addFunctionUseImport(Node $node, string $functionUseImport): void - { - $this->useAddingCommander->addFunctionUseImport($node, $functionUseImport); - } - /** * @param Node[] $nodes */ From fb8af74d4ab25272f162607952bd9a83c994c42f Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Tue, 6 Aug 2019 14:01:17 +0200 Subject: [PATCH 2/4] add spaceflow sponsor --- .../Rector/String_/ManualJsonStringToJsonEncodeArrayRector.php | 1 + .../Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php | 1 + .../RemoveUnusedDoctrineEntityMethodAndPropertyRector.php | 1 + .../src/Rector/Property/PropertyTypeDeclarationRector.php | 3 +++ 4 files changed, 6 insertions(+) diff --git a/packages/CodingStyle/src/Rector/String_/ManualJsonStringToJsonEncodeArrayRector.php b/packages/CodingStyle/src/Rector/String_/ManualJsonStringToJsonEncodeArrayRector.php index bd3e53b9f787..48342cfdf8f0 100644 --- a/packages/CodingStyle/src/Rector/String_/ManualJsonStringToJsonEncodeArrayRector.php +++ b/packages/CodingStyle/src/Rector/String_/ManualJsonStringToJsonEncodeArrayRector.php @@ -22,6 +22,7 @@ use Rector\RectorDefinition\RectorDefinition; /** + * @sponsor Thanks https://spaceflow.io/ for sponsoring this rule - visit them on https://github.com/SpaceFlow-app * @see \Rector\CodingStyle\Tests\Rector\String_\ManualJsonStringToJsonEncodeArrayRector\ManualJsonStringToJsonEncodeArrayRectorTest */ final class ManualJsonStringToJsonEncodeArrayRector extends AbstractRector diff --git a/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php b/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php index e6ae84890fe8..249677a7b443 100644 --- a/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php +++ b/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php @@ -15,6 +15,7 @@ use Rector\RectorDefinition\RectorDefinition; /** + * @sponsor Thanks https://spaceflow.io/ for sponsoring this rule - visit them on https://github.com/SpaceFlow-app * @see \Rector\DeadCode\Tests\Rector\Class_\RemoveSetterOnlyPropertyAndMethodCallRector\RemoveSetterOnlyPropertyAndMethodCallRectorTest */ final class RemoveSetterOnlyPropertyAndMethodCallRector extends AbstractRector diff --git a/packages/DeadCode/src/Rector/Class_/RemoveUnusedDoctrineEntityMethodAndPropertyRector.php b/packages/DeadCode/src/Rector/Class_/RemoveUnusedDoctrineEntityMethodAndPropertyRector.php index 3b1087c11c47..49c2f1fcbf9c 100644 --- a/packages/DeadCode/src/Rector/Class_/RemoveUnusedDoctrineEntityMethodAndPropertyRector.php +++ b/packages/DeadCode/src/Rector/Class_/RemoveUnusedDoctrineEntityMethodAndPropertyRector.php @@ -19,6 +19,7 @@ use Rector\RectorDefinition\RectorDefinition; /** + * @sponsor Thanks https://spaceflow.io/ for sponsoring this rule - visit them on https://github.com/SpaceFlow-app * @see \Rector\DeadCode\Tests\Rector\Class_\RemoveUnusedDoctrineEntityMethodAndPropertyRector\RemoveUnusedDoctrineEntityMethodAndPropertyRectorTest */ final class RemoveUnusedDoctrineEntityMethodAndPropertyRector extends AbstractRector diff --git a/packages/TypeDeclaration/src/Rector/Property/PropertyTypeDeclarationRector.php b/packages/TypeDeclaration/src/Rector/Property/PropertyTypeDeclarationRector.php index a6cc8e1190ba..4db17c7357af 100644 --- a/packages/TypeDeclaration/src/Rector/Property/PropertyTypeDeclarationRector.php +++ b/packages/TypeDeclaration/src/Rector/Property/PropertyTypeDeclarationRector.php @@ -13,6 +13,9 @@ use Rector\TypeDeclaration\Exception\ConflictingPriorityException; use Rector\TypeDeclaration\ValueObject\IdentifierValueObject; +/** + * @sponsor Thanks https://spaceflow.io/ for sponsoring this rule - visit them on https://github.com/SpaceFlow-app + */ final class PropertyTypeDeclarationRector extends AbstractRector { /** From 73ac67d5ef8465f565b17297dade0ee935e572fc Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Tue, 6 Aug 2019 14:08:17 +0200 Subject: [PATCH 3/4] skip __construct --- ...eSetterOnlyPropertyAndMethodCallRector.php | 4 +++ .../Fixture/in_constructor.php.inc | 28 +++++++++++++++++++ .../Fixture/keep_public_property.php.inc | 8 ++++++ ...terOnlyPropertyAndMethodCallRectorTest.php | 2 ++ 4 files changed, 42 insertions(+) create mode 100644 packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/in_constructor.php.inc create mode 100644 packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_public_property.php.inc diff --git a/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php b/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php index 249677a7b443..e2f10f998655 100644 --- a/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php +++ b/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php @@ -166,6 +166,10 @@ private function isClassMethodWithSinglePropertyAssignOfNames(Node $node, array return false; } + if ($this->isName($node, '__construct')) { + return false; + } + if (count((array) $node->stmts) !== 1) { return false; } diff --git a/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/in_constructor.php.inc b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/in_constructor.php.inc new file mode 100644 index 000000000000..a53ebdbbf0c5 --- /dev/null +++ b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/in_constructor.php.inc @@ -0,0 +1,28 @@ +name = $name; + } +} + +?> +----- + diff --git a/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_public_property.php.inc b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_public_property.php.inc new file mode 100644 index 000000000000..4b5c1f0593af --- /dev/null +++ b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_public_property.php.inc @@ -0,0 +1,8 @@ +doTestFiles([ __DIR__ . '/Fixture/fixture.php.inc', + __DIR__ . '/Fixture/in_constructor.php.inc', __DIR__ . '/Fixture/keep_static_property.php.inc', + __DIR__ . '/Fixture/keep_public_property.php.inc', ]); } From 2847bf2b8fd8193fd0be0f6eebf63f8db1e13216 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Tue, 6 Aug 2019 14:23:41 +0200 Subject: [PATCH 4/4] skip serialized props --- .../Fixture/keep_serializable_object.php.inc | 19 ++++++++++ ...terOnlyPropertyAndMethodCallRectorTest.php | 1 + .../Node/Manipulator/ClassManipulator.php | 38 ++++++++++++++++++- 3 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_serializable_object.php.inc diff --git a/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_serializable_object.php.inc b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_serializable_object.php.inc new file mode 100644 index 000000000000..9e5c1b084a48 --- /dev/null +++ b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/keep_serializable_object.php.inc @@ -0,0 +1,19 @@ +id = $id; + } +} diff --git a/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/RemoveSetterOnlyPropertyAndMethodCallRectorTest.php b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/RemoveSetterOnlyPropertyAndMethodCallRectorTest.php index 3d4984e608ae..5227c5e45b42 100644 --- a/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/RemoveSetterOnlyPropertyAndMethodCallRectorTest.php +++ b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/RemoveSetterOnlyPropertyAndMethodCallRectorTest.php @@ -14,6 +14,7 @@ public function test(): void __DIR__ . '/Fixture/in_constructor.php.inc', __DIR__ . '/Fixture/keep_static_property.php.inc', __DIR__ . '/Fixture/keep_public_property.php.inc', + __DIR__ . '/Fixture/keep_serializable_object.php.inc', ]); } diff --git a/src/PhpParser/Node/Manipulator/ClassManipulator.php b/src/PhpParser/Node/Manipulator/ClassManipulator.php index 49f8ae4d26bb..48d99da8417e 100644 --- a/src/PhpParser/Node/Manipulator/ClassManipulator.php +++ b/src/PhpParser/Node/Manipulator/ClassManipulator.php @@ -19,6 +19,7 @@ use PhpParser\Node\Stmt\Trait_; use PhpParser\Node\Stmt\TraitUse; use Rector\NodeTypeResolver\Node\AttributeKey; +use Rector\NodeTypeResolver\PhpDoc\NodeAnalyzer\DocBlockManipulator; use Rector\PhpParser\Node\BetterNodeFinder; use Rector\PhpParser\Node\Commander\NodeRemovingCommander; use Rector\PhpParser\Node\NodeFactory; @@ -58,13 +59,19 @@ final class ClassManipulator */ private $nodeRemovingCommander; + /** + * @var DocBlockManipulator + */ + private $docBlockManipulator; + public function __construct( NameResolver $nameResolver, NodeFactory $nodeFactory, ChildAndParentClassManipulator $childAndParentClassManipulator, BetterNodeFinder $betterNodeFinder, CallableNodeTraverser $callableNodeTraverser, - NodeRemovingCommander $nodeRemovingCommander + NodeRemovingCommander $nodeRemovingCommander, + DocBlockManipulator $docBlockManipulator ) { $this->nodeFactory = $nodeFactory; $this->nameResolver = $nameResolver; @@ -72,6 +79,7 @@ public function __construct( $this->betterNodeFinder = $betterNodeFinder; $this->callableNodeTraverser = $callableNodeTraverser; $this->nodeRemovingCommander = $nodeRemovingCommander; + $this->docBlockManipulator = $docBlockManipulator; } public function addConstructorDependency(Class_ $classNode, VariableInfo $variableInfo): void @@ -367,7 +375,10 @@ public function getAssignOnlyPrivatePropertyNames(Class_ $node): array $propertyNonAssignNames[] = $this->nameResolver->getName($node); }); - return array_diff($privatePropertyNames, $propertyNonAssignNames); + // skip serializable properties, because they are probably used in serialization even though assign only + $serializablePropertyNames = $this->getSerializablePropertyNames($node); + + return array_diff($privatePropertyNames, $propertyNonAssignNames, $serializablePropertyNames); } /** @@ -526,4 +537,27 @@ private function isNodeLeftPartOfAssign(Node $node): bool return $parentNode instanceof Assign && $parentNode->var === $node; } + + /** + * @return string[] + */ + private function getSerializablePropertyNames(Class_ $node): array + { + $serializablePropertyNames = []; + $this->callableNodeTraverser->traverseNodesWithCallable([$node], function (Node $node) use ( + &$serializablePropertyNames + ): void { + if (! $node instanceof Property) { + return; + } + + if (! $this->docBlockManipulator->hasTag($node, 'JMS\Serializer\Annotation\Type')) { + return; + } + + $serializablePropertyNames[] = $this->nameResolver->getName($node); + }); + + return $serializablePropertyNames; + } }