Skip to content

Support literals in ReturnTypeFromStrictTypedCallRector#4511

Merged
TomasVotruba merged 4 commits intorectorphp:mainfrom
staabm:strict-literals
Jul 14, 2023
Merged

Support literals in ReturnTypeFromStrictTypedCallRector#4511
TomasVotruba merged 4 commits intorectorphp:mainfrom
staabm:strict-literals

Conversation

@staabm
Copy link
Copy Markdown
Contributor

@staabm staabm commented Jul 14, 2023

before this PR ReturnTypeFromStrictTypedCallRector gave up adding return types, as soon as a single return was contained which cannot be resolved to the same type as others.

after this PR we also take returns into account which contain literal values, e.g. return 1;.

this allows in the following example to infer the type, which we couldn't without the patch:

final class SameTypedArrayReturns
{
-    public function getData()
+    public function getData(): array
    {
        if (rand(0,1)) {
            return []; // this path made the previous implementation give up adding a type
        }

        return $this->getArray();
    }

    public function getArray(): array
    {

    }
}

@staabm staabm marked this pull request as ready for review July 14, 2023 12:39
@staabm staabm requested a review from TomasVotruba as a code owner July 14, 2023 12:39
@TomasVotruba
Copy link
Copy Markdown
Member

I wonder how to make sure the strict type rules are idempotent, as we want to avoid one huge rule that handles all the script types everywhere. It was the case before and it was quite hard to handle and improve.

Yet this looks good, thank you 👍

@TomasVotruba TomasVotruba merged commit b3c4aff into rectorphp:main Jul 14, 2023
@staabm staabm deleted the strict-literals branch July 14, 2023 13:32
@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Jul 14, 2023

I wonder how to make sure the strict type rules are idempotent

I agree that they are pretty close together and its not that easy to differentiate what is doing what.

maybe we can find ways to combine existing rules which are "too similar" but still leave a line in-between - so some separation is still happing instead of a big ball of mud.

@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Jul 14, 2023

from a end-user point of view, I like those separate rector rules though, since it allows to refactor big code-bases in portions.

if we had less rules which do more stuff, it could lead to tooo big PRs on legacy projecs which are hard to review.
separation by file system is sometimes not that easy todo because type hierarchies need to be taken into account (at best people add types from the end of the dependency try and going up slowly)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants