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

Fix handling of $this return type in UnionTypes rule #1665

Closed
wants to merge 1 commit into from

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Jan 12, 2022

$this refers to the exact object identity, not just the same type. Therefore, it's valid and should not be removed
@see https://wiki.php.net/rfc/this_return_type for more context

Fixes #1246

`$this` refers to the exact object identity, not just the same type. Therefore, it's valid and should not be removed
@see https://wiki.php.net/rfc/this_return_type for more context
@TomasVotruba
Copy link
Member

The RFC targets PHP 8.2 and the voting did not yet started. We always way for the feature to be at least merged to PHP core before we apply the language. Sometimes the proposal can be denied and then Rector would work with non-existing feature.

@simPod
Copy link
Contributor Author

simPod commented Jan 12, 2022

The proposal is about adding a native typehint but $this lives in phpdoc since I've started the php and have always meant the same thing.

The $this return type follows an established convention from PHPDoc, where @return $this is commonly used in place of @return static where it comes to fluent interfaces. On a dataset of 2k composer packages, @return $this occurs 29k times, while @return static is used 38k times.

The $this return type is arguably not a type, in that enforces not just the type of the return value (which is the same as static), but also its object identity (that of $this). The object identity is important to the usage of the API, because it determines whether the return value can be safely discarded

I have added the RFC link for more context but it is not really relevant, whether it will be accepted or not since it only introduces $this natively.

}
}

?>
Copy link
Member

Choose a reason for hiding this comment

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

This fixture is correct. In case of final class, the doc is exact duplicate of self.
We've removed many of these cases manually, that's why this rule handled it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But self !== $this. Correct me if I'm wrong but the proper usage and difference is:

For mutable withers, you'd use $this. For immutable withers, you'd use self.

final class A
{
    public function withMute(): self
    {
        return clone $this;
    }

    /**
     * @return $this
     */
    public function withNoMute(): self
    {
        return $this;
    }
}

We've removed many of these cases manually, that's why this rule handled it.

So it depends what were your cases. If you had mutable withers, those should not have been removed AFAIK.

Copy link
Member

Choose a reason for hiding this comment

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

It depends how PHPStan handles these 2 cases, as we depend on its type system. Is it same or different from PHPStan view?

The change might be needed there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But even though phpstan might not distinguish between that, the union rule should not touch it. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking and I agree with you.

But in that case it must be resolved there first. Then we can use it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in that case it must be resolved there first. Then we can use it here.

Shouldn't this #1246 be reverted then? It's not working properly, I'd say there's a bug in behaviour now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomasVotruba BTW just found out PHPStan already supports it.

Method SimPod\GraphQLUtils\Builder\InputObjectBuilder::create() should return $this(SimPod\GraphQLUtils\Builder\InputObjectBuilder) but returns static(SimPod\GraphQLUtils\Builder\InputObjectBuilder)

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