[Code] Improve BetterNodeFinder::findFirstPrevious() to only locate p…

…revious of parent if no previous of Node (#2209) * [Code] Improve BetterNodeFinder::findFirstPrevious() to only locate previous of parent if no previous of Node * [ci-review] Rector Rectify * [ci-review] Rector Rectify * [ci-review] Rector Rectify * part of loop fix on ForeachItemsAssignToEmptyArrayToAssignRector * [ci-review] Rector Rectify * move LOOP_NODES constant to ControlStructure ValueObject * ref fix * [ci-review] Rector Rectify * [ci-review] Rector Rectify * final touch: rename method * final touch: clean up * final touch: clean up unused param * final touch: clean up unused param * [ci-review] Rector Rectify Co-authored-by: GitHub Action <action@github.com>
rectorphp · May 1, 2022 · 22bdd60 · 22bdd60
1 parent 0298204
commit 22bdd60
Show file tree

Hide file tree

Showing 8 changed files with 53 additions and 54 deletions.
diff --git a/packages/NodeNestingScope/ContextAnalyzer.php b/packages/NodeNestingScope/ContextAnalyzer.php
@@ -7,17 +7,13 @@
 use PhpParser\Node;
 use PhpParser\Node\Expr\Assign;
 use PhpParser\Node\FunctionLike;
-use PhpParser\Node\Stmt;
 use PhpParser\Node\Stmt\ClassMethod;
-use PhpParser\Node\Stmt\Do_;
-use PhpParser\Node\Stmt\For_;
-use PhpParser\Node\Stmt\Foreach_;
 use PhpParser\Node\Stmt\If_;
 use PhpParser\Node\Stmt\Return_;
 use PhpParser\Node\Stmt\Switch_;
-use PhpParser\Node\Stmt\While_;
 use PHPStan\Type\ObjectType;
 use Rector\Core\PhpParser\Node\BetterNodeFinder;
+use Rector\NodeNestingScope\ValueObject\ControlStructure;
 use Rector\NodeTypeResolver\Node\AttributeKey;
 use Rector\NodeTypeResolver\NodeTypeResolver;
 
@@ -29,11 +25,6 @@ final class ContextAnalyzer
      */
     private const BREAK_NODES = [FunctionLike::class, ClassMethod::class];
 
-    /**
-     * @var array<class-string<Stmt>>
-     */
-    private const LOOP_NODES = [For_::class, Foreach_::class, While_::class, Do_::class];
-
     public function __construct(
         private readonly BetterNodeFinder $betterNodeFinder,
         private readonly NodeTypeResolver $nodeTypeResolver,
@@ -42,14 +33,14 @@ public function __construct(
 
     public function isInLoop(Node $node): bool
     {
-        $stopNodes = array_merge(self::LOOP_NODES, self::BREAK_NODES);
+        $stopNodes = array_merge(ControlStructure::LOOP_NODES, self::BREAK_NODES);
 
         $firstParent = $this->betterNodeFinder->findParentByTypes($node, $stopNodes);
         if (! $firstParent instanceof Node) {
             return false;
         }
 
-        foreach (self::LOOP_NODES as $type) {
+        foreach (ControlStructure::LOOP_NODES as $type) {
             if (is_a($firstParent, $type, true)) {
                 return true;
             }
@@ -77,7 +68,7 @@ public function isInIf(Node $node): bool
 
     public function isHasAssignWithIndirectReturn(Node $node, If_ $if): bool
     {
-        foreach (self::LOOP_NODES as $loopNode) {
+        foreach (ControlStructure::LOOP_NODES as $loopNode) {
             $loopObjectType = new ObjectType($loopNode);
             $parentType = $this->nodeTypeResolver->getType($node);
             $superType = $parentType->isSuperTypeOf($loopObjectType);

diff --git a/packages/NodeNestingScope/ValueObject/ControlStructure.php b/packages/NodeNestingScope/ValueObject/ControlStructure.php
@@ -66,4 +66,9 @@ final class ControlStructure
         Switch_::class,
         Foreach_::class,
     ];
+
+    /**
+     * @var array<class-string<Node>>
+     */
+    public const LOOP_NODES = [For_::class, Foreach_::class, While_::class, Do_::class];
 }
diff --git a/rules-tests/Naming/Naming/PropertyNamingTest.php b/rules-tests/Naming/Naming/PropertyNamingTest.php
@@ -25,13 +25,13 @@ protected function setUp(): void
      */
     public function testGetExpectedNameFromMethodName(string $methodName, ?string $expectedPropertyName): void
     {
-        /** @var ExpectedName $actualPropertyName */
-        $actualPropertyName = $this->propertyNaming->getExpectedNameFromMethodName($methodName);
+        /** @var ExpectedName $expectedName */
+        $expectedName = $this->propertyNaming->getExpectedNameFromMethodName($methodName);
 
         if ($expectedPropertyName === null) {
-            $this->assertNull($actualPropertyName);
+            $this->assertNull($expectedName);
         } else {
-            $this->assertSame($expectedPropertyName, $actualPropertyName->getSingularized());
+            $this->assertSame($expectedPropertyName, $expectedName->getSingularized());
         }
     }
 

diff --git a/rules/CodeQuality/Rector/Foreach_/ForeachItemsAssignToEmptyArrayToAssignRector.php b/rules/CodeQuality/Rector/Foreach_/ForeachItemsAssignToEmptyArrayToAssignRector.php
@@ -13,6 +13,7 @@
 use PHPStan\Type\ThisType;
 use Rector\CodeQuality\NodeAnalyzer\ForeachAnalyzer;
 use Rector\Core\Rector\AbstractRector;
+use Rector\NodeNestingScope\ValueObject\ControlStructure;
 use Rector\NodeTypeResolver\Node\AttributeKey;
 use Rector\ReadWrite\NodeFinder\NodeUsageFinder;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
@@ -95,7 +96,7 @@ private function shouldSkip(Foreach_ $foreach): bool
             return true;
         }
 
-        if ($this->shouldSkipAsPartOfNestedForeach($foreach)) {
+        if ($this->shouldSkipAsPartOfOtherLoop($foreach)) {
             return true;
         }
 
@@ -135,9 +136,9 @@ private function shouldSkip(Foreach_ $foreach): bool
         return false;
     }
 
-    private function shouldSkipAsPartOfNestedForeach(Foreach_ $foreach): bool
+    private function shouldSkipAsPartOfOtherLoop(Foreach_ $foreach): bool
     {
-        $foreachParent = $this->betterNodeFinder->findParentType($foreach, Foreach_::class);
-        return $foreachParent !== null;
+        $foreachParent = $this->betterNodeFinder->findParentByTypes($foreach, ControlStructure::LOOP_NODES);
+        return $foreachParent instanceof Node;
     }
 }
diff --git a/rules/MysqlToMysqli/Rector/FuncCall/MysqlQueryMysqlErrorWithLinkRector.php b/rules/MysqlToMysqli/Rector/FuncCall/MysqlQueryMysqlErrorWithLinkRector.php
@@ -186,7 +186,7 @@ private function findConnectionVariable(FuncCall $funcCall): ?Expr
             }
 
             return $this->isObjectType($node->expr, new ObjectType('mysqli'));
-        }, $this->file);
+        });
 
         if (! $connectionAssign instanceof Assign) {
             return null;

diff --git a/rules/Renaming/NodeManipulator/ClassRenamer.php b/rules/Renaming/NodeManipulator/ClassRenamer.php
@@ -27,7 +27,6 @@
 use Rector\CodingStyle\Naming\ClassNaming;
 use Rector\Core\Configuration\Option;
 use Rector\Core\PhpParser\Node\BetterNodeFinder;
-use Rector\Core\ValueObject\Application\File;
 use Rector\Naming\Naming\UseImportsResolver;
 use Rector\NodeNameResolver\NodeNameResolver;
 use Rector\NodeRemoval\NodeRemover;
@@ -63,7 +62,7 @@ public function __construct(
     /**
      * @param array<string, string> $oldToNewClasses
      */
-    public function renameNode(Node $node, array $oldToNewClasses, ?File $file = null): ?Node
+    public function renameNode(Node $node, array $oldToNewClasses): ?Node
     {
         $oldToNewTypes = [];
         foreach ($oldToNewClasses as $oldClass => $newClass) {
@@ -73,7 +72,7 @@ public function renameNode(Node $node, array $oldToNewClasses, ?File $file = nul
         $this->refactorPhpDoc($node, $oldToNewTypes, $oldToNewClasses);
 
         if ($node instanceof Name) {
-            return $this->refactorName($node, $oldToNewClasses, $file);
+            return $this->refactorName($node, $oldToNewClasses);
         }
 
         if ($node instanceof Namespace_) {
@@ -148,7 +147,7 @@ private function shouldSkip(string $newName, Name $name, ?Node $parentNode = nul
     /**
      * @param array<string, string> $oldToNewClasses
      */
-    private function refactorName(Name $name, array $oldToNewClasses, ?File $file = null): ?Name
+    private function refactorName(Name $name, array $oldToNewClasses): ?Name
     {
         $stringName = $this->nodeNameResolver->getName($name);
 
@@ -181,18 +180,17 @@ private function refactorName(Name $name, array $oldToNewClasses, ?File $file =
         $importNames = $this->parameterProvider->provideBoolParameter(Option::AUTO_IMPORT_NAMES);
 
         if ($this->shouldRemoveUseName($last, $newNameLastName, $importNames)) {
-            $this->removeUseName($name, $file);
+            $this->removeUseName($name);
         }
 
         return new FullyQualified($newName);
     }
 
-    private function removeUseName(Name $oldName, ?File $file = null): void
+    private function removeUseName(Name $oldName): void
     {
         $uses = $this->betterNodeFinder->findFirstPrevious(
             $oldName,
-            fn (Node $node): bool => $node instanceof UseUse && $this->nodeNameResolver->areNamesEqual($node, $oldName),
-            $file
+            fn (Node $node): bool => $node instanceof UseUse && $this->nodeNameResolver->areNamesEqual($node, $oldName)
         );
 
         if (! $uses instanceof UseUse) {

diff --git a/rules/Renaming/Rector/Name/RenameClassRector.php b/rules/Renaming/Rector/Name/RenameClassRector.php
@@ -97,7 +97,7 @@ public function refactor(Node $node): ?Node
         $oldToNewClasses = $this->renamedClassesDataCollector->getOldToNewClasses();
 
         if (! $node instanceof Use_) {
-            return $this->classRenamer->renameNode($node, $oldToNewClasses, $this->file);
+            return $this->classRenamer->renameNode($node, $oldToNewClasses);
         }
 
         if (! $this->rectorConfigProvider->shouldImportNames()) {

diff --git a/src/PhpParser/Node/BetterNodeFinder.php b/src/PhpParser/Node/BetterNodeFinder.php
@@ -17,16 +17,13 @@
 use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\ClassLike;
 use PhpParser\Node\Stmt\ClassMethod;
-use PhpParser\Node\Stmt\Expression;
 use PhpParser\Node\Stmt\Function_;
 use PhpParser\Node\Stmt\Namespace_;
 use PhpParser\Node\Stmt\Property;
 use PhpParser\Node\Stmt\Return_;
 use PhpParser\NodeFinder;
-use Rector\Core\Exception\ShouldNotHappenException;
 use Rector\Core\NodeAnalyzer\ClassAnalyzer;
 use Rector\Core\PhpParser\Comparing\NodeComparator;
-use Rector\Core\ValueObject\Application\File;
 use Rector\NodeNameResolver\NodeNameResolver;
 use Rector\NodeTypeResolver\Node\AttributeKey;
 use Symplify\PackageBuilder\Php\TypeChecker;
@@ -308,47 +305,54 @@ public function findFirstPreviousOfNode(Node $node, callable $filter, bool $look
     /**
      * @param callable(Node $node): bool $filter
      */
-    public function findFirstPrevious(Node $node, callable $filter, ?File $file = null): ?Node
+    public function findFirstPrevious(Node $node, callable $filter): ?Node
     {
         $currentStmt = $this->resolveCurrentStatement($node);
-        if (! $currentStmt instanceof Node) {
+
+        // current Stmt not an Stmt may caused by Node already removed
+        if (! $currentStmt instanceof Stmt) {
             return null;
         }
 
-        $parentStmtIterator = $currentStmt->getAttribute(AttributeKey::PARENT_NODE);
+        $foundInCurrentStmt = $this->findFirst($currentStmt, $filter);
 
-        // @todo add root virtual node for namespace-less nodes
-        if ($parentStmtIterator instanceof Node) {
-            // @todo assert iteratble interface that will be added in vendor patch
-            $parentStmts = $parentStmtIterator->stmts;
-        } else {
-            // fallback to parent stmts iterator
-            if (! $file instanceof File) {
-                $errorMessage = sprintf('File argument is missing in "%s()" method', __METHOD__);
-                throw new ShouldNotHappenException($errorMessage);
-            }
+        if ($foundInCurrentStmt instanceof Node) {
+            return $foundInCurrentStmt;
+        }
+
+        // previous Stmt of Stmt must be Stmt if found
+        $previousStatement = $currentStmt->getAttribute(AttributeKey::PREVIOUS_NODE);
 
-            $parentStmts = $file->getNewStmts();
+        if ($previousStatement instanceof Stmt) {
+            return $this->findFirstPrevious($previousStatement, $filter);
         }
 
-        if ($parentStmts === null) {
+        $parent = $currentStmt->getAttribute(AttributeKey::PARENT_NODE);
+
+        // parent Stmt not an Stmt may caused by Node already removed
+        if (! $parent instanceof Stmt) {
             return null;
         }
 
-        // @todo we don't need the first one; maybe find first above current node... check for positions...?
-        return $this->findFirst($parentStmts, $filter);
+        // previous Stmt of Stmt must be Stmt if found
+        $previousStatement = $parent->getAttribute(AttributeKey::PREVIOUS_NODE);
+
+        if ($previousStatement instanceof Stmt) {
+            return $this->findFirstPrevious($previousStatement, $filter);
+        }
+
+        return null;
     }
 
     /**
      * @template T of Node
      * @param array<class-string<T>> $types
      */
-    public function findFirstPreviousOfTypes(Node $mainNode, array $types, ?File $file = null): ?Node
+    public function findFirstPreviousOfTypes(Node $mainNode, array $types): ?Node
     {
         return $this->findFirstPrevious(
             $mainNode,
-            fn (Node $node): bool => $this->typeChecker->isInstanceOf($node, $types),
-            $file
+            fn (Node $node): bool => $this->typeChecker->isInstanceOf($node, $types)
         );
     }