Incorrect behavior of DowngradeParameterTypeWideningRector

[leoloso]

Bug Report

Subject	Details
Rector version	last dev-main
Installed as	composer dependency

Minimal PHP Code Causing Issue

See https://getrector.com/demo/3d9663ae-6d21-43f7-90dc-cb4439dd70a8

<?php

trait WithInstanceManagerServiceTrait
{
    function setInstanceManager(InstanceManagerInterface $instanceManager): void
    {
    }
}

Responsible rules

• DowngradeParameterTypeWideningRector

Expected Behavior

DowngradeParameterTypeWideningRector is currently removing the type InstanceManagerInterface of the property:

 <?php
 
 trait WithInstanceManagerServiceTrait
 {
-    function setInstanceManager(InstanceManagerInterface $instanceManager): void
+    /**
+     * @param \InstanceManagerInterface $instanceManager
+     */
+    function setInstanceManager($instanceManager): void
     {
     }
 }

But this is wrong. DowngradeParameterTypeWideningRector should leave it as is.

This error started happening with v.0.17.2 I believe (but maybe from v0.17.1 or v0.17.0, as I hadn't fully tested it with those versions).

[leoloso]

Using class instead of trait also fails:

https://getrector.com/demo/b54d7404-7089-481e-a0e0-461b9df7f72d

[leoloso]

I tested with v0.17.0 and it works fine, so the issue started happening with v0.17.2 or v0.17.1

[samsonasik]

I think this is already correct, if it is not final class, the parameter is removed, and when final, it is leave as is, see

• https://getrector.com/demo/e020721c-2dad-4851-b6ff-160c4a2a20f6

[samsonasik]

Even the trait method is used by final class and overridden, it seems still changed, so I think it actually fine as both method that have methods are changed, see

https://getrector.com/demo/c5a38bf3-1b42-4177-89d0-b5e671636041

[leoloso]

Hmmm I'm not sure this is right, why should DowngradeParameterTypeWideningRector remove the type when the class is final? There is no such relationship in the feature itself:

https://www.php.net/manual/en/migration72.new-features.php#migration72.new-features.param-type-widening

Removing the type can actually be harmful to the application, so I think it should be done only when it has to be done. For instance, because the PHP version does not support typed parameters. But PHP, all the way down to 7.1 and even below, supports typed arguments, so there's no need to remove them.

I'll explain why removing the type is bad in my particular case (I believe that this same case might apply to Rector too, as it's also being downgraded and uses dependency injection): I use symfony/dependency-injection with autowiring, and this still works with PHP 7.1. However, autowiring checks the type of the parameter to identify what service to inject into it. By removing the type, I'm now getting this error:

Cannot autowire service "GatoGraphQL\GatoGraphQL\Settings\SettingsNormalizerInterface": argument "$instanceManager" of method "GatoGraphQL\GatoGraphQL\Settings\SettingsNormalizer::setInstanceManager()" has no type-hint, you should configure its value explicitly.

Stack trace:
#0 /app/wordpress/wp-content/plugins/gato-graphql/vendor/symfony/dependency-injection/Compiler/Compiler.php(85): PrefixedByPoP\Symfony\Component\DependencyInjection\Compiler\DefinitionErrorExceptionPass->process(Object(PoP\Root\Container\ContainerBuilder))
#1 /app/wordpress/wp-content/plugins/gato-graphql/vendor/symfony/dependency-injection/ContainerBuilder.php(691): PrefixedByPoP\Symfony\Component\DependencyInjection\Compiler\Compiler->compile(Object(PoP\Root\Container\ContainerBuilder))
...

To avoid this problem, I would now need to not make my class final anymore, which doesn't make a lot of sense to me.

Also, this creates confusion when a trait is used by both final and non-final classes:

https://getrector.com/demo/cffaa566-f8cc-4197-b0dd-838acdbcccb5

Take into account that these could be in different projects. For instance, in my case, I have a WordPress plugin providing a GraphQL server, and independent developers can extend the GraphQL schema via their own plugins. If their classes use the trait that I defined in my plugin, DowngradeParameterTypeWideningRector might remove the type or not depending on their classes being final or not? If that's the case, that will produce a bug, and the application will explode in runtime.

So unless I'm missing something, I don't see why DowngradeParameterTypeWideningRector should care about final or not (as it was up to v0.17.0).

Thoughts?

[samsonasik]

Could you create sample use case when it broke app on https://getrector.org/demo or https://3v4l.org ?

[leoloso]

Here: https://getrector.com/demo/9f4833fb-d588-438e-b508-2ed96152f9bf

I'm using the DOWN_TO_PHP_71 downgrade set.

The code in PHP 8.1 that I'm downgrading is this one (this bug report at the top is a shortened version of this):

trait WithInstanceManagerServiceTrait
{
    protected InstanceManagerInterface $instanceManager;

    #[Required]
    final public function setInstanceManager(InstanceManagerInterface $instanceManager): void
    {
        $this->instanceManager = $instanceManager;
    }
    final protected function getInstanceManager(): InstanceManagerInterface
    {
        return $this->instanceManager;
    }
}

That #[Required] is Symfony DependencyInjection's autowiring, when downgraded it becomes /** @required */ and it still works.

This trait WithInstanceManagerServiceTrait is used pretty much all over the codebase, by hundreds of services. Some of them are final, most of them are not.

When downgraded with v0.17.2 it becomes this:

trait WithInstanceManagerServiceTrait
{
    /**
     * @var \PoP\Root\Instances\InstanceManagerInterface
     */
    protected $instanceManager;
    /**
     * @param \PoP\Root\Instances\InstanceManagerInterface $instanceManager
     * @required
     */
    public final function setInstanceManager($instanceManager) : void
    {
        $this->instanceManager = $instanceManager;
    }
    protected final function getInstanceManager() : InstanceManagerInterface
    {
        return $this->instanceManager;
    }
}

Notice that the type InstanceManagerInterface has been removed from the parameter (that's why symfony/dependency-injection now explodes).

With v0.17.0, it was downgraded like this:

trait WithInstanceManagerServiceTrait
{
    /**
     * @var \PoP\Root\Instances\InstanceManagerInterface
     */
    protected $instanceManager;
    /**
     * @required
     */
    public final function setInstanceManager(InstanceManagerInterface $instanceManager) : void
    {
        $this->instanceManager = $instanceManager;
    }
    protected final function getInstanceManager() : InstanceManagerInterface
    {
        return $this->instanceManager;
    }
}

Notice that the type InstanceManagerInterface is still there, with the parameter.

[samsonasik]

DowngradeParameterTypeWideningRector is configurable, I think you can configure like this:

use Rector\Config\RectorConfig;
use Rector\Set\ValueObject\DowngradeLevelSetList;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->sets([
        DowngradeLevelSetList::DOWN_TO_PHP_71,
    ]);
    
   $rectorConfig
        ->ruleWithConfiguration(\Rector\DowngradePhp72\Rector\ClassMethod\DowngradeParameterTypeWideningRector::class, [
            WithInstanceManagerServiceTrait::class => ['getInstanceManager'],
        ]);
};

see https://getrector.com/demo/ac050d1f-fa18-40f8-9999-73ade354368c

is that will work for you?

[leoloso]

I guess it should work. I'll check tomorrow or Monday (now I'm busy with something).

The thing I need to check is how many such methods I need to add into the configuration. If it's only one (getInstanceManager) then it's good, but if it's hundreds, and I gotta update the config every time I add a new service, then not so much.

Question: Souldn't the configuration work the opposite way? i.e.:

• By default, do not downgrade parameter type widening for final
• If enabled by configuration, only then do it

Then if you really want that behavior (that, in my opinion, is not the expected behavior from that PHP feature), only then you explicitly enable it.

Also: Can it be enabled/disabled globally? (i.e. instead of having to define class by class and method by method, as in this example you provided)

Thoughts?

[leoloso]

I just noticed that, in the example you sent, DowngradeAttributeToAnnotationRector didn't work: it should've added /** @required */ in replacement of #[Required], but it didn't

[samsonasik]

That probably different issue or caused by multiple rules apply, could you narrow to specific rule that cause it not apply @required ?

[samsonasik]

@leoloso you're missing use statement:

use Symfony\Contracts\Service\Attribute\Required;

in the example code, the following convert to @required correctly

https://getrector.com/demo/8d49074d-319b-4a99-a6ef-a5a148cfecb7

[leoloso]

DowngradeParameterTypeWideningRector is configurable, I think you can configure like this:

use Rector\Config\RectorConfig;
use Rector\Set\ValueObject\DowngradeLevelSetList;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->sets([
        DowngradeLevelSetList::DOWN_TO_PHP_71,
    ]);
    
   $rectorConfig
        ->ruleWithConfiguration(\Rector\DowngradePhp72\Rector\ClassMethod\DowngradeParameterTypeWideningRector::class, [
            WithInstanceManagerServiceTrait::class => ['getInstanceManager'],
        ]);
};

see https://getrector.com/demo/ac050d1f-fa18-40f8-9999-73ade354368c

is that will work for you?

@samsonasik I did the config change (GatoGraphQL/GatoGraphQL#2396), and that specific file was downgraded OK, but the application is exploding somewhere else:

19.0.2:55776] PHP Warning:  Declaration of PrefixedByPoP\\Symfony\\Component\\DependencyInjection\\Loader\\FileLoader::import($resource, ?string $type = NULL, $ignoreErrors = false, ?string $sourceResource = NULL, $exclude = NULL) should be compatible with PrefixedByPoP\\Symfony\\Component\\Config\\Loader\\FileLoader::import($resource, ?string $type = NULL, bool $ignoreErrors = false, ?string $sourceResource = NULL, $exclude = NULL) in /app/wordpress/wp-content/plugins/gato-graphql/vendor/symfony/dependency-injection/Loader/FileLoader.php on line 34

You can see the issue happens on Symfony DependendyInjection and Symfony Config dependencies, so this code is out of my control. The error says that the same function from a class and its parent class was downgraded differently.

Class FileLoader in DependencyInjection inherits from class FileLoader in Config:

use PrefixedByPoP\Symfony\Component\Config\Loader\FileLoader as BaseFileLoader;

abstract class FileLoader extends BaseFileLoader

Then, this is how function import was downgraded in both classes:

In vendor/symfony/dependency-injection/Loader/FileLoader.php:

public function import($resource, string $type = null, $ignoreErrors = \false, string $sourceResource = null, $exclude = null)

Notice that $ignoreErrors doesn't have the type bool anymore.

In vendor/symfony/config/Loader/FileLoader.php:

public function import($resource, string $type = null, bool $ignoreErrors = \false, string $sourceResource = null, $exclude = null)

Notice that $ignoreErrors still has the type bool.

Could it be that DowngradeParameterTypeWideningRector is currently not doing the downgrade all the way up its ancestor classes?

[leoloso]

Here I recreated it:

https://getrector.com/demo/63323a2d-5182-4469-8d5a-6fca29baba85

But it seems to be downgraded alright there?

Btw, notice that the signature of import in both classes, for param $ignoreErrors, is different. Could that be causing some trouble?

[leoloso]

Could it be that one class is implemented by some final class, while the other one is not, and that's why they were downgraded differently in my project?

[leoloso]

Here it goes, I reproduced the issue:

https://getrector.com/demo/44f3d3df-8a0c-454b-8e5b-049fc00cb511

When adding this:

   $rectorConfig
        ->ruleWithConfiguration(\Rector\DowngradePhp72\Rector\ClassMethod\DowngradeParameterTypeWideningRector::class, [
            WithInstanceManagerServiceTrait::class => ['getInstanceManager'],
        ]);

...the $ignoreErrors gets the type removed in the first class, and not in the second class.

If not adding that config code, then both $ignoreErrors are the same.

[samsonasik]

The issue probably on other rules, as when only apply the DowngradeParameterTypeWideningRector, it just skipped, see https://getrector.com/demo/37185d42-4ef0-424c-b69d-dd28079e9cdc

could you narrow to specific rules that cause it?

[leoloso]

It is DowngradeUnionTypeDeclarationRector:

https://getrector.com/demo/1cbb1dd6-8f37-4ac2-a284-5d22d8859a6e

[leoloso]

It's independent of having that config or not:

https://getrector.com/demo/377c6d23-e96a-48df-b48a-9266b7429d55

[leoloso]

Having both DowngradeUnionTypeDeclarationRector and DowngradeParameterTypeWideningRector rules:

• With no config code, only DowngradeParameterTypeWideningRector was applied, and it works well:

https://getrector.com/demo/b517fe20-c542-4896-993c-f4f199416dff

• With the config code, only DowngradeUnionTypeDeclarationRector was applied (i.e. the other one!), and it does not work well:

https://getrector.com/demo/a7349da0-8687-4045-96d2-a375e14aed79

[samsonasik]

It seems due to the rule use Reflection that may be cached, could you try provide failing test case PR with configured rule provided and expected fixture output at tests/issues, you can check this as example:

https://github.com/rectorphp/rector-downgrade-php/tree/main/tests/Issues/IssueDowngradeArraySpread

Thank you.

[leoloso]

@samsonasik Done, please check: rectorphp/rector-downgrade-php#140

[leoloso]

Test recreated successfully, it's failing as it should:

https://github.com/rectorphp/rector-downgrade-php/actions/runs/5461465950/jobs/9939590731?pr=140

      /**
      * @param string|mixed[] $exclude
      */
-    public function import($ignoreErrors = false, string $sourceResource = null, $exclude = null)
+    public function import(bool $ignoreErrors = false, string $sourceResource = null, $exclude = null)
     {
     }
 }

[samsonasik]

@leoloso I think the solution is define both import and getInstanceManager to the config, see

• https://getrector.com/demo/46da785a-9c8e-4ea0-a14b-483ef16cf5da
• https://3v4l.org/bdEcj#v7.1.32

That will work on php 7.1.

[leoloso]

Hmmmm I find this solution unsatisfactory to be honest, because I don't know in advance what code will need to be downgraded, using what rule. The example itself involves 2 libraries from Symfony, which I don't know anything about.

So with the proposal to add ParentClass::class => ['import'], running the Rector downgrade process will become "run, explode, fix, run again". That import method is completely unknown to me, so I wouldn't know in advance it needs some configuration until running Rector and generating the downgraded code, testing it, and then noticing that it explodes.

Also, the GitHub Action task running the downgrade takes around 10 minutes, and after that one I might discover that the downgraded code fails somewhere else, so I'd need to find the source of the problem, apply the config also there, test again, and so on. This is far from ideal.

Instead of running Rector and having everything automatically work, it'll now become a process prone to failures that I need to manually fix, without ever knowing if that's conflicting with something else, or if there's another bug somewhere else that I haven't identified with my tests and will then explode in runtime.

Back to my previous comment (#8031 (comment)):

Question: Souldn't the configuration work the opposite way? i.e.:

* By default, do not downgrade parameter type widening for `final`

* If enabled by configuration, only then do it

Then if you really want that behavior (that, in my opinion, is not the expected behavior from that PHP feature), only then you explicitly enable it.

Can this be done? In other words, invert the behavior: Have Rector by default work always in a fail-safe way, and only for people who know what they are doing, they can enable configuration to customize the behavior (eg: removing the type for final classes).

I keep thinking that the type should not be removed for final classes. The PHP feature itself does not require it. If this feature is needed to downgrade Rector, then you can specifically enable it for Rector via configuration, since you know what you're doing, and you intimately know the Rector codebase.

What do you think?

[samsonasik]

Not downgrading final class seems will cause another bug if it has interface that downgraded, see https://3v4l.org/UmLn9#v7.1.33 which the final class's method param is not compatible with interface, and the child of interface can be final or not, so to be safe, just mark all of them non typed.

The hardest part of it is keeping work with php 7.1 tbh

[leoloso]

I don't understand why SomeFinalClass is not already downgraded: it should also have its type removed, independently of being final or not.

[samsonasik]

That's will be a bug that happen if we are going to apply "* By default, do not downgrade parameter type widening for final" as you noted above as interface downgraded, and child is not.

[leoloso]

I may be confused, but why will only the interface be downgraded, but not the child class? They should both be downgraded together (i.e. both should have their types removed).

I don't see how final makes a difference: if the interface is downgraded, the class should also be downgraded. Isn't that the case already?

[samsonasik]

That covered at https://github.com/rectorphp/rector-downgrade-php/blob/57ad03c027f04921edea6e081c7346f21d23d104/rules-tests/DowngradePhp72/Rector/ClassMethod/DowngradeParameterTypeWideningRector/Fixture/container_has_interface.php.inc#L29-L48

by skipping on final class, that will loop back search for parent which parent may be cached by reflection after changed.

Could you add new fixture to cover that? I don't know if that will be resolvable tho, but let's give it another try.

[leoloso]

https://github.com/rectorphp/rector-downgrade-php/pull/141/files

I added 4 fixture tests, with all combinations:

• Interface with type, classes not
• Interface and class with type, final class not
• Interface and final class with type, class not
• Interface and all classes with type

Is this what you wanted?

[samsonasik]

Thank you, I will check if that will be resolvable 👍

[leoloso]

interface_and_all_classes.php.inc is failing, it did the downgrade but it shouldn't

[leoloso]

@samsonasik just for your information, I've decided to bump my downgrade's PHP version to 7.2, following Rector (rectorphp/rector-src#1955).

So I won't have the issue with DowngradeParameterTypeWideningRector anymore, but I also won't be able to test it.

[samsonasik]

Thank you for the info, is it mean that the issue can be closed?

[leoloso]

Well, that's up to you, the bug is still happening... (both Rector and my project don't need to fix it anymore, but anyone else downgrading to PHP 7.1 will still experience the bug)

[samsonasik]

Ok, I am closing it then, the possible solution I can found is to configure it like in

#8031 (comment)