Implement ReturnTypeFromStrictParamRector by staabm · Pull Request #4501 · rectorphp/rector-src

staabm · 2023-07-13T10:48:13Z

<?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 · 2023-07-13T11:06:13Z

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

samsonasik · 2023-07-13T12:09:38Z

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.

rector-src/rules/TypeDeclaration/Rector/ClassMethod/ReturnTypeFromStrictTypedCallRector.php

Lines 135 to 140 in a10ddea

    
           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 · 2023-07-13T12:25:41Z

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 · 2023-07-13T12:29:11Z

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

staabm · 2023-07-13T12:29:51Z

exists in many legacy code

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

samsonasik · 2023-07-13T12:32:07Z

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

...laration/Rector/ClassMethod/ReturnTypeFromStrictParamRector/Fixture/skip_call_by_ref.php.inc

staabm · 2023-07-13T13:45:26Z

...ration/Rector/ClassMethod/ReturnTypeFromStrictParamRector/Fixture/skip_assign_by_ref.php.inc

+
+namespace Rector\Tests\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromStrictParamRector\Fixture;
+
+class SkipAssign {


covers https://3v4l.org/E2XOE

samsonasik · 2023-07-13T15:00:36Z

rules/TypeDeclaration/Rector/ClassMethod/ReturnTypeFromStrictParamRector.php

+        if ($node->stmts === null) {
+            return null;
+        }


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

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. ------ ------------------------------------------------------------------------------

No worries, I will check after it get merged 👍

rules/TypeDeclaration/Rector/ClassMethod/ReturnTypeFromStrictParamRector.php

Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>

This reverts commit 82107dd.

This reverts commit 6d61b59.

staabm · 2023-07-14T08:36:00Z

should be good to go

staabm · 2023-07-14T13:39:22Z

ping @TomasVotruba :)

TomasVotruba · 2023-07-14T16:11:23Z

Looks good, let's give it a try 👍

staabm commented Jul 13, 2023

View reviewed changes

...laration/Rector/ClassMethod/ReturnTypeFromStrictParamRector/Fixture/skip_call_by_ref.php.inc Show resolved Hide resolved

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

staabm requested a review from TomasVotruba as a code owner July 13, 2023 13:13

staabm commented Jul 13, 2023

View reviewed changes

samsonasik reviewed Jul 13, 2023

View reviewed changes

rules/TypeDeclaration/Rector/ClassMethod/ReturnTypeFromStrictParamRector.php Outdated Show resolved Hide resolved

samsonasik reviewed Jul 13, 2023

View reviewed changes

rules/TypeDeclaration/Rector/ClassMethod/ReturnTypeFromStrictParamRector.php Show resolved Hide resolved

staabm added 17 commits July 14, 2023 10:35

Implement ReturnTypeFromStrictParamRector

341ff56

more tests

61ecf87

Create skip_closure.php.inc

9d42efd

Update skip_closure.php.inc

ad135c7

fix

cf27dbf

more tests

850e98e

fix names

5fe5a29

more tests, symplify

0dde3b8

skip union return types

399cf91

simplify

7e97c31

skip magic

f32c13d

Update skip_closure.php.inc

56c22e4

cleanup

0afb93a

fix

19075c1

Update type-declaration.php

cd9f8fd

Create skip_already_return_typed.inc

1d45d9b

Update skip_already_return_typed.inc

cfa3ec4

staabm and others added 8 commits July 14, 2023 10:35

cover indirect modification by reference

3d5b7c4

Update skip_assign_by_ref.php.inc

fca0936

simplify

6c297f5

Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>

earlier $node->stmts check

1368d86

fix

f700d05

Revert "fix"

c0dd0b2

This reverts commit 82107dd.

Revert "earlier $node->stmts check"

0afa893

This reverts commit 6d61b59.

fix phpstan

f3a19cb

clxmstaab force-pushed the return-from-param branch from 2331300 to f3a19cb Compare July 14, 2023 08:35

samsonasik approved these changes Jul 14, 2023

View reviewed changes

TomasVotruba merged commit 875b2a3 into rectorphp:main Jul 14, 2023

staabm deleted the return-from-param branch July 14, 2023 16:26

samsonasik mentioned this pull request Jul 14, 2023

[TypeDeclaration] Null stmts check early on ReturnTypeFromStrictParamRector #4512

Merged


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

		class SkipAssign {

Uh oh!

Conversation

staabm commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staabm commented Jul 13, 2023

Uh oh!

samsonasik commented Jul 13, 2023

Uh oh!

staabm commented Jul 13, 2023

Uh oh!

samsonasik commented Jul 13, 2023

Uh oh!

staabm commented Jul 13, 2023

Uh oh!

samsonasik commented Jul 13, 2023

Uh oh!

Uh oh!

staabm Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

samsonasik Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

staabm Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

staabm Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

samsonasik Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

samsonasik Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

staabm commented Jul 14, 2023

Uh oh!

staabm commented Jul 14, 2023

Uh oh!

TomasVotruba commented Jul 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

staabm commented Jul 13, 2023 •

edited

Loading