Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ReturnTypeFromStrictParamRector #4501

Merged
merged 25 commits into from Jul 14, 2023

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jul 13, 2023

<?php

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

class Example {
-    public function doFoo(Example $param) {
+    public function doFoo(Example $param): Example {
        return $param;
    }
}

?>

@staabm
Copy link
Contributor Author

staabm commented Jul 13, 2023

@samsonasik hopefully I am right and this feature is not covered by a existing rule which I was not able to find ? :-)

@samsonasik
Copy link
Member

Possible void return fixture should be added:

class SkipPossibleVoid
{
    public function run(int $p)
    {
           if (rand(0, 1)) {
                return $p;
           }
    }
}

You can check with root Return validation like in other rule:

$inferReturnType = $this->returnTypeInferer->inferFunctionLike($node);

and verify the inferred return type.

private function isUnionPossibleReturnsVoid(ClassMethod | Function_ | Closure $node): bool
{
$inferReturnType = $this->returnTypeInferer->inferFunctionLike($node);
if ($inferReturnType instanceof UnionType) {
foreach ($inferReturnType->getTypes() as $type) {
if ($type->isVoid()->yes()) {

@staabm
Copy link
Contributor Author

staabm commented Jul 13, 2023

Possible void return fixture should be added:

great hint, thanks.

and verify the inferred return type.

I am not sure I would want to infer a union type at all (besides a simple nullable-object-type).
union return types are not good design in my opinion

@samsonasik
Copy link
Member

Yes, but that possible void are exists in many legacy code :)

@staabm
Copy link
Contributor Author

staabm commented Jul 13, 2023

exists in many legacy code

could you give me an example? the one you posted above is already covered

@samsonasik
Copy link
Member

not sure if I can find and show it here, but It seems you already covered it 👍

@staabm staabm marked this pull request as ready for review July 13, 2023 13:13

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

class SkipAssign {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +128 to +130
if ($node->stmts === null) {
return null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be moved to shouldSkipNode() method so early cheap check is handled for abstract method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted, since its required to make phpstan happy

 ------ ------------------------------------------------------------------------------
  Line   rules\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromStrictParamRector.php
 ------ ------------------------------------------------------------------------------
  128    Parameter #1 $nodes of method
         Rector\Core\Rector\AbstractRector::traverseNodesWithCallable()
         expects array<PhpParser\Node>|PhpParser\Node,
         array<PhpParser\Node\Stmt>|null given.
  157    Parameter #1 $nodes of method
         Rector\Core\Rector\AbstractRector::traverseNodesWithCallable()
         expects array<PhpParser\Node>|PhpParser\Node,
         array<PhpParser\Node\Stmt>|null given.
 ------ ------------------------------------------------------------------------------

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I will check after it get merged 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staabm
Copy link
Contributor Author

staabm commented Jul 14, 2023

should be good to go

@staabm
Copy link
Contributor Author

staabm commented Jul 14, 2023

ping @TomasVotruba :)

@TomasVotruba
Copy link
Member

Looks good, let's give it a try 👍

@TomasVotruba TomasVotruba merged commit 875b2a3 into rectorphp:main Jul 14, 2023
41 checks passed
@staabm staabm deleted the return-from-param branch July 14, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants