add ContaintsStmts interface to mark stmt classes with nested stmts inside

[TomasVotruba]

This PR is revision #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]

TryCatch_ is also needs implemens Node\ContainsStmts, see

https://github.com/rectorphp/vendor-patches/blob/main/patches/nikic-php-parser-lib-phpparser-node-stmt-trycatch-php.patch

[samsonasik]

The ClassLike doesn't necessarily implemens Node\ContainsStmts, we don't use ClassLike to implements StmtsAwareInterface which usually for remove/add logic, see comparison at

https://github.com/rectorphp/vendor-patches/tree/main/patches

[TomasVotruba]

Good point

[nikic]

I don't get this part, don't classes have stmts as well? Or is the point here that the contents of Class_::$stmts are not "normal" statements, so they should not be covered by this?

[samsonasik]

@nikic for ClassLike, we usually don't loop to get target stmt, just use existing getMethod(), getProperty().

The Smts interface is usually to check loop, then verify inner stmts instance, to append/remove node.

[TomasVotruba]

@nikic Or is the point here that the contents of Class_::$stmts are not "normal" statements, so they should not be covered by this?

Exactly, they should not be covered here. The ClassLike have $stmts property, but Property, ClassMethod, ClassConst etc. only wrap other $stmts we really want to work with.

[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 Not sure if should be a BC break.

In Rector we use $node->stmts === null check to avoid foreach on null errors. So if this will turn into [], there is no real change.

[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 #1113 (review) that will also make less noise on phpstan/psalm check, avoid property not exists notice.