AddReturnTypeDeclarationBasedOnParentClassMethodRector

[MartinMystikJonas]

First draft that adds return type only by first parent/interface found with that method. In future I will try to change it to detect correct intersection type that will satisfy all parents/interfaces.

It is heavily based on AddReturnTypeDeclaration and AddParamBasedOnParentClassMethodRector.

There were some duplicated code with existing rule so I moved it to service but not sure if it is correct place. Structure of services used in rules is quite confusing to me - is there any design guide for that?

I think this roule could also be added to some existing sets but not sure to which. It is quite universal rule that detects issues that would cause Fatal error.

Tests included and after PR #2660 by @samsonasik it works perfectly with interfaces too.

@TomasVotruba Let me know if you want something solved differently.

[samsonasik]

Could you add test for the following use case:

abstract class A
{
    public abstract function run(): mixed;
}

class B extends A
{
    public function run()
    {
    }
}

ref https://3v4l.org/i8EK4

[MartinMystikJonas]

@samsonasik Updated based on your feedback

[MartinMystikJonas]

Could you add test for the following use case:

abstract class A
{
    public abstract function run(): mixed;
}

class B extends A
{
    public function run()
    {
    }
}

ref https://3v4l.org/i8EK4

Of course I can. What is exactly poin of this test testing with abstract parent? Or testing of mixed return type?

[samsonasik]

MixedType return can mean:

• no type
• has type named "mixed"

so verify if it supported, add mixed type to the return type as well.

[samsonasik]

that's test mixed typed return, can be both abstract method or not.

[MartinMystikJonas]

Ok I will do it after lunch

[samsonasik]

Also test for skip this case:

class A
{
    public function run(): mixed
    {
        return null;
    }
}

class B extends A
{
    public function run(): string
    {
        return 'test';
    }    
}

echo (new B())->run();

https://3v4l.org/NcBW6

[MartinMystikJonas]

Test for explicit mixed return type added

[MartinMystikJonas]

Test for narrowed return type added

[MartinMystikJonas]

Any idea why extended_class_mixed.php.inc fails in CI but work just fine on local?

Both runs on same PHP version (8.1.7) and with latest dependencies.

[samsonasik]

class load maybe overlapped, you may need to register to composer.json classmap, eg:

rector-src/composer.json

Line 113 in db7012e

"rules-tests/Renaming/Rector/FileWithoutNamespace/PseudoNamespaceToNamespaceRector/Source"

[MartinMystikJonas]

What do you mean by overlapped class load? Class name is unique.

I tried to add line to composer.json so we will see as soon as CI finishes

[samsonasik]

afaik, that's depends of how PHPStan load it, sometime in CI, they are overlapped so it loaded after PHPStan

[MartinMystikJonas]

@samsonasik Well it helped. Thanks. I would not be able to figure this out myself.

[MartinMystikJonas]

@TomasVotruba Let me know if there is anything I should to to finish this PR.

[TomasVotruba]

This should not be needed, as the classes must be psr-4 compatible here

[MartinMystikJonas]

See discussion above. Without it PHPStan fails in CI. @samsonasik suggested this as fix and it worked.

[MartinMystikJonas]

Not sure why it is needed because it seems more like weird PHPStan bug to me. Any idea how it could be solved differently?

[TomasVotruba]

I see. The namespace is missing here, should be completed

[MartinMystikJonas]

So add to all fixtures

namespace Rector\Tests\TypeDeclaration\Rector\ClassMethod\AddReturnTypeDeclarationBasedOnParentClassMethodRector\Fixture;

and remove that line from composer.json?

[samsonasik]

While the namespace should be added in Fixture files, the issue is about resolving Source directory, like other registration in composer.json.

But, that's worth a try

[MartinMystikJonas]

OK namespaces added and line drom composer.json removed. So we will see.

I also rebased on current master. But there are still some PHPStan errors not related to this PR - it seems they are caused by new rules added to symplify/phpstan-rules.

[samsonasik]

test seems failure https://github.com/rectorphp/rector-src/runs/7468797088?check_suite_focus=true#step:5:84

please add new commit instead of squash commit so we can see the different between before and after registered in composer.json

[MartinMystikJonas]

@samsonasik I noticed it too and it is fixed now. But i squashed commits before I read your message. But it seems that Ci pases now even without change in composer.json.

[TomasVotruba]

Thanks 👍

Is it ready to merge now?

[MartinMystikJonas]

From my point of view it is ready

[TomasVotruba]

Thank you 😊