Best way to remove node with traverseNodesWithCallable in 0.17.1?

[timmipetit]

Question

Rector 0.17.1 removed effectively removed removeNode. In some of our custom rules we have some code that looks like this:

 $this->traverseNodesWithCallable((array) $classMethod->stmts, function (Node $subNode) : ?Node {
    if(someConditionThatWeDecide()) {
        $this->removeNode($subNode);
    }
});

What is the best way moving forward for code like this? When changing the traverseNodesWithCallable into a foreach($node->stmts as $key => $subNode) we obviously get less subnodes to process and have to add some extra code to get the nodes that we want. Is this the correct way?

[samsonasik]

You can define more Node inside getNodeTypes() method to point to the correct target node, and using unset() to specific key of stmt, like what I did in laminas-servicemanager-migration, there is stmt_key for that:

https://github.com/laminas/laminas-servicemanager-migration/pull/131/files#diff-40da94022f21a80bb7c833406e6092f80e0015c1c7b8f0e2121102affedb86bd

You can also traverse with StmtsAwareInterface, with "beware", ensure that the stmt is the correct stmt_key by StmtsAwareInterface that wrap it, like what I did in cakephp/upgrade:

https://github.com/cakephp/upgrade/pull/236/files#diff-a0b234faceec43fc851a50273b309febcaeda01b537e32eb8f57fd7310496051

Assuming you're trying to remove a MethodCall inside, you need remove the Stmt that wraps it. The node to be removed is the Stmt, and there is a stmt_key attribute for that, but that is depends of "parent" FunctionLike, so to ensure you remove the correct current Stmt, you need to assign $currentStmt when found, something like this pseudo code:

use Rector\Core\Contract\PhpParser\Node\StmtsAwareInterface;
use PhpParser\Node\Stmt;
use PhpParser\NodeTraverser;
use Rector\NodeTypeResolver\Node\AttributeKey;

// ...

$parentStmt = null;
$currentStmtKey = null;
$hasChanged = false;
$this->traverseNodesWithCallable(
        (array) $classMethod->stmts, 
        function (Node $subNode) use ($classMethod, &$parentStmt, &$currentStmtKey, &$hasChanged): ?Node {
                // maybe removal will be inside Closure or If, etc
                if ($subNode instanceof StmtsAwareInterface) {
                     $parentStmt = $subNode;
                     return null;
                }

                if ($subNode instanceof Stmt) {
                    $currentStmtKey = $subNode->getAttribute(AttributeKey::STMT_KEY);
                    return null;
                }
                
                // here you remove the Stmt if method call is "foo"
                if ($this->isName($methodCall->name, 'foo')) {
                          // first level, just remove by subnode key
                          if (! $parentStmt instanceof Node) {
                                  unset($classMethod->stmts[$currentStmtKey]);
                          } else {
                                  unset($parentStmt->stmts[$currentStmtKey]);
                          }
                          $hasChanged = true;
                }
               
                return null;
});

if ($hasChanged) {
    return $node;
}

return null;

I will prepare a blog post for that :)

[timmipetit]

Thanks for the quick reply! Defining more nodes in getNodeTypes seems like a good option for some basic cases. The rule we have here is a bit more complex (we delete some MethodCalls, but need the Arg to set a value in some other MethodCall). I think we'll be able to make it work with your StmtsAwareInterface example, thanks.

[timmipetit]

In case someone else has the same problem: in the end we solved this by calling our function recursively for StmtsAwareInterface, which seems like an easier option than setting parentStmt currently, which had some issues for us. So something like:

if ($subNode instanceof StmtsAwareInterface) {
    $this->myRecusriveReplaceFun($subNode);
    return NodeTraverser::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
}

[samsonasik]

If you use DONT_TRAVERSE_CURRENT_AND_CHILDREN, that mean it scoped, if you want change, you may need inside "else" part, so the change will always scoped, inside classmethod, function, or closure, something like this
https://github.com/rectorphp/rector-src/pull/4303/files#diff-a1492c76b5420483ca8dfd8a02beb05d56e5fd157ed97004db0a26060d586951

Then in getNodeTypes() method, you define what scoped nodes to be used.

[samsonasik]

@timmipetit here the blogpost https://getrector.com/blog/rector-017-using-scoped-traverse

[timmipetit]

Thanks for getting back to me and posting the link, much appreciated!