Skip to content

Refactoring order creates incompatible return types#1869

Merged
TomasVotruba merged 1 commit intorectorphp:masterfrom
scheb:return-type-refactoring-order
Sep 15, 2019
Merged

Refactoring order creates incompatible return types#1869
TomasVotruba merged 1 commit intorectorphp:masterfrom
scheb:return-type-refactoring-order

Conversation

@scheb
Copy link
Copy Markdown
Contributor

@scheb scheb commented Aug 19, 2019

This test case shows how the order in which files are parsed & refactored can lead to incompatible method signatures on the return type. The rector does populateChildren, but the implementation implicity relies on the superclass being processed before its subclasses to ensure return type compatibility. This in combination with the subtype logic leads to this result.

(Note: Actually I have to say I don't really understand why this subtype logic is there, since return type variance is only supported from PHP 7.4 on. Earlier versions support type variance for the "iterable" type which is there as the test case, but I have the impression that's the one single case when PHP <7.4 supports return type variance).

I'd guess an approach similar to #1842 would be the solution. Doing it the other way round: instead of refactoring children after refactoring the node itself, the parents should be refactored before the node is refactored. This would ensure the return type decision for the parent class was made by the rector and we can make a correct decision for the node itself.

Bonus objective would be to support refactoring with return type variance when PHP7.4 is set as the language level.

I'll have a look at a potential fix as soon as I find time to have a deeper look.

@scheb scheb closed this Aug 20, 2019
@scheb scheb deleted the return-type-refactoring-order branch August 20, 2019 19:48
@scheb scheb restored the return-type-refactoring-order branch August 20, 2019 19:50
@scheb scheb reopened this Aug 20, 2019
@TomasVotruba
Copy link
Copy Markdown
Member

There's been major refactoring of types from string to PHPStan value object types.
Please rebase and I'll give this priority

@scheb
Copy link
Copy Markdown
Contributor Author

scheb commented Sep 15, 2019

Added a new test case, which is technically the same as the inheritance_covariance.php.inc test but with different parsing order.

@TomasVotruba TomasVotruba merged commit 27ab83f into rectorphp:master Sep 15, 2019
@TomasVotruba
Copy link
Copy Markdown
Member

Thanks 👍

I'll look at it locally

TomasVotruba added a commit that referenced this pull request Feb 25, 2022
rectorphp/rector-src@4dee58b [Php81] Fix property with attribute inlined on ReadOnlyPropertyRector (#1869)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants