allow REMOVE_NODE (#249)

* allow REMOVE_NODE

* skip nested closure

Author: TomasVotruba

src/Rules/Rector/NoIntegerRefactorReturnRule.php

@@ -4,14 +4,20 @@
 
 namespace Symplify\PHPStanRules\Rules\Rector;
 
+use PhpParser\Node\Name;
 use PhpParser\Node;
+use PhpParser\Node\Expr\ClassConstFetch;
+use PhpParser\Node\Expr\Closure;
 use PhpParser\Node\Identifier;
 use PhpParser\Node\Stmt\ClassMethod;
 use PhpParser\Node\UnionType;
+use PhpParser\NodeTraverser;
+use PhpParser\NodeVisitor;
 use PHPStan\Analyser\Scope;
 use PHPStan\Rules\Rule;
 use PHPStan\Rules\RuleErrorBuilder;
 use Symplify\PHPStanRules\Enum\RuleIdentifier\RectorRuleIdentifier;
+use Symplify\PHPStanRules\NodeTraverser\SimpleCallableNodeTraverser;
 
 /**
  * @see \Symplify\PHPStanRules\Tests\Rules\Rector\NoIntegerRefactorReturnRule\NoIntegerRefactorReturnRuleTest
@@ -23,7 +29,7 @@ final class NoIntegerRefactorReturnRule implements Rule
     /**
      * @var string
      */
-    public const ERROR_MESSAGE = 'Instead of using int in refactor(), make use of attributes and return always node or a null. Using traverser enums might lead to unexpected results';
+    public const ERROR_MESSAGE = 'Instead of using DONT_TRAVERSE_CHILDREN* or STOP_TRAVERSAL in refactor() method, make use of attributes. Return always node, null or REMOVE_NODE. Using traverser enums might lead to unexpected results';
 
     public function getNodeType(): string
     {
@@ -52,15 +58,61 @@ public function processNode(Node $node, Scope $scope): array
                 continue;
             }
 
-            if ($type->name === 'int') {
-                $ruleError = RuleErrorBuilder::message(self::ERROR_MESSAGE)
-                    ->identifier(RectorRuleIdentifier::NO_INTEGER_REFACTOR_RETURN)
-                    ->build();
+            if ($type->name !== 'int') {
+                continue;
+            }
+
+            $constantNames = $this->findUsedNodeVisitorConstantNames($node);
 
-                return [$ruleError];
+            $undesiredConstantNames = array_diff($constantNames, ['REMOVE_NODE']);
+            if ($undesiredConstantNames === []) {
+                return [];
             }
+
+            $ruleError = RuleErrorBuilder::message(self::ERROR_MESSAGE)
+                ->identifier(RectorRuleIdentifier::NO_INTEGER_REFACTOR_RETURN)
+                ->build();
+
+            return [$ruleError];
         }
 
         return [];
     }
+
+    /**
+     * @return string[]
+     */
+    private function findUsedNodeVisitorConstantNames(ClassMethod $classMethod): array
+    {
+        $constantNames = [];
+
+        $simpleCallableNodeTraverser = new SimpleCallableNodeTraverser();
+        $simpleCallableNodeTraverser->traverseNodesWithCallable($classMethod, function (Node $subNode) use (&$constantNames): int|null {
+            // skip closure nodes as they have their own scope
+            if ($subNode instanceof Closure) {
+                return NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
+            }
+
+            if (! $subNode instanceof ClassConstFetch) {
+                return null;
+            }
+
+            if (! $subNode->class instanceof Name) {
+                return null;
+            }
+
+            if (! in_array($subNode->class->toString(), [NodeVisitor::class, NodeTraverser::class])) {
+                return null;
+            }
+
+            if (! $subNode->name instanceof Identifier) {
+                return null;
+            }
+
+            $constantNames[] = $subNode->name->toString();
+            return null;
+        });
+
+        return array_unique($constantNames);
+    }
 }

tests/Rules/Rector/NoIntegerRefactorReturnRule/Fixture/AllowNestedClosure.php

@@ -0,0 +1,40 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Symplify\PHPStanRules\Tests\Rules\Rector\NoIntegerRefactorReturnRule\Fixture;
+
+use PhpParser\Node;
+use PhpParser\Node\Stmt\Class_;
+use PhpParser\Node\Stmt\Interface_;
+use PhpParser\NodeFinder;
+use PhpParser\NodeTraverser;
+use PhpParser\NodeVisitor;
+use Rector\Rector\AbstractRector;
+
+final class AllowNestedClosure extends AbstractRector
+{
+    public function getNodeTypes(): array
+    {
+        return [Class_::class, Interface_::class];
+    }
+
+    public function refactor(Node $node): \PhpParser\Node|int
+    {
+        if ($node instanceof Class_) {
+            return $node;
+        }
+
+        $nodeFinder = new NodeFinder();
+        $nodeFinder->find($node, function (Node $subNode) {
+            if ($subNode instanceof Node\Stmt\Function_) {
+                return NodeTraverser::STOP_TRAVERSAL;
+            }
+
+            return true;
+        });
+
+
+        return NodeVisitor::REMOVE_NODE;
+    }
+}

tests/Rules/Rector/NoIntegerRefactorReturnRule/Fixture/AllowRemoveNode.php

@@ -0,0 +1,28 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Symplify\PHPStanRules\Tests\Rules\Rector\NoIntegerRefactorReturnRule\Fixture;
+
+use PhpParser\Node;
+use PhpParser\Node\Stmt\Class_;
+use PhpParser\Node\Stmt\Interface_;
+use PhpParser\NodeVisitor;
+use Rector\Rector\AbstractRector;
+
+final class AllowRemoveNode extends AbstractRector
+{
+    public function getNodeTypes(): array
+    {
+        return [Class_::class, Interface_::class];
+    }
+
+    public function refactor(Node $node): \PhpParser\Node|int
+    {
+        if ($node instanceof Class_) {
+            return $node;
+        }
+
+        return NodeVisitor::REMOVE_NODE;
+    }
+}

tests/Rules/Rector/NoIntegerRefactorReturnRule/NoIntegerRefactorReturnRuleTest.php

@@ -23,6 +23,8 @@ public static function provideData(): Iterator
         yield [__DIR__ . '/Fixture/SimpleReturnInt.php', [[$errorMessage, 20]]];
 
         yield [__DIR__ . '/Fixture/SkipBareNodeReturn.php', []];
+        yield [__DIR__ . '/Fixture/AllowRemoveNode.php', []];
+        yield [__DIR__ . '/Fixture/AllowNestedClosure.php', []];
     }
 
     protected function getRule(): NoIntegerRefactorReturnRule