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

AddVoidReturnTypeWhereNoReturnRector: fix never type handling #4918

Merged
merged 7 commits into from Sep 6, 2023

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Sep 6, 2023

No description provided.


final class ThrowsMethod
{
protected function getValues(): void
Copy link
Contributor Author

@staabm staabm Sep 6, 2023

Choose a reason for hiding this comment

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

this is the bug I am trying to solve. a method, which contains a non-top-level never type, should get the void return type as it cannot not get a never type

https://3v4l.org/gNmVO

@staabm
Copy link
Contributor Author

staabm commented Sep 6, 2023

I don't have an idea yet how to solve it

@staabm staabm changed the title AddVoidReturnTypeWhereNoReturnRector: added failling tests AddVoidReturnTypeWhereNoReturnRector: fix never type handling Sep 6, 2023
@staabm staabm marked this pull request as ready for review September 6, 2023 12:01

final class ExitMethod
{
protected function getValues()
Copy link
Member

Choose a reason for hiding this comment

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

when not yet supported, add void is fine imo, than use ReturnNeverTypeRector when feature available, the void will be changed to never, see

https://getrector.com/demo/b32ecefb-b0b4-40ff-be70-c8c2bd5eea5b

Copy link
Contributor Author

@staabm staabm Sep 6, 2023

Choose a reason for hiding this comment

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

I added this test-case because my interpretation of

private function hasNeverType(ClassMethod | Closure | Function_ $functionLike): bool
{
return $this->betterNodeFinder->hasInstancesOf($functionLike, [Throw_::class]);
}

was that we don't want to add void when a never type is contained.

the idea was to prevent regressions of previous behaviour.
I don't know why the logic is there otherwise


final class NeverMethod
{
protected function getValues()
Copy link
Member

Choose a reason for hiding this comment

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

same with here, the ReturnNeverTypeRector will change void to never when feature available, see

https://getrector.com/demo/dde48f5c-7497-489c-9392-3043e45cead5

Copy link
Contributor Author

@staabm staabm Sep 6, 2023

Choose a reason for hiding this comment

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

hm ok, I moved the pre PR-existing test from a "skip" into a regular test which expects :void beeing added.

staabm and others added 2 commits September 6, 2023 14:38
…peWhereNoReturnRector/Fixture/exception.php.inc

Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>
…peWhereNoReturnRector/Fixture/exception.php.inc

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

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

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

@staabm I am ok with this 👍

@samsonasik
Copy link
Member

@TomasVotruba I am merging it ;), this will improve php 7.x return void, when user use php 8.1, user can run ReturnNeverTypeRector to change void to never when possible ;)

@samsonasik samsonasik merged commit aee4200 into rectorphp:main Sep 6, 2023
38 checks passed
@staabm staabm deleted the fail branch September 6, 2023 13: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
2 participants