Merge pull request #1836 from rectorphp/setter-fix

Fixing NodeRemovingVisitor

packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php

@@ -123,6 +123,7 @@ CODE_SAMPLE
                 $this->methodCallNamesToBeRemoved[] = $classMethodName;
 
                 $this->removeNode($node);
+
                 return;
             }
 

src/Application/ErrorAndDiffCollector.php

@@ -2,12 +2,13 @@
 
 namespace Rector\Application;
 
+use PhpParser\Node;
 use PHPStan\AnalysedCodeException;
 use Rector\Application\FileSystem\RemovedAndAddedFilesCollector;
 use Rector\ConsoleDiffer\DifferAndFormatter;
 use Rector\Error\ExceptionCorrector;
+use Rector\PhpParser\Node\Commander\NodeRemovingCommander;
 use Rector\Reporting\FileDiff;
-use Rector\Reporting\RemovedNodesCollector;
 use Symplify\PackageBuilder\FileSystem\SmartFileInfo;
 use Throwable;
 
@@ -44,22 +45,22 @@ final class ErrorAndDiffCollector
     private $removedAndAddedFilesCollector;
 
     /**
-     * @var RemovedNodesCollector
+     * @var NodeRemovingCommander
      */
-    private $removedNodesCollector;
+    private $nodeRemovingCommander;
 
     public function __construct(
         DifferAndFormatter $differAndFormatter,
         AppliedRectorCollector $appliedRectorCollector,
         ExceptionCorrector $exceptionCorrector,
         RemovedAndAddedFilesCollector $removedAndAddedFilesCollector,
-        RemovedNodesCollector $removedNodesCollector
+        NodeRemovingCommander $nodeRemovingCommander
     ) {
         $this->differAndFormatter = $differAndFormatter;
         $this->appliedRectorCollector = $appliedRectorCollector;
         $this->exceptionCorrector = $exceptionCorrector;
         $this->removedAndAddedFilesCollector = $removedAndAddedFilesCollector;
-        $this->removedNodesCollector = $removedNodesCollector;
+        $this->nodeRemovingCommander = $nodeRemovingCommander;
     }
 
     public function addError(Error $error): void
@@ -82,7 +83,15 @@ final class ErrorAndDiffCollector
 
     public function getRemovedNodeCount(): int
     {
-        return $this->removedNodesCollector->getCount();
+        return $this->nodeRemovingCommander->getCount();
+    }
+
+    /**
+     * @return Node[]
+     */
+    public function getRemovedNodes(): array
+    {
+        return $this->nodeRemovingCommander->getNodesToRemove();
     }
 
     public function addFileDiff(SmartFileInfo $smartFileInfo, string $newContent, string $oldContent): void

src/Console/Output/ConsoleOutputFormatter.php

@@ -2,11 +2,15 @@
 
 namespace Rector\Console\Output;
 
+use Nette\Utils\Strings;
 use Rector\Application\Error;
 use Rector\Application\ErrorAndDiffCollector;
 use Rector\Contract\Console\Output\OutputFormatterInterface;
+use Rector\NodeTypeResolver\Node\AttributeKey;
+use Rector\PhpParser\Printer\BetterStandardPrinter;
 use Rector\Reporting\FileDiff;
 use Symfony\Component\Console\Style\SymfonyStyle;
+use Symplify\PackageBuilder\FileSystem\SmartFileInfo;
 
 final class ConsoleOutputFormatter implements OutputFormatterInterface
 {
@@ -20,9 +24,15 @@ final class ConsoleOutputFormatter implements OutputFormatterInterface
      */
     private $symfonyStyle;
 
-    public function __construct(SymfonyStyle $symfonyStyle)
+    /**
+     * @var BetterStandardPrinter
+     */
+    private $betterStandardPrinter;
+
+    public function __construct(SymfonyStyle $symfonyStyle, BetterStandardPrinter $betterStandardPrinter)
     {
         $this->symfonyStyle = $symfonyStyle;
+        $this->betterStandardPrinter = $betterStandardPrinter;
     }
 
     public function report(ErrorAndDiffCollector $errorAndDiffCollector): void
@@ -110,8 +120,44 @@ final class ConsoleOutputFormatter implements OutputFormatterInterface
             );
         }
 
-        if ($errorAndDiffCollector->getRemovedNodeCount()) {
-            $this->symfonyStyle->note(sprintf('%d nodes were removed', $errorAndDiffCollector->getRemovedNodeCount()));
+        $this->reportRemovedNodes($errorAndDiffCollector);
+    }
+
+    private function reportRemovedNodes(ErrorAndDiffCollector $errorAndDiffCollector): void
+    {
+        if ($errorAndDiffCollector->getRemovedNodeCount() === 0) {
+            return;
+        }
+
+        $this->symfonyStyle->warning(sprintf('%d nodes were removed', $errorAndDiffCollector->getRemovedNodeCount()));
+
+        if ($this->symfonyStyle->isVeryVerbose()) {
+            $i = 0;
+            foreach ($errorAndDiffCollector->getRemovedNodes() as $removedNode) {
+                /** @var SmartFileInfo $fileInfo */
+                $fileInfo = $removedNode->getAttribute(AttributeKey::FILE_INFO);
+
+                $this->symfonyStyle->writeln(sprintf(
+                    '<options=bold>%d) %s:%d</>',
+                    ++$i,
+                    $fileInfo->getRelativeFilePath(),
+                    $removedNode->getStartLine()
+                ));
+
+                $printedNode = $this->betterStandardPrinter->print($removedNode);
+
+                // color red + prefix with "-" to visually demonstrate removal
+                $printedNode = '-' . Strings::replace($printedNode, '#\n#', "\n-");
+                $printedNode = $this->colorTextToRed($printedNode);
+
+                $this->symfonyStyle->writeln($printedNode);
+                $this->symfonyStyle->newLine(1);
+            }
         }
     }
+
+    private function colorTextToRed(string $text): string
+    {
+        return '<fg=red>' . $text . '</fg=red>';
+    }
 }

src/PhpParser/Node/Commander/NodeRemovingCommander.php

@@ -3,19 +3,14 @@
 namespace Rector\PhpParser\Node\Commander;
 
 use PhpParser\Node;
-use PhpParser\Node\Expr;
 use PhpParser\Node\Expr\MethodCall;
 use PhpParser\Node\Stmt;
 use PhpParser\Node\Stmt\Expression;
 use PhpParser\NodeTraverser;
-use PhpParser\NodeVisitor;
-use PhpParser\NodeVisitorAbstract;
 use Rector\Contract\PhpParser\Node\CommanderInterface;
 use Rector\Exception\ShouldNotHappenException;
 use Rector\NodeTypeResolver\Node\AttributeKey;
-use Rector\PhpParser\Node\NodeFactory;
-use Rector\PhpParser\Node\Resolver\NameResolver;
-use Rector\Reporting\RemovedNodesCollector;
+use Rector\PhpParser\Node\NodeVisitorFactory\NodeRemovingNodeVisitorFactory;
 
 final class NodeRemovingCommander implements CommanderInterface
 {
@@ -25,46 +20,24 @@ final class NodeRemovingCommander implements CommanderInterface
     private $nodesToRemove = [];
 
     /**
-     * @var NodeFactory
+     * @var NodeRemovingNodeVisitorFactory
      */
-    private $nodeFactory;
+    private $nodeRemovingNodeVisitorFactory;
 
-    /**
-     * @var NameResolver
-     */
-    private $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 __construct(NodeRemovingNodeVisitorFactory $nodeRemovingNodeVisitorFactory)
+    {
+        $this->nodeRemovingNodeVisitorFactory = $nodeRemovingNodeVisitorFactory;
     }
 
     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(
-                'Chain method calls cannot be removed this way. It would remove the whole tree of calls. Remove them manually by creating new parent node with no following method.'
-            );
-        }
+        $this->ensureIsNotPartOfChainMethodCall($node);
 
-        if (! $node instanceof Expression && ($node->getAttribute(
-            AttributeKey::PARENT_NODE
-        ) instanceof Expression)) {
+        $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE);
+        if (! $node instanceof Expression && $parentNode instanceof Expression) {
             // only expressions can be removed
-            $node = $node->getAttribute(AttributeKey::PARENT_NODE);
+            $node = $parentNode;
         }
 
         /** @var Stmt $node */
@@ -78,96 +51,48 @@ final class NodeRemovingCommander implements CommanderInterface
     public function traverseNodes(array $nodes): array
     {
         $nodeTraverser = new NodeTraverser();
-        $nodeTraverser->addVisitor($this->createNodeVisitor());
+
+        $nodeRemovingNodeVisitor = $this->nodeRemovingNodeVisitorFactory->createFromNodesToRemove($this->nodesToRemove);
+        $nodeTraverser->addVisitor($nodeRemovingNodeVisitor);
 
         return $nodeTraverser->traverse($nodes);
     }
 
-    public function isActive(): bool
-    {
-        return count($this->nodesToRemove) > 0;
-    }
-
     public function isNodeRemoved(Node $node): bool
     {
         return in_array($node, $this->nodesToRemove, true);
     }
 
-    private function createNodeVisitor(): NodeVisitor
+    public function isActive(): bool
     {
-        return new class($this->nodesToRemove, $this->nodeFactory, $this->nameResolver) extends NodeVisitorAbstract {
-            /**
-             * @var Stmt[]|Expr[]
-             */
-            private $nodesToRemove = [];
+        return $this->getCount() > 0;
+    }
 
-            /**
-             * @var NodeFactory
-             */
-            private $nodeFactory;
+    public function getCount(): int
+    {
+        return count($this->nodesToRemove);
+    }
 
-            /**
-             * @var NameResolver
-             */
-            private $nameResolver;
+    /**
+     * @return Node[]
+     */
+    public function getNodesToRemove(): array
+    {
+        return $this->nodesToRemove;
+    }
 
-            /**
-             * @param Stmt[] $nodesToRemove
-             */
-            public function __construct(array $nodesToRemove, NodeFactory $nodeFactory, NameResolver $nameResolver)
-            {
-                $this->nodesToRemove = $nodesToRemove;
-                $this->nodeFactory = $nodeFactory;
-                $this->nameResolver = $nameResolver;
-            }
+    private function ensureIsNotPartOfChainMethodCall(Node $node): void
+    {
+        if (! $node instanceof MethodCall) {
+            return;
+        }
 
-            /**
-             * @return int|Node|null
-             */
-            public function enterNode(Node $node)
-            {
-                // special case for fluent methods
-                foreach ($this->nodesToRemove as $key => $nodeToRemove) {
-                    if (! $nodeToRemove instanceof MethodCall) {
-                        continue;
-                    }
+        if (! $node->var instanceof MethodCall) {
+            return;
+        }
 
-                    if (! $node instanceof MethodCall || ! $node->var instanceof MethodCall) {
-                        continue;
-                    }
-
-                    if ($nodeToRemove !== $node->var) {
-                        continue;
-                    }
-
-                    $methodName = $this->nameResolver->getName($node->name);
-                    if ($methodName === null) {
-                        continue;
-                    }
-
-                    unset($this->nodesToRemove[$key]);
-
-                    return $this->nodeFactory->createMethodCall($node->var->var, $methodName, $node->args);
-                }
-
-                return null;
-            }
-
-            /**
-             * @return int|Node|Node[]|null
-             */
-            public function leaveNode(Node $node)
-            {
-                foreach ($this->nodesToRemove as $key => $nodeToRemove) {
-                    if ($node === $nodeToRemove) {
-                        unset($this->nodesToRemove[$key]);
-
-                        return NodeTraverser::REMOVE_NODE;
-                    }
-                }
-
-                return $node;
-            }
-        };
+        throw new ShouldNotHappenException(
+            'Chain method calls cannot be removed this way. It would remove the whole tree of calls. Remove them manually by creating new parent node with no following method.'
+        );
     }
 }

src/PhpParser/Node/NodeVisitor/NodeRemovingNodeVisitor.php

@@ -0,0 +1,88 @@
+<?php declare(strict_types=1);
+
+namespace Rector\PhpParser\Node\NodeVisitor;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr;
+use PhpParser\Node\Expr\MethodCall;
+use PhpParser\Node\Stmt;
+use PhpParser\NodeTraverser;
+use PhpParser\NodeVisitorAbstract;
+use Rector\PhpParser\Node\NodeFactory;
+use Rector\PhpParser\Node\Resolver\NameResolver;
+
+final class NodeRemovingNodeVisitor extends NodeVisitorAbstract
+{
+    /**
+     * @var Stmt[]|Expr[]
+     */
+    private $nodesToRemove = [];
+
+    /**
+     * @var NodeFactory
+     */
+    private $nodeFactory;
+
+    /**
+     * @var NameResolver
+     */
+    private $nameResolver;
+
+    /**
+     * @param Stmt[] $nodesToRemove
+     */
+    public function __construct(array $nodesToRemove, NodeFactory $nodeFactory, NameResolver $nameResolver)
+    {
+        $this->nodesToRemove = $nodesToRemove;
+        $this->nodeFactory = $nodeFactory;
+        $this->nameResolver = $nameResolver;
+    }
+
+    /**
+     * @return int|Node|null
+     */
+    public function enterNode(Node $node)
+    {
+        // special case for fluent methods
+        foreach ($this->nodesToRemove as $key => $nodeToRemove) {
+            if (! $nodeToRemove instanceof MethodCall) {
+                continue;
+            }
+
+            if (! $node instanceof MethodCall || ! $node->var instanceof MethodCall) {
+                continue;
+            }
+
+            if ($nodeToRemove !== $node->var) {
+                continue;
+            }
+
+            $methodName = $this->nameResolver->getName($node->name);
+            if ($methodName === null) {
+                continue;
+            }
+
+            unset($this->nodesToRemove[$key]);
+
+            return $this->nodeFactory->createMethodCall($node->var->var, $methodName, $node->args);
+        }
+
+        return null;
+    }
+
+    /**
+     * @return int|Node|Node[]|null
+     */
+    public function leaveNode(Node $node)
+    {
+        foreach ($this->nodesToRemove as $key => $nodeToRemove) {
+            if ($node === $nodeToRemove) {
+                unset($this->nodesToRemove[$key]);
+
+                return NodeTraverser::REMOVE_NODE;
+            }
+        }
+
+        return $node;
+    }
+}

src/PhpParser/Node/NodeVisitorFactory/NodeRemovingNodeVisitorFactory.php

@@ -0,0 +1,35 @@
+<?php declare(strict_types=1);
+
+namespace Rector\PhpParser\Node\NodeVisitorFactory;
+
+use PhpParser\Node;
+use Rector\PhpParser\Node\NodeFactory;
+use Rector\PhpParser\Node\NodeVisitor\NodeRemovingNodeVisitor;
+use Rector\PhpParser\Node\Resolver\NameResolver;
+
+final class NodeRemovingNodeVisitorFactory
+{
+    /**
+     * @var NodeFactory
+     */
+    private $nodeFactory;
+
+    /**
+     * @var NameResolver
+     */
+    private $nameResolver;
+
+    public function __construct(NodeFactory $nodeFactory, NameResolver $nameResolver)
+    {
+        $this->nodeFactory = $nodeFactory;
+        $this->nameResolver = $nameResolver;
+    }
+
+    /**
+     * @param Node[] $nodesToRemove
+     */
+    public function createFromNodesToRemove(array $nodesToRemove): NodeRemovingNodeVisitor
+    {
+        return new NodeRemovingNodeVisitor($nodesToRemove, $this->nodeFactory, $this->nameResolver);
+    }
+}

src/Reporting/RemovedNodesCollector.php

@@ -1,23 +0,0 @@
-<?php declare(strict_types=1);
-
-namespace Rector\Reporting;
-
-use PhpParser\Node;
-
-final class RemovedNodesCollector
-{
-    /**
-     * @var Node[]
-     */
-    private $removedNodes = [];
-
-    public function collect(Node $node): void
-    {
-        $this->removedNodes[] = $node;
-    }
-
-    public function getCount(): int
-    {
-        return count($this->removedNodes);
-    }
-}