[Downgrade] A 2nd rule should modify the output from a previous rule in the set, but it doesn't happen

[leoloso]

Bug Report

Subject	Details
Rector version	e.g. v0.10.1
Installed as	composer dependency

Package symfony/cache has file vendor/symfony/cache/CacheItem.php, with this content (here):

final class CacheItem implements ItemInterface
{
    public function tag($tags): ItemInterface
    {
        // ...
        return $this;
    }
}

The interface ItemInterface has this content (here):

interface ItemInterface extends CacheItemInterface
{
    public function tag($tags): self;
}

The downgrade set for PHP 7.4 has these 2 rules, in this same order:

return static function (ContainerConfigurator $containerConfigurator): void {
    $services = $containerConfigurator->services();
    $services->set(DowngradeCovariantReturnTypeRector::class);
    $services->set(DowngradeSelfTypeDeclarationRector::class);
};

When downgrading CacheItem.php, function tag should be modified twice:

1. DowngradeCovariantReturnTypeRector must first transform the return type, from ItemInterface to self
2. DowngradeSelfTypeDeclarationRector should then remove the self return type

But the second step is not happening. As a consequence, after running the downgrade, function tag returns self, which is not accepted for PHP 7.3.

Minimal PHP Code Causing Issue

I tried to recreate the problem here, but the output is different:

https://getrector.org/demo/0d1b5ce4-c1a3-4fb8-bc3f-1fbe982f6e90

[TomasVotruba]

This is not possible, because of how php-parser traverses nodes. From top to bottom.

You can change the rule order:
https://github.com/rectorphp/rector/blob/main/docs/how_it_works.md#221-order-of-rectors

But if there is a class node with method node in it, all the class rules will be procesed first. Then all the rules for method node.

[leoloso]

Hmmmm... Is there some way to solve this issue?

For instance, what about having the 2 rules in two different sets? Would that solve the issue?

• DowngradeCovariantReturnTypeRector in DowngradeSetList::PHP_74_FIRST (well, something nicer than this)
• DowngradeSelfTypeDeclarationRector in DowngradeSetList::PHP_74_LAST

Or otherwise, how?

[TomasVotruba]

This should be handled in the rule itself, independent on the other.

E.g. If PHP 8 unin typed property is downgraded, there should be different results depending on target php version:

• PHP 7.4 with nullable type declaration
• PHP 5.6 with var do

Upgrades work the same way

[leoloso]

Hmmm then the logic would be completely mixed up: checking if the return type is self in rule DowngradeCovariantReturnTypeRector doesn't feel right.

Also, if somebody wants to only downgrade the covariant types and execute the rule directly, not within a set, then this rule would produce a bug.

I'll fix it with a hack instead: I'll run DowngradeSelfTypeDeclarationRector yet again, on a new execution applied to that file where it fails only.

Then, I will enhance testing the downgrade will fail as I explain here: GatoGraphQL/GatoGraphQL#544, so that if this same problem happens again with some other file, the test will fail, and then I manually fix it again.

[HenkPoley]

I'm not sure if DowngradeSelfTypeDeclarationRector should remove : self return type everywhere on the PHP 8.0 -> 7.4 transition. Since : self works on functions in classes down to at least PHP 7.2. But I suppose this was eagerly added because : self doesn't work on interfaces on PHP 7.x (or maybe some subtle variations thereof, Traits, abstract classes??).

Edit: I think : self works down to PHP 7.0 on instantiable classes: https://lukashajdu.com/post/php7-return-types/