[RFC] Introduce ImprovedParentConnectingVisitor

[samsonasik]

Background

@TomasVotruba @staabm currently, we use PhpParser\NodeVisitor\ParentConnectingVisitor which work with stack of every nodes, eg, the following code:

$a['a'['b'['c']]];

Above, c parent is b and b parent is a, and so on. That may cause a lot of stack saved on:

$this->stack[] = $node;

ref https://github.com/nikic/PHP-Parser/blob/19526a33fb561ef417e822e85f08a00db4059c17/lib/PhpParser/NodeVisitor/ParentConnectingVisitor.php#L34

Current approach to removing parent call usage is by:

• use top possible use case, which Expression, Return_, then traverse() inside to refactor.
• mark new attribute to know if the node is part of other node, eg: is_arg_value

That set and get will increased and possibly will bite use again, then the memory will increase instead of decrease.

Possible Solution

Scope is get from the parent node, and mostly if inside same Stmt, eg: on same Expression stmt, we will have same Scope, I think we can create improved ParentConnectingVisitor, let's say named: ImprovedParentConnectingVisitor, the pseudocode will be something like:

final class ImprovedParentConnectingVisitor extends NodeVisitorAbstract
{
    private ?Node $parentNode = null;

    public function enterNode(Node $node)
    {
        if ($node instanceof Stmt) {
            $this->parentNode = $node;
        }

        if ($node instanceof StmtsAwareInterface || $node instanceof ClassLike || $node instanceof Declare_) {
            if ($node->stmts !== null) {
                foreach ($node->stmts as $stmt) {
                    $stmt->setAttribute(AttributeKey::PARENT_NODE, $node);
                }
            }

            return null;
        }

        // parent already filled
        if ($node instanceof Stmt) {
            return null;
        }

        if ($this->parentNode instanceof Stmt) {    
            $node->setAttribute(AttributeKey::PARENT_NODE, $this->parentNode);
        }

        return null;
    }
}

That should will make the following code:

$a['a'['b'['c']]];

the c, b, and a parent will be a Expression stmt, which now no need stack to save previous passed node.

PARENT_NODE will no longer deprecated, but got slimmer relation it only do:

`Stmt` -> `Expr` 

instead of

`Stmt` -> `Expr` -> `Expr` -> `Expr`.

what do you guys think?

[samsonasik]

For side note, the memory consumption usage seems on stacking:

$this->stack[] = $node;

which posisbly, if we can remove the stack save, to append the visited inisde each stmt, that will be lower memory, even every containered node, eg: assign -> var (assign again) filled as parent.

[staabm]

From my understanding, the main problem https://phpstan.org/blog/preprocessing-ast-for-custom-rules is talking about is object references from AST nodes Attributes, keeping objects alive and therefore make garbage collection in-efficient and slow, and therefore lead to increased memory consumption and longer runtimes.

I don't think we need to work on e.g. AST Visitors, as long as we have these attributes on the AST Nodes. We should minimize AST attributes usage to a minimum. The references between AST Nodes are especially wasteful, because there are millions created while parsing code.

The 2nd case in my mind is Scope objects referenced from AST nodes, because Scope is a huge object, holding references to a lot of other objects and also a lot of AST Nodes - meaning it should also make problems for the garbage collection.

[staabm]

One way of removing AST Attributes could also be dropping rector rules which require too many of them, if we see no way getting rid of the attributes.

In such cases we need to prioritize whether the features the rules provides are worth the inefficiency they have on the whole

[samsonasik]

@staabm thank you, removing rules is actually just hiding the actual issue about memory usage.

as I commented at rectorphp/rector-src#4400 (comment) we may probably can resolve Scope from PHPStan virtual node that wrap the node itself during analyse, so we can reduce Scope filling, I don't have idea of how to do that for now tbh.

[samsonasik]

closing as parent attribute usages are removed.