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

PHP 8.1: Report missing typehints in overridden native methods #7363

Merged
merged 10 commits into from Jan 19, 2022

Conversation

danog
Copy link
Collaborator

@danog danog commented Jan 10, 2022

Allows detection of ReturnTypeWIllChange warnings, also fixes #7362

@danog danog changed the base branch from 4.x to master January 10, 2022 11:40
@danog danog marked this pull request as draft January 10, 2022 11:51
@danog danog marked this pull request as ready for review January 10, 2022 12:20
@weirdan
Copy link
Collaborator

weirdan commented Jan 10, 2022

@danog, is there any reason not to add this to 4.x line?

@danog danog changed the base branch from master to 4.x January 10, 2022 12:51
@danog
Copy link
Collaborator Author

danog commented Jan 10, 2022

Not really, no, I just wanted to switch directly to psalm v5 :D

@danog
Copy link
Collaborator Author

danog commented Jan 10, 2022

Hmm, that's weird, I rebased on top of 4.x...

@orklah
Copy link
Collaborator

orklah commented Jan 10, 2022

There must have been something wrong. isEmpty was only removed in master

@danog
Copy link
Collaborator Author

danog commented Jan 11, 2022

All fixed!

@orklah
Copy link
Collaborator

orklah commented Jan 11, 2022

Is this related to #6395 (comment) ?

@danog
Copy link
Collaborator Author

danog commented Jan 11, 2022

Yep, it's related precisely to that.

@orklah
Copy link
Collaborator

orklah commented Jan 11, 2022

Yep, it's related precisely to that.

but then, in your PR, you doesn't seem to distinguish phpdoc from real types in signature: https://github.com/vimeo/psalm/pull/7363/files#diff-c70a8399e3804f1bb260231f6af972513c1ca05d21e3f4221ae35ad5a9d4e6c0R121

Matt's branch was all about adding types in signature to be able to make the difference I believe?

@danog
Copy link
Collaborator Author

danog commented Jan 12, 2022

Matt's branch was all about adding types in signature to be able to make the difference I believe?
Yes, but not all native classes are declared as stubs: for example, the JsonSerializable class I used in the tests is populated at runtime using reflection.

I could've changed the reflection populator to populate signature_return_type instead of return_type (and fix existing stubs), but I decided it would be easier (safer?) to just create an explicit check that simply relies on the interaction of a native parent class and a user-defined child class.

Actually, do you think it should be made a separate issue, given that it signals a rather important PHP 8.1 BC semi-break?

@orklah
Copy link
Collaborator

orklah commented Jan 12, 2022

A different issue could be interesting indeed. About the signature_return_type vs return_type, did PHP add signatures types for every internal function/class/interface or did they do it only for certain cases?

@orklah orklah marked this pull request as draft January 15, 2022 10:38
@danog danog marked this pull request as ready for review January 19, 2022 11:21
@danog
Copy link
Collaborator Author

danog commented Jan 19, 2022

About the signature_return_type vs return_type, did PHP add signatures types for every internal function/class/interface or did they do it only for certain cases?

The documentation says Most non-final internal methods now require overriding methods to declare a compatible return type, so I just assumed all of them did.

@orklah orklah added the release:feature The PR will be included in 'Features' section of the release notes label Jan 19, 2022
@orklah
Copy link
Collaborator

orklah commented Jan 19, 2022

@danog is this ready?

@danog
Copy link
Collaborator Author

danog commented Jan 19, 2022

Yep!

@orklah orklah merged commit bbfdd57 into vimeo:4.x Jan 19, 2022
@orklah
Copy link
Collaborator

orklah commented Jan 19, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing mixed return typehint is not detected by psalm
3 participants