Skip to content

Consider MixedType explicitness in MethodParameterComparisonHelper::isTypeCompatible #1491

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

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Jul 1, 2022

Closes phpstan/phpstan#7415

Doesn't feel like the most elegant solution though.. I played around a bit with MixedType::isSuperTypeOf but did not find something better unfortunately.

@ondrejmirtes
Copy link
Member

Yeah, the issue phpstan/phpstan#7415 is really weird. I don't think your fix is correct, because the message "is not covariant with tentative return type void of method" should be triggered only when considering native return types. The codepath should only by executed for them. It looks wrong to me to exclude explicit mixed types to fix this case - comparing PHPDoc-based types (generics) shouldn't be done when checking tentative return types...

@herndlm
Copy link
Contributor Author

herndlm commented Jul 10, 2022

I need to think more about this but the changes here did not activate the "is not covariant with tentative return type void of method", they were already there. The errors about mixed returns are missing though.

If I understood it correctly, than the problem is that MethodParameterComparisonHelper::isTypeCompatible is currently completely ignoring mixed method parameters (explicitly set via native return type or not set at all, resulting in an implicit mixed). And the prototype parameter type became explicit mixed, which triggers an error on 8.1 if the parameter type is not also set to explicit mixed, right? And that was the only thing I was looking at here, basically trying to distinguish the explicit and implicit mixed types to make it trigger the expected error. I'm not excluding mixed, I think I'm less excluding it if I'm not mistaken.

But this is a kind of error I'm not good with and very unsure :)

@herndlm
Copy link
Contributor Author

herndlm commented Aug 23, 2022

I'm not sure what to do with this one. It makes sense to me, but maybe I misunderstood what was going on. I'll give it a good old last force-push and if this is the wrong solution we can just close it :)

@herndlm herndlm marked this pull request as ready for review August 23, 2022 18:47
@herndlm herndlm marked this pull request as draft August 23, 2022 19:16
@herndlm herndlm force-pushed the bug-7415 branch 2 times, most recently from fbd12ed to 70181d7 Compare August 23, 2022 19:50
@herndlm herndlm marked this pull request as ready for review August 23, 2022 20:34
@ondrejmirtes
Copy link
Member

Alright, thank you :)

@ondrejmirtes
Copy link
Member

I reviewed the situation and indeed it looks correct.

@ondrejmirtes ondrejmirtes merged commit 3025b15 into phpstan:1.8.x Aug 24, 2022
@herndlm herndlm deleted the bug-7415 branch August 24, 2022 09:25
@ondrejmirtes
Copy link
Member

I have a follow-up for you. This is allowed in PHP: https://3v4l.org/GAoP2

But PHPStan started reporting this as an error after this PR: https://phpstan.org/r/66db5967-78af-4abe-b21e-12b311dafcbb

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.

Missing return types on Iterator::key() and current() are not reported
2 participants