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

Type information lost when passing generic type of enum as argument #9084

Closed
JoyceBabu opened this issue Mar 24, 2023 · 17 comments
Closed

Type information lost when passing generic type of enum as argument #9084

JoyceBabu opened this issue Mar 24, 2023 · 17 comments
Labels
Milestone

Comments

@JoyceBabu
Copy link

JoyceBabu commented Mar 24, 2023

Bug report

When I pass an argument of type T, where T is T of EnumType::* to a function, the type of the changed parameter is changed to T<EnumType> instead of T<actual enum type>.

For example, if I pass Foo<Bar::Baz> as a function parameter to a function /** @param S $foo */ function foobar($foobar);, type of S is detected as Foo<Bar>, not Foo<Bar::Baz>. So, the parameter fails to match its own generic type. Instead an message about template covariance is reported.

Code snippet that reproduces the problem

https://phpstan.org/r/6056f0bd-2f52-456f-8bb1-bd746091fafb

Expected output

The generic type S is the type of the passed argument. So it should should always pass validation.

Did PHPStan help you today? Did it make you happy in any way?

@ondrejmirtes
Copy link
Member

Your original example doesn't show the problem you're describing, this does: https://phpstan.org/r/ace4e48a-b5e2-40a7-b191-c060db486d39

@ondrejmirtes ondrejmirtes added this to the Generics milestone Mar 25, 2023
@JoyceBabu
Copy link
Author

JoyceBabu commented Mar 27, 2023

Why is @template-covariant required in this case? The parameters type and the generic type are exactly the same (not just a super type of the generic type), so it should work with @template.

Or did you add the @template-covariant to highlight the actual issue, since the @template issue will be automatically fixed once the type detection is fixed?

@ondrejmirtes
Copy link
Member

Value<MassUnit::KiloGram> is a subtype of Value<MassUnit>. But since the template type isn't used in a parameter, @template-covariant can be used to allow this scenario.

@JoyceBabu
Copy link
Author

Similarly, the closed issue #8322 was closed as a invalid with explanation that @template-covariant is required. I apologise for not posting this update before the issue was locked, I though I already did. The example I posted was incorrect, here is a better example.

https://phpstan.org/r/b3f7353e-e4f9-4f8e-a21f-ca067a105d29

The expected return type @return ValidatorInterface<list<mixed>> of getListMixedValidator exactly matches the type of ValidatorInterface<list<mixed>> of ListMixedValidator. So, it should not raise a validation error. Here I am not trying to accept, ValidatorInterface<list<sub_type_of_mixed>> into ValidatorInterface<list<mixed>>. So, @template-covariant is not required there too.

@JoyceBabu
Copy link
Author

JoyceBabu commented Mar 27, 2023

Value<MassUnit::KiloGram> is a subtype of Value<MassUnit>. But since the template type isn't used in a parameter, @template-covariant can be used to allow this scenario.

Thank you for the clarification. That can work for a simple case like the original example. But, it will not work for a code like

https://phpstan.org/r/daac5dbf-315c-4745-9810-0db8748fa4da

But once the issue with the type detection is fixed, the linked example should also pass validation, without @template-covariant.

@ondrejmirtes
Copy link
Member

ListMixedValidator is also subtype of ValidatorInterface<list<mixed>>.

@JoyceBabu
Copy link
Author

JoyceBabu commented Mar 27, 2023

ListMixedValidator is also subtype of ValidatorInterface<list<mixed>>.

Yes it is, and @template-covariant would fix the the error because of that.

But that is only a workaround, and it shouldn't be needed since ListMixedValidator implements ValidatorInterface<list<mixed>>, which is exactly what getListMixedValidator is expected to return. So, #8322 is a valid issue, having the suggested workaround (with limitations). I request you to re-open it.

@ondrejmirtes
Copy link
Member

It's not a valid issue, and it's not a workaround.

@ondrejmirtes
Copy link
Member

But to be sure, asking the local variance expert @jiripudil: is this a bug or not? https://phpstan.org/r/b3f7353e-e4f9-4f8e-a21f-ca067a105d29

Thank you.

@jiripudil
Copy link
Contributor

That is a bug. It's just a wild guess, but I suspect it could be the same issue as in #9074

@ondrejmirtes
Copy link
Member

Alright, thanks, I'm gonna reopen it. Sorry for misleading, it looked like a legit report.

@ondrejmirtes
Copy link
Member

Bug explained: #9074 (reply in thread)

@phpstan-bot
Copy link
Contributor

@JoyceBabu After the latest push in 1.11.x, PHPStan now reports different result with your code snippet:

@@ @@
-PHP 8.1 – 8.2 (3 errors)
+PHP 8.1 – 8.2 (4 errors)
 ==========
 
+27: Enum case MassUnit::MilliGram type float doesn't match the "int" type.
 68: Dumped type: Value<UnitType::Mass>
 69: Parameter #1 $value of function duplicate expects Value<UnitType>, Value<UnitType::Mass> given.
 70: Dumped type: Value<UnitType>
Full report

PHP 8.1 – 8.2 (4 errors)

Line Error
27 Enum case MassUnit::MilliGram type float doesn't match the "int" type.
68 Dumped type: Value<UnitType::Mass>
69 Parameter #1 $value of function duplicate expects Value<UnitType>, Value<UnitType::Mass> given.
70 Dumped type: Value<UnitType>

PHP 7.2 – 8.0 (1 error)

Line Error
3 Syntax error, unexpected T_STRING on line 3

@JoyceBabu
Copy link
Author

I have updated the sample code to fix the unrelated error.

@phpstan-bot
Copy link
Contributor

@JoyceBabu After the latest push in 1.11.x, PHPStan now reports different result with your code snippet:

@@ @@
-PHP 8.1 – 8.2 (3 errors)
+PHP 8.1 – 8.2 (4 errors)
 ==========
 
+27: Enum case MassUnit::MilliGram value 0.001 does not match the "int" type.
 68: Dumped type: Value<UnitType::Mass>
 69: Parameter #1 $value of function duplicate expects Value<UnitType>, Value<UnitType::Mass> given.
 70: Dumped type: Value<UnitType>
Full report

PHP 8.1 – 8.2 (4 errors)

Line Error
27 Enum case MassUnit::MilliGram value 0.001 does not match the "int" type.
68 Dumped type: Value<UnitType::Mass>
69 Parameter #1 $value of function duplicate expects Value<UnitType>, Value<UnitType::Mass> given.
70 Dumped type: Value<UnitType>

PHP 7.2 – 8.0 (1 error)

Line Error
3 Syntax error, unexpected T_STRING on line 3

@ondrejmirtes
Copy link
Member

Last night I came up with the idea that we mostly shouldn't generalize the generic type variables, except when they're in object generics, like Foo<int>.

Here's the resulting PR: phpstan/phpstan-src#2818

We can't do this for objects, because I want new Collection([1, 2, 3]) to become Collection<int>. This could be improved in the future thanks to this suggestion: #6732 (comment) (but it's pretty complex to implement so for now it has to wait).

The new behaviour now only applies to bleeding edge (https://phpstan.org/blog/what-is-bleeding-edge) so definitely enable it to get the taste of the future 👍

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants