Skip to content

[TypeDeclaration] Add ReturnTypeFromStrictFluentReturnRector#4890

Merged
TomasVotruba merged 25 commits intomainfrom
add-fluent
Aug 31, 2023
Merged

[TypeDeclaration] Add ReturnTypeFromStrictFluentReturnRector#4890
TomasVotruba merged 25 commits intomainfrom
add-fluent

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@TomasVotruba @staabm let's add new rule ReturnTypeFromStrictFluentReturnRector to add return type on return $this; usage.

Closes rectorphp/rector#8067

@samsonasik samsonasik marked this pull request as draft August 31, 2023 15:25

class NonFinalClass
{
public function test(): static
Copy link
Copy Markdown
Member Author

@samsonasik samsonasik Aug 31, 2023

Choose a reason for hiding this comment

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

@staabm when return $this, return static or return self is actually same, see

so the fallback to return self is fine on php 7.x

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added test for php 7.x add return self instead of static 5e5ba5e

@samsonasik samsonasik marked this pull request as ready for review August 31, 2023 15:52
{
if (rand(0, 1)) {
$this->foo = 'foo';
return $this;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Another test with a return $this and return null case would be great to support nullable

Copy link
Copy Markdown
Member Author

@samsonasik samsonasik Aug 31, 2023

Choose a reason for hiding this comment

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

that's already covered on ReturnUnionTypeRector https://getrector.com/demo/cda3b293-ac7d-4432-83f5-f23cc833a4c1 while return ?static on final class is not needed, that should be ?self, but that will need separate PR for improvement and consideration.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I created PR:

for nullable $this return on ReturnUnionTypeRector

new class {
public function run()
{
return $this;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need support for return new self() or return new static(); ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think that need separate PR for easier to review ;)

@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

@TomasVotruba
Copy link
Copy Markdown
Member

Thank you 👏

@TomasVotruba TomasVotruba merged commit 7078b51 into main Aug 31, 2023
@TomasVotruba TomasVotruba deleted the add-fluent branch August 31, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Infer return type from $this

4 participants