Add ability to add new nodes

[theofidry]

See https://github.com/nikic/PHP-Parser/issues/507#issuecomment-1014940669

---

Original post:

Is it a feature you would be willing to consider? Right now it seems this is possible only in leaveNode() and if the parent is an array.

Maybe this could be done by introducing a NullNode class that can wrap several nodes.

[nikic]

Can you please provide an example of what this means? I don't understand what "add" would do if the parent is not an array.

[theofidry]

Yes sorry. For example with the following example, I could replace:

const X = 'foo', Y = 'bar';

by:

const X = 'foo';
const Y = 'bar';

To do so you could image the following node visitor:

final class ReplaceConstStmts extends NodeVisitorAbstract
{
    /**
     * {@inheritdoc}
     */
    public function enterNode(Node $node): Node
    {
        if (false === ($node instanceof Node\Stmt\Const_)) {
            return $node;
        }

        return new NullNode(
            array_map(
                function (Node\Const_ $constant) use ($node): Node\Stmt\Const_ {
                    return new Node\Stmt\Const_([$constant], $node->getAttributes());
                },
                $node->consts
            )
        );
    }
}

And when printing the array of nodes, NullNode would actually not be rendered but only its wrapped nodes

The example above is one kind of transformation that makes much easier for PHP-Scoper since sometimes I need to transform const into define(). I already do a similar transformation for the grouped use statements although I can do that one in the traverser directly.

More generally though, I think it would make it easier to manipulate the AST that way.

[nikic]

@theofidry Isn't this the same as using the array return value in leaveNode()? Is the feature request here to also allow this in enterNode(), or does NullNode provide some different/additional functionality?

[theofidry]

Isn't this the same as using the array return value in leaveNode()?

It differs in several way:

• It can be used in enterNode() as well as you noted
• The parent does not require to be an array

I would be fine with returning an array as well instead. I suggested NullNode because it looks easier to deal with

[Kolesar]

I do not know if this is the right topic, but do not want open one more.

I cannot find how to add a new node in any case here.

I want to add one new use, but I do not know how :(

I guess it is something here:

public function enterNode(Node $node)
{
   if ($node instanceof Node\Stmt\Use_) {
   }
}

[TomasVotruba]

Do you still need help with this?

I now know how to replace, remove, add new nodes on many places in @rectorphp

[theofidry]

@TomasVotruba I do, however I really have no time to look into this right now and unlikely to have any before new year.

I'm also still not sure wether or not @nikic is cool with it.

Nonetheless, if you have a couple of examples they would be welcome as they may help to move forward with the discussion regarding the API and to decide if worth having here in the library or not

[TomasVotruba]

@theofidry Sure :+1: I'm pretty busy this month myself, crazy December :)

I'm also still not sure wether or not @nikic is cool with it.

I don't understand. php-parser already support adding of any node you could think of.

Nonetheless, if you have a couple of examples they would be welcome as they may help to move forward with the discussion regarding the API and to decide if worth having here in the library or not

It's hard to tell in general. I can comment specific code though. If you want something in php-scope, just create issue there with current code and desired one. And ping me there.

[theofidry]

I don't understand. php-parser already support adding of any node you could think of.

I'm confused. Do you have examples? Because my two first comments are exactly this

[TomasVotruba]

Like this? https://github.com/rectorphp/rector/blob/master/docs/AllRectorsOverview.md#delegateexceptionargumentsrector

[danog]

A temporary workaround could be returning the following node:

new Node\Stmt\If_(
    new Node\Expr\ConstFetch(new Node\Name('true')),
    ['stmts' => $arrayOfStmts]
);

A bit ugly, but it does the job.

[theofidry]

@TomasVotruba it is a good solution to delegate the adding/removal of nodes the way you did. But overall I find the solution quite complex compared to adding a NullNode which would be only here to nest nodes/statements and not be rendered at all as explained here

[TomasVotruba]

Why not just return array of nodes?

[theofidry]

I just thought a null object would be easier to deal with semantically. But that's a suggestion like another, so if it's easier to deal with an array of nodes instead of a null object it's fine by me.

@nikic any opinion about this? I may try to do a PR but I would rather make sure we're on the same page first

[nikic]

I still don't understand what this is about. The ability to return arrays already exists and the example in https://github.com/nikic/PHP-Parser/issues/507#issuecomment-395967805 would already work with it for leaveNode. Is this about extending support for enterNode?

[nikic]

If this is about enterNode, then yeah I'm fine with that in principle. One tricky issue is how this will interact, in particular will the newly inserted nodes be visited by other visitors, or possibly even the visitor that inserted them?

[TomasVotruba]

@nikic I think so. Is there a reason for different behavior in these 2 methods?

[theofidry]

Lil' heads up because I find funny to find this issue again 2 years later when checking how to fix a TODO ;)

So to re-cap since it's been a while and there's been a bit of a discussion since the original post: it is about the ability to extend the possibility to return an array of nodes (to replace the current one) existing on leaveNode() to enterNode() to allow the transformation described in https://github.com/nikic/PHP-Parser/issues/507#issuecomment-395967805 to be done on enterNode().

For the record: checking the parent is not really an option here as the parent may be anything, or worse non-existent (e.g. if you only have the constant declaration without even a namespace declaration).

Meanwhile I've been using @danog workaround: https://github.com/nikic/PHP-Parser/issues/507#issuecomment-450058619.

[nikic]

Implemented in https://github.com/nikic/PHP-Parser/commit/a3b0541c71cf439526beffcfe72d6e1aaeae9a3a. Both array return and REMOVE_NODE are now supported from enterNode().

[theofidry]

Thanks a lot @nikic!