add ContaintsStmts interface to mark stmt classes with nested stmts inside

[TomasVotruba]

This PR is revision https://github.com/nikic/PHP-Parser/pull/836 to address only mentioned issue with the ClassMethod. It's the only node, that can have $stmts = null value. Such method can be abstract or part of Interface_, so there is nothing to iterate on. This revision exclude the ClassMethod

This would solve run custom Rector tests on PHPUnit 12, remove lot of vendor patching in Rector /vendor (ugly hack), make make writing PHPStan rules for all nodes that have $stmts easier and streamline working with the $stmts property in userland

[samsonasik]

@TomasVotruba just for note: we have custom node: FileWithoutNamespace that is StmtsAwareInterface instance, so that need update, if this is accepted, we need to have keep compat of StmtsAwareInterface to keep working without bc break before a major version as possible, like class_alias() usage.

[nikic]

Is this actually useful to you if it does not include ClassMethod?

[TomasVotruba]

It's very useful, as instead of 18 nodes types we can check single one.

Also, the ClassMethod has to always be checked for $classMethods->stmts !== null. In many rules, we check ClassMethod for parent class behavior as well.

[samsonasik]

@TomasVotruba imo, ClassMethod should implements Node\ContainsStmts as well, otherwise, we need to patch our getNodeTypes() to include ClassMethod as well in Rector classes to use:

    public function getNodeTypes(): array
    {
-        return [StmtsAwareInterface::class];
+        return [Node\ContainsStmts::class, ClassMethod::class];
    }

for example, this required when we need to use it in required node, eg: downgrade rules which required code needs to be downgraded.

we need also effort to update to include ClassMethod as well on if instanceof StmtsAwareInterface check in many areas.

If the ClassMethod included, we can reduce effort to just use replace it, with the following steps:

0. Pre-requisit: Wait until PHPStan require latest nikic/php-parser that has this new node interface, since rector/rector require phpstan that load nikic/php-parser.

1. Remove StmtsAwareInterface interface

2. Create class_alias() compatible code to ensure rector community rules keep working before next major release, see https://3v4l.org/6ivv3

interface ContainsStmts {}

// BC break patch, can be placed in our `bootstrap.php`
class_alias(ContainsStmts::class, \Rector\Contract\PhpParser\Node\StmtsAwareInterface::class);

class SomeNode implements ContainsStmts {}

$obj = new SomeNode();

// old should keep working
var_dump($obj instanceof \Rector\Contract\PhpParser\Node\StmtsAwareInterface);

// new 
var_dump($obj instanceof ContainsStmts);

3. Make our custom FileWithoutNamespace to implements ContainsStmts as well:

-final class FileWithoutNamespace extends Stmt implements StmtsAwareInterface
+final class FileWithoutNamespace extends Stmt implements Node\ContainsStmts

4. Just replace StmtsAwareInterface to Node\ContainsStmts everywhere, and it will keep working

[nikic]

A way to include ClassMethod would be to give this a getStmts(): array method, where ClassMethod can convert null to []. I think having a method makes more sense than a pure marker-interface anyway.

[TomasVotruba]

@nikic That sounds good. I think ArrowFunction uses getStmts() as well already. Should I implement it?

[samsonasik]

@TomasVotruba ArrowFunction imo is not needed, as it actually just single Expr, and no real Stmt there, we then also need tweak everywhere for ArrowFunction instance, avoid append/add/remove real stmt.

[TomasVotruba]

@samsonasik I agree. It was just example of such method :)

[TomasVotruba]

@nikic I'm testing the interface and getStmts() method and so far most works well.

There is one issue with writing the modified stmts back to the node, e.g.:

$stmts = $if->getStmts();
foreach ($stmts as $key => $value) {
    if (mt_rand(0, 1) {
        // remove, add or shuffle nodes here
        unset($stmts[$key]));
    }
}

// then we want to update stmts array in the main node
$node->setStmts($stmts);

Logical approach is to add setStmts() to ContainsStmt as well:

/** 
 * @param Stmt[] $stmts
 */
public function setStmts(array $stmts): void
{
    $this->stmts = $stmts;
}

Thoughts?

[samsonasik]

@TomasVotruba imo, getStmts() should only be used for verification/check/search that so null/cast check is no longer needed.

For apply change, use direct ->stmts instead, and the property itself is public.

That's why I suggest to add:

/** 
 * @property Stmt[]|null $stmts
 */

tag in the interface before https://github.com/nikic/PHP-Parser/pull/1113#pullrequestreview-3354581423 that will also make less noise on phpstan/psalm check, avoid property not exists notice.

[TomasVotruba]

@samsonasik Rebased on latest dev-main and added the @property :+1: It's the best way at the moment to allow writing stmts back to the node.

The getStmts() is useful for read-only tools like static analysis.

[staabm]

Maybe it makes sense to add new common base-classes (simular to CallLike), so you can at least reduce the number of "instanceof checks" required.

E.g. LoopLike as base for loop expressions

Maybe something similar for the different "IF" types

[samsonasik]

Looks good to me 👍

[TomasVotruba]

@staabm I want to keep this as simple as possible to avoid any changes in userland.

@nikic From our side it's ready, Is there anything we can do to get this merged in next release?

[samsonasik]

@TomasVotruba looking at Declare_ behaviour, declare can have its stmts

declare(ticks=1) {
    echo 'test';
}

see https://3v4l.org/QmPYL#v8.0.26 .

that's why it has the stmts property

https://github.com/nikic/PHP-Parser/blob/e4810261db718545ed1a64d021dc9ecba3dbd68d/lib/PhpParser/Node/Stmt/Declare_.php#L8-L12

It likely less people use of the declare and its stmts syntax above, but that still possible, so I think we can add Declare_ to implements Node\ContainsStmts as well, but that can be in separate PR after this PR merged ;)