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

Override static in return types with self in final classes #17724

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

rekmixa
Copy link

@rekmixa rekmixa commented Feb 6, 2025

This PR adds the ability to replace static with self in the return types in the final classes, provided that the prototype of the final class has a static return method.

Example of how it should work:

interface A {
    public function someMethod(): static;
}
final class B implements A {
    public function someMethod(): self { /** */ }
}
  • There are no changes that break backward compatibility or covariance logic.

More information in the issue: #17725

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

@rekmixa please define what exactly you propose. (e.g. some code didn't work as expected and started to work after the patch).

I'm not sure if this patch is a fix or a new feature that requries RFC and discussion, how it's going to coexists with type variance, etc

@iluuu1994 @nielsdos @arnaud-lb please take care about this PR.

…tests to folder Zend/tests/type_declarations/variance/override_static_with_self
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Conceptually, this seems correct to me. Please see my preliminary review comments.

@rekmixa
Copy link
Author

rekmixa commented Feb 13, 2025

@nielsdos do i need to squash all commits in this PR?

@rekmixa rekmixa requested a review from nielsdos February 13, 2025 07:34
@nielsdos
Copy link
Member

do i need to squash all commits in this PR?

No, we will squash on merge. Squashing makes it harder for us to review what has changed since our reviews.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

I think this is right now, thanks. But I haven't worked with this code a lot, so this needs more eyes, especially since this is deep engine code.
cc @iluuu1994 @arnaud-lb

This may also need discussion on the ML so we know for sure there are no objections.

@nielsdos nielsdos dismissed their stale review February 14, 2025 22:04

Requested changes were performed

@rekmixa
Copy link
Author

rekmixa commented Feb 15, 2025

This may also need discussion on the ML so we know for sure there are no objections.

By the way, i already created discussion in the ML: https://externals.io/message/126367

@nielsdos
Copy link
Member

This may also need discussion on the ML so we know for sure there are no objections.

By the way, i already created discussion in the ML: https://externals.io/message/126367

Ah good to know, I missed this.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM now. Thank you @rekmixa! Given there were no objections on the list, it should be ok to merge this in a week or so.

@rekmixa
Copy link
Author

rekmixa commented Feb 19, 2025

LGTM now. Thank you @rekmixa! Given there were no objections on the list, it should be ok to merge this in a week or so.

Thanks for your comments. Can you tell me if this PR will be merged in 8.4 or 8.5?

@iluuu1994
Copy link
Member

8.5. Stable branches only receive bug fixes.

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

Successfully merging this pull request may close these issues.

4 participants